{"thread":{"id":"38949","subject":"[PATCH] show-branch: show all local heads when only giving one rev along --topics","startedAt":"2015-03-30T22:12:23Z","lastAt":"2015-03-30T22:37:52Z","messageCount":4,"participants":["Mike Hommey","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"258698","messageId":"1427753543-19680-1-git-send-email-mh@glandium.org","threadId":"38949","inReplyTo":null,"subject":"[PATCH] show-branch: show all local heads when only giving one rev along --topics","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2015-03-30T22:12:23Z","receivedAt":"2015-03-30T22:12:23Z","isPatch":true,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"\"git show-branch --topics <rev> <revs>...\" displays ancestry graph, only\nconsidering commits that are in all given revs, except the first one.\n\n\"git show-branch\" displays ancestry graph for all local branches.\n\nUnfortunately, \"git show-branch --topics <rev>\" only prints out the rev\ninfo for the given rev, and nothing else, e.g.:\n\n  $ git show-branch --topics origin/master\n  [origin/master] Sync with 2.3.3\n\nWhile there is an option to add all remote-tracking branches (-r), and\nanother to add all local+remote branches (-a), there is no option to add\nonly local branches. Adding such an option could be considered, but a\nuser would likely already expect that the above command line considers\nthe lack of rev other than for --topics as meaning all local branches,\nlike when there is no argument at all.\n\nMoreover, when using -r and -a along with --topics, the first local or\nremote-tracking branch, depending on alphabetic order is used instead of\nthe one given after --topics (any rev given on the command line is\nactually simply ignored when either -r or -a is given). And if no rev is\ngiven at all, the fact that the first alphetical branch is the base of\ntopics is probably not expected by users (Maybe --topics should always\nrequire one rev on the command line?)\n\nThis change makes\n  \"show-branch --topics $rev\"\nact as\n  \"show-branch --topics $rev $(git for-each-ref refs/heads\n                               --format='%(refname:short)')\"\n\n  \"show-branch -r --topics $rev ...\"\nact as\n  \"show-branch --topics $rev ... $(git for-each-ref refs/remotes\n                                   --format='%(refname:short)')\"\ninstead of\n  \"show-branch --topics $(git for-each-ref refs/remotes\n                          --format='%(refname:short)')\"\n\nand\n  \"show-branch -a --topics $rev ...\"\nact as\n  \"show-branch --topics $rev ... $(git for-each-ref refs/heads refs/remotes\n                                   --format='%(refname:short)')\"\ninstead of\n  \"show-branch --topics $(git for-each-ref refs/heads refs/remotes\n                          --format='%(refname:short)')\"\n\nSigned-off-by: Mike Hommey <mh@glandium.org>\n---\n builtin/show-branch.c | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/show-branch.c b/builtin/show-branch.c\nindex f3fb5fb..e69fb7c 100644\n--- a/builtin/show-branch.c\n+++ b/builtin/show-branch.c\n@@ -718,7 +718,7 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)\n \t}\n \n \t/* If nothing is specified, show all branches by default */\n-\tif (ac + all_heads + all_remotes == 0)\n+\tif (ac <= topics && all_heads + all_remotes == 0)\n \t\tall_heads = 1;\n \n \tif (reflog) {\n@@ -785,13 +785,13 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)\n \t\t}\n \t\tfree(ref);\n \t}\n-\telse if (all_heads + all_remotes)\n-\t\tsnarf_refs(all_heads, all_remotes);\n \telse {\n \t\twhile (0 < ac) {\n \t\t\tappend_one_rev(*av);\n \t\t\tac--; av++;\n \t\t}\n+\t\tif (all_heads + all_remotes)\n+\t\t\tsnarf_refs(all_heads, all_remotes);\n \t}\n \n \thead_p = resolve_ref_unsafe(\"HEAD\", RESOLVE_REF_READING,\n-- \n2.4.0.rc0.1.g7f94b17\n"},{"id":"258700","messageId":"xmqqd23qw2cl.fsf@gitster.dls.corp.google.com","threadId":"38949","inReplyTo":"1427753543-19680-1-git-send-email-mh@glandium.org","subject":"Re: [PATCH] show-branch: show all local heads when only giving one rev along --topics","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-03-30T22:24:26Z","receivedAt":"2015-03-30T22:24:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Mike Hommey <mh@glandium.org> writes:\n\n> (Maybe --topics should always require one rev on the command\n> line?)\n\nThat sounds line a good thing to do.\n\n> -\telse if (all_heads + all_remotes)\n> -\t\tsnarf_refs(all_heads, all_remotes);\n>  \telse {\n>  \t\twhile (0 < ac) {\n>  \t\t\tappend_one_rev(*av);\n>  \t\t\tac--; av++;\n>  \t\t}\n> +\t\tif (all_heads + all_remotes)\n> +\t\t\tsnarf_refs(all_heads, all_remotes);\n\nHmmmmmm.  Is this safe and will not cause problems by possibly\nduplicated refnames that came from the command line and the ones\nthat came from for-each-ref iteration?  I am not saying the change\nis problematic; it is just I haven't looked at this code for a long\ntime that the existing machinery is already designed to tolerate\nduplicated input.\n\nThanks.\n"},{"id":"258701","messageId":"xmqq8ueew1wc.fsf@gitster.dls.corp.google.com","threadId":"38949","inReplyTo":"xmqqd23qw2cl.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] show-branch: show all local heads when only giving one rev along --topics","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-03-30T22:34:11Z","receivedAt":"2015-03-30T22:34:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> ...?  I am not saying the change\n> is problematic; it is just I haven't looked at this code for a long\n> time that the existing machinery is already designed to tolerate\n> duplicated input.\n\n\"for a long time to say that the existing code is OK or not\" is what\nI meant to say.  Sorry for the noise.\n\n> Thanks.\n"},{"id":"258702","messageId":"20150330223752.GA19987@glandium.org","threadId":"38949","inReplyTo":"xmqqd23qw2cl.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] show-branch: show all local heads when only giving one rev along --topics","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2015-03-30T22:37:52Z","receivedAt":"2015-03-30T22:37:52Z","isPatch":true,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"On Mon, Mar 30, 2015 at 03:24:26PM -0700, Junio C Hamano wrote:\n> Mike Hommey <mh@glandium.org> writes:\n> \n> > (Maybe --topics should always require one rev on the command\n> > line?)\n> \n> That sounds line a good thing to do.\n> \n> > -\telse if (all_heads + all_remotes)\n> > -\t\tsnarf_refs(all_heads, all_remotes);\n> >  \telse {\n> >  \t\twhile (0 < ac) {\n> >  \t\t\tappend_one_rev(*av);\n> >  \t\t\tac--; av++;\n> >  \t\t}\n> > +\t\tif (all_heads + all_remotes)\n> > +\t\t\tsnarf_refs(all_heads, all_remotes);\n> \n> Hmmmmmm.  Is this safe and will not cause problems by possibly\n> duplicated refnames that came from the command line and the ones\n> that came from for-each-ref iteration?  I am not saying the change\n> is problematic; it is just I haven't looked at this code for a long\n> time that the existing machinery is already designed to tolerate\n> duplicated input.\n\nIt is:\nhttps://github.com/git/git/blob/master/builtin/show-branch.c#L382\n\nIn case you wonder about allow_dups, the only case in which it's 1 is:\nhttps://github.com/git/git/blob/master/builtin/show-branch.c#L784\nwhich is the reflog case, which is the case before that `else` in the\npatch.\n\nThat is, both append_one_rev and snarf_refs end up calling append_ref\nwith allow_dups=0.\n\nCheers,\n\nMike\n"}]}