Re: [PATCH v2 1/2] format-patch: create leading components of output directory

From: Bert Wesarg <bert.wesarg@googlemail.com>
Date: Oct 8, 2019, 09:06 UTC
Message-ID: <CAKPyHN2pX+QnktdWZh7huwnj_6eZ=s06A8mfWhUDKOc+Yn8qZQ@mail.gmail.com>
In-Reply-To: <20191007210306.GF11529@szeder.dev>

On Mon, Oct 7, 2019 at 11:03 PM SZEDER Gábor <szeder.dev@gmail.com> wrote:

Show 49 quoted lines

>
> On Sat, Oct 05, 2019 at 10:43:51AM +0200, Bert Wesarg wrote:
> > diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
> > index 83f52614d3..2f2cd6fea6 100755
> > --- a/t/t4014-format-patch.sh
> > +++ b/t/t4014-format-patch.sh
> > @@ -1606,6 +1606,26 @@ test_expect_success 'From line has expected format' '
> >       test_cmp from filtered
> >  '
> >
> > +test_expect_success 'format-patch -o with no leading directories' '
> > +     rm -fr patches &&
> > +     git format-patch -o patches master..side &&
> > +     git rev-list master..side >list &&
> > +     test_line_count = $(ls patches | wc -l) list
>
> This is sort of a nit...
>
> So, these tests check that 'git rev-list ...' lists as many commits as
> the number of files created by 'git format-patch'.  While it doesn't
> affect the tests' correctness, this is subtly different from checking
> that 'git format-patch' created as many files as the number of commits
> listed by 'git rev-list'.
>
> Consider how the tests' output would look like on failure:
> 'test_line_count' shows an error message that includes the content of
> the file to be checked, which in this case would consist of a bunch of
> commit object ids:
>
>   test_line_count: line count for list != 3
>   f7af51d27933a90554b6e9212a7e5d4ad1c74569
>   bd89fce9f5096eb5cad67c342b40818b7e3ce9e4
>
> On one hand, these object ids won't mean much to anyone who might have
> to debug such a test failure in the future, and on the other these
> tests are about 'git format-patch', not about 'git rev-list'.  If the
> check were written like this:
>
>   count=$(git rev-list --count master..side) &&
>   ls patches >list &&
>   test_line_count = $count list
>
> then the error message on failure would look something like this:
>
>   test_line_count: line count for list != 3
>   0001-first.patch
>   0002-second.patch
>
> which, I think, would be more useful.

thanks for this detail. As I copied an existing test with this pattern, I will add a new pre-patch to this serires which applies your idea to the existing test first, before I add new tests for this patch.

Bert

Show 23 quoted lines

>
>
> > +'
> > +
> > +test_expect_success 'format-patch -o with leading existing directories' '
> > +     git format-patch -o patches/side master..side &&
> > +     git rev-list master..side >list &&
> > +     test_line_count = $(ls patches/side | wc -l) list
> > +'
> > +
> > +test_expect_success 'format-patch -o with leading non-existing directories' '
> > +     rm -fr patches &&
> > +     git format-patch -o patches/side master..side &&
> > +     git rev-list master..side >list &&
> > +     test_line_count = $(ls patches/side | wc -l) list
> > +'
> > +
> >  test_expect_success 'format-patch format.outputDirectory option' '
> >       test_config format.outputDirectory patches &&
> >       rm -fr patches &&
> > --
> > 2.23.0.11.g242cf7f110
> >