Skip to content

mv from in-cone to out-of-cone #3

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
ffyuanda wants to merge 4 commits into
base: mv-v5
Choose a base branch
from
Open

mv from in-cone to out-of-cone #3

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8063b21
t7002: add tests for moving from in-cone to out-of-cone
ffyuanda Jun 30, 2022
29400d6
mv: check if <destination> is a directory in the index with
ffyuanda Jun 21, 2022
044cd40
mv: rename BOTH flag to DEFAULT
ffyuanda Jun 21, 2022
34b53e9
mv: from in-cone to out-of-cone
ffyuanda Jun 22, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions advice.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -261,3 +261,17 @@ void detach_advice(const char *new_name)

fprintf(stderr, fmt, new_name);
}

void advise_on_moving_dirty_path(struct string_list *pathspec_list)
{
struct string_list_item *item;

if (!pathspec_list->nr)
return;

fprintf(stderr, _("The following dirty paths and/or pathspecs are moved\n"
"but not sparsified. Use \"git add\" to stage them then\n"
"use \"git sparse-checkout reapply\" to sparsify them.\n"));
for_each_string_list_item(item, pathspec_list)
fprintf(stderr, "%s\n", item->string);
Comment on lines +272 to +276
Copy link
Copy Markdown

@vdye vdye Jul 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple of suggestions to make the advice clearer:

  • Separate the warning message from the advice to fix the issue and allow the user to disable the non-warning advice with the advice.updateSparsePath config (see advise_on_updating_sparse_paths()).
  • Recommend --sparse flag for git add (since the user would be adding paths outside the sparse checkout definition).
  • Change "sparsify" to more precise references to "sparse-checkout definition" and/or "sparsity rules" (for consistency with advise_on_updating_sparse_paths())

You don't need to use my exact wording (below) if you'd rather write something else, but at a high level these suggestions should clarify things for users.

Suggested change
fprintf(stderr, _("The following dirty paths and/or pathspecs are moved\n"
"but not sparsified. Use \"git add\" to stage them then\n"
"use \"git sparse-checkout reapply\" to sparsify them.\n"));
for_each_string_list_item(item, pathspec_list)
fprintf(stderr, "%s\n", item->string);
fprintf(stderr, _("The following paths have been moved outside the\n"
"sparse-checkout definition but are not sparse due to local\n"
"modifications. then use \"git sparse-checkout reapply\" to\n"
"reapply sparsity rules patterns.\n"));
for_each_string_list_item(item, pathspec_list)
fprintf(stderr, "%s\n", item->string);
advise_if_enabled(ADVICE_UPDATE_SPARSE_PATH,
_("To correct the sparsity of these paths, do the following:\n"
"* Use \"git add --sparse <paths>\" to update the index\n"
"* Use \"git sparse-checkout reapply\" to apply the sparsity rules"));

Copy link
Copy Markdown
Owner Author

@ffyuanda ffyuanda Jul 15, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the comments, they are really helpful! I was thinking about adding a new advice config, but you pointed out that I can just use ADVICE_UPDATE_SPARSE_PATH, which is neat!

}
1 change: 1 addition & 0 deletions advice.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,5 +74,6 @@ void NORETURN die_conclude_merge(void);
void NORETURN die_ff_impossible(void);
void advise_on_updating_sparse_paths(struct string_list *pathspec_list);
void detach_advice(const char *new_name);
void advise_on_moving_dirty_path(struct string_list *pathspec_list);

#endif /* ADVICE_H */
50 changes: 43 additions & 7 deletions builtin/mv.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -174,6 +174,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
struct lock_file lock_file = LOCK_INIT;
struct cache_entry *ce;
struct string_list only_match_skip_worktree = STRING_LIST_INIT_NODUP;
struct string_list dirty_paths = STRING_LIST_INIT_NODUP;

git_config(git_default_config, NULL);

Expand Down Expand Up @@ -404,6 +405,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
const char *src = source[i], *dst = destination[i];
enum update_mode mode = modes[i];
int pos;
int up_to_date = 0;
struct checkout state = CHECKOUT_INIT;
state.istate = &the_index;

