Re: [PATCH 4/4] ls-remote: add support for showing symrefs

[Thomas Gummerer <t.gummerer@gmail.com>]

On 01/17, Jeff King wrote:
> On Sun, Jan 17, 2016 at 12:04:03PM +0100, Thomas Gummerer wrote:
>
> > Sometimes it's useful to know the main branch of a git repository
> > without actually downloading the repository.  This can be done by
> > looking at the symrefs stored in the remote repository.  Currently git
> > doesn't provide a simple way to show the symrefs stored on the remote
> > repository, even though the information is available.  Add a --symrefs
> > command line argument to the ls-remote command, which shows the symrefs
> > on the remote repository.
> >
> > The new argument works similar to the --heads and --tags arguments.
> > When only --symrefs is given, only the symrefs are shown.  It can
> > however be combined with the --refs, --heads or --tags arguments, to
> > show all refs, heads, or tags as well.
>
> I would have expected --symrefs to be "also show symref destinations for
> refs we are showing" and not otherwise affect the set of refs we pick.
> That would make:
>
>   git ls-remote --symrefs
>
> show everything, including symrefs and peeled tags (which I think is not
> possible with your patch). It would also make:
>
>   git ls-remote --symrefs --heads
>
> show symrefs and refs in refs/heads, but _not_ show symrefs outside
> of refs/heads.
>
> On the flip side, though, it does not provide a way to just get the
> symrefs without any other output.

That's why I decided to implement it this way.  However I think the
below makes sense, so I'll change the behavior in the re-roll.  In
case we really want to have only symrefs we could still introduce
something like --symrefs-only, though I'm doubtful we'll need that.

> But I think if you just want a specific
> symref (say, HEAD), you can ask for it:
>
>   git ls-remote --symrefs $remote HEAD
>
> This is all somewhat moot, perhaps, as the server side currently only
> shares symref information for HEAD. But that may change in the future
> (it's a limitation of the current protocol).
>
> I'm also somewhat doubtful that people regularly use ls-remote much at
> all these days, let alone with "--heads" or "--tags". So it's hard to
> come up with concrete use cases for any of this. The above is just what
> I would expect for general flexibility and consistency with other
> commands.
>
> > ---
> >  Documentation/git-ls-remote.txt |  8 +++++++-
> >  builtin/ls-remote.c             |  9 ++++++++-
> >  t/t5512-ls-remote.sh            | 20 ++++++++++++++++++++
>
> Documentation _and_ tests. Yay. :)
>
> > @@ -98,6 +101,10 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
> >  	if (!dest && !quiet)
> >  		fprintf(stderr, "From %s\n", *remote->url);
> >  	for ( ; ref; ref = ref->next) {
> > +		if (symrefs && ref->symref)
> > +			printf("symref: %s	%s\n", ref->symref, ref->name);
>
> I assume that's a raw tab in the string. Please use "\t", which makes it
> more obvious to readers what is going on.

Thanks, will change.

> Since this output is only triggered by a new option, we're not
> constrained by compatibility in the output. But I think it's still a
> good idea to keep the general "<content>\t<refname>" pattern set by the
> other lines, as you did.
>
> Here's my obligatory bikeshedding for the format. Feel free to ignore.
>
> I wondered if just:
>
>   refs/heads/master       HEAD
>   1bd44cb9d13204b0fe1958db0082f5028a16eb3a       refs/heads/master
>
> would look nicer. It is technically ambiguous if a symref can point to a
> 40-hex refname. They generally will start with refs/, but I'm not sure
> that is strict requirement. It also makes it harder to add other output
> later if we choose to. So some kind of keyword like "symref:" is a good
> idea.
>
> We could also do it as:
>
>   ref: refs/heads/master     HEAD
>
> which matches the symref format itself. I guess that doesn't really
> matter here, but somehow it seems more aesthetically pleasing to me.

I like this format the most so far, so unless I hear any objections
I'll do this.

> The output would look a lot nicer for humans if we right-padded the
> symref destination to match the 40-hex that is on all the other lines
> (so all of the refnames line up).  But that makes machine-parsing a lot
> harder. We could do something clever with isatty(1), but I don't think
> it's worth the effort.
>
> > +test_expect_success 'ls-remote with symrefs and refs combined' '
> > +	cat >expect <<-EOF &&
> > +	symref: refs/heads/master	HEAD
> > +	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/heads/master
> > +	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/remotes/origin/HEAD
> > +	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/remotes/origin/master
> > +	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/tags/mark
> > +	EOF
>
> I expected there to be a:
>
>   1bd44cb9d13204b0fe1958db0082f5028a16eb3a	HEAD
>
> line. It's technically redundant, since the caller can dereference
> refs/heads/master themselves, but it potentially makes things easier for
> a caller. I realized why it isn't here, though. We print all symrefs,
> regardless of whether they match the flags, but "HEAD" doesn't match
> "--refs", so we don't show its value.  Under the semantics I proposed
> above, "ls-remote --symrefs" would show both.
>
> -Peff

--
Thomas