Re: [PATCH v4 1/2] describe: setup working tree for --dirty

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Feb 3, 2019, 03:35 UTC
Message-ID: <CAPig+cRg55g_x7XvfHuP4J17ODnmWp+o7x=Wqpq6v2TCWcgcuw@mail.gmail.com>
In-Reply-To: <CA+xP2Sa2zLeLg-6pFn9UEufurd=EOOf8UFx3Qh7o0dwzJgN-ag@mail.gmail.com>

On Sat, Feb 2, 2019 at 5:05 AM Sebastian Staudt <koraktor@gmail.com> wrote:

Show 23 quoted lines

> Am Fr., 1. Feb. 2019 um 21:12 Uhr schrieb Eric Sunshine
> <sunshine@sunshineco.com>:
> > On Fri, Feb 1, 2019 at 8:55 AM Sebastian Staudt <koraktor@gmail.com> wrote:
> > > diff --git a/t/t6120-describe.sh b/t/t6120-describe.sh
> > > @@ -145,14 +145,38 @@ check_describe A-* HEAD
> > > +test_expect_success 'describe --dirty with --work-tree' '
> > > +       [...]
> > > +'
> > > +test_expect_success 'describe --dirty with --work-tree' '
> > > +       [...]
> > > +'
> >
> > Can you give these two new tests different titles to make it easier to
> > narrow down a problem to one or the other if one of them does fail?
> > Perhaps the second test could be titled:
> >
> >     test_expect_success 'describe --dirty with dirty --work-tree' '
> >
> > or something.
>
> Thanks, didn‘t notice this.
> I‘d use a suffix (dirty) for my test titles. But this won‘t work for tests
> using check_describe(). Any objections?

I have no objections to using suffix "(dirty)".

It's true that there are a few duplicate test titles due to check_describe() invocations, however, this patch isn't introducing any new callers, so it's not strictly a concern of this series. If you did want to address that issue, one possibility would be to add a 'title' argument to check_describe() and adjust callers to use a unique title, however, such a change would not be part of this particular patch, and I'm not convinced that it's even worth the effort and churn at this point.