Re: [PATCH 1/3] bundle-uri: drop unused 'uri' parameter
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);
>
> /**
Derrick Stolee via GitGitGadget
