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

[Taylor Blau <me@ttaylorr.com>]

On Fri, Oct 18, 2024 at 12:33:06AM -0400, Jeff King wrote:
> 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.

Yeah... I was pretty sure that was the case thinking back to 6dd3456a8c
(upload-pack.c: allow banning certain object filter(s), 2020-08-03),
which somehow was already more than four years ago :-<.

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

Yup.

> So I still think we're better off to just document (and of course in the
> long run all of these become less and less interesting as they are
> v0-only options).

I agree completely. I think the consensus here seems to be that, if
anything, we should update the documentation and move on :-).

Thanks,
Taylor