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
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 all commits
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

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
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 */
64 changes: 53 additions & 11 deletions builtin/mv.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ static const char * const builtin_mv_usage[] = {
};

enum update_mode {
BOTH = 0,
DEFAULT = 0,
Copy link

Choose a reason for hiding this comment

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

Rather than rename this, why not remove it entirely? The elements of this enum are being treated as flags, but 0 isn't really a "flag" value - you don't |= it to "apply" it (it's not even really "applied"), and it's mutually exclusive with all the other flag values.

Since it's (as far as I can tell?) not used explicitly anywhere, I think it's more confusing than not to keep it.

WORKING_DIRECTORY = (1 << 1),
INDEX = (1 << 2),
SPARSE = (1 << 3),
Expand Down Expand Up @@ -168,12 +168,13 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
OPT_END(),
};
const char **source, **destination, **dest_path, **submodule_gitfile;
enum update_mode *modes;
enum update_mode *modes, dst_mode = 0;
struct stat st;
struct string_list src_for_dst = STRING_LIST_INIT_NODUP;
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 @@ -207,9 +208,15 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
dest_path[0] = add_slash(dest_path[0]);
destination = internal_prefix_pathspec(dest_path[0], argv, argc, DUP_BASENAME);
} else {
if (argc != 1)
if (!check_dir_in_index(dest_path[0])) {
Copy link

Choose a reason for hiding this comment

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

The commit message mentions that this should only be done if '--sparse' is provided ("Change the logic so that when supplied with --sparse...", but I don't see any validation of that flag here. Is that done implicitly as part of 'check_dir_in_index()'? If so, it would help to have a comment in this if/else block clarifying that.

dest_path[0] = add_slash(dest_path[0]);
destination = internal_prefix_pathspec(dest_path[0], argv, argc, DUP_BASENAME);
dst_mode |= SKIP_WORKTREE_DIR;
} else if (argc != 1) {
die(_("destination '%s' is not a directory"), dest_path[0]);
destination = dest_path;
} else {
destination = dest_path;
}
}

/* Checking */
Expand Down Expand Up @@ -398,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 @@ -408,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 @@ -427,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

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
Owner Author

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.


/* 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];

dst_pos = cache_name_pos(dst, strlen(dst));
active_cache[dst_pos]->ce_flags &= ~CE_SKIP_WORKTREE;
dst_ce->ce_flags &= ~CE_SKIP_WORKTREE;

if (checkout_entry(active_cache[dst_pos], &state, NULL, NULL))
die(_("cannot checkout %s"), active_cache[dst_pos]->name);
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 @@ -449,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
80 changes: 80 additions & 0 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,4 +290,84 @@ test_expect_success 'move sparse file to existing destination with --force and -
test_cmp expect sub/file1
'

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

git mv --sparse sub/d folder1 2>stderr &&
test_must_be_empty stderr &&

test_path_is_missing sub/d &&
test_path_is_missing folder1/d &&
git ls-files -t >actual &&
! grep -x "H sub/d" actual &&
grep -x "S folder1/d" actual
'

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 &&

git mv --sparse sub/d folder1 2>stderr &&
cat >expect <<-EOF &&
The following dirty paths and/or pathspecs are moved
but not sparsified. Use "git add" to stage them then
use "git sparse-checkout reapply" to sparsify them.
folder1/d
EOF
test_cmp expect stderr &&

test_path_is_missing sub/d &&
test_path_is_file folder1/d &&
git ls-files -t >actual &&
! grep -x "H sub/d" actual &&
grep -x "H folder1/d" actual
'

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

git mv --sparse sub/dir folder1 2>stderr &&
test_must_be_empty stderr &&

test_path_is_missing sub/dir &&
test_path_is_missing folder1 &&
git ls-files -t >actual &&
! grep -x "H sub/dir/e" actual &&
grep -x "S folder1/dir/e" actual
'

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 &&
git add sub/dir/e2 sub/dir/e3 &&
echo "modified" >>sub/dir/e2 &&
echo "modified" >>sub/dir/e3 &&

git mv --sparse sub/dir folder1 2>stderr &&
cat >expect <<-EOF &&
The following dirty paths and/or pathspecs are moved
but not sparsified. Use "git add" to stage them then
use "git sparse-checkout reapply" to sparsify them.
folder1/dir/e2
folder1/dir/e3
EOF
test_cmp expect stderr &&
Comment on lines +350 to +358
Copy link

@vdye vdye Jul 11, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

After this section, you might want to verify that the non-dirty contents of the folder were not moved, i.e.:

test_path_is_file sub/dir/e

EDIT: nevermind, didn't read the warning message. Instead, you can check that sub/dir/e was moved, and exists on disk:

test_path_is_empty sub/dir &&
test_path_is_empty folder1/e &&
test_path_is_file folder1/e2 &&

Copy link
Owner Author

@ffyuanda ffyuanda Jul 14, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

The logic in this in-cone to out-of-cone move is that:

  1. Clean paths are sparsified.
  2. Dirty paths are moved to the required (usually requires mkdir -p), and they are not sparsified.

So I think the "non-dirty contents of the folder" should actually be moved and sparsified?

Copy link

Choose a reason for hiding this comment

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

Correct, I added an update after the "EDIT" in my previous comment.


test_path_is_missing sub/dir &&
test_path_is_missing folder1/dir/e &&
test_path_is_file folder1/dir/e2 &&
test_path_is_file folder1/dir/e3 &&
git ls-files -t >actual &&
! grep -x "H sub/dir/e" actual &&
! grep -x "H sub/dir/e2" actual &&
! grep -x "H sub/dir/e3" actual &&
grep -x "S folder1/dir/e" actual &&
grep -x "H folder1/dir/e2" actual &&
grep -x "H folder1/dir/e3" actual
'

test_done