Re: [PATCH/RFC] receive-pack: allow for hiding refs outside the namespace

From: Jeff King <peff@peff.net>
Date: Oct 28, 2015, 13:42 UTC
Message-ID: <20151028134212.GA9657@sigill.intra.peff.net>
In-Reply-To: <20151028070045.5031.43810@s-8d3a2f8b.on.site.uni-stuttgart.de>

On Wed, Oct 28, 2015 at 08:00:45AM +0100, Lukas Fleischer wrote:

Show 7 quoted lines

> My original question remains: Do we want to continue supporting things
> like transfer.hideRefs=.have (which currently magically hides all refs
> outside the current namespace)? For 100% backwards compatibility, we
> would have to. On the other hand, one could consider the current
> behavior a bug and one could argue that it is weird enough that probably
> nobody (apart from me) relies on it right now. If we decide to keep it
> anyway, I think it should be documented.

I don't think that hiding ".have" refs at that level is especially useful. I do not use namespaces, but I do use alternates extensively, and that is the original source of these ".have" refs. But filtering them at the advertisement layer is very inefficient, as it is expensive to get the list in the first place (we spawn ls-remote, which spawns upload-pack in the alternate!). So we'd want to prevent that process much earlier.

I have an unpublished patch to specially disable alternates advertisement entirely (i.e., adding a new boolean config, receive.advertiseAlternates). In my case, it is because the alternates repositories have huge numbers of refs (sometimes ranging into the gigabytes) and the performance hit on even loading that packed-refs file is too large.

I suppose that behavior _could_ be triggered by ".have" appearing in the hiderefs config, though (i.e., before accessing the alternate, check ref_is_hidden(".have")). That seems a bit too subtle to me, though.

Show 18 quoted lines

> Another patch I have in my patch queue adds support for a whitelist mode
> to hideRefs. There are several ways to implement that:
> 
> 1. Make transfer.hideRefs='' hide all refs (it currently does not). The
>    user can then whitelist refs explicitly using negative patterns
>    below that rule. This is how my current implementation works. Using
>    the empty string seemed most natural since hideRefs matches prefixes
>    and every string has the empty string as a prefix. If that seems too
>    weird, we could probably special case something like
>    transfer.hideRefs='*' instead.
> 
> 2. Detect whether hideRefs only contains negative patterns. Switch to
>    whitelist mode ("hide by default") in that case.
> 
> 3. Add another option to switch between "hide by default" and "show by
>    default".
> 
> I personally prefer the first option. Any other opinions?

I am just a bystander and would not use this myself, but I think the 1st is the least ugly. I am not sure why ignoring "refs/" does not work, though (it does not catch ".have", of course, but I think that is a feature; there are a finite set of pseudo-refs, so you can ignore those, too, if you want).

-Peff