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

From: Jeff King <peff@peff.net>
Date: Jan 17, 2016, 15:15 UTC
Message-ID: <20160117151510.GC15519@sigill.intra.peff.net>
In-Reply-To: <1453028643-13978-6-git-send-email-t.gummerer@gmail.com>

On Sun, Jan 17, 2016 at 12:04:03PM +0100, Thomas Gummerer wrote:

Show 12 quoted lines

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

Show 6 quoted lines

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

Show 8 quoted lines

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