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

From: Taylor Blau <me@ttaylorr.com>
Date: Oct 17, 2024, 18:46 UTC
Message-ID: <ZxFblRri8Xo/lGsK@nand.local>
In-Reply-To: <a328b6d5-e604-4517-b041-59db19e1f66c@gmail.com>

On Thu, Oct 17, 2024 at 05:59:46PM +0200, Piotr Szlazak wrote:

Show 39 quoted lines

> On 17.10.2024 17:23, Taylor Blau wrote:
> > 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));
>
> Much better! :-)
>
> > > 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.
>
> Rather not as config file is parsed only once:
>
> https://github.com/git/git/blob/15030f9556f545b167b1879b877a5d780252dc16/upload-pack.c#L1368

I am not sure I follow... upload_pack_config() is invoked on every configuration line that we see. So the first time when we read "allowAnySHA1InWant = true", we take the first path through that function you linked. The second time, we take the else branch, and correspondingly unset the bits from ALLOW_ANY_SHA1.

So today that is doing the right thing (it will end with the same bits set that we started with). But under the proposed patch that behavior would change, and the lower two bits would still be set.

> So as suggested I will prepare a patch for documentation, so it will be also
> clear for others. Should it be done using same thread or new one should be
> created?

Either is fine.

Thanks, Taylor