On 17.10.2024 17:23, Taylor Blau wrote:
On Wed, Oct 16, 2024 at 10:37:35PM -0400, Jeff King wrote:
I think it would probably make more sense to write this as:
data->allow_uor &= ~(ALLOW_ANY_SHA1 ^ (ALLOW_TIP_SHA1 | ALLOW_REACHABLE_SHA1));
Much better! :-)
I think we have to treat them as a complete unit, as we don't know which
bits were set by independent config lines and which were OR-ed in by
ALLOW_ANY.
So this case:
Stepping back a moment, I suppose this is handling the case where a user
writes:
[uploadpack]
allowTipSHA1InWant = true
allowReachableSHA1InWant = true
allowAnySHA1InWant = false
and is surprised when the final "uploadPack.allowAnySHA1InWant" unsets
the previous two options.
Yeah, I think that you and I are in agreement here.
is the one that Piotr is thinking about. But what about:
[uploadpack]
allowAnySHA1InWant = true
allowAnySHA1InWant = false
Right now that pair is a noop, which is what I'd expect. But after the
proposed patch, it quietly enables ALLOW_TIP_SHA1 and
ALLOW_REACHABLE_SHA1.
Rather not as config file is parsed only once:
https://github.com/git/git/blob/15030f9556f545b167b1879b877a5d780252dc16/upload-pack.c#L1368
So I think the code has to stay the same, but we perhaps should document
that "allow any" has the user-visible side effect of enabling/disabling
the other two.
That would be a useful direction, I think. Double checking
git-config(1), there is in deed no mention of allowAnySHA1InWant
implying the other two options, which seems like a gap that would be
good to address.
Piotr: what do you think?
I agree. I completely missed to test it like that (which works OK):
[uploadpack]
allowTipSHA1InWant = true
allowReachableSHA1InWant = true
EOF
I was always testing with allowAnySHAInWant either set to 'true' or
'false'. But always in place.
And having it set to 'false' was disabling my previously set other
allowXyzInWant options. Which was a surprise, as I was
considering allowAnySHAInWant as a final level of openness for
client-side requests[1]. In contradiction to what Taylor expressed here:
I'm not sure that the current behavior is actually wrong. The final line
in the example above seems to indicate "do not allow any SHA-1 in the
'wants'", which would indeed imply that the other two options should be
set to false as well.
So as suggested I will prepare a patch for documentation, so it will be
also clear for others. Should it be done using same thread or new one
should be created?
[1] For example client can request not reachable objects, trees, blobs.
Regards,
Piotr
