Re: [PATCH 1/2] t4255: test am submodule with diff.submodule

>
> On Fri, Dec 26, 2014 at 6:11 PM, Doug Kelly <dougk.ff7@gmail.com> wrote:
> > git am will break when using diff.submodule=log; add some test cases
> > to illustrate this breakage as simply as possible.  There are
> > currently two ways this can fail:
> >
> > * With errors ("unrecognized input"), if only change
> > * Silently (no submodule change), if other files change
> >
> > Test for both conditions and ensure without diff.submodule this works.
> >
> > Signed-off-by: Doug Kelly <dougk.ff7@gmail.com>
> > ---
> > diff --git a/t/t4255-am-submodule.sh b/t/t4255-am-submodule.sh
> > index 8bde7db..d9a1d79 100755
> > --- a/t/t4255-am-submodule.sh
> > +++ b/t/t4255-am-submodule.sh
> > @@ -18,4 +18,87 @@ am_3way () {
> >  KNOWN_FAILURE_NOFF_MERGE_ATTEMPTS_TO_MERGE_REMOVED_SUBMODULE_FILES=1
> >  test_submodule_switch "am_3way"
> >
> > +test_expect_success 'setup diff.submodule' '
>
> Since the tests are actually expected to fail at this point (before
> you've fixed the problem), use test_expect_failure. The follow-up
> patch, which fixes the problem, should flip them to
> test_expect_success.

>
> > +       echo one >one &&
> > +       git add one &&
> > +       test_tick &&
> > +       git commit -m initial &&
>
> Rather than performing these steps manually (here and below), perhaps
> test_commit would be suitable and more succinct.
>
> > +       git rev-parse HEAD >initial &&
>
> Other scripts in the test suite don't bother with this indirection.
> Instead, they assign the variable here, then reference it in
> subsequent tests (and no need to redirect to a file).
>
>     INITIAL=$(git rev-parse HEAD) &&
>

> > +
> > +       git init submodule &&
> > +       (cd submodule &&
> > +               echo two >two &&
> > +               git add two &&
> > +               test_tick &&
> > +               git commit -m "initial submodule" &&
> > +               git rev-parse HEAD >../initial-submodule) &&
>
> Style: Format the subshell like this:
>
>     (
>         ...commands...
>     ) &&

>
> > +       git submodule add ./submodule &&
> > +       test_tick &&
> > +       git commit -m first &&
> > +       git rev-parse HEAD >first &&
>
> Is file 'first' ever used anywhere?

>
> > +       (cd submodule &&
> > +               echo three >three &&
> > +               git add three &&
> > +               test_tick &&
> > +               git commit -m "first submodule" &&
> > +               git rev-parse HEAD >../first-submodule) &&
> > +       git add submodule &&
> > +       test_tick &&
> > +       git commit -m second &&
> > +       git rev-parse HEAD >second &&
> > +
> > +       (cd submodule &&
> > +               git mv two four &&
> > +               test_tick &&
> > +               git commit -m "second submodule" &&
> > +               git rev-parse HEAD >../second-submodule) &&
> > +       git add submodule &&
> > +       echo four >four &&
> > +       git add four &&
> > +       test_tick &&
> > +       git commit -m third &&
> > +       git rev-parse HEAD >third &&
> > +       git submodule update --init
> > +'
> > +
> > +INITIAL=$(cat initial)
> > +SECOND=$(cat second)
> > +THIRD=$(cat third)
>
> No need for this extra level of indirection. See above.
>
> > +run_test() {
> > +       START_COMMIT=$1
> > +       EXPECT=$2
>
> Although it's not specifically wrong here, someone adding code above
> these two lines later on may not notice the broken &&-chain, so it
> would be a good idea to keep the &&-chain intact.

>
> > +       (git am --abort || true) &&
> > +       git reset --hard $START_COMMIT &&
> > +       rm -f *.patch &&
> > +       git format-patch -1 &&
> > +       git reset --hard $START_COMMIT^ &&
> > +       git submodule update &&
> > +       git am *.patch &&
> > +       git submodule update &&
> > +       (cd submodule && git rev-parse HEAD >../actual) &&
> > +       test_cmp $EXPECT actual
> > +}
> > +
> > +test_expect_success 'diff.submodule unset' '
> > +       (git config --unset diff.submodule || true) &&
> > +       run_test $SECOND 'first-submodule'
>
> Note that you're already inside a single-quoted string here, so
> 'first-submodule' is not quite doing what you expect. Double quotes
> would be more appropriate. Or, better, drop the quoting of
> first-submodule altogether since it's unnecessary.

>
> > +'
> > +
> > +test_expect_success 'diff.submodule unset with extra file' '
> > +       (git config --unset diff.submodule || true) &&
> > +       run_test $THIRD 'second-submodule'
> > +'
> > +
> > +test_expect_success 'diff.submodule=log' '
> > +       git config diff.submodule log &&
> > +       run_test $SECOND 'first-submodule'
> > +'
> > +
> > +test_expect_success 'diff.submodule=log with extra file' '
> > +       git config diff.submodule log &&
> > +       run_test $THIRD 'second-submodule'
> > +'
> > +
> >  test_done
> > --
> > 2.0.5