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
