[PATCH 0/3] Bundle URIs 4.5: fixups from part IV

[PATCH 0/3] Bundle URIs 4.5: fixups from part IV

[Derrick Stolee via GitGitGadget]

These patches represent small fixups that came in review from the last
version of 'ds/bundle-uri-4'. Since it was merged to 'next', these patches
are forward-fixes on that branch.

Note: I did not include any changes that could be solved by adding an UNUSED
macro, saving that for Peff and his already-prepared patches in that area.

Thanks, -Stolee

Derrick Stolee (3):
  bundle-uri: drop unused 'uri' parameter
  bundle-uri: advertise based on repo config
  bundle-uri: remove GIT_TEST_BUNDLE_URI env variable

 builtin/clone.c              |  1 -
 bundle-uri.c                 |  4 ++--
 bundle-uri.h                 |  8 ++++----
 t/lib-bundle-uri-protocol.sh | 12 ++++++++----
 t/t5601-clone.sh             |  8 ++++----
 transport.c                  |  5 ++---
 6 files changed, 20 insertions(+), 18 deletions(-)


base-commit: cec58f9965c845be74753aac6f49b4f177faa064
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1443%2Fderrickstolee%2Fbundle-redo%2Fadvertise-fix-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1443/derrickstolee/bundle-redo/advertise-fix-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1443
-- 
gitgitgadget

[PATCH 1/3] bundle-uri: drop unused 'uri' parameter

[Derrick Stolee via GitGitGadget]

From: Derrick Stolee <derrickstolee@github.com>

The 'uri' parameter of fetch_bundle_list() was added early in
development, but is not required since the 'list' parameter includes a
'baseURI' member. Remove the 'uri' parameter and make this expectation
explicit.

Signed-off-by: Derrick Stolee <derrickstolee@github.com>
---
 builtin/clone.c | 1 -
 bundle-uri.c    | 2 +-
 bundle-uri.h    | 8 ++++----
 3 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/builtin/clone.c b/builtin/clone.c
index 12fb527d7bb..430b2e981e3 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -1284,7 +1284,6 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 			if (repo_init(the_repository, git_dir, work_tree))
 				warning(_("failed to initialize the repo, skipping bundle URI"));
 			else if (fetch_bundle_list(the_repository,
-						   remote->url[0],
 						   transport->bundles))
 				warning(_("failed to fetch advertised bundles"));
 		} else {
diff --git a/bundle-uri.c b/bundle-uri.c
index c411b871bdd..8efb4e7acad 100644
--- a/bundle-uri.c
+++ b/bundle-uri.c
@@ -577,7 +577,7 @@ cleanup:
 	return result;
 }
 
