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

[Piotr Szlazak <piotr.szlazak@gmail.com>]

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