| Message ID | 845904853eeae1741d681a35fdd7816ea16b939a.1687270849.git.me@ttaylorr.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | refs: implement jump lists for packed backend | expand |
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. The point may be somewhat academic for "^", as it is not allowed in a refname anyway. But I don't think "!" is forbidden (as stupid as it would be to include it in this way), is it? It feels like the hiderefs code should be the one checking for these, and then feeding only non-adorned refnames to the "exclude" list (though there is no need to un-adorn them; once we see any with either form of magic, we know we cannot use this "exclude" feature at all). Something along the lines of (you'd want a similar tweak for upload-pack): diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c index 2b2faa5d18..80a6b11c90 100644 --- a/builtin/receive-pack.c +++ b/builtin/receive-pack.c @@ -339,7 +339,8 @@ static void write_head_info(void) static struct oidset seen = OIDSET_INIT; refs_for_each_fullref_in(get_main_ref_store(the_repository), "", - hidden_refs.v, show_ref_cb, &seen); + hidden_refs_to_excludes(&hidden_refs), + show_ref_cb, &seen); for_each_alternate_ref(show_one_alternate_ref, &seen); oidset_clear(&seen); if (!sent_capabilities) diff --git a/refs.c b/refs.c index 3065e514fd..213412efd4 100644 --- a/refs.c +++ b/refs.c @@ -1482,6 +1482,31 @@ int ref_is_hidden(const char *refname, const char *refname_full, return 0; } +const char **hidden_refs_to_excludes(const struct strvec *hide_refs) +{ + const char **pattern; + + for (pattern = hide_refs->v; *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 NULL; + } + return hide_refs->v; +} + const char *find_descendant_ref(const char *dirname, const struct string_list *extras, const struct string_list *skip) diff --git a/refs.h b/refs.h index 27d341d282..50c92d1f55 100644 --- a/refs.h +++ b/refs.h @@ -829,6 +829,8 @@ int parse_hide_refs_config(const char *var, const char *value, const char *, */ int ref_is_hidden(const char *, const char *, const struct strvec *); +const char **hidden_refs_to_excludes(const struct strvec *hide_refs); + /* Is this a per-worktree ref living in the refs/ namespace? */ int is_per_worktree_ref(const char *refname); diff --git a/refs/packed-backend.c b/refs/packed-backend.c index 9dd1795bf2..59c3fe9d91 100644 --- a/refs/packed-backend.c +++ b/refs/packed-backend.c @@ -1009,25 +1009,6 @@ 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; - } - for (pattern = excluded_patterns; *pattern; pattern++) { struct jump_list_entry *e;
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@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
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; + } + for (pattern = excluded_patterns; *pattern; pattern++) { struct jump_list_entry *e; diff --git a/t/t1419-exclude-refs.sh b/t/t1419-exclude-refs.sh index 5d8c86b657..f8abf75ab8 100755 --- a/t/t1419-exclude-refs.sh +++ b/t/t1419-exclude-refs.sh @@ -119,4 +119,13 @@ test_expect_success 'meta-characters are discarded' ' assert_no_jumps perf ' +test_expect_success 'complex hidden ref rules are discarded' ' + for_each_ref__exclude refs/heads refs/heads/foo "!refs/heads/foo/1" \ + >actual 2>perf && + for_each_ref >expect && + + test_cmp expect actual && + assert_no_jumps +' + test_done
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. Signed-off-by: Taylor Blau <me@ttaylorr.com> --- refs/packed-backend.c | 19 +++++++++++++++++++ t/t1419-exclude-refs.sh | 9 +++++++++ 2 files changed, 28 insertions(+)