[12/13] mv: add '--sparse' option to ignore sparse-checkout

Commit Message

Derrick Stolee Aug. 24, 2021, 9:54 p.m. UTC

From: Derrick Stolee <[e-mail]>

Users can get into strange situations if 'git mv' allows moving files
into, out of, or around the sparse-checkout cone. However, some users
may still want to do it. Allow knowledgeable users to do so via a new
'--sparse' option.

There are some special cases that occur in this change, such as the case
of a directory that doesn't match the sparse-checkout cone, but exists
in the working tree because a subset of its contents do match. We need
to communicate that index entries with the SKIP_WORKTREE bit are not
expected to be in the working directory and hence are not needed when
moving the contents. This is only a check for the existence of the
source file. The call to rename_cache_entry_at() still changes the index
appropriately in these cases.

Signed-off-by: Derrick Stolee <[e-mail]>
---
 builtin/mv.c                  | 24 ++++++++++++++----------
 t/t7002-mv-sparse-checkout.sh | 34 ++++++++++++++++++++++++++++------
 2 files changed, 42 insertions(+), 16 deletions(-)

Comments

Matheus Tavares Aug. 28, 2021, 2:18 p.m. UTC | #1

On Tue, Aug 24, 2021 at 6:54 PM Derrick Stolee via GitGitGadget
<[e-mail]> wrote:
>
> diff --git a/builtin/mv.c b/builtin/mv.c
> index b58fd4ce5ba..92ea9f0ca37 100644
> --- a/builtin/mv.c
> +++ b/builtin/mv.c
> @@ -118,17 +118,18 @@ static int index_range_of_same_dir(const char *src, int length,
>  int cmd_mv(int argc, const char **argv, const char *prefix)
>  {
>         int i, flags, gitmodules_modified = 0;
> -       int verbose = 0, show_only = 0, force = 0, ignore_errors = 0;
> +       int verbose = 0, show_only = 0, force = 0, ignore_errors = 0, ignore_sparse = 0;
>         struct option builtin_mv_options[] = {
>                 OPT__VERBOSE(&verbose, N_("be verbose")),
>                 OPT__DRY_RUN(&show_only, N_("dry run")),
>                 OPT__FORCE(&force, N_("force move/rename even if target exists"),
>                            PARSE_OPT_NOCOMPLETE),
>                 OPT_BOOL('k', NULL, &ignore_errors, N_("skip move/rename errors")),
> +               OPT_BOOL(0, "sparse", &ignore_sparse, N_("allow updating entries outside of the sparse-checkout cone")),

Should this include a doc update too?

>                 OPT_END(),
>         };
>         const char **source, **destination, **dest_path, **submodule_gitfile;
> -       enum update_mode { BOTH = 0, WORKING_DIRECTORY, INDEX } *modes;
> +       enum update_mode { BOTH = 0, WORKING_DIRECTORY, INDEX, SPARSE } *modes;
>         struct stat st;
>         struct string_list src_for_dst = STRING_LIST_INIT_NODUP;
>         struct lock_file lock_file = LOCK_INIT;
> @@ -182,11 +183,11 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
>                 if (show_only)
>                         printf(_("Checking rename of '%s' to '%s'\n"), src, dst);
>
> -               if (!path_in_sparse_checkout(src, &the_index)) {
> +               if (!ignore_sparse && !path_in_sparse_checkout(src, &the_index)) {
>                         string_list_append(&only_match_skip_worktree, src);
>                         skip_sparse = 1;
>                 }
> -               if (!path_in_sparse_checkout(dst, &the_index)) {
> +               if (!ignore_sparse && !path_in_sparse_checkout(dst, &the_index)) {
>                         string_list_append(&only_match_skip_worktree, dst);
>                         skip_sparse = 1;
>                 }
> @@ -194,9 +195,11 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
>                         continue;
>
>                 length = strlen(src);
> -               if (lstat(src, &st) < 0)
> -                       bad = _("bad source");
> -               else if (!strncmp(src, dst, length) &&
> +               if (lstat(src, &st) < 0) {
> +                       /* only error if existence is expected. */
> +                       if (modes[i] != SPARSE)
> +                               bad = _("bad source");

OK, so this is about the directories which contain sparse entries in
it, right? In this case, we don't expect such entries to be in the
working tree, so we don't error out if they are missing and still let
the parent directory be moved.

This made me wonder about a slightly different case: would it be
interesting to also allow `git mv --sparse` to rename a single sparse
entry even if it's not in the working tree? I mean, something like:

echo a >a
echo b >b
git add a b
git commit -m files
git sparse-checkout set a
git mv --sparse b c

This currently wouldn't be allowed because "b" is not in the working
tree ("fatal: bad source" error). But, perhaps, would it be
interesting to allow the index to be updated anyway?

> +               } else if (!strncmp(src, dst, length) &&
>                                 (dst[length] == 0 || dst[length] == '/')) {
>                         bad = _("can not move directory into itself");
>                 } else if ((src_is_dir = S_ISDIR(st.st_mode))
> @@ -225,11 +228,12 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
>                                 dst_len = strlen(dst);
>
>                                 for (j = 0; j < last - first; j++) {
> -                                       const char *path = active_cache[first + j]->name;
> +                                       const struct cache_entry *ce = active_cache[first + j];
> +                                       const char *path = ce->name;
>                                         source[argc + j] = path;
>                                         destination[argc + j] =
>                                                 prefix_path(dst, dst_len, path + length + 1);
> -                                       modes[argc + j] = INDEX;
> +                                       modes[argc + j] = ce_skip_worktree(ce) ? SPARSE : INDEX;

OK, here we are assigning the SPARSE mode to sparse index entries that
are inside a directory we want to move. Later iterations of the loop
will then process these entries, see this mode, and not error out if
the files are missing.

>                                         submodule_gitfile[argc + j] = NULL;
>                                 }
>                                 argc += last - first;
> @@ -293,7 +297,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
>                         printf(_("Renaming %s to %s\n"), src, dst);
>                 if (show_only)
>                         continue;
> -               if (mode != INDEX && rename(src, dst) < 0) {
> +               if (mode != INDEX && mode != SPARSE && rename(src, dst) < 0) {

And finally, we don't want to rename the SPARSE working tree file (if
it exists) because its parent directory was already moved.

>                         if (ignore_errors)
>                                 continue;
>                         die_errno(_("renaming '%s' failed"), src);
> diff --git a/t/t7002-mv-sparse-checkout.sh b/t/t7002-mv-sparse-checkout.sh
> index 5397c6d07bd..517fd587fa8 100755
> --- a/t/t7002-mv-sparse-checkout.sh
> +++ b/t/t7002-mv-sparse-checkout.sh
> @@ -31,7 +31,9 @@ test_expect_success 'mv refuses to move sparse-to-sparse' '
>         echo b >>expect &&
>         echo e >>expect &&
>         cat sparse_hint >>expect &&
> -       test_cmp expect stderr
> +       test_cmp expect stderr &&
> +       git mv --sparse b e 2>stderr &&
> +       test_must_be_empty stderr
>  '
>
>  test_expect_success 'mv refuses to move sparse-to-sparse, ignores failure' '
> @@ -44,7 +46,9 @@ test_expect_success 'mv refuses to move sparse-to-sparse, ignores failure' '
>         echo b >>expect &&
>         echo e >>expect &&
>         cat sparse_hint >>expect &&
> -       test_cmp expect stderr
> +       test_cmp expect stderr &&
> +       git mv --sparse -k b e 2>stderr &&
> +       test_must_be_empty stderr

Nit: Isn't this case a bit redundant considering the test before this
one? That is, with `--sparse` there should be no error for `-k` to
ignore, and in the test above it we already checked that this command
indeed succeeds with `--sparse`.

>
>  test_expect_success 'recursive mv refuses to move (possible) sparse' '
> @@ -80,7 +88,14 @@ test_expect_success 'recursive mv refuses to move (possible) sparse' '
>         echo sub >>expect &&
>         echo sub2 >>expect &&
>         cat sparse_hint >>expect &&
> -       test_cmp expect stderr
> +       test_cmp expect stderr &&
> +       git mv --sparse sub sub2 2>stderr &&
> +       test_must_be_empty stderr &&
> +       git commit -m "moved sub to sub2" &&
> +       git rev-parse HEAD~1:sub >expect &&
> +       git rev-parse HEAD:sub2 >actual &&
> +       test_cmp expect actual &&
> +       git reset --hard HEAD~1

Perhaps this could be a `test_when_finished` (maybe right after the
`git commit` invocation), so that we can restore the original state
for the next tests even if this one fails?

Patch

diff --git a/builtin/mv.c b/builtin/mv.c
index b58fd4ce5ba..92ea9f0ca37 100644
--- a/builtin/mv.c
+++ b/builtin/mv.c
@@ -118,17 +118,18 @@  static int index_range_of_same_dir(const char *src, int length,
 int cmd_mv(int argc, const char **argv, const char *prefix)
 {
 	int i, flags, gitmodules_modified = 0;
-	int verbose = 0, show_only = 0, force = 0, ignore_errors = 0;
+	int verbose = 0, show_only = 0, force = 0, ignore_errors = 0, ignore_sparse = 0;
 	struct option builtin_mv_options[] = {
 		OPT__VERBOSE(&verbose, N_("be verbose")),
 		OPT__DRY_RUN(&show_only, N_("dry run")),
 		OPT__FORCE(&force, N_("force move/rename even if target exists"),
 			   PARSE_OPT_NOCOMPLETE),
 		OPT_BOOL('k', NULL, &ignore_errors, N_("skip move/rename errors")),
+		OPT_BOOL(0, "sparse", &ignore_sparse, N_("allow updating entries outside of the sparse-checkout cone")),
 		OPT_END(),
 	};
 	const char **source, **destination, **dest_path, **submodule_gitfile;
-	enum update_mode { BOTH = 0, WORKING_DIRECTORY, INDEX } *modes;
+	enum update_mode { BOTH = 0, WORKING_DIRECTORY, INDEX, SPARSE } *modes;
 	struct stat st;
 	struct string_list src_for_dst = STRING_LIST_INIT_NODUP;
 	struct lock_file lock_file = LOCK_INIT;
@@ -182,11 +183,11 @@  int cmd_mv(int argc, const char **argv, const char *prefix)
 		if (show_only)
 			printf(_("Checking rename of '%s' to '%s'\n"), src, dst);
 
-		if (!path_in_sparse_checkout(src, &the_index)) {
+		if (!ignore_sparse && !path_in_sparse_checkout(src, &the_index)) {
 			string_list_append(&only_match_skip_worktree, src);
 			skip_sparse = 1;
 		}
-		if (!path_in_sparse_checkout(dst, &the_index)) {
+		if (!ignore_sparse && !path_in_sparse_checkout(dst, &the_index)) {
 			string_list_append(&only_match_skip_worktree, dst);
 			skip_sparse = 1;
 		}
@@ -194,9 +195,11 @@  int cmd_mv(int argc, const char **argv, const char *prefix)
 			continue;
 
 		length = strlen(src);
-		if (lstat(src, &st) < 0)
-			bad = _("bad source");
-		else if (!strncmp(src, dst, length) &&
+		if (lstat(src, &st) < 0) {
+			/* only error if existence is expected. */
+			if (modes[i] != SPARSE)
+				bad = _("bad source");
+		} else if (!strncmp(src, dst, length) &&
 				(dst[length] == 0 || dst[length] == '/')) {
 			bad = _("can not move directory into itself");
 		} else if ((src_is_dir = S_ISDIR(st.st_mode))
@@ -225,11 +228,12 @@  int cmd_mv(int argc, const char **argv, const char *prefix)
 				dst_len = strlen(dst);
 
 				for (j = 0; j < last - first; j++) {
-					const char *path = active_cache[first + j]->name;
+					const struct cache_entry *ce = active_cache[first + j];
+					const char *path = ce->name;
 					source[argc + j] = path;
 					destination[argc + j] =
 						prefix_path(dst, dst_len, path + length + 1);
-					modes[argc + j] = INDEX;
+					modes[argc + j] = ce_skip_worktree(ce) ? SPARSE : INDEX;
 					submodule_gitfile[argc + j] = NULL;
 				}
 				argc += last - first;
@@ -293,7 +297,7 @@  int cmd_mv(int argc, const char **argv, const char *prefix)
 			printf(_("Renaming %s to %s\n"), src, dst);
 		if (show_only)
 			continue;
-		if (mode != INDEX && rename(src, dst) < 0) {
+		if (mode != INDEX && mode != SPARSE && rename(src, dst) < 0) {
 			if (ignore_errors)
 				continue;
 			die_errno(_("renaming '%s' failed"), src);
diff --git a/t/t7002-mv-sparse-checkout.sh b/t/t7002-mv-sparse-checkout.sh
index 5397c6d07bd..517fd587fa8 100755
--- a/t/t7002-mv-sparse-checkout.sh
+++ b/t/t7002-mv-sparse-checkout.sh
@@ -31,7 +31,9 @@  test_expect_success 'mv refuses to move sparse-to-sparse' '
 	echo b >>expect &&
 	echo e >>expect &&
 	cat sparse_hint >>expect &&
-	test_cmp expect stderr
+	test_cmp expect stderr &&
+	git mv --sparse b e 2>stderr &&
+	test_must_be_empty stderr
 '
 
 test_expect_success 'mv refuses to move sparse-to-sparse, ignores failure' '
@@ -44,7 +46,9 @@  test_expect_success 'mv refuses to move sparse-to-sparse, ignores failure' '
 	echo b >>expect &&
 	echo e >>expect &&
 	cat sparse_hint >>expect &&
-	test_cmp expect stderr
+	test_cmp expect stderr &&
+	git mv --sparse -k b e 2>stderr &&
+	test_must_be_empty stderr
 '
 
 test_expect_success 'mv refuses to move non-sparse-to-sparse' '
@@ -55,7 +59,9 @@  test_expect_success 'mv refuses to move non-sparse-to-sparse' '
 	cat sparse_error_header >expect &&
 	echo e >>expect &&
 	cat sparse_hint >>expect &&
-	test_cmp expect stderr
+	test_cmp expect stderr &&
+	git mv --sparse a e 2>stderr &&
+	test_must_be_empty stderr
 '
 
 test_expect_success 'mv refuses to move sparse-to-non-sparse' '
@@ -67,7 +73,9 @@  test_expect_success 'mv refuses to move sparse-to-non-sparse' '
 	cat sparse_error_header >expect &&
 	echo b >>expect &&
 	cat sparse_hint >>expect &&
-	test_cmp expect stderr
+	test_cmp expect stderr &&
+	git mv --sparse b e 2>stderr &&
+	test_must_be_empty stderr
 '
 
 test_expect_success 'recursive mv refuses to move (possible) sparse' '
@@ -80,7 +88,14 @@  test_expect_success 'recursive mv refuses to move (possible) sparse' '
 	echo sub >>expect &&
 	echo sub2 >>expect &&
 	cat sparse_hint >>expect &&
-	test_cmp expect stderr
+	test_cmp expect stderr &&
+	git mv --sparse sub sub2 2>stderr &&
+	test_must_be_empty stderr &&
+	git commit -m "moved sub to sub2" &&
+	git rev-parse HEAD~1:sub >expect &&
+	git rev-parse HEAD:sub2 >actual &&
+	test_cmp expect actual &&
+	git reset --hard HEAD~1
 '
 
 test_expect_success 'recursive mv refuses to move sparse' '
@@ -93,7 +108,14 @@  test_expect_success 'recursive mv refuses to move sparse' '
 	echo sub/dir2/e >>expect &&
 	echo sub2/dir2/e >>expect &&
 	cat sparse_hint >>expect &&
-	test_cmp expect stderr
+	test_cmp expect stderr &&
+	git mv --sparse sub sub2 2>stderr &&
+	test_must_be_empty stderr &&
+	git commit -m "moved sub to sub2" &&
+	git rev-parse HEAD~1:sub >expect &&
+	git rev-parse HEAD:sub2 >actual &&
+	test_cmp expect actual &&
+	git reset --hard HEAD~1
 '
 
 test_done