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

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

>> +                       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;
>> +

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

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

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