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

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

[Piotr Szlazak via GitGitGadget]

From: Piotr Szlazak <piotr.szlazak@gmail.com>

ALLOW_ANY_SHA1 implies ALLOW_TIP_SHA1 and ALLOW_REACHABLE_SHA1.
Yet ALLOW_TIP_SHA1 and ALLOW_REACHABLE_SHA1 flags can be enabled
independently.
If uploadpack.allowAnySHA1InWant is not enabled in config file,
other flags should not be disabled together with ALLOW_ANY_SHA1.
They should be kept enabled if they were separately enabled in
config file with they respective options.

Signed-off-by: Piotr Szlazak <piotr.szlazak@gmail.com>
---
    upload-pack: fix how ALLOW_ANY_SHA1 flag is disabled

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1814%2Fpszlazak%2Fupload-pack-allow-flags-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1814/pszlazak/upload-pack-allow-flags-v1
Pull-Request: https://github.com/git/git/pull/1814

 upload-pack.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/upload-pack.c b/upload-pack.c
index 6d6e0f9f980..cf99b228719 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -53,6 +53,7 @@ enum allow_uor {
 	/* Allow request of a sha1 if it is reachable from a ref (possibly hidden ref). */
 	ALLOW_REACHABLE_SHA1 = 0x02,
 	/* Allow request of any sha1. Implies ALLOW_TIP_SHA1 and ALLOW_REACHABLE_SHA1. */
+	/* As this flag implies other two flags, be careful when it must be disabled. */
 	ALLOW_ANY_SHA1 = 0x07
 };
 
