Re: [PATCH 3/3] transport.c: introduce core.alternateRefsPrefixes

[Taylor Blau <me@ttaylorr.com>]

On Fri, Sep 21, 2018 at 03:19:20AM -0400, Eric Sunshine wrote:
> On Thu, Sep 20, 2018 at 2:04 PM Taylor Blau <ttaylorr@github.com> wrote:
> > The recently-introduced "core.alternateRefsCommand" allows callers to
> > specify with high flexibility the tips that they wish to advertise from
> > alternates. This flexibility comes at the cost of some inconvenience
> > when the caller only wishes to limit the advertisement to one or more
> > prefixes.
> > [...]
> > Signed-off-by: Taylor Blau <me@ttaylorr.com>
> > ---
> > diff --git a/t/t5410-receive-pack.sh b/t/t5410-receive-pack.sh
> > @@ -44,4 +44,15 @@ test_expect_success 'with core.alternateRefsCommand' '
> > +test_expect_success 'with core.alternateRefsPrefixes' '
> > +       test_config -C fork core.alternateRefsPrefixes "refs/tags" &&
> > +       cat >expect <<-EOF &&
> > +       $(git rev-parse one) .have
> > +       $(git rev-parse three) .have
> > +       $(git rev-parse two) .have
> > +       EOF
>
> It's probably a matter of taste as to which is more readable, but this
> entire "cat <<EOF" block could be replaced with a simple one-liner:
>
>     printf "%s .have\n" $(git rev-parse one three two) >expect &&
>
> Same comment applies to previous patch, as well.

That's a good idea. I amended both patches to replace the 'cat <<-EOF
...' block with your suggestion above. It's tempting to introduce it as:

  expect_haves() {
    printf "%s .have\n" $(git rev-parse -- $@)
  }

And call it as:

  expect_haves one three two >expect

But I'm not sure whether I think that this is better or worse than
writing it twice inline. I think that the test is small enough that it
doesn't really matter either way, but I think that I've convinced myself
while composing this email that expect_haves() is an OK idea.

If you feel strongly that it isn't, please let me know, and I'll write
them inline before sending v2.

Thanks,
Taylor