Re: [PATCH] gitk: fix --all behavior combined with --not

[Heiko Voigt <hvoigt@hvoigt.net>]

On Wed, Jul 10, 2019 at 11:40:50AM -0700, Junio C Hamano wrote:
> Heiko Voigt <hvoigt@hvoigt.net> writes:
> 
> > behavior. How about '--all-include-head'. Then e.g.
> >
> >     git rev-parse --all-include-head --all --not origin/master
> >
> > would include the head ref like you proposed below?
> >
> > What do you think? Or would you rather go the route of changing
> > rev-parse behavior?
> 
> Depends on what you mean by the above.  Do you mean that now the end
> user needs to say
> 
> 	gitk --all-include-head --not origin/master
> 
> to get a rough equivalent of
> 
> 	git log --graph --oneline --all --not origin/master
> 
> due to the discrepancy between how "rev-parse" and "rev-list" treat
> their "--all" option?  Or do you mean that the end user still says
> "--all", and after (reliably by some means) making sure that "--all"
> given by the end-user is a request for "all refs and HEAD", we turn
> that into the above internal rev-parse call?

Sorry for being not specific enough. I would be aiming for the latter
and gitk would prepend --all-include-head to its rev-parse call. To have some
code to talk about something like this (based on your pointer and also not
compile tested):

diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
index f8bbe6d47e..03928ee566 100644
--- a/builtin/rev-parse.c
+++ b/builtin/rev-parse.c
@@ -585,6 +585,7 @@ static void handle_ref_opt(const char *pattern, const char *prefix)
 int cmd_rev_parse(int argc, const char **argv, const char *prefix)
 {
        int i, as_is = 0, verify = 0, quiet = 0, revs_count = 0, type = 0;
+       int all_include_head = 0;
        int did_repo_setup = 0;
        int has_dashdash = 0;
        int output_prefix = 0;
@@ -764,8 +765,14 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
                                }
                                continue;
                        }
+                       if (!strcmp(arg, "--all-include-head")) {
+                               all_include_head = 1;
+                               continue;
+                       }
                        if (!strcmp(arg, "--all")) {
                                for_each_ref(show_reference, NULL);
+                               if (all_include_head)
+                                       head_ref(show_reference, NULL);
                                clear_ref_exclusion(&ref_excludes);
                                continue;
                        }
diff --git a/gitk-git/gitk b/gitk-git/gitk
index a14d7a16b2..ddd1de5377 100755
--- a/gitk-git/gitk
+++ b/gitk-git/gitk
@@ -294,8 +294,8 @@ proc parseviewrevs {view revs} {
 
     if {$revs eq {}} {
        set revs HEAD
-    } elseif {[lsearch -exact $revs --all] >= 0} {
-       lappend revs HEAD
+    } else {
+       linsert revs 0 --all-include-head
     }
     if {[catch {set ids [eval exec git rev-parse $revs]} err]} {
        # we get stdout followed by stderr in $err

> If the former, then quite honestly, we shouldn't doing anything,
> perhaps other than reverting 4d5e1b1319.  The users can type
> 
> 	$ gitk --all HEAD --not origin/master
> 	$ gitk $commit --not --all HEAD
> 
> themselves, instead of --all-include-head.

Yes the former would not make anything better.

> If the latter, I am not sure what the endgame should be.  

Please see the diff above.

> It certainly *is* safer not to unconditionallyl and unilaterally
> change the behaviour of "rev-parse --all", so I am all for starting
> with small and fully backward compatible change, but wouldn't
> scripts other than gitk want the same behaviour?  

Yes probably, but in my experience, if some behavior is around for a long time,
someone will rely on it and rev-parse seems like a candidate that might get
used in scripts for CIs or similar. E.g. in a bare repo someone might
explicitely want to omit HEAD.

> To put it the other way around, what use case would we have that we
> want to enumerate all refs but not HEAD, *and* exclude HEAD only
> when HEAD is detached?  I can see the use of "what are commits
> reachable from the current HEAD but not reachable from any of the
> refs/*?" and that would be useful whether HEAD is detached or is on
> a concrete branch, so "rev-parse --all" that does not include
> detached HEAD alone does not feel so useful at least to me.

What about my example. My use case is: Show me everything that is not merged
into a stable branch (i.e. origin/master). For a human viewer it does not
really matter if an extra detachted HEAD is shown, but for a CI script it
might. Ok this might be quite artificial, what do you think?

> I am reasonably sure that back when "rev-parse --all" was invented,
> the use of detached HEAD was not all that prevalent (I would not be
> surprised if it hadn't been invented yet), so it being documented to
> enumerate all refs does not necessarily contradict to include HEAD
> if it is different from any of the ref tips (i.e. detached).

I just dug up the old discussion to this to find some reasoning why this was
not changed. So you have changed your mind about this? [1]

> And if we cannot commit to changing the "rev-parse --all" (and I am
> not sure I can at this point---I am wary of changes), as we know
> where "--all" appeared on the command line, inserting HEAD immediately
> after it at the script level is probably the change with the least
> potential damage we can make, without changing anything else.

Well this should be better than the current solution. But there is still your
point about not taking -- into account. So how about my backwards compatible
suggestion above, what do you think?

Cheers Heiko

[1] https://public-inbox.org/git/xmqqsika2c2i.fsf@gitster.dls.corp.google.com/