Re: [PATCH 11/13] mv: refuse to move sparse paths

On 8/27/2021 5:20 PM, Matheus Tavares Bernardino wrote:

On Tue, Aug 24, 2021 at 6:54 PM Derrick Stolee via GitGitGadget
[off-list ref] wrote:

quoted

diff --git a/builtin/mv.c b/builtin/mv.c
index c2f96c8e895..b58fd4ce5ba 100644
--- a/builtin/mv.c
+++ b/builtin/mv.c

@@ -176,10 +177,22 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
                const char *src = source[i], *dst = destination[i];
                int length, src_is_dir;
                const char *bad = NULL;
+               int skip_sparse = 0;

                if (show_only)
                        printf(_("Checking rename of '%s' to '%s'\n"), src, dst);

+               if (!path_in_sparse_checkout(src, &the_index)) {

`git mv` can only move/rename tracked paths, but since we check
whether `src` is sparse before checking if it is in the index, the
user will get the sparse error message instead. This is OK, but the
advice might be misleading, as it says they can use `--sparse` if they
really want to move the file, but repeating the command with
`--sparse` will now fail for another reason. I wonder if we should
check whether `src` is tracked before checking if it is sparse, or if
that is not really an issue we should bother with.

I will move the logic to the last possible place before "accepting"
the move, then add a comment detailing why it should be there.

quoted

+                       string_list_append(&only_match_skip_worktree, src);
+                       skip_sparse = 1;
+               }
+               if (!path_in_sparse_checkout(dst, &the_index)) {
+                       string_list_append(&only_match_skip_worktree, dst);
+                       skip_sparse = 1;
+               }
+               if (skip_sparse)
+                       continue;
+

...

quoted

+test_expect_success 'mv refuses to move sparse-to-sparse' '
+       rm -f e &&

At first glance, it confused me a bit that we are removing `e` when
the setup didn't create it. But then I realized the test itself might
create `e` if `git mv` succeeds in moving the `b` file. Could perhaps
this and the others `rm -f e` be a `test_when_finished`, to make it
clearer that it is a cleanup?

test_when_finished is cleaner.

quoted

+       git reset --hard &&
+       git sparse-checkout set a &&
+       touch b &&
+       test_must_fail git mv b e 2>stderr &&

Here we try to move a "tracked sparse path" to an "untracked sparse
path". Do we also want to test with a tracked to tracked operation?
(Although the code path will be the same, of course.)

I can expand these tests to include tracked and untracked targets.

quoted

+       cat sparse_error_header >expect &&
+       echo b >>expect &&
+       echo e >>expect &&
+       cat sparse_hint >>expect &&
+       test_cmp expect stderr
+'
+
+test_expect_success 'mv refuses to move sparse-to-sparse, ignores failure' '
+       rm -f e &&
+       git reset --hard &&
+       git sparse-checkout set a &&
+       touch b &&
+       git mv -k b e 2>stderr &&

Maybe also check that `b` is still there, and `e` is missing?

Good idea.

In fact, there is a problem that the '-k' gets around the
protections because it doesn't return 1 early. I'll fix this
by jumping to the end of the loop which removes the entries
from the arrays.

Thanks,
-Stolee