On Tue, Dec 20 2022, Derrick Stolee wrote:
> 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@xxxxxxxxxx>
>>>
>>> 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.
