On 8/27/2021 5:20 PM, Matheus Tavares Bernardino wrote:
> On Tue, Aug 24, 2021 at 6:54 PM Derrick Stolee via GitGitGadget
> <gitgitgadget@xxxxxxxxx> 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.
I will move the logic to the last possible place before "accepting"
the move, then add a comment detailing why it should be there.
>> + 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?
test_when_finished is cleaner.
>
>> + 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.
>> + 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
