Re: [PATCH v6 0/8] Sparse Index: integrate with reset

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Dec 1, 2021, 20:26 UTC
Message-ID: <211201.86v9089i8m.gmgdl@evledraar.gmail.com>
In-Reply-To: <9c5b7067-b0e3-0153-5cd3-042bae92f91e@github.com>

On Mon, Nov 29 2021, Victoria Dye wrote:

Show 53 quoted lines

> Elijah Newren wrote:
>> Hi,
>> 
>> On Mon, Nov 29, 2021 at 7:52 AM Victoria Dye via GitGitGadget
>> <gitgitgadget@gmail.com> wrote:
>>>
>>> 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...
>> 
>>> 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