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

Elijah Newren wrote:

quoted

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

quoted

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

quoted

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

quoted

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

[...]

quoted

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

quoted

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