{"thread":{"id":"51426","subject":"[PATCH] gitk: fix --all behavior combined with --not","startedAt":"2019-07-04T08:09:34Z","lastAt":"2019-07-11T18:55:17Z","messageCount":12,"participants":["Heiko Voigt","Johannes Schindelin","Junio C Hamano","Johannes Sixt"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"378578","messageId":"20190704080907.GA45656@book.hvoigt.net","threadId":"51426","inReplyTo":null,"subject":"[PATCH] gitk: fix --all behavior combined with --not","fromName":"Heiko Voigt","fromEmail":"hvoigt@hvoigt.net","sentAt":"2019-07-04T08:09:07Z","receivedAt":"2019-07-04T08:09:34Z","isPatch":true,"sender":{"key":"hvoigt@hvoigt.net","avatar":"https://avatars.githubusercontent.com/u/184958?v=4"},"body":"In commit 4d5e1b1319 (\"gitk: Show detached HEAD if --all is specified\",\n2014-09-09) the intention was to have detached HEAD shown when the --all\nargument is given.\n\nThis was solved by appending HEAD to the revs list. By doing that the\nbehavior using the --not argument is now broken, since that inverts the\nmeaning of all following arguments passed to git rev-parse.\n\nLets fix this by prepending HEAD instead of appending, this way there\ncan not be any '--not' in front.\n\nThis was discovered because\n\n\tgitk --all --not origin/master\n\ndoes not display the same revs as\n\n\tgitk --all ^origin/master\n\nwhich it should.\n\nSigned-off-by: Heiko Voigt <hvoigt@hvoigt.net>\n---\n gitk | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/gitk b/gitk\nindex a14d7a1..19d95cd 100755\n--- a/gitk\n+++ b/gitk\n@@ -295,7 +295,7 @@ proc parseviewrevs {view revs} {\n     if {$revs eq {}} {\n \tset revs HEAD\n     } elseif {[lsearch -exact $revs --all] >= 0} {\n-\tlappend revs HEAD\n+\tlinsert revs 0 HEAD\n     }\n     if {[catch {set ids [eval exec git rev-parse $revs]} err]} {\n \t# we get stdout followed by stderr in $err\n-- \n2.21.0\n\n"},{"id":"378595","messageId":"nycvar.QRO.7.76.6.1907041236200.44@tvgsbejvaqbjf.bet","threadId":"51426","inReplyTo":"20190704080907.GA45656@book.hvoigt.net","subject":"Re: [PATCH] gitk: fix --all behavior combined with --not","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-07-04T10:38:44Z","receivedAt":"2019-07-04T10:38:20Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Heiko,\n\nOn Thu, 4 Jul 2019, Heiko Voigt wrote:\n\n> In commit 4d5e1b1319 (\"gitk: Show detached HEAD if --all is specified\",\n> 2014-09-09) the intention was to have detached HEAD shown when the --all\n> argument is given.\n>\n> This was solved by appending HEAD to the revs list. By doing that the\n> behavior using the --not argument is now broken, since that inverts the\n> meaning of all following arguments passed to git rev-parse.\n>\n> Lets fix this by prepending HEAD instead of appending, this way there\n> can not be any '--not' in front.\n>\n> This was discovered because\n>\n> \tgitk --all --not origin/master\n>\n> does not display the same revs as\n>\n> \tgitk --all ^origin/master\n>\n> which it should.\n>\n> Signed-off-by: Heiko Voigt <hvoigt@hvoigt.net>\n\nGood description.\n\n> ---\n>  gitk | 2 +-\n>  1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/gitk b/gitk\n> index a14d7a1..19d95cd 100755\n> --- a/gitk\n> +++ b/gitk\n> @@ -295,7 +295,7 @@ proc parseviewrevs {view revs} {\n>      if {$revs eq {}} {\n>  \tset revs HEAD\n>      } elseif {[lsearch -exact $revs --all] >= 0} {\n> -\tlappend revs HEAD\n> +\tlinsert revs 0 HEAD\n\nFor a moment, I wondered whether there is any case where `HEAD` might not\nbe appropriate as first argument, but you're right, the revision parsing\nmachinery allows mixing options and rev arguments.\n\nIn short: this patch looks good to me.\n\nThanks,\nDscho\n\n>      }\n>      if {[catch {set ids [eval exec git rev-parse $revs]} err]} {\n>  \t# we get stdout followed by stderr in $err\n> --\n> 2.21.0\n>\n>\n"},{"id":"378598","messageId":"20190704113114.GA52663@book.hvoigt.net","threadId":"51426","inReplyTo":"nycvar.QRO.7.76.6.1907041236200.44@tvgsbejvaqbjf.bet","subject":"Re: [PATCH] gitk: fix --all behavior combined with --not","fromName":"Heiko Voigt","fromEmail":"hvoigt@hvoigt.net","sentAt":"2019-07-04T11:31:14Z","receivedAt":"2019-07-04T11:31:26Z","isPatch":true,"sender":{"key":"hvoigt@hvoigt.net","avatar":"https://avatars.githubusercontent.com/u/184958?v=4"},"body":"Hi Dscho,\n\nOn Thu, Jul 04, 2019 at 12:38:44PM +0200, Johannes Schindelin wrote:\n> On Thu, 4 Jul 2019, Heiko Voigt wrote:\n[...]\n> > Signed-off-by: Heiko Voigt <hvoigt@hvoigt.net>\n> \n> Good description.\n\nThanks. I am actually surprised that for almost 5 years nobody noticed\nthis. It seems either nobody is using --not this way or everyone took it\nas a feature that HEAD would be removed and will complain once this get\nreleased ;)\n\nI usually use the caret notation, but I guess this time I was lazy and\nthe dash was easier to type...\n\n> > ---\n> >  gitk | 2 +-\n> >  1 file changed, 1 insertion(+), 1 deletion(-)\n> >\n> > diff --git a/gitk b/gitk\n> > index a14d7a1..19d95cd 100755\n> > --- a/gitk\n> > +++ b/gitk\n> > @@ -295,7 +295,7 @@ proc parseviewrevs {view revs} {\n> >      if {$revs eq {}} {\n> >  \tset revs HEAD\n> >      } elseif {[lsearch -exact $revs --all] >= 0} {\n> > -\tlappend revs HEAD\n> > +\tlinsert revs 0 HEAD\n> \n> For a moment, I wondered whether there is any case where `HEAD` might not\n> be appropriate as first argument, but you're right, the revision parsing\n> machinery allows mixing options and rev arguments.\n\nThanks for double checking.\n\n> In short: this patch looks good to me.\n\nThanks for the quick review!\n\nCheers Heiko\n"},{"id":"378698","messageId":"xmqq4l3wz6y8.fsf@gitster-ct.c.googlers.com","threadId":"51426","inReplyTo":"20190704080907.GA45656@book.hvoigt.net","subject":"Re: [PATCH] gitk: fix --all behavior combined with --not","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-07-08T19:01:35Z","receivedAt":"2019-07-08T19:01:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Heiko Voigt <hvoigt@hvoigt.net> writes:\n\n> In commit 4d5e1b1319 (\"gitk: Show detached HEAD if --all is specified\",\n> 2014-09-09) the intention was to have detached HEAD shown when the --all\n> argument is given.\n\nThe \"do we have --all?\" test added by that old commit is not quite\nsatisfying in the first place.  E.g. we do not check if there is a\ndouble-dash before it.  This change also relies on an ancient design\nmistake of allowing non-dashed options before a dashed one, adding\nmore to dissatisfaction by making a future change to correct the\ndesign mistake harder.\n\nI think in the longer term we should consider changing \"git\nrev-parse --all\" to include HEAD in the concept of \"all refs\"\ninstead.  But in the meantime, this patch is not making things\ndrastically wrong, so let's take it as is.\n\nThanks.\n\n>\n> Signed-off-by: Heiko Voigt <hvoigt@hvoigt.net>\n> ---\n>  gitk | 2 +-\n>  1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/gitk b/gitk\n> index a14d7a1..19d95cd 100755\n> --- a/gitk\n> +++ b/gitk\n> @@ -295,7 +295,7 @@ proc parseviewrevs {view revs} {\n>      if {$revs eq {}} {\n>  \tset revs HEAD\n>      } elseif {[lsearch -exact $revs --all] >= 0} {\n> -\tlappend revs HEAD\n> +\tlinsert revs 0 HEAD\n>      }\n>      if {[catch {set ids [eval exec git rev-parse $revs]} err]} {\n>  \t# we get stdout followed by stderr in $err\n"},{"id":"378718","messageId":"xmqqr26zx0wr.fsf@gitster-ct.c.googlers.com","threadId":"51426","inReplyTo":"xmqq4l3wz6y8.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] gitk: fix --all behavior combined with --not","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-07-09T04:55:00Z","receivedAt":"2019-07-09T04:55:10Z","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> Heiko Voigt <hvoigt@hvoigt.net> writes:\n>\n>> In commit 4d5e1b1319 (\"gitk: Show detached HEAD if --all is specified\",\n>> 2014-09-09) the intention was to have detached HEAD shown when the --all\n>> argument is given.\n>\n> The \"do we have --all?\" test added by that old commit is not quite\n> satisfying in the first place.  E.g. we do not check if there is a\n> double-dash before it.  This change also relies on an ancient design\n> mistake of allowing non-dashed options before a dashed one, adding\n> more to dissatisfaction by making a future change to correct the\n> design mistake harder.\n\nActually, I do not think this patch is a good idea.\n\nThis command\n\n   $ git rev-list $commit --not --all\n\nis a good way to ask \"See if $commit is anchored to the repository\nwith any of refs or HEAD\".  It does so by marking the tips of all\nrefs and HEAD as negative (i.e. stop the travesal) endpoints and\nmark given $commit as a positive endpoint.\n\nThe commits listed by feeding the output of the above to the\nrev-list command would be the ones that are only reachable by\n$commit and not any of the refs.\n\nThe \"--all\" in rev-list family (including \"git log\") unconditionally\ninclude HEAD.  The glitch here is that \"--all\" in rev-parse does\nnot.  And 4d5e1b1319 was an attempt to \"fix\" that, i.e. make \"--all\"\nimply \"HEAD\".  That is, the original code we can see in your patch\nappends \"HEAD\" to the list of args, so\n\n   $ gitk $commit --not --all\n\nends up in running\n\n   $ git rev-parse $commit --not --all HEAD\n\nand the result are used as the traversal endpoints (aka \"arguments\nto rev-list command\").  And that is exactly what the user wants to\nsee happen.\n\nBut you do not want to *prepend* HEAD to make the command line look\nthis way:\n\n   $ git rev-parse HEAD $commit --not --all\n\nwhich I think is what your patch does.  It asks a completely\ndifferent question: what are the commits reachable from either HEAD\nor $commit that are not reachable from any of our refs?\n\nWhat you want to do is to make sure your additional \"HEAD\" always\ngoes together with the existing \"--all\" the user gave you.\n\nAs the code is _already_ finding the _exact_ location on the command\nline where \"--all\" appears, I think you can go one step further and\nmake sure you insert the \"HEAD\" immediately after \"--all\", as that\nexactly matches what you (and the ancient 4d5e1b1319) are trying to\nachieve: pretend as if \"--all\" always include \"HEAD\", even when it\nis detached.\n\nThis is orthogonal to the question I posed in my earlier reply\n(i.e. \"we found --all; is it really a 'give me all refs' request\ngiven by the user, or something else (is it an argument to another\noption, like \"--grep '--all'\", or is it pathspec after '--'), but\nassuming that we have reliably found the \"--all\" on the command line\nthe user meant as \"give me all refs\", I think inserting HEAD\nimmediately after that location would be the right solution.  It is\nincorrect to unconditionally append as your original example shows,\nbut it is equally incorrect to unconditionally prepend.\n"},{"id":"378719","messageId":"xmqqk1crwzwd.fsf@gitster-ct.c.googlers.com","threadId":"51426","inReplyTo":"xmqqr26zx0wr.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] gitk: fix --all behavior combined with --not","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-07-09T05:16:50Z","receivedAt":"2019-07-09T05:16:56Z","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> The \"--all\" in rev-list family (including \"git log\") unconditionally\n> include HEAD.  The glitch here is that \"--all\" in rev-parse does\n> not.  And 4d5e1b1319 was an attempt to \"fix\" that, i.e. make \"--all\"\n> imply \"HEAD\".\n\nAnd it becomes really tempting to get rid of that \"let's tweak\n--all\" hack and declare that \"rev-parse --all\" is simply buggy,\nproposing a simple \"bugfix\" that may look like this (not even\ncompile tested, but you get the idea).\n\n builtin/rev-parse.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/builtin/rev-parse.c b/builtin/rev-parse.c\nindex f8bbe6d47e..94f9a6efba 100644\n--- a/builtin/rev-parse.c\n+++ b/builtin/rev-parse.c\n@@ -766,6 +766,7 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)\n \t\t\t}\n \t\t\tif (!strcmp(arg, \"--all\")) {\n \t\t\t\tfor_each_ref(show_reference, NULL);\n+\t\t\t\thead_ref(show_reference, NULL);\n \t\t\t\tclear_ref_exclusion(&ref_excludes);\n \t\t\t\tcontinue;\n \t\t\t}\n"},{"id":"378755","messageId":"20190710074428.GA65621@book.hvoigt.net","threadId":"51426","inReplyTo":"xmqqr26zx0wr.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] gitk: fix --all behavior combined with --not","fromName":"Heiko Voigt","fromEmail":"hvoigt@hvoigt.net","sentAt":"2019-07-10T07:44:28Z","receivedAt":"2019-07-10T07:44:44Z","isPatch":true,"sender":{"key":"hvoigt@hvoigt.net","avatar":"https://avatars.githubusercontent.com/u/184958?v=4"},"body":"On Mon, Jul 08, 2019 at 09:55:00PM -0700, Junio C Hamano wrote:\n> Junio C Hamano <gitster@pobox.com> writes:\n> \n> > Heiko Voigt <hvoigt@hvoigt.net> writes:\n> >\n> >> In commit 4d5e1b1319 (\"gitk: Show detached HEAD if --all is specified\",\n> >> 2014-09-09) the intention was to have detached HEAD shown when the --all\n> >> argument is given.\n> >\n> > The \"do we have --all?\" test added by that old commit is not quite\n> > satisfying in the first place.  E.g. we do not check if there is a\n> > double-dash before it.  This change also relies on an ancient design\n> > mistake of allowing non-dashed options before a dashed one, adding\n> > more to dissatisfaction by making a future change to correct the\n> > design mistake harder.\n> \n> Actually, I do not think this patch is a good idea.\n> \n[...]\n> \n> As the code is _already_ finding the _exact_ location on the command\n> line where \"--all\" appears, I think you can go one step further and\n> make sure you insert the \"HEAD\" immediately after \"--all\", as that\n> exactly matches what you (and the ancient 4d5e1b1319) are trying to\n> achieve: pretend as if \"--all\" always include \"HEAD\", even when it\n> is detached.\n> \n> This is orthogonal to the question I posed in my earlier reply\n> (i.e. \"we found --all; is it really a 'give me all refs' request\n> given by the user, or something else (is it an argument to another\n> option, like \"--grep '--all'\", or is it pathspec after '--'), but\n> assuming that we have reliably found the \"--all\" on the command line\n> the user meant as \"give me all refs\", I think inserting HEAD\n> immediately after that location would be the right solution.  It is\n> incorrect to unconditionally append as your original example shows,\n> but it is equally incorrect to unconditionally prepend.\n\nYes I agree, there are too many other use cases that my change will\nbreak. I tried to replace a hack with another quick hack, but that did\nnot make it better.\n\nWill reply to the other mail with some more questions.\n\nCheers Heiko\n"},{"id":"378757","messageId":"20190710075835.GB65621@book.hvoigt.net","threadId":"51426","inReplyTo":"xmqqk1crwzwd.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] gitk: fix --all behavior combined with --not","fromName":"Heiko Voigt","fromEmail":"hvoigt@hvoigt.net","sentAt":"2019-07-10T07:58:35Z","receivedAt":"2019-07-10T07:58:57Z","isPatch":true,"sender":{"key":"hvoigt@hvoigt.net","avatar":"https://avatars.githubusercontent.com/u/184958?v=4"},"body":"On Mon, Jul 08, 2019 at 10:16:50PM -0700, Junio C Hamano wrote:\n> Junio C Hamano <gitster@pobox.com> writes:\n> \n> > The \"--all\" in rev-list family (including \"git log\") unconditionally\n> > include HEAD.  The glitch here is that \"--all\" in rev-parse does\n> > not.  And 4d5e1b1319 was an attempt to \"fix\" that, i.e. make \"--all\"\n> > imply \"HEAD\".\n> \n> And it becomes really tempting to get rid of that \"let's tweak\n> --all\" hack and declare that \"rev-parse --all\" is simply buggy,\n> proposing a simple \"bugfix\" that may look like this (not even\n> compile tested, but you get the idea).\n\nThanks for this nice pointer.\n\nLets think about this a little more, because this would give us a proper\nsolution. There would be a need to be backwards compatible to not break\npeoples scripts right? The documentation says --all \"Show all refs found\nin refs/\" so IMO we need some extra option that changes the '--all'\nbehavior. How about '--all-include-head'. Then e.g.\n\n    git rev-parse --all-include-head --all --not origin/master\n\nwould include the head ref like you proposed below?\n\nWhat do you think? Or would you rather go the route of changing\nrev-parse behavior?\n\nCheers Heiko\n\n> \n>  builtin/rev-parse.c | 1 +\n>  1 file changed, 1 insertion(+)\n> \n> diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c\n> index f8bbe6d47e..94f9a6efba 100644\n> --- a/builtin/rev-parse.c\n> +++ b/builtin/rev-parse.c\n> @@ -766,6 +766,7 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)\n>  \t\t\t}\n>  \t\t\tif (!strcmp(arg, \"--all\")) {\n>  \t\t\t\tfor_each_ref(show_reference, NULL);\n> +\t\t\t\thead_ref(show_reference, NULL);\n>  \t\t\t\tclear_ref_exclusion(&ref_excludes);\n>  \t\t\t\tcontinue;\n>  \t\t\t}\n"},{"id":"378771","messageId":"xmqqa7dlu40d.fsf@gitster-ct.c.googlers.com","threadId":"51426","inReplyTo":"20190710075835.GB65621@book.hvoigt.net","subject":"Re: [PATCH] gitk: fix --all behavior combined with --not","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-07-10T18:40:50Z","receivedAt":"2019-07-10T18:40:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Heiko Voigt <hvoigt@hvoigt.net> writes:\n\n> behavior. How about '--all-include-head'. Then e.g.\n>\n>     git rev-parse --all-include-head --all --not origin/master\n>\n> would include the head ref like you proposed below?\n>\n> What do you think? Or would you rather go the route of changing\n> rev-parse behavior?\n\nDepends on what you mean by the above.  Do you mean that now the end\nuser needs to say\n\n\tgitk --all-include-head --not origin/master\n\nto get a rough equivalent of\n\n\tgit log --graph --oneline --all --not origin/master\n\ndue to the discrepancy between how \"rev-parse\" and \"rev-list\" treat\ntheir \"--all\" option?  Or do you mean that the end user still says\n\"--all\", and after (reliably by some means) making sure that \"--all\"\ngiven by the end-user is a request for \"all refs and HEAD\", we turn\nthat into the above internal rev-parse call?\n\nIf the former, then quite honestly, we shouldn't doing anything,\nperhaps other than reverting 4d5e1b1319.  The users can type\n\n\t$ gitk --all HEAD --not origin/master\n\t$ gitk $commit --not --all HEAD\n\nthemselves, instead of --all-include-head.\n\nIf the latter, I am not sure what the endgame should be.  \n\nIt certainly *is* safer not to unconditionallyl and unilaterally\nchange the behaviour of \"rev-parse --all\", so I am all for starting\nwith small and fully backward compatible change, but wouldn't\nscripts other than gitk want the same behaviour?  \n\nTo put it the other way around, what use case would we have that we\nwant to enumerate all refs but not HEAD, *and* exclude HEAD only\nwhen HEAD is detached?  I can see the use of \"what are commits\nreachable from the current HEAD but not reachable from any of the\nrefs/*?\" and that would be useful whether HEAD is detached or is on\na concrete branch, so \"rev-parse --all\" that does not include\ndetached HEAD alone does not feel so useful at least to me.\n\nI am reasonably sure that back when \"rev-parse --all\" was invented,\nthe use of detached HEAD was not all that prevalent (I would not be\nsurprised if it hadn't been invented yet), so it being documented to\nenumerate all refs does not necessarily contradict to include HEAD\nif it is different from any of the ref tips (i.e. detached).\n\nAnd if we cannot commit to changing the \"rev-parse --all\" (and I am\nnot sure I can at this point---I am wary of changes), as we know\nwhere \"--all\" appeared on the command line, inserting HEAD immediately\nafter it at the script level is probably the change with the least\npotential damage we can make, without changing anything else.\n\n\n\n>\n> Cheers Heiko\n>\n>> \n>>  builtin/rev-parse.c | 1 +\n>>  1 file changed, 1 insertion(+)\n>> \n>> diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c\n>> index f8bbe6d47e..94f9a6efba 100644\n>> --- a/builtin/rev-parse.c\n>> +++ b/builtin/rev-parse.c\n>> @@ -766,6 +766,7 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)\n>>  \t\t\t}\n>>  \t\t\tif (!strcmp(arg, \"--all\")) {\n>>  \t\t\t\tfor_each_ref(show_reference, NULL);\n>> +\t\t\t\thead_ref(show_reference, NULL);\n>>  \t\t\t\tclear_ref_exclusion(&ref_excludes);\n>>  \t\t\t\tcontinue;\n>>  \t\t\t}\n"},{"id":"378803","messageId":"20190711122452.GC65621@book.hvoigt.net","threadId":"51426","inReplyTo":"xmqqa7dlu40d.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] gitk: fix --all behavior combined with --not","fromName":"Heiko Voigt","fromEmail":"hvoigt@hvoigt.net","sentAt":"2019-07-11T12:24:52Z","receivedAt":"2019-07-11T12:25:29Z","isPatch":true,"sender":{"key":"hvoigt@hvoigt.net","avatar":"https://avatars.githubusercontent.com/u/184958?v=4"},"body":"On Wed, Jul 10, 2019 at 11:40:50AM -0700, Junio C Hamano wrote:\n> Heiko Voigt <hvoigt@hvoigt.net> writes:\n> \n> > behavior. How about '--all-include-head'. Then e.g.\n> >\n> >     git rev-parse --all-include-head --all --not origin/master\n> >\n> > would include the head ref like you proposed below?\n> >\n> > What do you think? Or would you rather go the route of changing\n> > rev-parse behavior?\n> \n> Depends on what you mean by the above.  Do you mean that now the end\n> user needs to say\n> \n> \tgitk --all-include-head --not origin/master\n> \n> to get a rough equivalent of\n> \n> \tgit log --graph --oneline --all --not origin/master\n> \n> due to the discrepancy between how \"rev-parse\" and \"rev-list\" treat\n> their \"--all\" option?  Or do you mean that the end user still says\n> \"--all\", and after (reliably by some means) making sure that \"--all\"\n> given by the end-user is a request for \"all refs and HEAD\", we turn\n> that into the above internal rev-parse call?\n\nSorry for being not specific enough. I would be aiming for the latter\nand gitk would prepend --all-include-head to its rev-parse call. To have some\ncode to talk about something like this (based on your pointer and also not\ncompile tested):\n\ndiff --git a/builtin/rev-parse.c b/builtin/rev-parse.c\nindex f8bbe6d47e..03928ee566 100644\n--- a/builtin/rev-parse.c\n+++ b/builtin/rev-parse.c\n@@ -585,6 +585,7 @@ static void handle_ref_opt(const char *pattern, const char *prefix)\n int cmd_rev_parse(int argc, const char **argv, const char *prefix)\n {\n        int i, as_is = 0, verify = 0, quiet = 0, revs_count = 0, type = 0;\n+       int all_include_head = 0;\n        int did_repo_setup = 0;\n        int has_dashdash = 0;\n        int output_prefix = 0;\n@@ -764,8 +765,14 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)\n                                }\n                                continue;\n                        }\n+                       if (!strcmp(arg, \"--all-include-head\")) {\n+                               all_include_head = 1;\n+                               continue;\n+                       }\n                        if (!strcmp(arg, \"--all\")) {\n                                for_each_ref(show_reference, NULL);\n+                               if (all_include_head)\n+                                       head_ref(show_reference, NULL);\n                                clear_ref_exclusion(&ref_excludes);\n                                continue;\n                        }\ndiff --git a/gitk-git/gitk b/gitk-git/gitk\nindex a14d7a16b2..ddd1de5377 100755\n--- a/gitk-git/gitk\n+++ b/gitk-git/gitk\n@@ -294,8 +294,8 @@ proc parseviewrevs {view revs} {\n \n     if {$revs eq {}} {\n        set revs HEAD\n-    } elseif {[lsearch -exact $revs --all] >= 0} {\n-       lappend revs HEAD\n+    } else {\n+       linsert revs 0 --all-include-head\n     }\n     if {[catch {set ids [eval exec git rev-parse $revs]} err]} {\n        # we get stdout followed by stderr in $err\n\n> If the former, then quite honestly, we shouldn't doing anything,\n> perhaps other than reverting 4d5e1b1319.  The users can type\n> \n> \t$ gitk --all HEAD --not origin/master\n> \t$ gitk $commit --not --all HEAD\n> \n> themselves, instead of --all-include-head.\n\nYes the former would not make anything better.\n\n> If the latter, I am not sure what the endgame should be.  \n\nPlease see the diff above.\n\n> It certainly *is* safer not to unconditionallyl and unilaterally\n> change the behaviour of \"rev-parse --all\", so I am all for starting\n> with small and fully backward compatible change, but wouldn't\n> scripts other than gitk want the same behaviour?  \n\nYes probably, but in my experience, if some behavior is around for a long time,\nsomeone will rely on it and rev-parse seems like a candidate that might get\nused in scripts for CIs or similar. E.g. in a bare repo someone might\nexplicitely want to omit HEAD.\n\n> To put it the other way around, what use case would we have that we\n> want to enumerate all refs but not HEAD, *and* exclude HEAD only\n> when HEAD is detached?  I can see the use of \"what are commits\n> reachable from the current HEAD but not reachable from any of the\n> refs/*?\" and that would be useful whether HEAD is detached or is on\n> a concrete branch, so \"rev-parse --all\" that does not include\n> detached HEAD alone does not feel so useful at least to me.\n\nWhat about my example. My use case is: Show me everything that is not merged\ninto a stable branch (i.e. origin/master). For a human viewer it does not\nreally matter if an extra detachted HEAD is shown, but for a CI script it\nmight. Ok this might be quite artificial, what do you think?\n\n> I am reasonably sure that back when \"rev-parse --all\" was invented,\n> the use of detached HEAD was not all that prevalent (I would not be\n> surprised if it hadn't been invented yet), so it being documented to\n> enumerate all refs does not necessarily contradict to include HEAD\n> if it is different from any of the ref tips (i.e. detached).\n\nI just dug up the old discussion to this to find some reasoning why this was\nnot changed. So you have changed your mind about this? [1]\n\n> And if we cannot commit to changing the \"rev-parse --all\" (and I am\n> not sure I can at this point---I am wary of changes), as we know\n> where \"--all\" appeared on the command line, inserting HEAD immediately\n> after it at the script level is probably the change with the least\n> potential damage we can make, without changing anything else.\n\nWell this should be better than the current solution. But there is still your\npoint about not taking -- into account. So how about my backwards compatible\nsuggestion above, what do you think?\n\nCheers Heiko\n\n[1] https://public-inbox.org/git/xmqqsika2c2i.fsf@gitster.dls.corp.google.com/\n"},{"id":"378832","messageId":"ca11f7c4-d6d4-3813-3066-37775ce3f48f@kdbg.org","threadId":"51426","inReplyTo":"xmqqa7dlu40d.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] gitk: fix --all behavior combined with --not","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2019-07-11T17:11:15Z","receivedAt":"2019-07-11T17:11:20Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 10.07.19 um 20:40 schrieb Junio C Hamano:\n> Heiko Voigt <hvoigt@hvoigt.net> writes:\n> \n>> behavior. How about '--all-include-head'. Then e.g.\n>>\n>>     git rev-parse --all-include-head --all --not origin/master\n>>\n>> would include the head ref like you proposed below?\n>>\n>> What do you think? Or would you rather go the route of changing\n>> rev-parse behavior?\n> \n> Depends on what you mean by the above.  Do you mean that now the end\n> user needs to say\n> \n> \tgitk --all-include-head --not origin/master\n> \n> to get a rough equivalent of\n> \n> \tgit log --graph --oneline --all --not origin/master\n> \n> due to the discrepancy between how \"rev-parse\" and \"rev-list\" treat\n> their \"--all\" option?  Or do you mean that the end user still says\n> \"--all\", and after (reliably by some means) making sure that \"--all\"\n> given by the end-user is a request for \"all refs and HEAD\", we turn\n> that into the above internal rev-parse call?\n> \n> If the former, then quite honestly, we shouldn't doing anything,\n> perhaps other than reverting 4d5e1b1319.  The users can type\n> \n> \t$ gitk --all HEAD --not origin/master\n> \t$ gitk $commit --not --all HEAD\n> \n> themselves, instead of --all-include-head.\n\nWhen --all is in the game, HEAD of the current worktree isn't all that\nspecial among the heads of all worktrees, I would think. What if we\nadded a new option --heads that incorporates all worktree heads?\n\nIf we require users to type something to tell what they mean, then I\nthink a more generally useful command line option would be preferable\nover an option that modifies the meaning of another option.\n\n-- Hannes\n"},{"id":"378854","messageId":"xmqq7e8os8oi.fsf@gitster-ct.c.googlers.com","threadId":"51426","inReplyTo":"20190711122452.GC65621@book.hvoigt.net","subject":"Re: [PATCH] gitk: fix --all behavior combined with --not","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-07-11T18:55:09Z","receivedAt":"2019-07-11T18:55:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Heiko Voigt <hvoigt@hvoigt.net> writes:\n\n>      if {$revs eq {}} {\n>         set revs HEAD\n> -    } elseif {[lsearch -exact $revs --all] >= 0} {\n> -       lappend revs HEAD\n> +    } else {\n> +       linsert revs 0 --all-include-head\n>      }\n\nOK.  So the new option means \"from here on, the meaning of the\n'--all' option changes its meaning from 'all refs' to 'all refs and\nHEAD'\".  That way, gitk does not have to guess if '--all' found on\nthe command line is an option or something else (e.g. pathspec etc.)\n\nThat makes sense.  It would be a no-op if '--all' is not used, which\nis also good.\n\n>> To put it the other way around, what use case would we have that we\n>> want to enumerate all refs but not HEAD, *and* exclude HEAD only\n>> when HEAD is detached?  I can see the use of \"what are commits\n>> reachable from the current HEAD but not reachable from any of the\n>> refs/*?\" and that would be useful whether HEAD is detached or is on\n>> a concrete branch, so \"rev-parse --all\" that does not include\n>> detached HEAD alone does not feel so useful at least to me.\n>\n> What about my example. My use case is: Show me everything that is not merged\n> into a stable branch (i.e. origin/master). For a human viewer it does not\n> really matter if an extra detachted HEAD is shown, but for a CI script it\n> might. Ok this might be quite artificial, what do you think?\n\nThat is, to drive \"gitk --all ^origin/master\"?  If HEAD is detached,\nisn't the history that leads to it something that is \"not merged\ninto a stable branch\", too?  IOW, I think you would want the same\nbehaviour as \"git log --all ^origin/master\" for that use case, and\ntreat HEAD just like any of the refs.\n\nBack in the days, detached HEAD was mostly tentative state, but\nthese days, especially for those who use submodules, wouldn't it be\na norm to have your checkout associated with a detached HEAD?  I\nthink treating (detached) HEAD just like any of the refs matches\nthe end-user expectations even more these days.\n\n>> I am reasonably sure that back when \"rev-parse --all\" was invented,\n>> the use of detached HEAD was not all that prevalent (I would not be\n>> surprised if it hadn't been invented yet), so it being documented to\n>> enumerate all refs does not necessarily contradict to include HEAD\n>> if it is different from any of the ref tips (i.e. detached).\n>\n> I just dug up the old discussion to this to find some reasoning why this was\n> not changed. So you have changed your mind about this? [1]\n\nYup.  See above.  I think the time has changed the needs.\n\nThanks.\n"}]}