threads / patch / 62348

patchupload-pack: fix how ALLOW_ANY_SHA1 flag is disabled

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

## tl;dr

14 messages between Oct 16, 2024 and Oct 21, 2024. Diffs are folded; open one to read it.

replies: 13people: 3as markdown or json

Piotr Szlazak via GitGitGadget· Oct 16, 2024, 21:06 UTC · lore
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(-)
Show changes to upload-pack.c +2 −1
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
Taylor Blau· Oct 16, 2024, 21:18 UTC · re: Piotr Szlazak via GitGitGadget · lore

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

On Wed, Oct 16, 2024 at 09:06:34PM +0000, Piotr Szlazak via GitGitGadget wrote:
Show 39 quoted lines
> 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

Jeff King· Oct 17, 2024, 02:37 UTC · re: Taylor Blau · lore

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

On Wed, Oct 16, 2024 at 05:18:44PM -0400, Taylor Blau wrote:
Show 28 quoted lines
> > 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:
Show 10 quoted lines
> 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
Taylor Blau· Oct 17, 2024, 15:23 UTC · re: Jeff King · lore

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

On Wed, Oct 16, 2024 at 10:37:35PM -0400, Jeff King wrote:
Show 20 quoted lines
> > 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.
Show 9 quoted lines
> 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

Piotr Szlazak· Oct 17, 2024, 15:59 UTC · re: Taylor Blau · lore

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

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! :-)
Show 27 quoted lines
>> 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
Show 9 quoted lines
>> 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
Taylor Blau· Oct 17, 2024, 18:46 UTC · re: Piotr Szlazak · lore

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

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

Jeff King· Oct 18, 2024, 04:33 UTC · re: Taylor Blau · lore

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

On Thu, Oct 17, 2024 at 02:46:45PM -0400, Taylor Blau wrote:
Show 13 quoted lines
> > 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
Taylor Blau· Oct 18, 2024, 21:46 UTC · re: Jeff King · lore

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

On Fri, Oct 18, 2024 at 12:33:06AM -0400, Jeff King wrote:
Show 19 quoted lines
> 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 :-<.

Show 25 quoted lines
> 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

Piotr Szlazak via GitGitGadget· Oct 19, 2024, 16:39 UTC · re: Piotr Szlazak via GitGitGadget · lore

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

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(-)
Show changes to Documentation/config/uploadpack.txt +5 −1
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
Piotr Szlazak· Oct 19, 2024, 16:46 UTC · re: Piotr Szlazak via GitGitGadget · lore

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

On 19.10.2024 18:39, Piotr Szlazak via GitGitGadget wrote:
Show 43 quoted lines
> 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
Piotr Szlazak· Oct 21, 2024, 05:55 UTC · re: Piotr Szlazak · lore

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

On 19.10.2024 18:46, Piotr Szlazak wrote:
Show 61 quoted lines
>
> 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

Jeff King· Oct 21, 2024, 19:03 UTC · re: Piotr Szlazak · lore

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

On Mon, Oct 21, 2024 at 07:55:06AM +0200, Piotr Szlazak wrote:
Show 13 quoted lines
> 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
Taylor Blau· Oct 21, 2024, 19:47 UTC · re: Jeff King · lore

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

On Mon, Oct 21, 2024 at 03:03:09PM -0400, Jeff King wrote:
Show 20 quoted lines
> 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

Taylor Blau· Oct 21, 2024, 19:48 UTC · re: Piotr Szlazak via GitGitGadget · lore

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

On Sat, Oct 19, 2024 at 04:39:57PM +0000, Piotr Szlazak via GitGitGadget wrote:
Show 10 quoted lines
> 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

← back to recent threads