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

[Derrick Stolee <derrickstolee@github.com>]

On 12/19/22 6:04 AM, Ævar Arnfjörð Bjarmason wrote:
> 
> On Mon, Dec 12 2022, Derrick Stolee via GitGitGadget wrote:
> 
>> 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...>.

As I mentioned in another thread, we should always be using a
local 'struct repository *' if we can so we aren't contributing
to that particular dimension of technical debt, but submodules
have a lot of work ahead before these kinds of things can be
tested individually.

This is especially the case when working with server code, where
we would never be in the submodule, but we don't want to block
future removals of the non-repo versions of these methods.

Thanks,
-Stolee