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

> On Thu, Oct 17, 2024 at 02:46:45PM -0400, Taylor Blau wrote:
>
> > > 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.
>
> Reading Piotr's response I wondered if upload-pack might be using the
> configset API instead of a config callback. But no, it does look like a
> config callback.

> But it does hint at a possible path if we wanted to process these
> independently: we could read each of the config options separately,
> resolving them in the usual last-one-wins way, and then turning the
> result into a bitfield. Something like this, outside of the callback
> (totally untested):
>
>   int v;
>   unsigned bits = 0;
>
>   if (!git_config_get_bool("uploadpack.allowtipsha1inwant", &v) && v)
> 	bits |= ALLOW_TIP_SHA1;
>   if (!git_config_get_bool("uploadpack.allowreachablesha1inwant", &v) && v)
> 	bits |= ALLOW_REACHABLE_SHA1;
>   if (!git_config_get_bool("uploadpack.allowanysha1inwant", &v) && v)
> 	bits |= ALLOW_ANY_SHA1;
>
> That makes the config flags independent. But the features themselves are
> not really independent. If you do:
>
>   [uploadpack]
>   allowAnySHA1InWant = true
>   allowTipSHA1InWant = false
>
> it is still going to allow the "tip" flag, because it's a subset of the
> "any" flag. And you can't escape that.