-int fetch_bundle_list(struct repository *r, const char *uri, struct bundle_list *list)
+int fetch_bundle_list(struct repository *r, struct bundle_list *list)
 {
 	int result;
 	struct bundle_list global_list;
diff --git a/bundle-uri.h b/bundle-uri.h
index b2c9c160a52..29b0c98ee65 100644
--- a/bundle-uri.h
+++ b/bundle-uri.h
@@ -68,8 +68,8 @@ struct bundle_list {
 	 * In the case of the 'bundle-uri' protocol v2 command, the base
 	 * URI is the URI of the Git remote.
 	 *
-	 * Otherewise, the bundle list was downloaded over HTTP from some
-	 * known URI.
+	 * Otherwise, the bundle list was downloaded over HTTP from some
+	 * known URI. 'baseURI' is set to that value.
 	 *
 	 * The baseURI is used as the base for any relative URIs
 	 * advertised by the bundle list at that location.
@@ -112,10 +112,10 @@ int fetch_bundle_uri(struct repository *r, const char *uri);
  * bundle-uri protocol v2 verb) at the given uri, fetch and unbundle the
  * bundles according to the bundle strategy of that list.
  *
- * Returns non-zero if no bundle information is found at the given 'uri'.
+ * It is expected that the given 'list' is initialized, including its
+ * 'baseURI' value.
  */
 int fetch_bundle_list(struct repository *r,
-		      const char *uri,
 		      struct bundle_list *list);
 
 /**
-- 
gitgitgadget

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

[Derrick Stolee via GitGitGadget]

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;
-- 
gitgitgadget

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

[Derrick Stolee via GitGitGadget]

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.

The environment variable has a name that implies it would be useful
outside of these tests. It is not useful to set across all tests since
it would do very little without some setup on the server side. Remove
it.

Signed-off-by: Derrick Stolee <derrickstolee@github.com>
---
 t/lib-bundle-uri-protocol.sh | 12 ++++++++----
 t/t5601-clone.sh             |  8 ++++----
 transport.c                  |  5 ++---
 3 files changed, 14 insertions(+), 11 deletions(-)

diff --git a/t/lib-bundle-uri-protocol.sh b/t/lib-bundle-uri-protocol.sh
index 73e2d45bc8b..a4a1af8d029 100644
--- a/t/lib-bundle-uri-protocol.sh
+++ b/t/lib-bundle-uri-protocol.sh
@@ -88,8 +88,8 @@ test_expect_success "clone with $BUNDLE_URI_PROTOCOL:// using protocol v2: reque
 	test_when_finished "rm -rf log* cloned*" &&
 
 	GIT_TRACE_PACKET="$PWD/log" \
-	GIT_TEST_BUNDLE_URI=0 \
 	git \
+		-c transfer.bundleURI=false \
 		-c protocol.version=2 \
 		clone "$BUNDLE_URI_REPO_URI" cloned \
 		>actual 2>err &&
@@ -137,6 +137,13 @@ test_expect_success "clone with $BUNDLE_URI_PROTOCOL:// using protocol v2: reque
 	! grep "> command=bundle-uri" log3
 '
 
+# The remaining tests will all assume transfer.bundleURI=true
+#
+# This test can be removed when transfer.bundleURI is enabled by default.
+test_expect_success 'enable transfer.bundleURI for remaining tests' '
+	git config --global transfer.bundleURI true
+'
+
 test_expect_success "test bundle-uri with $BUNDLE_URI_PROTOCOL:// using protocol v2" '
 	test_config -C "$BUNDLE_URI_PARENT" \
 		bundle.only.uri "$BUNDLE_URI_BUNDLE_URI_ESCAPED" &&
@@ -150,7 +157,6 @@ test_expect_success "test bundle-uri with $BUNDLE_URI_PROTOCOL:// using protocol
 		uri = $BUNDLE_URI_BUNDLE_URI_ESCAPED
 	EOF
 
-	GIT_TEST_BUNDLE_URI=1 \
 	test-tool bundle-uri \
 		ls-remote \
 		"$BUNDLE_URI_REPO_URI" \
@@ -174,7 +180,6 @@ test_expect_success "test bundle-uri with $BUNDLE_URI_PROTOCOL:// using protocol
 		uri = $BUNDLE_URI_BUNDLE_URI_ESCAPED
 	EOF
 
-	GIT_TEST_BUNDLE_URI=1 \
 	test-tool bundle-uri \
 		ls-remote \
 		"$BUNDLE_URI_REPO_URI" \
@@ -203,7 +208,6 @@ test_expect_success "test bundle-uri with $BUNDLE_URI_PROTOCOL:// using protocol
 		uri = $BUNDLE_URI_BUNDLE_URI_ESCAPED-3.bdl
 	EOF
 
-	GIT_TEST_BUNDLE_URI=1 \
 	test-tool bundle-uri \
 		ls-remote \
 		"$BUNDLE_URI_REPO_URI" \
diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh
index a08a691ec4c..1928ea1dd7c 100755
--- a/t/t5601-clone.sh
+++ b/t/t5601-clone.sh
@@ -786,9 +786,9 @@ test_expect_success 'auto-discover bundle URI from HTTP clone' '
 	git -C "$HTTPD_DOCUMENT_ROOT_PATH/repo2.git" config \
 		bundle.everything.uri "$HTTPD_URL/everything.bundle" &&
 
-	GIT_TEST_BUNDLE_URI=1 \
 	GIT_TRACE2_EVENT="$(pwd)/trace.txt" \
-		git -c protocol.version=2 clone \
+		git -c protocol.version=2 \
+		    -c transfer.bundleURI=true clone \
 		$HTTPD_URL/smart/repo2.git repo2 &&
 	cat >pattern <<-EOF &&
 	"event":"child_start".*"argv":\["git-remote-https","$HTTPD_URL/everything.bundle"\]
@@ -815,9 +815,9 @@ test_expect_success 'auto-discover multiple bundles from HTTP clone' '
 	git -C "$HTTPD_DOCUMENT_ROOT_PATH/repo3.git" config \
 		bundle.new.uri "$HTTPD_URL/new.bundle" &&
 
-	GIT_TEST_BUNDLE_URI=1 \
 	GIT_TRACE2_EVENT="$(pwd)/trace.txt" \
-		git -c protocol.version=2 clone \
+		git -c protocol.version=2 \
+		    -c transfer.bundleURI=true clone \
 		$HTTPD_URL/smart/repo3.git repo3 &&
 
 	# We should fetch _both_ bundles
diff --git a/transport.c b/transport.c
index 957dca4923c..241f8a6ba2d 100644
--- a/transport.c
+++ b/transport.c
@@ -1533,10 +1533,9 @@ int transport_get_remote_bundle_uri(struct transport *transport)
 
 	/*
 	 * Don't request bundle-uri from the server unless configured to
-	 * do so by GIT_TEST_BUNDLE_URI=1 or transfer.bundleURI=true.
+	 * do so by the transfer.bundleURI=true config option.
 	 */
-	if (!git_env_bool("GIT_TEST_BUNDLE_URI", 0) &&
-	    (git_config_get_bool("transfer.bundleuri", &value) || !value))
+	if (git_config_get_bool("transfer.bundleuri", &value) || !value)
 		return 0;
 
 	if (!transport->bundles->baseURI)
-- 
gitgitgadget

Re: [PATCH 0/3] Bundle URIs 4.5: fixups from part IV

[Victoria Dye]

Derrick Stolee via GitGitGadget wrote:
> These patches represent small fixups that came in review from the last
> version of 'ds/bundle-uri-4'. Since it was merged to 'next', these patches
> are forward-fixes on that branch.
> 
> Note: I did not include any changes that could be solved by adding an UNUSED
> macro, saving that for Peff and his already-prepared patches in that area.
> 
> Thanks, -Stolee
> 
> Derrick Stolee (3):
>   bundle-uri: drop unused 'uri' parameter
>   bundle-uri: advertise based on repo config
>   bundle-uri: remove GIT_TEST_BUNDLE_URI env variable

The first two patches (unused arg removal & using repo to get config) are
straightforward fixes for issues mentioned earlier ([1] and [2],
respectively). The last patch replaces the 'GIT_TEST_BUNDLE_URI' environment
variable with globally setting 'transfer.bundleURI' for a subset of the
'lib-bundle-uri-protocol.sh' tests. The comment you added in that file ("The
remaining tests will all assume transfer.bundleURI=true") clearly explains
what you're doing there as a reference for future updates to the tests.

These patches all look good to me. Thanks!

[1] https://lore.kernel.org/git/affbc458-d4f5-525f-d431-5ec1d489afc8@github.com/
[2] https://lore.kernel.org/git/4d4e02c3-89dc-8372-7e8a-7ec76fdd6f4e@github.com/

Re: [PATCH 0/3] Bundle URIs 4.5: fixups from part IV

[Jeff King]

On Mon, Dec 12, 2022 at 05:33:23PM +0000, Derrick Stolee via GitGitGadget wrote:

> These patches represent small fixups that came in review from the last
> version of 'ds/bundle-uri-4'. Since it was merged to 'next', these patches
> are forward-fixes on that branch.
> 
> Note: I did not include any changes that could be solved by adding an UNUSED
> macro, saving that for Peff and his already-prepared patches in that area.

Thanks, that sounds good on my part. The first two look obviously
correct to me. The third looks sensible, too, but isn't an area I'd
looked at before.

-Peff

Re: [PATCH 1/3] bundle-uri: drop unused 'uri' parameter

[Ævar Arnfjörð Bjarmason]

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

> From: Derrick Stolee <derrickstolee@github.com>
>
> The 'uri' parameter of fetch_bundle_list() was added early in
> development, but is not required since the 'list' parameter includes a
> 'baseURI' member. Remove the 'uri' parameter and make this expectation
> explicit.

Makes sense, and this is a straightforward commit, but...

> diff --git a/bundle-uri.h b/bundle-uri.h
> index b2c9c160a52..29b0c98ee65 100644
> --- a/bundle-uri.h
> +++ b/bundle-uri.h
> @@ -68,8 +68,8 @@ struct bundle_list {
>  	 * In the case of the 'bundle-uri' protocol v2 command, the base
>  	 * URI is the URI of the Git remote.
>  	 *
> -	 * Otherewise, the bundle list was downloaded over HTTP from some
> -	 * known URI.
> +	 * Otherwise, the bundle list was downloaded over HTTP from some
> +	 * known URI. 'baseURI' is set to that value.

...here we have a stray typo fix, not called out in the commit
message. I think that can pass, but if you're re-rolling I think any
such "while-at-it" would be better split into their own commits.

But more importantly:

>  	 *
>  	 * The baseURI is used as the base for any relative URIs
>  	 * advertised by the bundle list at that location.
> @@ -112,10 +112,10 @@ int fetch_bundle_uri(struct repository *r, const char *uri);
>   * bundle-uri protocol v2 verb) at the given uri, fetch and unbundle the
>   * bundles according to the bundle strategy of that list.
>   *
> - * Returns non-zero if no bundle information is found at the given 'uri'.
> + * It is expected that the given 'list' is initialized, including its
> + * 'baseURI' value

At first sight this seems like a regression. Why have we stopped
documenting the exit code?

But looking again is this because in 7cc47a980ac (bundle-uri: download
bundles from an advertised list, 2022-12-05) you accidentally
copy/pasted the documentation for fetch_bundle_uri(), and this was never
applicable to this function?

Even then, not documenting the code we return now is a regression. If it
was wrong before, let's make it correct, not stop discussing it
entirely.

In either case this is another while-at-it entirely unrelated to the
$subject here of dropping an unused parameter.

The same goes for the added docs, that we "expect [that] 'list' is
initialized" may be true, but that would have been true before we
removed this unused parameter, so let's not stick that in this unrelated
"UNUSED" change.

>   */
>  int fetch_bundle_list(struct repository *r,
> -		      const char *uri,
>  		      struct bundle_list *list);
>  
>  /**

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

[Ævar Arnfjörð Bjarmason]

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...>.

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

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

[Ævar Arnfjörð Bjarmason]

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).

But in this case we'd never like to combine the two?

> The environment variable has a name that implies it would be useful
> outside of these tests. It is not useful to set across all tests since
> it would do very little without some setup on the server side. Remove
> it.

This part I really don't get, why does its name imply that? We use
GIT_TEST_* variables for this sort of thing, maybe it's not needed here,
but what should it have been named? GIT_TEST_T57XX_* or something?

I named it like that for consistency with existing test variables, ...

Re: [PATCH 1/3] bundle-uri: drop unused 'uri' parameter

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> ...here we have a stray typo fix, not called out in the commit
> message. I think that can pass, but if you're re-rolling I think any
> such "while-at-it" would be better split into their own commits.
>
> But more importantly:
> ...
> But looking again is this because in 7cc47a980ac (bundle-uri: download
> bundles from an advertised list, 2022-12-05) you accidentally
> copy/pasted the documentation for fetch_bundle_uri(), and this was never
> applicable to this function?
> ...

I think these three patches were designed to be "oops, that was
wrong and here is a band-aid" follow-up fixes on top of what was
back then in 'next'.  Now the base topic has been kicked out of
'next' together with these, we can afford to roll them into the base
series before merging it back to 'next', but due to things generally
being slow toward the end of the year, that hasn't happened yet.

Thanks for reviewing these carefully.

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

[Derrick Stolee]

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.

Thanks,
-Stolee

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

[Derrick Stolee]

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

Re: [PATCH 1/3] bundle-uri: drop unused 'uri' parameter

[Derrick Stolee]

On 12/19/22 5:57 AM, Ævar Arnfjörð Bjarmason wrote:
> On Mon, Dec 12 2022, Derrick Stolee via GitGitGadget wrote:

>> @@ -112,10 +112,10 @@ int fetch_bundle_uri(struct repository *r, const char *uri);
>>   * bundle-uri protocol v2 verb) at the given uri, fetch and unbundle the
>>   * bundles according to the bundle strategy of that list.
>>   *
>> - * Returns non-zero if no bundle information is found at the given 'uri'.
>> + * It is expected that the given 'list' is initialized, including its
>> + * 'baseURI' value

> The same goes for the added docs, that we "expect [that] 'list' is
> initialized" may be true, but that would have been true before we
> removed this unused parameter, so let's not stick that in this unrelated
> "UNUSED" change.

It is _not_ unrelated. The 'uri' parameter looks like it should
be used to determine relative URLs for the included list. However,
this reasoning around the 'baseURI' value points out that we are
using that value _instead_ of the 'uri' value, which is why it
is safe to remove the parameter.

Thanks,
-Stolee

Re: [PATCH 1/3] bundle-uri: drop unused 'uri' parameter

[Derrick Stolee]

On 12/19/22 7:49 PM, Junio C Hamano wrote:

> I think these three patches were designed to be "oops, that was
> wrong and here is a band-aid" follow-up fixes on top of what was
> back then in 'next'.  Now the base topic has been kicked out of
> 'next' together with these, we can afford to roll them into the base
> series before merging it back to 'next', but due to things generally
> being slow toward the end of the year, that hasn't happened yet.

I wasn't expecting to re-roll the base topic, but I'll get
started on that now.

However, the comments in this review are barely actionable. They
provide very little value especially for how verbose they are. I'm
frustrated to see such a drive-by review so late in the process.

Thanks,
-Stolee

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

[Ævar Arnfjörð Bjarmason]

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@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.

Re: [PATCH 1/3] bundle-uri: drop unused 'uri' parameter

[Ævar Arnfjörð Bjarmason]

On Tue, Dec 20 2022, Derrick Stolee wrote:

> On 12/19/22 5:57 AM, Ævar Arnfjörð Bjarmason wrote:
>> On Mon, Dec 12 2022, Derrick Stolee via GitGitGadget wrote:
>
>>> @@ -112,10 +112,10 @@ int fetch_bundle_uri(struct repository *r, const char *uri);
>>>   * bundle-uri protocol v2 verb) at the given uri, fetch and unbundle the
>>>   * bundles according to the bundle strategy of that list.
>>>   *
>>> - * Returns non-zero if no bundle information is found at the given 'uri'.
>>> + * It is expected that the given 'list' is initialized, including its
>>> + * 'baseURI' value
>
>> The same goes for the added docs, that we "expect [that] 'list' is
>> initialized" may be true, but that would have been true before we
>> removed this unused parameter, so let's not stick that in this unrelated
>> "UNUSED" change.
>
> It is _not_ unrelated. The 'uri' parameter looks like it should
> be used to determine relative URLs for the included list. However,
> this reasoning around the 'baseURI' value points out that we are
> using that value _instead_ of the 'uri' value, which is why it
> is safe to remove the parameter.

I'm saying that the function expected that baseURI to be initialized
before, but with your rationale I think it's fine to also do it in one
commit.

I also pointed out (and you elided) that we should still document the
return code, do you agree that we should keep that?

Re: [PATCH 1/3] bundle-uri: drop unused 'uri' parameter

[Ævar Arnfjörð Bjarmason]

On Tue, Dec 20 2022, Derrick Stolee wrote:

> On 12/19/22 7:49 PM, Junio C Hamano wrote:
>
>> I think these three patches were designed to be "oops, that was
>> wrong and here is a band-aid" follow-up fixes on top of what was
>> back then in 'next'.  Now the base topic has been kicked out of
>> 'next' together with these, we can afford to roll them into the base
>> series before merging it back to 'next', but due to things generally
>> being slow toward the end of the year, that hasn't happened yet.
>
> I wasn't expecting to re-roll the base topic, but I'll get
> started on that now.
>
> However, the comments in this review are barely actionable.

Skimming over my comments again the actionable bits are:

 * 1/3: Commit says it's removing an unused param, also has while-at-it
   typo fix, maybe split that up?
 * 1/3: Commit says <same>, has while-at-it removal of documenting what
   a function's return value means, maybe keep that?
 * 1/3: Commit says <same>, has seeming while-at-it discussion of what
   another parameter is expected to contain (you replied saying they
   were related)
 * 2/3: Question about whether reading "r" v.s. "the_repository" has an
   observable behavior change. If yes let's add a test, if no let's note
   "it's for good measure".
 * 3/3: A question about whether we really didn't need
   GIT_TEST_BUNDLE_URI. You managed to convince me that no, we don't
 * 3/3: Question about what the 2nd paragraph of the commit message is
   trying to convey (you didn't reply to this bit). The actionable thing
   would be to clarify it, or remove it.

> They provide very little value especially for how verbose they are.

I agree that this was all of relatively little value, these are all
rather trivial patches after all, and the bundle-uri feature is opt-in
and WIP at this point.

But even trivial patches can be helped along by review. I'm just trying
to help this topic land & show Junio that others have reviewed this
carefully.

I agree that my E-Mails are verbose, sorry. This isn't my native
language, it's a balancing act between trying to be unambiguously
understood, and verbosity. sorry.

> frustrated to see such a drive-by review so late in the process.

This I'm confused by. You submitted this ~1 week ago on the 12th, Junio
rewound the parent topic out of "next" on the 14th.

Isn't this the appropriate time to comment on both this topic & its
parent?