| Message ID | pull.1227.v3.git.1651933221216.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v3] builtin/remote.c: teach `-v` to list filters for promisor remotes | expand |
Hi Abhradeep, Le 2022-05-07 à 10:20, Abhradeep Chakraborty via GitGitGadget a écrit : > From: Abhradeep Chakraborty <chakrabortyabhradeep79@gmail.com> > > `git remote -v` (`--verbose`) lists down the names of remotes along with > their urls. small nit: I would capitalize URLs. > It would be beneficial for users to also specify the filter > types for promisor remotes. Something like this - > > origin remote-url (fetch) [blob:none] > origin remote-url (push) > > Teach `git remote -v` to also specify the filters for promisor remotes. > > Signed-off-by: Abhradeep Chakraborty <chakrabortyabhradeep79@gmail.com> > --- > builtin/remote.c: teach -v to list filters for promisor remotes > > Fixes #1211 [1] I don't think this matters much, but if Junio is OK with that, it would be nice to include the reference to the GitGitGadget issue in the commit message itself, though with its full URL, something like: Closes: https://github.com/gitgitgadget/git/issues/1211 as another trailer before your signed-off-by. By including it in the commit message we allow the issue to be closed automatically when your topic branch is merged to 'master'. By using the full link we make sure that GitHub knows we are targetting that issue specifically, not any other issue or PR in any fork of Git with the same number. > > In the previous version, documentation is updated (describing the > proposed change) and url_buf is renamed into remote_info_buf. In this > varsion, some more test cases are added and broken indentations are > fixed. Again, small nit to make it easier for reviewers: usually we prefer to see what has changed since the previous version first, and then (if you want, it's not strictly necessary) what changed in the other previous versions. It's not necessary since if we want that info we can refer to the cover letters of the previous iterations directly. And ideally, in bullet points. So something like: Changes since v2: - added more test cases - fixed broken indentations Changes since v1: - updated documentation - renamed url_buf into remote_info_buf > > [1] https://github.com/gitgitgadget/git/issues/1211 > > Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1227%2FAbhra303%2Fpromisor_remote-v3 > Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1227/Abhra303/promisor_remote-v3 > Pull-Request: https://github.com/gitgitgadget/git/pull/1227 Thanks, Philippe.
Forgot to comment on the patch itself :P Le 2022-05-07 à 10:20, Abhradeep Chakraborty via GitGitGadget a écrit : > From: Abhradeep Chakraborty <chakrabortyabhradeep79@gmail.com> > > Documentation/git-remote.txt | 3 +++ > builtin/remote.c | 18 +++++++++++++----- > t/t5616-partial-clone.sh | 28 ++++++++++++++++++++++++++++ I think the tests woud fit better in t5505-remote.sh, since the patch really adds a feature to the 'git remote' command. > 3 files changed, 44 insertions(+), 5 deletions(-) > > diff --git a/Documentation/git-remote.txt b/Documentation/git-remote.txt > index cde9614e362..a125bd839f7 100644 > --- a/Documentation/git-remote.txt > +++ b/Documentation/git-remote.txt > @@ -35,6 +35,9 @@ OPTIONS > -v:: > --verbose:: > Be a little more verbose and show remote url after name. > + For promisor remotes it will show an extra information I found it sligtly awkward to use the future tense here. Maybe just: For promisor remotes, also show which filter (`blob:none` etc.) that promisor remote use, wrapped in square brackets. And technically, it's not really the remote that "uses" the filter, but more the local Git client. So maybe something like this would be more accurate and simpler: For promisor remotes, also show which filter (`blob:none` etc.) are configured, wrapped in square brackets. And even then "wrapped in square brackets" *could* be dropped, I think. Apart from that, the patch and test look good, thanks for working on that! Cheers, Philippe.
Philippe Blain <levraiphilippeblain@gmail.com> wrote: > I think the tests woud fit better in t5505-remote.sh, since the patch really > adds a feature to the 'git remote' command. I think you're right. Thanks! > I found it sligtly awkward to use the future tense here. Maybe just: > > For promisor remotes, also show which filter > (`blob:none` etc.) that promisor remote use, wrapped in square brackets. > > And technically, it's not really the remote that "uses" the filter, > but more the local Git client. So maybe something like this would > be more accurate and simpler: > > For promisor remotes, also show which filter (`blob:none` etc.) > are configured, wrapped in square brackets. > > And even then "wrapped in square brackets" *could* be dropped, I > think. Got it. Thanks for the suggestions about both the PR and the patch. Will update it. Thanks :)
Philippe Blain <levraiphilippeblain@gmail.com> writes: >> Signed-off-by: Abhradeep Chakraborty <chakrabortyabhradeep79@gmail.com> >> --- >> builtin/remote.c: teach -v to list filters for promisor remotes >> >> Fixes #1211 [1] > > I don't think this matters much, but if Junio is OK with that, it would > be nice to include the reference to the GitGitGadget issue in the commit > message itself, though with its full URL, something like: > > Closes: https://github.com/gitgitgadget/git/issues/1211 > > as another trailer before your signed-off-by. By including it in the > commit message we allow the issue to be closed automatically when your topic > branch is merged to 'master'. By using the full link we make sure that GitHub > knows we are targetting that issue specifically, not any other issue or PR in > any fork of Git with the same number. Nice to know. Is there a handy GGG users' guide that mentions these "magic trailers" (the other one I have seen used is "Cc:")? > Again, small nit to make it easier for reviewers: usually we prefer to see > what has changed since the previous version first, and then (if you want, > it's not strictly necessary) what changed in the other previous versions. Yup. For a single-patch topic, the following may not apply, but for a multi-patch topic, a full "topic overview" should also be available in the cover letter of the latest version. A reviewer who was absent while older iterations were reviewed should not have to fish for cover letters of previous iterations to learn what the topic is about to decide if the topic is worth their time to review. Once they get interested enough, they can of course dig older iterations, but the job of the cover letter in each iteration is to allow them to become interested with the least effort. Thanks.
Hi Junio, Le 2022-05-09 à 12:29, Junio C Hamano a écrit : > Philippe Blain <levraiphilippeblain@gmail.com> writes: > >>> Signed-off-by: Abhradeep Chakraborty <chakrabortyabhradeep79@gmail.com> >>> --- >>> builtin/remote.c: teach -v to list filters for promisor remotes >>> >>> Fixes #1211 [1] >> >> I don't think this matters much, but if Junio is OK with that, it would >> be nice to include the reference to the GitGitGadget issue in the commit >> message itself, though with its full URL, something like: >> >> Closes: https://github.com/gitgitgadget/git/issues/1211 >> >> as another trailer before your signed-off-by. By including it in the >> commit message we allow the issue to be closed automatically when your topic >> branch is merged to 'master'. By using the full link we make sure that GitHub >> knows we are targetting that issue specifically, not any other issue or PR in >> any fork of Git with the same number. > > Nice to know. Is there a handy GGG users' guide that mentions these > "magic trailers" (the other one I have seen used is "Cc:")? "CC:" is GGG-specific, it is mentioned on the GGG homepage, https://gitgitgadget.github.io/, under "How can you use GitGitGadget?". It's also mentioned on the Welcome message GGG adds to the PR for new contributors [1]. "Fixes", "Closes" etc. are GitHub features (though GitLab implements the same feature), see [2], [3]. [1] https://github.com/gitgitgadget/gitgitgadget/blob/main/res/WELCOME.md#welcome-to-gitgitgadget [2] https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue#linking-a-pull-request-to-an-issue-using-a-keyword [3] https://docs.gitlab.com/ee/user/project/issues/managing_issues.html#closing-issues-automatically Philippe.
diff --git a/Documentation/git-remote.txt b/Documentation/git-remote.txt index cde9614e362..a125bd839f7 100644 --- a/Documentation/git-remote.txt +++ b/Documentation/git-remote.txt @@ -35,6 +35,9 @@ OPTIONS -v:: --verbose:: Be a little more verbose and show remote url after name. + For promisor remotes it will show an extra information + (wrapped in square brackets) describing which filter + (`blob:none` etc.) that promisor remote use. NOTE: This must be placed between `remote` and subcommand. diff --git a/builtin/remote.c b/builtin/remote.c index 5f4cde9d784..d4b69fe7789 100644 --- a/builtin/remote.c +++ b/builtin/remote.c @@ -1185,14 +1185,22 @@ static int show_push_info_item(struct string_list_item *item, void *cb_data) static int get_one_entry(struct remote *remote, void *priv) { struct string_list *list = priv; - struct strbuf url_buf = STRBUF_INIT; + struct strbuf remote_info_buf = STRBUF_INIT; const char **url; int i, url_nr; if (remote->url_nr > 0) { - strbuf_addf(&url_buf, "%s (fetch)", remote->url[0]); + struct strbuf promisor_config = STRBUF_INIT; + const char *partial_clone_filter = NULL; + + strbuf_addf(&promisor_config, "remote.%s.partialclonefilter", remote->name); + strbuf_addf(&remote_info_buf, "%s (fetch)", remote->url[0]); + if (!git_config_get_string_tmp(promisor_config.buf, &partial_clone_filter)) + strbuf_addf(&remote_info_buf, " [%s]", partial_clone_filter); + + strbuf_release(&promisor_config); string_list_append(list, remote->name)->util = - strbuf_detach(&url_buf, NULL); + strbuf_detach(&remote_info_buf, NULL); } else string_list_append(list, remote->name)->util = NULL; if (remote->pushurl_nr) { @@ -1204,9 +1212,9 @@ static int get_one_entry(struct remote *remote, void *priv) } for (i = 0; i < url_nr; i++) { - strbuf_addf(&url_buf, "%s (push)", url[i]); + strbuf_addf(&remote_info_buf, "%s (push)", url[i]); string_list_append(list, remote->name)->util = - strbuf_detach(&url_buf, NULL); + strbuf_detach(&remote_info_buf, NULL); } return 0; diff --git a/t/t5616-partial-clone.sh b/t/t5616-partial-clone.sh index 4a3778d04a8..26756d616cd 100755 --- a/t/t5616-partial-clone.sh +++ b/t/t5616-partial-clone.sh @@ -49,6 +49,34 @@ test_expect_success 'do partial clone 1' ' test "$(git -C pc1 config --local remote.origin.partialclonefilter)" = "blob:none" ' +test_expect_success 'filters for promisor remotes are listed by git remote -v' ' + test_when_finished "rm -rf pc2" && + git clone --filter=blob:none "file://$(pwd)/srv.bare" pc2 && + git -C pc2 remote -v >out && + grep "srv.bare (fetch) \[blob:none\]" out && + + git -C pc2 config remote.origin.partialCloneFilter object:type=commit && + git -C pc2 remote -v >out && + grep "srv.bare (fetch) \[object:type=commit\]" out +' + +test_expect_success 'filters should not be listed for non promisor remotes (remote -v)' ' + test_when_finished "rm -rf pc2" && + git clone "file://$(pwd)/srv.bare" pc2 && + git -C pc2 remote -v >out && + ! grep "(fetch) \[.*\]" out +' + +test_expect_success 'filters are listed by git remote -v only' ' + test_when_finished "rm -rf pc2" && + git clone --filter=blob:none "file://$(pwd)/srv.bare" pc2 && + git -C pc2 remote >out && + ! grep "\[blob:none\]" out && + + git -C pc2 remote show >out && + ! grep "\[blob:none\]" out +' + test_expect_success 'verify that .promisor file contains refs fetched' ' ls pc1/.git/objects/pack/pack-*.promisor >promisorlist && test_line_count = 1 promisorlist &&