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

[John Cai <johncai86@gmail.com>]

Hi Phillip,

On 9 Apr 2024, at 11:25, Phillip Wood wrote:

> Hi John
>
> On 08/04/2024 18:38, John Cai via GitGitGadget wrote:
>> +--symbolic-name::
>> +
>> +	Print out the value the reference points to without dereferencing. This
>> +	is useful to know the reference that a symbolic ref is pointing to.
>
> It would be helpful to clarify that this prints the contents of the symbolic ref without dereferencing but also prints the OID after dereferencing the given ref.

Thanks, will clarify in the next version.

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

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