From: phillip.wood123@gmail.com Date: Fri, 12 Apr 2024 09:37:16 GMT Subject: Re: [PATCH v2 3/3] show-ref: add --symbolic-name option Message-ID: <4c0b6da2-56d3-4441-aaed-8d9a2d28d9c0@gmail.com> In-Reply-To: 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: >>> + >>> +----------- >>> + SP SP >>> +----------- >>> + >>> +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