On 17.10.2024 17:23, Taylor Blau wrote:

On Wed, Oct 16, 2024 at 10:37:35PM -0400, Jeff King wrote:

quoted

quoted

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! :-)

quoted

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:

quoted

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.

quoted

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

quoted

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