Re: [PATCH] upload-pack: fix how ALLOW_ANY_SHA1 flag is disabled
- From
Jeff King <peff@peff.net>
- Date
- Oct 17, 2024, 02:37 UTC
- Message-ID
- <20241017023735.GB1858436@coredump.intra.peff.net>
- In-Reply-To
- <ZxAttC1dQUllR76m@nand.local>
On Wed, Oct 16, 2024 at 05:18:44PM -0400, Taylor Blau wrote:
Show 28 quoted lines
> > diff --git a/upload-pack.c b/upload-pack.c
> > index 6d6e0f9f980..cf99b228719 100644
> > --- a/upload-pack.c
> > +++ b/upload-pack.c
> > @@ -53,6 +53,7 @@ enum allow_uor {
> > /* Allow request of a sha1 if it is reachable from a ref (possibly hidden ref). */
> > ALLOW_REACHABLE_SHA1 = 0x02,
> > /* Allow request of any sha1. Implies ALLOW_TIP_SHA1 and ALLOW_REACHABLE_SHA1. */
> > + /* As this flag implies other two flags, be careful when it must be disabled. */
> > ALLOW_ANY_SHA1 = 0x07
> > };
> >
> > @@ -1368,7 +1369,7 @@ static int upload_pack_config(const char *var, const char *value,
> > if (git_config_bool(var, value))
> > data->allow_uor |= ALLOW_ANY_SHA1;
> > else
> > - data->allow_uor &= ~ALLOW_ANY_SHA1;
> > + data->allow_uor &= ~(ALLOW_ANY_SHA1 -(ALLOW_TIP_SHA1|ALLOW_REACHABLE_SHA1));
>
> Subtracting the result of a bitwise-OR feels a little odd to me.
>
> Since ALLOW_TIP_SHA1 and ALLOW_REACHABLE_SHA1 are defined as 0x1 and
> 0x2, respectively, I think the end result is as you described it, but
> the route to get there feels a little odd to me.
>
> I think it would probably make more sense to write this as:
>
> data->allow_uor &= ~(ALLOW_ANY_SHA1 ^ (ALLOW_TIP_SHA1 | ALLOW_REACHABLE_SHA1));I think we have to treat them as a complete unit, as we don't know which bits were set by independent config lines and which were OR-ed in by ALLOW_ANY.
So this case:
Show 10 quoted lines
> Stepping back a moment, I suppose this is handling the case where a user > writes: > > [uploadpack] > allowTipSHA1InWant = true > allowReachableSHA1InWant = true > allowAnySHA1InWant = false > > and is surprised when the final "uploadPack.allowAnySHA1InWant" unsets > the previous two options.
is the one that Piotr is thinking about. But what about:
[uploadpack] allowAnySHA1InWant = true allowAnySHA1InWant = false
Right now that pair is a noop, which is what I'd expect. But after the proposed patch, it quietly enables ALLOW_TIP_SHA1 and ALLOW_REACHABLE_SHA1.
So I think the code has to stay the same, but we perhaps should document that "allow any" has the user-visible side effect of enabling/disabling the other two.
-Peff