@@ -1368,7 +1369,7 @@ static int upload_pack_config(const char *var, const char *value,
 		if (git_config_bool(var, value))
 			data->allow_uor |= ALLOW_ANY_SHA1;
 		else
-			data->allow_uor &= ~ALLOW_ANY_SHA1;
+			data->allow_uor &= ~(ALLOW_ANY_SHA1 -(ALLOW_TIP_SHA1|ALLOW_REACHABLE_SHA1));
 	} else if (!strcmp("uploadpack.keepalive", var)) {
 		data->keepalive = git_config_int(var, value, ctx->kvi);
 		if (!data->keepalive)

base-commit: ef8ce8f3d4344fd3af049c17eeba5cd20d98b69f
-- 
gitgitgadget

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

[Taylor Blau]

On Wed, Oct 16, 2024 at 09:06:34PM +0000, Piotr Szlazak via GitGitGadget wrote:
> From: Piotr Szlazak <piotr.szlazak@gmail.com>
>
> ALLOW_ANY_SHA1 implies ALLOW_TIP_SHA1 and ALLOW_REACHABLE_SHA1.
> Yet ALLOW_TIP_SHA1 and ALLOW_REACHABLE_SHA1 flags can be enabled
> independently.
> If uploadpack.allowAnySHA1InWant is not enabled in config file,
> other flags should not be disabled together with ALLOW_ANY_SHA1.
> They should be kept enabled if they were separately enabled in
> config file with they respective options.
>
> Signed-off-by: Piotr Szlazak <piotr.szlazak@gmail.com>
> ---
>     upload-pack: fix how ALLOW_ANY_SHA1 flag is disabled
>
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1814%2Fpszlazak%2Fupload-pack-allow-flags-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1814/pszlazak/upload-pack-allow-flags-v1
> Pull-Request: https://github.com/git/git/pull/1814
>
>  upload-pack.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/upload-pack.c b/upload-pack.c
> index 6d6e0f9f980..cf99b228719 100644
> --- a/upload-pack.c
> +++ b/upload-pack.c
> @@ -53,6 +53,7 @@ enum allow_uor {
>  	/* Allow request of a sha1 if it is reachable from a ref (possibly hidden ref). */
>  	ALLOW_REACHABLE_SHA1 = 0x02,
>  	/* Allow request of any sha1. Implies ALLOW_TIP_SHA1 and ALLOW_REACHABLE_SHA1. */
> +	/* As this flag implies other two flags, be careful when it must be disabled. */
>  	ALLOW_ANY_SHA1 = 0x07
>  };
>
> @@ -1368,7 +1369,7 @@ static int upload_pack_config(const char *var, const char *value,
>  		if (git_config_bool(var, value))
>  			data->allow_uor |= ALLOW_ANY_SHA1;
>  		else
> -			data->allow_uor &= ~ALLOW_ANY_SHA1;
> +			data->allow_uor &= ~(ALLOW_ANY_SHA1 -(ALLOW_TIP_SHA1|ALLOW_REACHABLE_SHA1));

Subtracting the result of a bitwise-OR feels a little odd to me.

Since ALLOW_TIP_SHA1 and ALLOW_REACHABLE_SHA1 are defined as 0x1 and
0x2, respectively, I think the end result is as you described it, but
the route to get there feels a little odd to me.

I think it would probably make more sense to write this as:

    data->allow_uor &= ~(ALLOW_ANY_SHA1 ^ (ALLOW_TIP_SHA1 | ALLOW_REACHABLE_SHA1));

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.

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.

And that is what the current code is doing, which I think is correct.

I do see that our upload-pack section of "git-config(1)" is lacking in
this area, though, as it does not indicate that
uploadPack.allowAnySHA1InWant implies the other two options. It may be
worth saying something like:

    NOTE: this option implies both uploadPack.allowTipSHA1InWant and
    uploadPack.allowReachableSHA1InWant. Setting this option to "false"
    will do the same for the implied ones.

Thanks,
Taylor

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

[Jeff King]

On Wed, Oct 16, 2024 at 05:18:44PM -0400, Taylor Blau wrote:

> > diff --git a/upload-pack.c b/upload-pack.c
> > index 6d6e0f9f980..cf99b228719 100644
> > --- a/upload-pack.c
> > +++ b/upload-pack.c
> > @@ -53,6 +53,7 @@ enum allow_uor {
> >  	/* Allow request of a sha1 if it is reachable from a ref (possibly hidden ref). */
> >  	ALLOW_REACHABLE_SHA1 = 0x02,
> >  	/* Allow request of any sha1. Implies ALLOW_TIP_SHA1 and ALLOW_REACHABLE_SHA1. */
> > +	/* As this flag implies other two flags, be careful when it must be disabled. */
> >  	ALLOW_ANY_SHA1 = 0x07
> >  };
> >
> > @@ -1368,7 +1369,7 @@ static int upload_pack_config(const char *var, const char *value,
> >  		if (git_config_bool(var, value))
> >  			data->allow_uor |= ALLOW_ANY_SHA1;
> >  		else
> > -			data->allow_uor &= ~ALLOW_ANY_SHA1;
> > +			data->allow_uor &= ~(ALLOW_ANY_SHA1 -(ALLOW_TIP_SHA1|ALLOW_REACHABLE_SHA1));
> 
> Subtracting the result of a bitwise-OR feels a little odd to me.
> 
> Since ALLOW_TIP_SHA1 and ALLOW_REACHABLE_SHA1 are defined as 0x1 and
> 0x2, respectively, I think the end result is as you described it, but
> the route to get there feels a little odd to me.
> 
> I think it would probably make more sense to write this as:
> 
>     data->allow_uor &= ~(ALLOW_ANY_SHA1 ^ (ALLOW_TIP_SHA1 | ALLOW_REACHABLE_SHA1));

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.

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.

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.

-Peff

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

[Taylor Blau]

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));
>
> 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.

That's an even clearer example of a new gotcha that would occur with
this proposed patch, IMHO. I don't think in general that successive

    $ git config core.foo true
    $ git config core.foo false

should have any user-visible effect, as the latter should nullify the
former.

> 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?

Thanks,
Taylor

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

[Piotr Szlazak]

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

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

[Taylor Blau]

On Thu, Oct 17, 2024 at 05:59:46PM +0200, Piotr Szlazak wrote:
> 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

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.

> 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?

Either is fine.

Thanks,
Taylor

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

[Jeff King]

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.

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.

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).

-Peff

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

[Taylor Blau]

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

[PATCH v2] doc: document how uploadpack.allowAnySHA1InWant impact other allow options

[Piotr Szlazak via GitGitGadget]

From: Piotr Szlazak <piotr.szlazak@gmail.com>

Document how setting of `uploadpack.allowAnySHA1InWant`
influences other `uploadpack` options - `allowTipSHA1InWant`
and `allowReachableSHA1InWant`.

Signed-off-by: Piotr Szlazak <piotr.szlazak@gmail.com>
---
    doc: document how uploadpack.allowAnySHA1InWant impact other allow
    options

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1814%2Fpszlazak%2Fupload-pack-allow-flags-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1814/pszlazak/upload-pack-allow-flags-v2
Pull-Request: https://github.com/git/git/pull/1814

Range-diff vs v1:

 1:  8a2673bdf31 < -:  ----------- upload-pack: fix how ALLOW_ANY_SHA1 flag is disabled
 -:  ----------- > 1:  2a9fa4dabba doc: document how uploadpack.allowAnySHA1InWant impact other allow options


 Documentation/config/uploadpack.txt | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/Documentation/config/uploadpack.txt b/Documentation/config/uploadpack.txt
