On 01/18, Jeff King wrote:
> 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.
Will do, thanks!
> > + 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
> +'
I'm a bit confused by this. Shouldn't the "ref: refs/heads/bar
refs/tags/mark" line only show up when we use --tags, not --heads?
Also should refs/heads/bar be refs/heads/foo?
> +
> +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
>
--
Thomas
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html