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

[Jeff King <peff@peff.net>]

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

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.

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