Re: [PATCH 2/2] name-rev: favor describing with tags and use committer date to tiebreak

[Lars Schneider <larsxschneider@gmail.com>]

> On 17 Mar 2017, at 13:56, Junio C Hamano <gitster@pobox.com> wrote:
> 
> Junio C Hamano <gitster@pobox.com> writes:
> 
>> Lars Schneider <larsxschneider@gmail.com> writes:
>> 
>>> A quick bisect indicates that this patch might break 
>>> t9807-git-p4-submit.sh 8 and 13. I haven't looked into
>>> it further, yet.
>> 
>> As I do not do P4, help in diagnosing why it breaks is appreciated.
>> If the test script expects...
>> On the other hand, if git-p4 command internally uses name-rev and it
>> is not prepared to properly handle commits that can be named in more
>> than one way, the problem would be deeper, as it would mean it can
>> misbehave even without the change to name-rev when multiple branches
>> point at the same commit.
> 
> Yikes.  Perhaps something along this line?  
> 
> This function seems to want to learn which branch we are on, and
> running "name-rev HEAD" is *NEVER* the right way to do so.  If you
> are on branch B which happens to point at the same commit as branch
> A, "name-rev HEAD" can say either A or B (and it is likely it would
> say A simply because it sorts earlier, and the logic seems to favor
> the one that was discovered earlier when all else being equal).
> 
> git-p4.py | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/git-p4.py b/git-p4.py
> index eab319d76e..351d1ab58e 100755
> --- a/git-p4.py
> +++ b/git-p4.py
> @@ -582,7 +582,7 @@ def currentGitBranch():
>         # on a detached head
>         return None
>     else:
> -        return read_pipe(["git", "name-rev", "HEAD"]).split(" ")[1].strip()
> +        return read_pipe(["git", "symbolic-ref", "HEAD"]).strip()[11:]
> 
> def isValidGitDir(path):
>     return git_dir(path) != None

Following your explanation this patch looks good to me and this fixes the
test failure. TBH I never thought about the difference of these commands
before. "rev" and "ref" sound so similar although they denote completely 
different things.

Thanks,
Lars