On Wed, Oct 16, 2024 at 10:37:35PM -0400, Jeff King wrote:
> > 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.
Yeah, I think that you and I are in agreement here.
> 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.
That's an even clearer example of a new gotcha that would occur with
this proposed patch, IMHO. I don't think in general that successive
$ git config core.foo true
$ git config core.foo false
should have any user-visible effect, as the latter should nullify the
former.
> 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.
That would be a useful direction, I think. Double checking
git-config(1), there is in deed no mention of allowAnySHA1InWant
implying the other two options, which seems like a gap that would be
good to address.
Piotr: what do you think?
Thanks,
Taylor
