Re: [PATCH v4 12/16] refs/packed-backend.c: ignore complicated hidden refs rules

From: Taylor Blau <me@ttaylorr.com>
Date: Jul 4, 2023, 18:22 UTC
Message-ID: <ZKRjUkN/ggLBeIUu@nand.local>
In-Reply-To: <20230703061839.GH3502534@coredump.intra.peff.net>

On Mon, Jul 03, 2023 at 02:18:39AM -0400, Jeff King wrote:

Show 51 quoted lines

> On Tue, Jun 20, 2023 at 10:22:02AM -0400, Taylor Blau wrote:
>
> > In subsequent commits, we'll teach `receive-pack` and `upload-pack` to
> > use the new jump list feature in the packed-refs iterator by ignoring
> > references which are mentioned via its respective hideRefs lists.
> >
> > However, the packed-ref jump lists cannot handle un-hiding rules (that
> > begin with '!'), or namespace comparisons (that begin with '^'). Detect
> > and avoid these cases by falling back to the normal enumeration without
> > a jump list when such patterns exist.
>
> I'm a fan of punting on such cases to keep things simple and
> incremental. But the location here seems weird to me:
>
> > diff --git a/refs/packed-backend.c b/refs/packed-backend.c
> > index 80b877e00c..2aeec5c601 100644
> > --- a/refs/packed-backend.c
> > +++ b/refs/packed-backend.c
> > @@ -1008,6 +1008,25 @@ static void populate_excluded_jump_list(struct packed_ref_iterator *iter,
> >  	if (!excluded_patterns)
> >  		return;
> >
> > +	for (pattern = excluded_patterns; *pattern; pattern++) {
> > +		/*
> > +		 * We also can't feed any excludes from hidden refs
> > +		 * config sections, since later rules may override
> > +		 * previous ones. For example, with rules "refs/foo" and
> > +		 * "!refs/foo/bar", we should show "refs/foo/bar" (and
> > +		 * everything underneath it), but the earlier exclusion
> > +		 * would cause us to skip all of "refs/foo". We likewise
> > +		 * don't implement the namespace stripping required for
> > +		 * '^' rules.
> > +		 *
> > +		 * Both are possible to do, but complicated, so avoid
> > +		 * populating the jump list at all if we see either of
> > +		 * these patterns.
> > +		 */
> > +		if (**pattern == '!' || **pattern == '^')
> > +			return;
> > +	}
> > +
>
> This is deep in the packed-refs code, but the magic of "!" and "^" are
> specific to ref_is_hidden().
>
> So if I did:
>
>   git for-each-ref --exclude='!refs/heads/foo'
>
> my understanding is that "!" would _not_ have an affect normally, but
> now it is turning off this optimization.

Yeah, that makes sense -- I agree that it is silly to have a reference with "!" at the beginning, but since it's allowed we should absolutely support it.

> Something along the lines of (you'd want a similar tweak for
> upload-pack):

Yep. Here's the extra tweak for upload-pack:

--- 8< ---
commit 5a8902731b91a8fc6900586968a79ebc6272e502
Author: Taylor Blau <me@ttaylorr.com>
Date:   Tue Jul 4 14:21:22 2023 -0400

    fixup! upload-pack.c: avoid enumerating hidden refs where possible

diff --git a/upload-pack.c b/upload-pack.c
index 3a176a7209..ef2ca36feb 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -610,6 +610,7 @@ static int allow_hidden_refs(enum allow_uor allow_uor)
 static void for_each_namespaced_ref_1(each_ref_fn fn,
 				      struct upload_pack_data *data)
 {
+	const char **excludes = NULL;
 	/*
 	 * If `data->allow_uor` allows fetching hidden refs, we need to
 	 * mark all references (including hidden ones), to check in
@@ -619,12 +620,13 @@ static void for_each_namespaced_ref_1(each_ref_fn fn,
 	 * has the OUR_REF bit set or not, so do not need to visit
 	 * hidden references.
 	 */
-	if (allow_hidden_refs(data->allow_uor))
-		for_each_namespaced_ref(NULL, fn, data);
-	else
-		for_each_namespaced_ref(data->hidden_refs.v, fn, data);
+	if (!allow_hidden_refs(data->allow_uor))
+		excludes = hidden_refs_to_excludes(&data->hidden_refs);
+
+	for_each_namespaced_ref(excludes, fn, data);
 }

+
 static int is_our_ref(struct object *o, enum allow_uor allow_uor)
 {
 	return o->flags & ((allow_hidden_refs(allow_uor) ? HIDDEN_REF : 0) | OUR_REF);
--- >8 ---

Thanks,
Taylor