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

> On Mon, Jan 18, 2016 at 05:57:18PM +0100, Thomas Gummerer wrote:
>
> > While there, replace a literal tab in the format string with \t to make
> > it more obvious to the reader.
>
> Heh, I didn't notice in the first review that you actually inherited
> that from the original. Definitely worth doing, IMHO.
>
> > @@ -101,7 +104,9 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
> >  			continue;
> >  		if (!tail_match(pattern, ref->name))
> >  			continue;
> > -		printf("%s	%s\n", oid_to_hex(&ref->old_oid), ref->name);
> > +		if (symrefs && ref->symref)
> > +			printf("ref: %s\t%s\n", ref->symref, ref->name);
> > +		printf("%s\t%s\n", oid_to_hex(&ref->old_oid), ref->name);
> >  		status = 0; /* we found something */
>
> Yeah, this looks like the right logic to me.
>
> > +test_expect_success 'ls-remote --symrefs' '
> > +	cat >expect <<-EOF &&
>
> Please use "<<-\EOF" here (and in the test below) to prevent
> interpolation. It's not wrong in your case, but it's easier for a reader
> (or somebody who later modifies the test) to not have to wonder what you
> were expecting to be expanded. So as a general style, we quote our
> here-doc markers.

> > +	ref: refs/heads/master	HEAD
> > +	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	HEAD
> > +	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/heads/master
> > +	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/remotes/origin/HEAD
> > +	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/remotes/origin/master
> > +	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/tags/mark
> > +	EOF
> > +	git ls-remote --symrefs >actual &&
> > +	test_cmp expect actual
> > +'
>
> This test covers "symrefs, along with everything". And this one:
>
> > +test_expect_success 'ls-remote with filtered symrefs' '
> > +	cat >expect <<-EOF &&
> > +	ref: refs/heads/master	HEAD
> > +	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	HEAD
> > +	EOF
> > +	git ls-remote --symrefs . HEAD >actual &&
> > +	test_cmp expect actual
> > +'
>
> covers symrefs plus a refname filter. It would be nice to also test that
> "git ls-remote --symrefs --heads" shows "refs/heads/foo" as a symref.
> But that cannot work with the current code, because upload-pack only
> tells us about the symref HEAD, and not any others.
>
> This may change in the future, though. I'm not sure if it's worth
> squashing in the expect_failure test below. The "negative" one below
> that does tell us something, though it is somewhat redundant (it does
> catch the "always show symrefs" logic from your original version, but
> it seems unlikely that would pop up as a regression).
>
> ---
> diff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh
> index 3edbc9e..92fc7e9 100755
> --- a/t/t5512-ls-remote.sh
> +++ b/t/t5512-ls-remote.sh
> @@ -176,7 +176,7 @@ test_expect_success 'ls-remote --symrefs' '
>  	test_cmp expect actual
>  '
>
> -test_expect_success 'ls-remote with filtered symrefs' '
> +test_expect_success 'ls-remote with filtered symrefs (refname)' '
>  	cat >expect <<-EOF &&
>  	ref: refs/heads/master	HEAD
>  	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	HEAD
> @@ -185,4 +185,27 @@ test_expect_success 'ls-remote with filtered symrefs' '
>  	test_cmp expect actual
>  '
>
> +test_expect_failure 'ls-remote with filtered symrefs (--heads)' '
> +	git symbolic-ref refs/heads/foo refs/tags/mark &&
> +	cat >expect <<-\EOF &&
> +	ref: refs/heads/bar	refs/tags/mark
> +	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/heads/foo
> +	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/heads/master
> +	EOF
> +	git ls-remote --symrefs --heads . >actual &&
> +	test_cmp expect actual
> +'

> +
> +test_expect_success 'ls-remote --symrefs omits filtered-out matches' '
> +	cat >expect <<-\EOF &&
> +	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/heads/foo
> +	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/heads/master
> +	EOF
> +	git ls-remote --symrefs --heads . >actual &&
> +	test_cmp expect actual &&
> +	git ls-remote --symrefs . "refs/heads/*" >actual &&
> +	test_cmp expect actual
> +'
> +
> +
>  test_done
>