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

quoted

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