index 16264d82a72..0e1dda944a5 100644
--- a/Documentation/config/uploadpack.txt
+++ b/Documentation/config/uploadpack.txt
@@ -25,7 +25,11 @@ uploadpack.allowReachableSHA1InWant::
 uploadpack.allowAnySHA1InWant::
 	Allow `upload-pack` to accept a fetch request that asks for any
 	object at all.
-	Defaults to `false`.
+	It implies `uploadpack.allowTipSHA1InWant` and
+	`uploadpack.allowReachableSHA1InWant`. If set to `true` it will
+	enable both of them, it set to `false` it will disable both of
+	them.
+	By default not set.
 
 uploadpack.keepAlive::
 	When `upload-pack` has started `pack-objects`, there may be a

base-commit: 34b6ce9b30747131b6e781ff718a45328aa887d0
-- 
gitgitgadget

Re: [PATCH v2] doc: document how uploadpack.allowAnySHA1InWant impact other allow options

[Piotr Szlazak]

On 19.10.2024 18:39, Piotr Szlazak via GitGitGadget wrote:
> From: Piotr Szlazak <piotr.szlazak@gmail.com>
>
> Document how setting of `uploadpack.allowAnySHA1InWant`
> influences other `uploadpack` options - `allowTipSHA1InWant`
> and `allowReachableSHA1InWant`.
>
> Signed-off-by: Piotr Szlazak <piotr.szlazak@gmail.com>
> ---
>      doc: document how uploadpack.allowAnySHA1InWant impact other allow
>      options
>
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1814%2Fpszlazak%2Fupload-pack-allow-flags-v2
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1814/pszlazak/upload-pack-allow-flags-v2
> Pull-Request: https://github.com/git/git/pull/1814
>
> Range-diff vs v1:
>
>   1:  8a2673bdf31 < -:  ----------- upload-pack: fix how ALLOW_ANY_SHA1 flag is disabled
>   -:  ----------- > 1:  2a9fa4dabba doc: document how uploadpack.allowAnySHA1InWant impact other allow options
>
>
>   Documentation/config/uploadpack.txt | 6 +++++-
>   1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/config/uploadpack.txt b/Documentation/config/uploadpack.txt
> index 16264d82a72..0e1dda944a5 100644
> --- a/Documentation/config/uploadpack.txt
> +++ b/Documentation/config/uploadpack.txt
> @@ -25,7 +25,11 @@ uploadpack.allowReachableSHA1InWant::
>   uploadpack.allowAnySHA1InWant::
>   	Allow `upload-pack` to accept a fetch request that asks for any
>   	object at all.
> -	Defaults to `false`.
> +	It implies `uploadpack.allowTipSHA1InWant` and
> +	`uploadpack.allowReachableSHA1InWant`. If set to `true` it will
> +	enable both of them, it set to `false` it will disable both of
> +	them.
> +	By default not set.
>   
>   uploadpack.keepAlive::
>   	When `upload-pack` has started `pack-objects`, there may be a
>
> base-commit: 34b6ce9b30747131b6e781ff718a45328aa887d0

PATCH v2 which updates documentation.

I wrote 'By default not set', as definitely 'Defaults to `false`' in not 
true.

Only when `uploadpack.allowAnySHA1InWant` set to `true` or `false` it 
will affect reported capabilities.

Regards,

Piotr

Re: [PATCH v2] doc: document how uploadpack.allowAnySHA1InWant impact other allow options

[Piotr Szlazak]

