Re: [PATCH v2 6/7] reset: make --mixed sparse-aware

From: Victoria Dye <vdye@github.com>
Date: Oct 7, 2021, 14:34 UTC
Message-ID: <8cc2ad6b-393c-9961-b911-be042bd2fb68@github.com>
In-Reply-To: <CABPp-BHDog6U1bFKurNq+sKbNPOnmqbhB8W2wg0UeF=jb+ZScw@mail.gmail.com>

Elijah Newren wrote:

Show 15 quoted lines

>> +       /*
>> +        * When pathspec is given for resetting a cone-mode sparse checkout, it may
>> +        * identify entries that are nested in sparse directories, in which case the
>> +        * index should be expanded. For the sake of efficiency, this check is
>> +        * overly-cautious: anything with a wildcard or a magic prefix requires
>> +        * expansion, as well as literal paths that aren't in the sparse checkout
>> +        * definition AND don't match any directory in the index.
> 
> s/efficiency/efficiency of checking/ ?  Being overly-cautious suggests
> you'll expand to a full index more than is needed, and full indexes
> are more expensive.  But perhaps the checking would be expensive too
> so you have a tradeoff?
> 
> Or maybe s/efficiency/simplicity/?
>

"Simplicity" is probably more appropriate, although the original intent was "efficiency of checking". I wanted to avoid repeated iteration over the index (for example, matching the `no_wildcard_len` of each wildcard pathspec item against each sparse directory in the index). However, to your point, expanding the index is a more expensive operation anyway, so it's probably worth the more involved checks.

Show 12 quoted lines

>> +        */
>> +       if (pathspec->nr && the_index.sparse_index) {
>> +               if (pathspec->magic || pathspec->has_wildcard) {
>> +                       ensure_full_index(&the_index);
> 
> dir.c has the notion of matching the characters preceding the wildcard
> characters; look for "no_wildcard_len".  If the pathspec doesn't match
> a path up to no_wildcard_len, then the wildcard character(s) later in
> the pathspec can't make the pathspec match that path.
> 
> It might at least be worth mentioning this as a possible future optimization.
>

I'll incorporate a something like this into the next version.

Show 14 quoted lines

>> +               } else {
>> +                       for (i = 0; i < pathspec->nr; i++) {
>> +                               if (!path_in_cone_mode_sparse_checkout(pathspec->items[i].original, &the_index) &&
>> +                                   !matches_skip_worktree(pathspec, i, &skip_worktree_seen)) {
> 
> What if the pathspec corresponds to a sparse-directory in the index,
> but possibly without the trailing '/' character?  e.g.:
> 
>    git reset HEAD~1 -- sparse-directory
> 
> One should be able to reset that directory without recursing into
> it...does this code handle that?  Does it handle it if we add the
> trailing slash on the path for the reset command line?
>

It handles both cases (with and without trailing slash), the former due to `!matches_skip_worktree(...)` and the latter due to `!path_in_cone_mode_sparse_checkout(...)`.

Show 30 quoted lines

>> +                                       ensure_full_index(&the_index);
>> +                                       break;
>> +                               }
>> +                       }
>> +               }
>> +       }
>> +
>> +       free(skip_worktree_seen);
>>
>> -       ensure_full_index(&the_index);
>>         if (do_diff_cache(tree_oid, &opt))
>>                 return 1;
>>         diffcore_std(&opt);
>> diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh
>> index e301ef5633a..4afcbc2d673 100755
>> --- a/t/t1092-sparse-checkout-compatibility.sh
>> +++ b/t/t1092-sparse-checkout-compatibility.sh
>> @@ -804,11 +804,22 @@ test_expect_success 'sparse-index is not expanded' '
>>                 ensure_not_expanded reset --hard $ref || return 1
>>         done &&
>>
>> +       ensure_not_expanded reset --mixed base &&
>>         ensure_not_expanded reset --hard update-deep &&
>>         ensure_not_expanded reset --keep base &&
>>         ensure_not_expanded reset --merge update-deep &&
>> -       ensure_not_expanded reset --hard &&
> 
> This commit was only touching the --mixed case; why is it removing one
> of the tests for --hard?
>

[...]

Show 11 quoted lines

>> +       ensure_not_expanded reset --hard update-folder1 &&
> 
> Wait...is update-folder1 a branch or a path?  And if this commit is
> about --mixed, why are --hard testcases being added?
> 
>> +       ensure_not_expanded reset base -- folder1 &&
>> +
>> +       ensure_not_expanded reset --hard update-deep &&
> 
> another --hard testcase...was this an accidental squash by chance?
>

I included `git reset --hard` between the "actual" test cases so that the `git reset --mixed` tests would start in a "clean" state (clear out any modified files), but it's unnecessary in most cases so I'll remove them in V3. To answer your other question, `update-folder1` is a branch.