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

[Taylor Blau <me@ttaylorr.com>]

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