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

[Matheus Tavares Bernardino <matheus.bernardino@usp.br>]

On Tue, Aug 24, 2021 at 6:54 PM Derrick Stolee via GitGitGadget
<gitgitgadget@gmail.com> 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?