Re: [PATCH 2/3] bundle-uri: advertise based on repo config

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Dec 19, 2022, 11:04 UTC
Message-ID: <221219.86pmcfzm7g.gmgdl@evledraar.gmail.com>
In-Reply-To: <857d1abec4cf124e011c7f84276ce105cb5b3a96.1670866407.git.gitgitgadget@gmail.com>

On Mon, Dec 12 2022, Derrick Stolee via GitGitGadget wrote:

Show 24 quoted lines

> From: Derrick Stolee <derrickstolee@github.com>
>
> The bundle_uri_advertise() method was not using its repository
> parameter, but this is a mistake. Use repo_config_get_maybe_bool()
> instead of git_config_maybe_bool().
>
> Signed-off-by: Derrick Stolee <derrickstolee@github.com>
> ---
>  bundle-uri.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/bundle-uri.c b/bundle-uri.c
> index 8efb4e7acad..5f158cc52e1 100644
> --- a/bundle-uri.c
> +++ b/bundle-uri.c
> @@ -610,7 +610,7 @@ int bundle_uri_advertise(struct repository *r, struct strbuf *value)
>  		goto cached;
>  
>  	advertise_bundle_uri = 0;
> -	git_config_get_maybe_bool("uploadpack.advertisebundleuris", &advertise_bundle_uri);
> +	repo_config_get_maybe_bool(r, "uploadpack.advertisebundleuris", &advertise_bundle_uri);
>  
>  cached:
>  	return advertise_bundle_uri;

This looks good, but as with another parallel topic of yours that I commented on[1] leaves us wondering if this had any effect.

I.e. is this just for good measure because we have a "r" parameter, or did we do the wrong thing for submodules before this change? In that case let's add the missing test coverage.

Or, if it's the former let's update the commit message here, saying e.g.:

	While we should use "r" for <good measure or other reason>, we
	already did the right thing for submodules, as "the_repository"
	would be set to the submodule because <reasons I don't
	know about...>.

1. https://lore.kernel.org/git/221215.865yec3b1j.gmgdl@evledraar.gmail.com/