On 19.10.2024 18:46, Piotr Szlazak wrote:
>
> On 19.10.2024 18:39, Piotr Szlazak via GitGitGadget wrote:
>> From: Piotr Szlazak <piotr.szlazak@gmail.com>
>>
>> Document how setting of `uploadpack.allowAnySHA1InWant`
>> influences other `uploadpack` options - `allowTipSHA1InWant`
>> and `allowReachableSHA1InWant`.
>>
>> Signed-off-by: Piotr Szlazak <piotr.szlazak@gmail.com>
>> ---
>>      doc: document how uploadpack.allowAnySHA1InWant impact other allow
>>      options
>>
>> Published-As: 
>> https://github.com/gitgitgadget/git/releases/tag/pr-git-1814%2Fpszlazak%2Fupload-pack-allow-flags-v2
>> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git 
>> pr-git-1814/pszlazak/upload-pack-allow-flags-v2
>> Pull-Request: https://github.com/git/git/pull/1814
>>
>> Range-diff vs v1:
>>
>>   1:  8a2673bdf31 < -:  ----------- upload-pack: fix how 
>> ALLOW_ANY_SHA1 flag is disabled
>>   -:  ----------- > 1:  2a9fa4dabba doc: document how 
>> uploadpack.allowAnySHA1InWant impact other allow options
>>
>>
>>   Documentation/config/uploadpack.txt | 6 +++++-
>>   1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/config/uploadpack.txt 
>> b/Documentation/config/uploadpack.txt
>> index 16264d82a72..0e1dda944a5 100644
>> --- a/Documentation/config/uploadpack.txt
>> +++ b/Documentation/config/uploadpack.txt
>> @@ -25,7 +25,11 @@ uploadpack.allowReachableSHA1InWant::
>>   uploadpack.allowAnySHA1InWant::
>>       Allow `upload-pack` to accept a fetch request that asks for any
>>       object at all.
>> -    Defaults to `false`.
>> +    It implies `uploadpack.allowTipSHA1InWant` and
>> +    `uploadpack.allowReachableSHA1InWant`. If set to `true` it will
>> +    enable both of them, it set to `false` it will disable both of
>> +    them.
>> +    By default not set.
>>     uploadpack.keepAlive::
>>       When `upload-pack` has started `pack-objects`, there may be a
>>
>> base-commit: 34b6ce9b30747131b6e781ff718a45328aa887d0
>
> PATCH v2 which updates documentation.
>
> I wrote 'By default not set', as definitely 'Defaults to `false`' in 
> not true.
>
> Only when `uploadpack.allowAnySHA1InWant` set to `true` or `false` it 
> will affect reported capabilities.
>
> Regards,
>
> Piotr

On the second look code changes will be needed, as at the moment final 
result will differ between:
[uploadpack]
         allowTipSHA1InWant = true
         allowReachableSHA1InWant = true
         allowAnySHAInWant = false

and:

[uploadpack]
         allowAnySHAInWant = false
         allowTipSHA1InWant = true
         allowReachableSHA1InWant = true

Regards,
Piotr

Re: [PATCH v2] doc: document how uploadpack.allowAnySHA1InWant impact other allow options

[Jeff King]

On Mon, Oct 21, 2024 at 07:55:06AM +0200, Piotr Szlazak wrote:

> On the second look code changes will be needed, as at the moment final
> result will differ between:
> [uploadpack]
>         allowTipSHA1InWant = true
>         allowReachableSHA1InWant = true
>         allowAnySHAInWant = false
> 
> and:
> 
> [uploadpack]
>         allowAnySHAInWant = false
>         allowTipSHA1InWant = true
>         allowReachableSHA1InWant = true

I'd expect them to differ. The config is read in order with a "last one
wins" rule. The thing that the documentation should be making clear is
that the three overlap in what they are affecting, and so "last one" is
not just a single key, but these three inter-related keys.

-Peff

Re: [PATCH v2] doc: document how uploadpack.allowAnySHA1InWant impact other allow options

[Taylor Blau]

On Mon, Oct 21, 2024 at 03:03:09PM -0400, Jeff King wrote:
> On Mon, Oct 21, 2024 at 07:55:06AM +0200, Piotr Szlazak wrote:
>
> > On the second look code changes will be needed, as at the moment final
> > result will differ between:
> > [uploadpack]
> >         allowTipSHA1InWant = true
> >         allowReachableSHA1InWant = true
> >         allowAnySHAInWant = false
> >
> > and:
> >
> > [uploadpack]
> >         allowAnySHAInWant = false
> >         allowTipSHA1InWant = true
> >         allowReachableSHA1InWant = true
>
> I'd expect them to differ. The config is read in order with a "last one
> wins" rule. The thing that the documentation should be making clear is
> that the three overlap in what they are affecting, and so "last one" is
> not just a single key, but these three inter-related keys.

Agreed. I think the "last one wins" policy is well understood throughout
Git's rules for how configuration layers are parsed, and not worth
repeating purely for the purposes of this specific piece of the
documentation.

Thanks,
Taylor

Re: [PATCH v2] doc: document how uploadpack.allowAnySHA1InWant impact other allow options

[Taylor Blau]

On Sat, Oct 19, 2024 at 04:39:57PM +0000, Piotr Szlazak via GitGitGadget wrote:
> From: Piotr Szlazak <piotr.szlazak@gmail.com>
>
> Document how setting of `uploadpack.allowAnySHA1InWant`
> influences other `uploadpack` options - `allowTipSHA1InWant`
> and `allowReachableSHA1InWant`.
>
> Signed-off-by: Piotr Szlazak <piotr.szlazak@gmail.com>
> ---
>     doc: document how uploadpack.allowAnySHA1InWant impact other allow
>     options

Thanks, this version looks quite good to me.

Thanks,
Taylor