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

From: Matheus Tavares Bernardino <matheus.bernardino@usp.br>
Date: Aug 28, 2021, 14:18 UTC
Message-ID: <CAHd-oW4ANhC4QF9qWc==3BR1uMWjw-Qb4nNCYdd3s8shWwwT=w@mail.gmail.com>
In-Reply-To: <f6c0d4e3a0695c331b8216103bb46bfb4a461d1e.1629842085.git.gitgitgadget@gmail.com>

On Tue, Aug 24, 2021 at 6:54 PM Derrick Stolee via GitGitGadget <gitgitgadget@gmail.com> wrote:

Show 18 quoted lines

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

Show 33 quoted lines

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

Show 16 quoted lines

> +               } 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.

Show 9 quoted lines

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

Show 26 quoted lines

>                         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`.

Show 15 quoted lines

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