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

[Victoria Dye <vdye@github.com>]

Elijah Newren wrote:
>> +       /*
>> +        * 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.

>> +        */
>> +       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.

>> +               } 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(...)`.

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

[...]

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