Re: [PATCH 1/6] refs: properly apply exclude patterns to namespaced refs

[Patrick Steinhardt <ps@pks.im>]

On Fri, Sep 13, 2024 at 04:35:35AM -0700, karthik nayak wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> > Handling such exclude patterns in `refs_for_each_namespaced_ref()` and
> > `refs_for_each_fullref_in_prefixes()` is broken though, as both support
> > that the user passes both namespaces and exclude patterns. In the case
> > where both are set we will exclude references with unstripped names,
> > even though we really wanted to exclude references based on their
> > stripped names.
> >
> > This only surfaces when:
> >
> >   - A repository uses reference namespaces.
> >
> >   - "transfer.hideRefs" is active.
> >
> >   - The namespaced references are packed into the "packed-refs" file.
> >
> 
> So this is because we don't even apply exclude patterns to the loose
> refs right?
> 
> To understand correctly, the transport layer passes on
> 'transfer.hideRefs' as `exclude_refs` to the generic refs layer. This is
> mostly to optimize the reference backend to skip such refs. This is used
> by the packed-refs currently but not used for loose refs.
> 
> The transfer layer also uses this list in `mark_our_ref()` to skip refs
> as needed.
> 
> So all in all `exclude_refs` here is mostly for optimization.

Yup.

> > diff --git a/refs.c b/refs.c
> > index ceb72d4bd74..b3a367ea12c 100644
> > --- a/refs.c
> > +++ b/refs.c
> > @@ -1517,6 +1517,19 @@ const char **hidden_refs_to_excludes(const struct strvec *hide_refs)
> >  	return hide_refs->v;
> >  }
> >
> > +const char **get_namespaced_exclude_patterns(const char **exclude_patterns,
> > +					     const char *namespace,
> > +					     struct strvec *out)
> > +{
> > +	if (!namespace || !*namespace || !exclude_patterns || !*exclude_patterns)
> 
> What scenario would `!*namespace` be possible?

It's the default value of `get_git_namespace()`.

> > diff --git a/refs.h b/refs.h
> > index f8b919a1388..3f774e96d18 100644
> > --- a/refs.h
> > +++ b/refs.h
> > @@ -859,6 +859,15 @@ int ref_is_hidden(const char *, const char *, const struct strvec *);
> >   */
> >  const char **hidden_refs_to_excludes(const struct strvec *hide_refs);
> >
> > +/*
> > + * Prefix all exclude patterns with the namespace, if any. This is required
> > + * because exclude patterns apply to the stripped reference name, not the full
> > + * reference name with the namespace.
> > + */
> > +const char **get_namespaced_exclude_patterns(const char **exclude_patterns,
> > +					     const char *namespace,
> > +					     struct strvec *out);
> > +
> 
> Do we need to expose this? Can't it be made static?

It will be used by the next patch. I'll amen the commit message to point
this out.

Patrick