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

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

[Heiko Voigt]

In commit 4d5e1b1319 ("gitk: Show detached HEAD if --all is specified",
2014-09-09) the intention was to have detached HEAD shown when the --all
argument is given.

This was solved by appending HEAD to the revs list. By doing that the
behavior using the --not argument is now broken, since that inverts the
meaning of all following arguments passed to git rev-parse.

Lets fix this by prepending HEAD instead of appending, this way there
can not be any '--not' in front.

This was discovered because

	gitk --all --not origin/master

does not display the same revs as

	gitk --all ^origin/master

which it should.

Signed-off-by: Heiko Voigt <hvoigt@hvoigt.net>
---
 gitk | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gitk b/gitk
index a14d7a1..19d95cd 100755
--- a/gitk
+++ b/gitk
@@ -295,7 +295,7 @@ proc parseviewrevs {view revs} {
     if {$revs eq {}} {
 	set revs HEAD
     } elseif {[lsearch -exact $revs --all] >= 0} {
-	lappend revs HEAD
+	linsert revs 0 HEAD
     }
     if {[catch {set ids [eval exec git rev-parse $revs]} err]} {
 	# we get stdout followed by stderr in $err
-- 
2.21.0

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

[Johannes Schindelin]

Hi Heiko,

On Thu, 4 Jul 2019, Heiko Voigt wrote:

> In commit 4d5e1b1319 ("gitk: Show detached HEAD if --all is specified",
> 2014-09-09) the intention was to have detached HEAD shown when the --all
> argument is given.
>
> This was solved by appending HEAD to the revs list. By doing that the
> behavior using the --not argument is now broken, since that inverts the
> meaning of all following arguments passed to git rev-parse.
>
> Lets fix this by prepending HEAD instead of appending, this way there
> can not be any '--not' in front.
>
> This was discovered because
>
> 	gitk --all --not origin/master
>
> does not display the same revs as
>
> 	gitk --all ^origin/master
>
> which it should.
>
> Signed-off-by: Heiko Voigt <hvoigt@hvoigt.net>

Good description.

> ---
>  gitk | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/gitk b/gitk
> index a14d7a1..19d95cd 100755
> --- a/gitk
> +++ b/gitk
> @@ -295,7 +295,7 @@ proc parseviewrevs {view revs} {
>      if {$revs eq {}} {
>  	set revs HEAD
>      } elseif {[lsearch -exact $revs --all] >= 0} {
> -	lappend revs HEAD
> +	linsert revs 0 HEAD

For a moment, I wondered whether there is any case where `HEAD` might not
be appropriate as first argument, but you're right, the revision parsing
machinery allows mixing options and rev arguments.

In short: this patch looks good to me.

Thanks,
Dscho

>      }
>      if {[catch {set ids [eval exec git rev-parse $revs]} err]} {
>  	# we get stdout followed by stderr in $err
> --
> 2.21.0
>
>

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

[Heiko Voigt]

Hi Dscho,

On Thu, Jul 04, 2019 at 12:38:44PM +0200, Johannes Schindelin wrote:
> On Thu, 4 Jul 2019, Heiko Voigt wrote:
[...]
> > Signed-off-by: Heiko Voigt <hvoigt@hvoigt.net>
> 
> Good description.

Thanks. I am actually surprised that for almost 5 years nobody noticed
this. It seems either nobody is using --not this way or everyone took it
as a feature that HEAD would be removed and will complain once this get
released ;)

I usually use the caret notation, but I guess this time I was lazy and
the dash was easier to type...

> > ---
> >  gitk | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/gitk b/gitk
> > index a14d7a1..19d95cd 100755
> > --- a/gitk
> > +++ b/gitk
> > @@ -295,7 +295,7 @@ proc parseviewrevs {view revs} {
> >      if {$revs eq {}} {
> >  	set revs HEAD
> >      } elseif {[lsearch -exact $revs --all] >= 0} {
> > -	lappend revs HEAD
> > +	linsert revs 0 HEAD
> 
> For a moment, I wondered whether there is any case where `HEAD` might not
> be appropriate as first argument, but you're right, the revision parsing
> machinery allows mixing options and rev arguments.

Thanks for double checking.

> In short: this patch looks good to me.

Thanks for the quick review!

Cheers Heiko

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

[Junio C Hamano]

Heiko Voigt <hvoigt@hvoigt.net> writes:

> In commit 4d5e1b1319 ("gitk: Show detached HEAD if --all is specified",
> 2014-09-09) the intention was to have detached HEAD shown when the --all
> argument is given.

The "do we have --all?" test added by that old commit is not quite
satisfying in the first place.  E.g. we do not check if there is a
double-dash before it.  This change also relies on an ancient design
mistake of allowing non-dashed options before a dashed one, adding
more to dissatisfaction by making a future change to correct the
design mistake harder.

I think in the longer term we should consider changing "git
rev-parse --all" to include HEAD in the concept of "all refs"
instead.  But in the meantime, this patch is not making things
drastically wrong, so let's take it as is.

Thanks.

>
> Signed-off-by: Heiko Voigt <hvoigt@hvoigt.net>
> ---
>  gitk | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/gitk b/gitk
> index a14d7a1..19d95cd 100755
> --- a/gitk
> +++ b/gitk
> @@ -295,7 +295,7 @@ proc parseviewrevs {view revs} {
>      if {$revs eq {}} {
>  	set revs HEAD
>      } elseif {[lsearch -exact $revs --all] >= 0} {
> -	lappend revs HEAD
> +	linsert revs 0 HEAD
>      }
>      if {[catch {set ids [eval exec git rev-parse $revs]} err]} {
>  	# we get stdout followed by stderr in $err

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

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Heiko Voigt <hvoigt@hvoigt.net> writes:
>
>> In commit 4d5e1b1319 ("gitk: Show detached HEAD if --all is specified",
>> 2014-09-09) the intention was to have detached HEAD shown when the --all
>> argument is given.
>
> The "do we have --all?" test added by that old commit is not quite
> satisfying in the first place.  E.g. we do not check if there is a
> double-dash before it.  This change also relies on an ancient design
> mistake of allowing non-dashed options before a dashed one, adding
> more to dissatisfaction by making a future change to correct the
> design mistake harder.

Actually, I do not think this patch is a good idea.

This command

   $ git rev-list $commit --not --all

is a good way to ask "See if $commit is anchored to the repository
with any of refs or HEAD".  It does so by marking the tips of all
refs and HEAD as negative (i.e. stop the travesal) endpoints and
mark given $commit as a positive endpoint.

The commits listed by feeding the output of the above to the
rev-list command would be the ones that are only reachable by
$commit and not any of the refs.

The "--all" in rev-list family (including "git log") unconditionally
include HEAD.  The glitch here is that "--all" in rev-parse does
not.  And 4d5e1b1319 was an attempt to "fix" that, i.e. make "--all"
imply "HEAD".  That is, the original code we can see in your patch
appends "HEAD" to the list of args, so

   $ gitk $commit --not --all

ends up in running

   $ git rev-parse $commit --not --all HEAD

and the result are used as the traversal endpoints (aka "arguments
to rev-list command").  And that is exactly what the user wants to
see happen.

But you do not want to *prepend* HEAD to make the command line look
this way:

   $ git rev-parse HEAD $commit --not --all

which I think is what your patch does.  It asks a completely
different question: what are the commits reachable from either HEAD
or $commit that are not reachable from any of our refs?

What you want to do is to make sure your additional "HEAD" always
goes together with the existing "--all" the user gave you.

As the code is _already_ finding the _exact_ location on the command
line where "--all" appears, I think you can go one step further and
make sure you insert the "HEAD" immediately after "--all", as that
exactly matches what you (and the ancient 4d5e1b1319) are trying to
achieve: pretend as if "--all" always include "HEAD", even when it
is detached.

This is orthogonal to the question I posed in my earlier reply
(i.e. "we found --all; is it really a 'give me all refs' request
given by the user, or something else (is it an argument to another
option, like "--grep '--all'", or is it pathspec after '--'), but
assuming that we have reliably found the "--all" on the command line
the user meant as "give me all refs", I think inserting HEAD
immediately after that location would be the right solution.  It is
incorrect to unconditionally append as your original example shows,
but it is equally incorrect to unconditionally prepend.

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

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> The "--all" in rev-list family (including "git log") unconditionally
> include HEAD.  The glitch here is that "--all" in rev-parse does
> not.  And 4d5e1b1319 was an attempt to "fix" that, i.e. make "--all"
> imply "HEAD".

And it becomes really tempting to get rid of that "let's tweak
--all" hack and declare that "rev-parse --all" is simply buggy,
proposing a simple "bugfix" that may look like this (not even
compile tested, but you get the idea).

 builtin/rev-parse.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
index f8bbe6d47e..94f9a6efba 100644
--- a/builtin/rev-parse.c
+++ b/builtin/rev-parse.c
@@ -766,6 +766,7 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
 			}
 			if (!strcmp(arg, "--all")) {
 				for_each_ref(show_reference, NULL);
+				head_ref(show_reference, NULL);
 				clear_ref_exclusion(&ref_excludes);
 				continue;
 			}

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

[Heiko Voigt]

On Mon, Jul 08, 2019 at 10:16:50PM -0700, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
> 
> > The "--all" in rev-list family (including "git log") unconditionally
> > include HEAD.  The glitch here is that "--all" in rev-parse does
> > not.  And 4d5e1b1319 was an attempt to "fix" that, i.e. make "--all"
> > imply "HEAD".
> 
> And it becomes really tempting to get rid of that "let's tweak
> --all" hack and declare that "rev-parse --all" is simply buggy,
> proposing a simple "bugfix" that may look like this (not even
> compile tested, but you get the idea).

Thanks for this nice pointer.

Lets think about this a little more, because this would give us a proper
solution. There would be a need to be backwards compatible to not break
peoples scripts right? The documentation says --all "Show all refs found
in refs/" so IMO we need some extra option that changes the '--all'
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?

Cheers Heiko

> 
>  builtin/rev-parse.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
> index f8bbe6d47e..94f9a6efba 100644
> --- a/builtin/rev-parse.c
> +++ b/builtin/rev-parse.c
> @@ -766,6 +766,7 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
>  			}
>  			if (!strcmp(arg, "--all")) {
>  				for_each_ref(show_reference, NULL);
> +				head_ref(show_reference, NULL);
>  				clear_ref_exclusion(&ref_excludes);
>  				continue;
>  			}

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

[Junio C Hamano]

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?

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.

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

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?  

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.

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).

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.



>
> Cheers Heiko
>
>> 
>>  builtin/rev-parse.c | 1 +
>>  1 file changed, 1 insertion(+)
>> 
>> diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
>> index f8bbe6d47e..94f9a6efba 100644
>> --- a/builtin/rev-parse.c
>> +++ b/builtin/rev-parse.c
>> @@ -766,6 +766,7 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
>>  			}
>>  			if (!strcmp(arg, "--all")) {
>>  				for_each_ref(show_reference, NULL);
>> +				head_ref(show_reference, NULL);
>>  				clear_ref_exclusion(&ref_excludes);
>>  				continue;
>>  			}

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

[Heiko Voigt]

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/

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

[Junio C Hamano]

Heiko Voigt <hvoigt@hvoigt.net> writes:

>      if {$revs eq {}} {
>         set revs HEAD
> -    } elseif {[lsearch -exact $revs --all] >= 0} {
> -       lappend revs HEAD
> +    } else {
> +       linsert revs 0 --all-include-head
>      }

OK.  So the new option means "from here on, the meaning of the
'--all' option changes its meaning from 'all refs' to 'all refs and
HEAD'".  That way, gitk does not have to guess if '--all' found on
the command line is an option or something else (e.g. pathspec etc.)

That makes sense.  It would be a no-op if '--all' is not used, which
is also good.

>> 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?

That is, to drive "gitk --all ^origin/master"?  If HEAD is detached,
isn't the history that leads to it something that is "not merged
into a stable branch", too?  IOW, I think you would want the same
behaviour as "git log --all ^origin/master" for that use case, and
treat HEAD just like any of the refs.

Back in the days, detached HEAD was mostly tentative state, but
these days, especially for those who use submodules, wouldn't it be
a norm to have your checkout associated with a detached HEAD?  I
think treating (detached) HEAD just like any of the refs matches
the end-user expectations even more these days.

>> 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]

Yup.  See above.  I think the time has changed the needs.

Thanks.

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

[Johannes Sixt]

Am 10.07.19 um 20:40 schrieb Junio C Hamano:
> 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?
> 
> 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.

When --all is in the game, HEAD of the current worktree isn't all that
special among the heads of all worktrees, I would think. What if we
added a new option --heads that incorporates all worktree heads?

If we require users to type something to tell what they mean, then I
think a more generally useful command line option would be preferable
over an option that modifies the meaning of another option.

-- Hannes

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

[Heiko Voigt]

On Mon, Jul 08, 2019 at 09:55:00PM -0700, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
> 
> > Heiko Voigt <hvoigt@hvoigt.net> writes:
> >
> >> In commit 4d5e1b1319 ("gitk: Show detached HEAD if --all is specified",
> >> 2014-09-09) the intention was to have detached HEAD shown when the --all
> >> argument is given.
> >
> > The "do we have --all?" test added by that old commit is not quite
> > satisfying in the first place.  E.g. we do not check if there is a
> > double-dash before it.  This change also relies on an ancient design
> > mistake of allowing non-dashed options before a dashed one, adding
> > more to dissatisfaction by making a future change to correct the
> > design mistake harder.
> 
> Actually, I do not think this patch is a good idea.
> 
[...]
> 
> As the code is _already_ finding the _exact_ location on the command
> line where "--all" appears, I think you can go one step further and
> make sure you insert the "HEAD" immediately after "--all", as that
> exactly matches what you (and the ancient 4d5e1b1319) are trying to
> achieve: pretend as if "--all" always include "HEAD", even when it
> is detached.
> 
> This is orthogonal to the question I posed in my earlier reply
> (i.e. "we found --all; is it really a 'give me all refs' request
> given by the user, or something else (is it an argument to another
> option, like "--grep '--all'", or is it pathspec after '--'), but
> assuming that we have reliably found the "--all" on the command line
> the user meant as "give me all refs", I think inserting HEAD
> immediately after that location would be the right solution.  It is
> incorrect to unconditionally append as your original example shows,
> but it is equally incorrect to unconditionally prepend.

Yes I agree, there are too many other use cases that my change will
break. I tried to replace a hack with another quick hack, but that did
not make it better.

Will reply to the other mail with some more questions.

Cheers Heiko