upload-pack: fix how ALLOW_ANY_SHA1 flag is disabled

PATCH In review Piotr Szlazak via GitGitGadget 4 participants 14 messages Oct 16, 2024 – Oct 21, 2024 Documentation

lore

Share & tools

lore

Piotr Szlazak via GitGitGadget

author

2024-10-16 21:06 UTC #0 lore raw

PATCH 1/1 upload-pack: fix how ALLOW_ANY_SHA1 flag is disabled

From: Piotr Szlazak <[email protected]>

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 <[email protected]>

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

upload-pack.c +2 -1

index 6d6e0f9f980..cf99b228719 100644
@@ -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

Signature · 1 line

gitgitgadget

Taylor Blau

2024-10-16 21:18 UTC #1 lore raw

Quoted 34 lines · Piotr Szlazak via GitGitGadget

From: Piotr Szlazak <[email protected]>
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 <[email protected]>
---
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(-)
upload-pack.c +1 -0
index 6d6e0f9f980..cf99b228719 100644
@@ -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
 };
 

upload-pack.c static int upload_pack_config(const char *var, const char *value, · line 1372

Quoted 5 lines

upload-pack.c +1 -1

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

2024-10-17 02:37 UTC #2 lore raw

Quoted 21 lines · Taylor Blau

Quoted 13 lines

upload-pack.c +1 -0

index 6d6e0f9f980..cf99b228719 100644
@@ -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
 };

upload-pack.c static int upload_pack_config(const char *var, const char *value, · line 1372

Quoted 5 lines

upload-pack.c +1 -1

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

Quoted 7 lines

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:

Quoted 10 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

2024-10-17 15:23 UTC #3 lore raw

Quoted 20 lines · Jeff King

Quoted 3 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:
Quoted 10 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.

Yeah, I think that you and I are in agreement here.

Quoted 9 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.

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

Thanks,
Taylor

Piotr Szlazak

2024-10-17 15:59 UTC #4 lore raw

Quoted 4 lines · Taylor Blau

Quoted 3 lines · Jeff King
Quoted 3 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));

Much better! :-)

Quoted 9 lines

Quoted 9 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:
Quoted 3 lines
Stepping back a moment, I suppose this is handling the case where a user writes:

Quoted 18 lines

Quoted 7 lines
Quoted 7 lines
[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 9 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.

Rather not as config file is parsed only once:

https://github.com/git/git/blob/15030f9556f545b167b1879b877a5d780252dc16/upload-pack.c#L1368

Quoted 9 lines

Quoted 3 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:

Quoted 1 line

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

2024-10-17 18:46 UTC #5 lore raw

Quoted 32 lines · Piotr Szlazak

Quoted 4 lines · Taylor Blau
Quoted 3 lines · Jeff King
Quoted 3 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));
Much better! :-)
Quoted 24 lines
Quoted 4 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.
Quoted 12 lines
So this case:
Quoted 10 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.
Yeah, I think that you and I are in agreement here.
Quoted 6 lines
is the one that Piotr is thinking about. But what about:
[uploadpack]
allowAnySHA1InWant = true
allowAnySHA1InWant = false

Quoted 7 lines

Quoted 3 lines
Quoted 3 lines
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.

Quoted 3 lines

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

2024-10-18 04:33 UTC #6 lore raw

Quoted 10 lines · Taylor Blau

Quoted 3 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.

Quoted 3 lines

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

2024-10-18 21:46 UTC #7 lore raw

Quoted 12 lines · Jeff King

Quoted 10 lines · Taylor Blau
Quoted 3 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.

Quoted 7 lines

Quoted 3 lines
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 :-<.

Quoted 19 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:

Quoted 6 lines

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

Quoted 3 lines

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

author

2024-10-19 16:40 UTC #8 lore raw

PATCH v2 1/1 doc: document how uploadpack.allowAnySHA1InWant impact other allow options

In reply to Piotr Szlazak via GitGitGadget

From: Piotr Szlazak <[email protected]>

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

Signed-off-by Piotr Szlazak <[email protected]>

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

Documentation/config/uploadpack.txt +5 -1

index 16264d82a72..0e1dda944a5 100644
@@ -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

Signature · 1 line

gitgitgadget

Piotr Szlazak

2024-10-19 16:47 UTC #9 lore raw

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

Quoted 42 lines · Piotr Szlazak via GitGitGadget

From: Piotr Szlazak <[email protected]>
Document how setting of uploadpack.allowAnySHA1InWant influences other uploadpack options - allowTipSHA1InWant and allowReachableSHA1InWant.
Signed-off-by Piotr Szlazak <[email protected]>
---
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(-)
Documentation/config/uploadpack.txt +5 -1
index 16264d82a72..0e1dda944a5 100644
@@ -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

Quoted 1 line

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

2024-10-21 05:55 UTC #10 lore raw

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

Quoted 47 lines · Piotr Szlazak

Quoted 41 lines · Piotr Szlazak via GitGitGadget
From: Piotr Szlazak <[email protected]>
Document how setting of uploadpack.allowAnySHA1InWant influences other uploadpack options - allowTipSHA1InWant and allowReachableSHA1InWant.
Signed-off-by Piotr Szlazak <[email protected]>
---
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(-)
Documentation/config/uploadpack.txt +0 -0
index 16264d82a72..0e1dda944a5 100644
@@ -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
Quoted 1 line
base-commit: 34b6ce9b30747131b6e781ff718a45328aa887d0
PATCH v2 which updates documentation.

Quoted 7 lines

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

2024-10-21 19:03 UTC #11 lore raw

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

Quoted 7 lines · Piotr Szlazak

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

Quoted 6 lines

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

2024-10-21 19:47 UTC #12 lore raw

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

Quoted 16 lines · Jeff King

Quoted 7 lines · Piotr Szlazak
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
Quoted 6 lines
and:
[uploadpack]
        allowAnySHAInWant = false
        allowTipSHA1InWant = true
        allowReachableSHA1InWant = true

Quoted 4 lines

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

2024-10-21 19:48 UTC #13 lore raw

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

In reply to Piotr Szlazak via GitGitGadget

Quoted 10 lines · Piotr Szlazak via GitGitGadget

From: Piotr Szlazak <[email protected]>
Document how setting of uploadpack.allowAnySHA1InWant influences other uploadpack options - allowTipSHA1InWant and allowReachableSHA1InWant.
Signed-off-by Piotr Szlazak <[email protected]>
---
doc: document how uploadpack.allowAnySHA1InWant impact other allow options

Thanks, this version looks quite good to me.

Thanks,
Taylor