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

[Junio C Hamano <gitster@pobox.com>]

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.