Re: [PATCH 3/3] bundle-uri: remove GIT_TEST_BUNDLE_URI env variable

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Dec 20, 2022, 20:41 UTC
Message-ID: <221220.86o7rxx0so.gmgdl@evledraar.gmail.com>
In-Reply-To: <de1aca07-dd03-3f46-9f56-1cc7616b573a@github.com>

On Tue, Dec 20 2022, Derrick Stolee wrote:

Show 34 quoted lines

> On 12/19/22 6:09 AM, Ævar Arnfjörð Bjarmason wrote:
>> 
>> On Mon, Dec 12 2022, Derrick Stolee via GitGitGadget wrote:
>> 
>>> From: Derrick Stolee <derrickstolee@github.com>
>>>
>>> The GIT_TEST_BUNDLE_URI environment variable is used in the t573* suite
>>> of tests that consume the bundle URIs advertised by the Git server. This
>>> variable is equivalent to setting transfer.bundleURI=true, so we can do
>>> that in these tests instead.
>> 
>> I think this is probably OK. I can't remember why I added both the env
>> variable and the setting in what became 0ef961dda05 (bundle-uri client:
>> add boolean transfer.bundleURI setting, 2022-12-05).
>> 
>> But I think this commit message really doesn't explain why it's OK to
>> remove it. In general we do have GIT_TEST_* settings that duplicate
>> config, e.g. GIT_TEST_PROTOCOL_VERSION.
>> 
>> We do so because we'd like the environment variable to override the
>> setting, or the other way around (I think depending on the GIT_TEST_*
>> variable it's either-or, it's a mess).
>
> If the variable is named GIT_TEST_* then it should be intended for
> use within tests. However, it provides _no value_ over the existing
> config option, so the tests are updated to use the config value
> instead.
>
> As mentioned, the one exception is where we don't want to uddate
> every test to use the config variable and instead want to set the
> GIT_TEST_* variable across all tests and see how it interacts with
> other tests. However, _as mentioned in the commit message_ this
> variable would not have any effect in other tests because the
> advertisement depends on other config options on the server side.

Makes sense, I think I had a bit of a mental block on it in my initial look at it. Re-reading your commit message it does make sense.

I think it makes sense to squash this in:
	
	diff --git a/transport.c b/transport.c
	index 77a61a9d7bb..efc8fa43183 100644
	--- a/transport.c
	+++ b/transport.c
	@@ -1523,7 +1523,7 @@ int transport_fetch_refs(struct transport *transport, struct ref *refs)
	 
	 int transport_get_remote_bundle_uri(struct transport *transport)
	 {
	-	int value = 0;
	+	int value;
	 	const struct transport_vtable *vtable = transport->vtable;
	 
	 	/* Check config only once. */

The reason we init'd the variable was because we were looking for this either in the environment or the config, so we needed to have a fallback in case we used the env var.

But with the logic after your fix-up we know that if we end up in the RHS of the "||" that "value" was initialized, as we returned 0 from the config API call.