Re: [PATCH v2 3/3] show-ref: add --symbolic-name option

[phillip.wood123@gmail.com]

Hi John

On 11/04/2024 20:57, John Cai wrote:
> On 9 Apr 2024, at 11:25, Phillip Wood wrote:
>>> +When using `--symbolic-name`, the output is in the format:
>>> +
>>> +-----------
>>> +<oid> SP <ref> SP <symbolic-name>
>>> +-----------
>>> +
>>> +For example,
>>> +
>>> +-----------------------------------------------------------------------------
>>> +$ git show-ref --symbolic-name
>>> +b75428bae1d090f60bdd4b67185f814bc8f0819d refs/heads/SYMBOLIC_REF ref:refs/heads/main
>>
>> Do we really need the "ref:" prefix? It is not specified above and I think anyone calling this would have to remove the prefix before they could use the value.
> 
> I can see how it would be more ergonimic to just have the value without the
> "ref: " prefix. I kept it because that's how the refs are represented on disk
> and git-symbolic-ref prints them out with the "ref: " prefix.
> 
> I don't have a strong preference, but I lean a bit towards keeping it consistent
> with the output of other commands.

I agree it is a good idea to keep things consistent, and dropping the 
"ref:" prefix is consistent with other commands:

$ git symbolic-ref HEAD
refs/heads/rebase-fix-signoff

Best Wishes

Phillip

> 
>>
>>
>>> +test_expect_success '--symbolic-name with symbolic ref' '
>>> +	test_when_finished "git symbolic-ref -d refs/heads/SYMBOLIC_REF_A" &&
>>> +	commit_oid=$(git rev-parse refs/heads/$GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME) &&
>>> +	cat >expect <<-EOF &&
>>> +	$commit_oid refs/heads/SYMBOLIC_REF_A ref:refs/heads/$GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
>>> +	EOF
>>> +	git symbolic-ref refs/heads/SYMBOLIC_REF_A refs/heads/$GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME &&
>>> +	git show-ref --symbolic-name SYMBOLIC_REF_A >actual &&
>>> +	test_cmp expect actual
>>> +'
>>
>> I think it would be nice to see a test along the lines of
>>
>> 	git symbolic-ref refs/symref-c refs/heads/master
>> 	git symbolic-ref refs/symref-b refs/symref-c &&
>> 	git symbolic-ref refs/symref-a refs/symref-b &&
>> 	git show-ref --symbolic-name refs/symref-a >actual &&
>> 	cat >expect <<\EOF &&
>> 	$(git show-ref -s --verify refs/heads/master) refs/heads/symref-a refs/heads/symref-b
>> 	EOF
>> 	test_cmp expect actual
>>
>> to show what this command is expected to return when there is a chain of symbolic references.
> 
> good point, will add this in the next series.
> 
>>
>> Best Wishes
>>
>> Phillip
> 
> thanks for the review!
> John