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

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 10, 2019, 18:40 UTC
Message-ID: <xmqqa7dlu40d.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <20190710075835.GB65621@book.hvoigt.net>

Heiko Voigt <hvoigt@hvoigt.net> writes:

Show 8 quoted lines

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

Show 19 quoted lines

>
> 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;
>>  			}