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:

quoted

quoted

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