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

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Dec 19, 2022, 10:57 UTC
Message-ID: <221219.86tu1rzmbs.gmgdl@evledraar.gmail.com>
In-Reply-To: <d17f08ed4b68d711b452b5cfb54a949845bdea81.1670866407.git.gitgitgadget@gmail.com>

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

Show 6 quoted lines

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

Show 12 quoted lines

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

Show 10 quoted lines

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

Show 6 quoted lines

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