On Wed, Oct 16, 2024 at 05:18:44PM -0400, Taylor Blau wrote:
> > 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:
> 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
