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

From: Jeff King <peff@peff.net>
Date: Oct 18, 2024, 04:33 UTC
Message-ID: <20241018043306.GB2408674@coredump.intra.peff.net>
In-Reply-To: <ZxFblRri8Xo/lGsK@nand.local>

On Thu, Oct 17, 2024 at 02:46:45PM -0400, Taylor Blau wrote:

Show 13 quoted lines

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

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

-Peff