Re: [PATCH] upload-pack: fix how ALLOW_ANY_SHA1 flag is disabled

From: Taylor Blau <me@ttaylorr.com>
Date: Oct 17, 2024, 15:23 UTC
Message-ID: <ZxEr4+sb4DfmtrKv@nand.local>
In-Reply-To: <20241017023735.GB1858436@coredump.intra.peff.net>

On Wed, Oct 16, 2024 at 10:37:35PM -0400, Jeff King wrote:

Show 20 quoted lines

> > 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.

Show 9 quoted lines

> 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