Expand All @@ -414,6 +416,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
if (show_only)
continue;
if (!(mode & (INDEX | SPARSE | SKIP_WORKTREE_DIR)) &&
!(dst_mode & SKIP_WORKTREE_DIR) &&
rename(src, dst) < 0) {
if (ignore_errors)
continue;
Expand All @@ -433,20 +436,52 @@ int cmd_mv(int argc, const char **argv, const char *prefix)

pos = cache_name_pos(src, strlen(src));
assert(pos >= 0);
if (!(mode & SPARSE) && !lstat(src, &st))
up_to_date = !ce_modified(active_cache[pos], &st, 0);
rename_cache_entry_at(pos, dst);

if ((mode & SPARSE) &&
(path_in_sparse_checkout(dst, &the_index))) {
int dst_pos;
if (ignore_sparse &&
!init_sparse_checkout_patterns(&the_index) &&
the_index.sparse_checkout_patterns->use_cone_patterns) {
Comment on lines +443 to +445
Copy link
Copy Markdown

@vdye vdye Jul 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

path_in_sparse_checkout() will initialize the patterns implicitly, so there's no need to setup the patterns early (especially because subsequent conditions could make it unnecessary). A less computationally expensive way to do this condition's check would be:

Suggested change
if (ignore_sparse &&
!init_sparse_checkout_patterns(&the_index) &&
the_index.sparse_checkout_patterns->use_cone_patterns) {
if (ignore_sparse &&
core_apply_sparse_checkout &&
core_sparse_checkout_cone) {

Copy link
Copy Markdown
Owner Author

@ffyuanda ffyuanda Jul 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure! This looks a lot cheaper, I did not know these two flags.


dst_pos = cache_name_pos(dst, strlen(dst));
active_cache[dst_pos]->ce_flags &= ~CE_SKIP_WORKTREE;
/* from out-of-cone to in-cone */
if ((mode & SPARSE) &&
path_in_sparse_checkout(dst, &the_index)) {
int dst_pos = cache_name_pos(dst, strlen(dst));
struct cache_entry *dst_ce = active_cache[dst_pos];

if (checkout_entry(active_cache[dst_pos], &state, NULL, NULL))
die(_("cannot checkout %s"), active_cache[dst_pos]->name);
dst_ce->ce_flags &= ~CE_SKIP_WORKTREE;

if (checkout_entry(dst_ce, &state, NULL, NULL))
die(_("cannot checkout %s"), dst_ce->name);
continue;
}

/* from in-cone to out-of-cone */
if ((dst_mode & SKIP_WORKTREE_DIR) &&
!(mode & SPARSE) &&
!path_in_sparse_checkout(dst, &the_index)) {
int dst_pos = cache_name_pos(dst, strlen(dst));
struct cache_entry *dst_ce = active_cache[dst_pos];
char *src_dir = dirname(xstrdup(src));

if (up_to_date) {
dst_ce->ce_flags |= CE_SKIP_WORKTREE;
unlink_or_warn(src);
} else {
string_list_append(&dirty_paths, dst);
safe_create_leading_directories(xstrdup(dst));
rename(src, dst);
}
if ((mode & INDEX) && is_empty_dir(src_dir))
rmdir_or_warn(src_dir);
}
}
}

if (dirty_paths.nr)
advise_on_moving_dirty_path(&dirty_paths);

if (gitmodules_modified)
stage_updated_gitmodules(&the_index);

Expand All @@ -455,6 +490,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
die(_("Unable to write new index file"));

string_list_clear(&src_for_dst, 0);
string_list_clear(&dirty_paths, 0);
UNLEAK(source);
UNLEAK(dest_path);
free(submodule_gitfile);
Expand Down
8 changes: 4 additions & 4 deletions t/t7002-mv-sparse-checkout.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -290,7 +290,7 @@ test_expect_success 'move sparse file to existing destination with --force and -
test_cmp expect sub/file1
'

test_expect_failure 'move clean path from in-cone to out-of-cone' '
test_expect_success 'move clean path from in-cone to out-of-cone' '
test_when_finished "cleanup_sparse_checkout" &&
setup_sparse_checkout &&

Expand All @@ -304,7 +304,7 @@ test_expect_failure 'move clean path from in-cone to out-of-cone' '
grep -x "S folder1/d" actual
'

test_expect_failure 'move dirty path from in-cone to out-of-cone' '
test_expect_success 'move dirty path from in-cone to out-of-cone' '
test_when_finished "cleanup_sparse_checkout" &&
setup_sparse_checkout &&
echo "modified" >>sub/d &&
Expand All @@ -325,7 +325,7 @@ test_expect_failure 'move dirty path from in-cone to out-of-cone' '
grep -x "H folder1/d" actual
'

test_expect_failure 'move dir from in-cone to out-of-cone' '
test_expect_success 'move dir from in-cone to out-of-cone' '
test_when_finished "cleanup_sparse_checkout" &&
setup_sparse_checkout &&

Expand All @@ -339,7 +339,7 @@ test_expect_failure 'move dir from in-cone to out-of-cone' '
grep -x "S folder1/dir/e" actual
'

test_expect_failure 'move partially-dirty dir from in-cone to out-of-cone' '
test_expect_success 'move partially-dirty dir from in-cone to out-of-cone' '
test_when_finished "cleanup_sparse_checkout" &&
setup_sparse_checkout &&
touch sub/dir/e2 sub/dir/e3 &&
Expand Down