On Mon, Sep 16, 2024 at 10:50:03AM +0200, Patrick Steinhardt wrote:
> This only surfaces when:
>
> - A repository uses reference namespaces.
>
> - "transfer.hideRefs" is active.
>
> - The namespaced references are packed into the "packed-refs" file.
>
> None of our tests exercise this scenario, and thus we haven't ever hit
> it. While t5509 exercises both (1) and (2), it does not happen to hit
> (3). It is trivial to demonstrate the bug though by explicitly packing
> refs in the tests, and then we indeed surface the breakage.
>
> Fix this bug by prefixing exclude patterns with the namespace in the
> generic layer. The newly introduced function will be used outside of
> "refs.c" in the next patch, so we add a declaration to "refs.h".
Thanks for finding and fixing this bug!
> 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)
> + return exclude_patterns;
> +
> + for (size_t i = 0; exclude_patterns[i]; i++)
> + strvec_pushf(out, "%s%s", namespace, exclude_patterns[i]);
> +
> + return out->v;
> +}
> +
Is it safe to concatenate each exclude pattern with the specified
namespace? If I'm reading this correctly, I think we silently do the
wrong thing for exclude patterns that start with '^'.
I guess we reject such patterns in the hidden_refs_to_excludes()
function, but perhaps we wouldn't have to if this function stripped
those prefixes for us when the caller does or doesn't specify exclude
patterns with a '^'?
Thanks,
Taylor
