On Mon, Nov 29 2021, Victoria Dye wrote:

Elijah Newren wrote:

quoted

Hi,

On Mon, Nov 29, 2021 at 7:52 AM Victoria Dye via GitGitGadget
[off-list ref] wrote:

quoted

Changes since V5
================

 * Update t1092 test 'reset with wildcard pathspec' with more cases and
   better checks
 * Add "special case" wildcard pathspec check when determining whether to
   expand the index (avoids double-loop over pathspecs & index entries)

Looks pretty good.  However, I'm worried this special case you added
at my prodding might be problematic, and that I may have been wrong to
prod you into it...

quoted

Thanks! -Victoria

Range-diff vs v5:

 7:  a9135a5ed64 ! 7:  822d7344587 reset: make --mixed sparse-aware
     @@ Commit message

          Remove the `ensure_full_index` guard on `read_from_tree` and update `git
          reset --mixed` to ensure it can use sparse directory index entries wherever
     -    possible. Sparse directory entries are reset use `diff_tree_oid`, which
     +    possible. Sparse directory entries are reset using `diff_tree_oid`, which
          requires `change` and `add_remove` functions to process the internal
          contents of the sparse directory. The `recursive` diff option handles cases
          in which `reset --mixed` must diff/merge files that are nested multiple
     @@ builtin/reset.c: static void update_index_from_diff(struct diff_queue_struct *q,
      +          * (since we can reset whole sparse directories without expanding them).
      +          */
      +         if (item.nowildcard_len < item.len) {
     ++                 /*
     ++                  * Special case: if the pattern is a path inside the cone
     ++                  * followed by only wildcards, the pattern cannot match
     ++                  * partial sparse directories, so we don't expand the index.
     ++                  */
     ++                 if (path_in_cone_mode_sparse_checkout(item.original, &the_index) &&
     ++                     strspn(item.original + item.nowildcard_len, "*") == item.len - item.nowildcard_len)

I usually expect in an &&-chain to see the cheaper function call first
(because that ordering often avoids the need to call the second
function), and I would presume that strspn() would be the cheaper of
the two.  Did you switch the order because you expect the strspn call
to nearly always return true, though?

This is a miss on my part, the `strspn()` check is probably less expensive
and should be first.

I doubt it matters either way, and I didn't look into this to any degree
of carefulness.

But having followed the breadcrumb trail from the "What's Cooking"
discussion & looked at the code one thing that stuck out for me was that
path_in_cone_mode_sparse_checkout() appears returns 1 inconditionally in
some cases based on global state:

	/*
	 * We default to accepting a path if there are no patterns or
	 * they are of the wrong type.
	 */
	if (init_sparse_checkout_patterns(istate) ||
	    (require_cone_mode &&
	     !istate->sparse_checkout_patterns->use_cone_patterns))
		return 1;

So moreso than the nano-optimization of strspn()
v.s. path_in_cone_mode_sparse_checkout() I found it a bit odd that we're
calling something in a loop where presumably we can punt out a lot
earlier, and at least make that "continue" a "break" or "return" in that
case.

I.e. something in this direction (this patch obviously doesn't even
compile, but should clarify what I'm blathering about :); but again, I
really haven't looked at this properly, so just food for thought:

diff --git a/builtin/reset.c b/builtin/reset.c
index b1ff699b43a..cefdabb09c2 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c

@@ -187,6 +187,9 @@ static int pathspec_needs_expanded_index(const struct pathspec *pathspec)
 	if (pathspec->magic)
 		return 1;
 
+	if (cant_possibly_have_path_in_cone_mode_blah_blah(&the_index))
+		return 1;
+
 	for (i = 0; i < pathspec->nr; i++) {
 		struct pathspec_item item = pathspec->items[i];
 

diff --git a/dir.c b/dir.c
index 5aa6fbad0b7..19f2d989dd3 100644
--- a/dir.c
+++ b/dir.c

@@ -1456,14 +1456,8 @@ int init_sparse_checkout_patterns(struct index_state *istate)
 	return 0;
 }
 
-static int path_in_sparse_checkout_1(const char *path,
-				     struct index_state *istate,
-				     int require_cone_mode)
+int cant_possibly_have_path_in_cone_mode_blah_blah(...)
 {
-	int dtype = DT_REG;
-	enum pattern_match_result match = UNDECIDED;
-	const char *end, *slash;
-
 	/*
 	 * We default to accepting a path if there are no patterns or
 	 * they are of the wrong type.

@@ -1472,6 +1466,16 @@ static int path_in_sparse_checkout_1(const char *path,
 	    (require_cone_mode &&
 	     !istate->sparse_checkout_patterns->use_cone_patterns))
 		return 1;
+}
+
+
+static int path_in_sparse_checkout_1(const char *path,
+				     struct index_state *istate,
+				     int require_cone_mode)
+{
+	int dtype = DT_REG;
+	enum pattern_match_result match = UNDECIDED;
+	const char *end, *slash;
 
 	/*
 	 * If UNDECIDED, use the match from the parent dir (recursively), or