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

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 17, 2016, 22:14 UTC
Message-ID: <xmqqwpr7etvs.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20160117151510.GC15519@sigill.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 6 quoted lines

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

Yes, I think this should be the way to go.

Show 5 quoted lines

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

And the target can be longer than 40-hex. Don't play games with padding.

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

Yes, there should be, as I would imagine the most natural interpretation of "--symrefs" by end users would be "show me ALSO the symref information.", not "show me ONLY the symref information" (if it were the latter we woudln't be seeing refs/tags/mark in the above output).

I also suspect that this part of the patch is wrong (or at least misleading):

@@ -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);
+		if (symrefs && !flags)
+			continue;
 		if (!check_ref_type(ref, flags))
 			continue;
 		if (!tail_match(pattern, ref->name))

It looks wrong that the usual filtering with check_ref_type() and
tail_match() are bypassed for symbolic refs.  Even though the server
side currently feeds reflog information for only "HEAD", the code
should be prepared to do the sane thing in a future in which that
server-side limitation is corrected, and allow the user to ask

    ls-remote $there --symref refs/remotes/origin/HEAD

to learn the information on one specific ref, without having to see
the primary branch of $there repository, for example.

Thanks.