| Message ID | d17f08ed4b68d711b452b5cfb54a949845bdea81.1670866407.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Bundle URIs 4.5: fixups from part IV | expand |
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); > > /**
Æ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.
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
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
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?
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?
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); /**