| Message ID | pull.1227.v4.git.1652095969026.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | ef6d15ca536a50d916f8d9c7f600d650810161b1 |
| Headers | show |
| Series | [v4] builtin/remote.c: teach `-v` to list filters for promisor remotes | expand |
On Mon, May 09, 2022 at 11:32:48AM +0000, Abhradeep Chakraborty via GitGitGadget wrote: > From: Abhradeep Chakraborty <chakrabortyabhradeep79@gmail.com> > > `git remote -v` (`--verbose`) lists down the names of remotes along with > their URLs. It would be beneficial for users to also specify the filter > types for promisor remotes. Something like this - This version looks like it has addressed many (all?) of the comments previously discussed during review. On a quick scan, the code and tests look good to my eyes, too. But there was a good question raised by Phillip in https://lore.kernel.org/git/ab047b4b-6037-af78-1af6-ad35ac6d7c90@iee.email/ that I didn't see addressed in your response, which was "why not put this behind a new `--show-partial-filter` option"? I share (what I think is) Junio's feeling that having information that is readily available from e.g., running "git config --get remote.<name>.partialObjectFilter" seems redundant. I could understand forcing a user to know the config key's name feels like a hurdle. But cluttering the output of `git remote -v` seems like the wrong solution to that hurdle. But I can see where it _would_ be useful. So it would be nice to be able to turn the extra output on in those cases, but _only_ those cases, and a flag would be a nice way to go about doing that. Thanks, Taylor
Hi Taylor, Le 2022-05-09 à 11:34, Taylor Blau a écrit : > On Mon, May 09, 2022 at 11:32:48AM +0000, Abhradeep Chakraborty via GitGitGadget wrote: >> From: Abhradeep Chakraborty <chakrabortyabhradeep79@gmail.com> >> >> `git remote -v` (`--verbose`) lists down the names of remotes along with >> their URLs. It would be beneficial for users to also specify the filter >> types for promisor remotes. Something like this - > > This version looks like it has addressed many (all?) of the comments > previously discussed during review. On a quick scan, the code and tests > look good to my eyes, too. > > But there was a good question raised by Phillip in > > https://lore.kernel.org/git/ab047b4b-6037-af78-1af6-ad35ac6d7c90@iee.email/ > > that I didn't see addressed in your response, which was "why not put > this behind a new `--show-partial-filter` option"? I originally opened the issue on GGG that this series adresses. My justification, and asnwer to that question, is simple: 'git remote -v', for me, is a way to ask Git to give me all the information it knows about my configured remotes. That's why I thought that it would be really nice if partial clones filters would be shown. After all, 'git remote' is listed in the 'porcelain' section of the Git commands [1], and isn't the goal of declaring commands "porcelain" that we can make their output more useful to the users without worrying as much about backwards compatibility than with plumbing commands? > I share (what I think is) Junio's feeling that having information that > is readily available from e.g., running "git config --get > remote.<name>.partialObjectFilter" seems redundant. I could understand > forcing a user to know the config key's name feels like a hurdle. But > cluttering the output of `git remote -v` seems like the wrong solution > to that hurdle. As I said above, I have 'git rem' (my alias for 'git remote -v') in my muscle memory and use it when I want to have an outline of my configured remotes. I think it would be really easier to add the filters info to the existing output. It's really faster to type than using 'git config', and you do not have to remember which remote name to query. I think "clutter" is a little strong word here :) > But I can see where it _would_ be useful. So it would be nice to be able > to turn the extra output on in those cases, but _only_ those cases, and > a flag would be a nice way to go about doing that. If really this topic is blocked by "we do not want to change the default output of 'git remote -v'", then I agree it would be nice to be able to set 'remote.showFilters' (or similar) to get such output, I agree. Or, making 'git remote' act like 'git branch' and accept a second '-v', i.e. 'git remote -vv' would list filters (then I would just adjust my alias :P). Then we can outright declare "the output of 'git remote -vv' is subject to future changes to show more useful information", or similar, so we do not have to do the same dance the next time we want to add some other info. The downside of hiding such new features behing config values or additional flags is that it really, really limits their discoverability. This is something that I often think about and think we should really do better in Git, in general. For example, features like 'remote.pushDefault' or the 'diff=*' attribute for language-aware hunk headers (and funcname-limited log/blame etc) are immensely useful, but often even experienced and long-time Git users do not even know they exist, because they are not covered in "regular" Git tutorials... Cheers, Philippe. [1] https://git-scm.com/docs/git#Documentation/git.txt-ahrefdocsgit-remotegit-remote1a
Taylor Blau <me@ttaylorr.com> wrote: > But there was a good question raised by Phillip in > > https://lore.kernel.org/git/ab047b4b-6037-af78-1af6-ad35ac6d7c90@iee.email/ > > that I didn't see addressed in your response, which was "why not put > this behind a new `--show-partial-filter` option"? Actually, I addressed it[1] - > ... Another point is that > it's important for an user to know which one is a promisor remote and what > filter type they use. If we go with the current implementation the output > would be let's say - > origin <remote-url> (fetch) > origin <remote-url> (push) > upstream <remote-url> (fetch) > upstream <remote-url> (push) > > By seeing the above output anyone may assume that all the remotes are > normal remotes. If the user now try to run `git pull origin` and suddenly > he/she discover that some blobs are not downloaded. He/she run the above > mentioned (1) command and find that this is a promisor remote! > > Here `remote -v` didn't warn the user about the origin remote being an > promisor remote. Instead it makes him/her assume that all are normal > remotes. Providing only these three info (i.e. <remote-name>, <remote-url> > and <direction>) is not sufficient - it only shows the half of the picture. If we use a new `--show-partial-clone` flag, users can get to know about promisor remotes only if he/she use this flag. As I said in the refered comment, it may happen that the user unfortunately use the flag AFTER the accident - to know about if that was the promisor remote! See this also[2] - > ... If > we can specify `(fetch)` in the output then why not the filter of that > `fetch` on which the behaviour of `fetch` functionality highly depends? Taylor Blau <me@ttaylorr.com> wrote: > But I can see where it _would_ be useful. So it would be nice to be able > to turn the extra output on in those cases, but _only_ those cases, and > a flag would be a nice way to go about doing that. Adding the extra flag is not a good approach to me due to the above reason. But at the end of the day, all of you have a lots of experience in this field than me. You all could better tell which one is better approach. [1] https://lore.kernel.org/git/20220501193807.94369-1-chakrabortyabhradeep79@gmail.com/ [2] https://lore.kernel.org/git/20220502145624.12702-1-chakrabortyabhradeep79@gmail.com/ Thanks :)
Taylor Blau <me@ttaylorr.com> wrote:
> This version looks like it has addressed many (all?) of the comments
previously discussed during review.
To my knowledge, yeah, I addressed all the comments :)
Thanks :)
Philippe Blain <levraiphilippeblain@gmail.com> writes: > Or, making 'git remote' act like 'git branch' and accept a second '-v', i.e. > 'git remote -vv' would list filters (then I would just adjust my alias :P). > Then we can outright declare "the output of 'git remote -vv' is subject to > future changes to show more useful information", or similar, so we do not > have to do the same dance the next time we want to add some other info. Isn't it where we already are with "remote -v", though? I am not sure addition of excess information that may not be universally useful is a very welcome change, even with "remote -v -v". I am not worried about showing the "list-object-filter", but I worry about managing temptations of future developers to add other stuff. > The downside of hiding such new features behing config values or additional flags > is that it really, really limits their discoverability. This is something that I > often think about and think we should really do better in Git, in general. > For example, features like 'remote.pushDefault' or the 'diff=*' attribute > for language-aware hunk headers (and funcname-limited log/blame etc) are immensely > useful, but often even experienced and long-time Git users do not even know they exist, > because they are not covered in "regular" Git tutorials... Unfortunately, it is not exactly a solution for that to update the tutorial, because experienced and long-time users rightly consider themselves beyond tutorials and sometimes documentation.
On Mon, May 09, 2022 at 10:51:57PM +0530, Abhradeep Chakraborty wrote: > Taylor Blau <me@ttaylorr.com> wrote: > > > But there was a good question raised by Phillip in > > > > https://lore.kernel.org/git/ab047b4b-6037-af78-1af6-ad35ac6d7c90@iee.email/ > > > > that I didn't see addressed in your response, which was "why not put > > this behind a new `--show-partial-filter` option"? > > Actually, I addressed it[1] - Ah, sorry that I missed it! I think Phillipe's GGG issue is probably a good signal that we are not making this information as discoverable to users as we could be. I share Junio's concern that this change may tempt future contributors to add more output still to "git remote", but perhaps not. So I'd be OK with this change as-is. > [1] https://lore.kernel.org/git/20220501193807.94369-1-chakrabortyabhradeep79@gmail.com/ Thanks, Taylor
Junio C Hamano <gitster@pobox.com> wrote: > Isn't it where we already are with "remote -v", though? I am not > sure addition of excess information that may not be universally > useful is a very welcome change, even with "remote -v -v". I am not > worried about showing the "list-object-filter", but I worry about > managing temptations of future developers to add other stuff. If future developers come up with some really useful stuff (i.e. universally useful), I think those should be added in the output irrespective of the no of existing info in the output. If the output becomes messy, we should focus on how we can make the output clear may be using tabular format. Else you can drop the idea and suggest them to introduce a new flag (depending on the situation). If you still have some doubt about my PR i.e. if you can not determine which category my PR belongs to, I can go with adding `show-partial-clone` flag. The downside would be that `remote -v` will not give the full summary in case of partial clone. If you like the tabular format approach, I am further going to propose a table format - +---------------+----------------------------------------------+ | remote name | remote info | +---------------+--------+--------+----------------------------+ | | | url | https://blah.com/blah.git | | origin | +--------+----------------------------+ | | | filter | blob:none | | | fetch +--------+----------------------------+ | | | . | | | | . (some important data) | | +--------+--------+----------------------------+ | | | url | https://blah.com/blah.git | | | push +--------+----------------------------+ | | | ... (some important data) | +---------------+--------+-------------------------------------+ In this way, user can see the summary of all remotes with visual ease. Of course it is not suitable for scripting. In that case we can use a new flag `--raw` which will let `-v` to provide a space/tab seperated sequence of info (similar to current format). Let me know if you (as in all) like/dislike my view and give your arguments regarding my proposal. Thanks :)
Abhradeep Chakraborty <chakrabortyabhradeep79@gmail.com> writes: > Else you can drop the idea and suggest them to introduce a new flag > (depending on the situation). If you still have some doubt about my > PR i.e. if you can not determine which category my PR belongs to, I > can go with adding `show-partial-clone` flag. The downside would > be that `remote -v` will not give the full summary in case of partial > clone. If majority of partial-clone users find it unnecessary noise, then it may be an upside to give only reduced summary that is less than full that may be given by `remote -v -v`. Worse downside of adding it as an option is that it invites more options. It is less worse to add new ones to `remote -v -v` (or to `remote -v`, or not adding it at all) than adding another option, I would think. Perhaps tagged output that can be easier to parse would be better "extensible" output format for adding more random pieces of information than going tabular. I dunno.
Junio C Hamano <gitster@pobox.com> wrote: > If majority of partial-clone users find it unnecessary noise, then > it may be an upside to give only reduced summary that is less than > full that may be given by `remote -v -v`. Should I add this to `remote -v -v` then? `remote -vv` is currently not implemented I guess. > Perhaps tagged output that can be easier to parse would be better > "extensible" output format for adding more random pieces of > information than going tabular. I dunno. I am not sure what exactly you are refering by 'tagged output' but it is true that tabular form is hard to parse. That's why I suggested `--raw` flag which would be used for parsing. It would give the info following the currently implemented format. If you like the tagged output format, then should we implement `-vv` which would give the output as the tagged output format and also can be extended?
diff --git a/Documentation/git-remote.txt b/Documentation/git-remote.txt index cde9614e362..1dec3148348 100644 --- a/Documentation/git-remote.txt +++ b/Documentation/git-remote.txt @@ -35,6 +35,8 @@ OPTIONS -v:: --verbose:: Be a little more verbose and show remote url after name. + For promisor remotes, also show which filter (`blob:none` etc.) + are configured. 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/t5505-remote.sh b/t/t5505-remote.sh index c90cf47acdb..fff14e13ed4 100755 --- a/t/t5505-remote.sh +++ b/t/t5505-remote.sh @@ -78,6 +78,40 @@ test_expect_success 'add another remote' ' ) ' +test_expect_success 'setup bare clone for server' ' + git clone --bare "file://$(pwd)/one" srv.bare && + git -C srv.bare config --local uploadpack.allowfilter 1 && + git -C srv.bare config --local uploadpack.allowanysha1inwant 1 +' + +test_expect_success 'filters for promisor remotes are listed by git remote -v' ' + test_when_finished "rm -rf pc" && + git clone --filter=blob:none "file://$(pwd)/srv.bare" pc && + git -C pc remote -v >out && + grep "srv.bare (fetch) \[blob:none\]" out && + + git -C pc config remote.origin.partialCloneFilter object:type=commit && + git -C pc 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 pc" && + git clone one pc && + git -C pc remote -v >out && + ! grep "(fetch) \[.*\]" out +' + +test_expect_success 'filters are listed by git remote -v only' ' + test_when_finished "rm -rf pc" && + git clone --filter=blob:none "file://$(pwd)/srv.bare" pc && + git -C pc remote >out && + ! grep "\[blob:none\]" out && + + git -C pc remote show >out && + ! grep "\[blob:none\]" out +' + test_expect_success 'check remote-tracking' ' ( cd test &&