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

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 17, 2017, 17:17 UTC
Message-ID: <xmqqd1df961c.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <015A7026-960F-450C-9276-AAC3A0B11207@gmail.com>

Lars Schneider <larsxschneider@gmail.com> writes:

Show 21 quoted lines

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

The above was done merely to point out the problematic place and a possible solution. As I am not familiar with the code in git-p4.py, I didn't even try to check if the code already has a helper function that strips "refs/heads/" from the beginning of the string (iow, I am not happy with the [11:]). I didn't and don't like the fact that this function now runs "symbolic-ref HEAD" twice but I didn't try to see if there are more suitable and idiomatic ways to do this with a single invocation.

Hence, I would really prefer not to commit mine myself. I'd rather see somebody from git-p4 circle to come up with a version that is more in line with the way things are done in the existing code and send a tested version for me to apply.

Thanks.