On Mon, Jul 03, 2023 at 02:18:39AM -0400, Jeff King wrote:
> 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@xxxxxxxxxxxx>
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
