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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Dec 28, 2014, 00:37 UTC
Message-ID: <CAPig+cT3gA2YpiT2Vr=F5-hB+Zy4ask-kz8DtpL3eFvz9PJb5Q@mail.gmail.com>
In-Reply-To: <1419635506-5045-2-git-send-email-dougk.ff7@gmail.com>

On Fri, Dec 26, 2014 at 6:11 PM, Doug Kelly <dougk.ff7@gmail.com> wrote:

Show 20 quoted lines

> 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) &&

Show 8 quoted lines

> +
> +       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?

Show 28 quoted lines

> +       (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.

Show 15 quoted lines

> +       (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.

Show 20 quoted lines

> +'
> +
> +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