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;
