Re: [PATCH 1/3] t4150-am: refactor and clean common setup

From: Eric Sunshine <sunshine@sunshineco.com>
Date: May 28, 2015, 19:09 UTC
Message-ID: <CAPig+cQ_PrY8=-iP-FJm_HZ+XKOVoc4NfDu6rtuBj8zfM5oG+w@mail.gmail.com>
In-Reply-To: <1432675975-11020-1-git-send-email-remi.lespinet@ensimag.grenoble-inp.fr>

On Tue, May 26, 2015 at 5:32 PM, Remi Lespinet <remi.lespinet@ensimag.grenoble-inp.fr> wrote:

Show 5 quoted lines

> Add new functions to keep the setup cleaner:
>  - setup_temporary_branch: creates a new branch, check it out
>    and automatically delete it after the test is over
>  - setup_fixed_branch: creates a fixed branch, which can be re-used
>    in later tests

See below for review comments beyond those already provided by Paul...

Show 35 quoted lines

> Signed-off-by: Remi Lespinet <remi.lespinet@ensimag.grenoble-inp.fr>
> ---
> diff --git a/t/t4150-am.sh b/t/t4150-am.sh
> index 306e6f3..8370951 100755
> --- a/t/t4150-am.sh
> +++ b/t/t4150-am.sh
> @@ -4,6 +4,20 @@ test_description='git am running'
>
>  . ./test-lib.sh
>
> +setup_temporary_branch () {
> +       tmp_name=${2-"temporary"}
> +       git reset --hard &&
> +       rm -fr .git/rebase-apply &&
> +       test_when_finished "git checkout $1 && git branch -D $tmp_name" &&
> +       git checkout -b "$tmp_name" "$1"
> +}
> +
> +setup_fixed_branch () {
> +       git reset --hard &&
> +       rm -fr .git/rebase-apply &&
> +       git checkout -b "$1" "$2"
> +}
> +
>  test_expect_success 'setup: messages' '
>         cat >msg <<-\EOF &&
>         second
> @@ -143,9 +157,7 @@ test_expect_success setup '
>  '
>
>  test_expect_success 'am applies patch correctly' '
> -       rm -fr .git/rebase-apply &&
> -       git reset --hard &&
> -       git checkout first &&
> +       setup_temporary_branch first &&

This is confusing. The commit message seems to advertise this patch as primarily a refactoring change (pulling boilerplate out of tests and into a new function), but the operation of that new function is surprisingly different from the boilerplate it's replacing: The old code did not create a branch, the new function does.

If your intention really is to create new branches in tests which previously did not need throwaway branches, then the commit message should state that as its primary purpose and explain why doing so is desirable, since it is not clear from this patch what you gain from doing so.

Moreover, typically, you should only either refactor or change behavior in a single patch, not both. For instance, patch 1 would add the new function and update tests to call it in place of the boilerplate; and patch 2 would change behavior (such as creating a temporary branch).

On the other hand, if these tests really don't benefit from having a throw-away branch, then this change seems suspect and obscures the intent of the test rather than clarifying or simplifying.

Show 22 quoted lines

>         test_tick &&
>         git am <patch1 &&
>         test_path_is_missing .git/rebase-apply &&
> @@ -275,9 +273,7 @@ test_expect_success 'am --keep-non-patch really keeps the non-patch part' '
>  '
>
>  test_expect_success 'am -3 falls back to 3-way merge' '
> -       rm -fr .git/rebase-apply &&
> -       git reset --hard &&
> -       git checkout -b lorem2 master2 &&
> +       setup_fixed_branch lorem2 master2 &&
>         sed -n -e "3,\$p" msg >file &&
>         head -n 9 msg >>file &&
>         git add file &&
> @@ -289,9 +285,7 @@ test_expect_success 'am -3 falls back to 3-way merge' '
>  '
>
>  test_expect_success 'am -3 -p0 can read --no-prefix patch' '
> -       rm -fr .git/rebase-apply &&
> -       git reset --hard &&
> -       git checkout -b lorem3 master2 &&
> +       setup_temporary_branch lorem2 &&

Unlike the test prior to this one which creates a "fixed" branch (via setup_fixed_branch) named 'lorem2' which is used by other tests, the 'lorem3' branch in this test is never used elsewhere, so setup_temporary_branch is used instead. Makes sense.

Show 12 quoted lines

>         sed -n -e "3,\$p" msg >file &&
>         head -n 9 msg >>file &&
>         git add file &&
> @@ -303,10 +297,8 @@ test_expect_success 'am -3 -p0 can read --no-prefix patch' '
>  '
>
>  test_expect_success 'am can rename a file' '
> +       setup_temporary_branch lorem &&
>         grep "^rename from" rename.patch &&
> -       rm -fr .git/rebase-apply &&
> -       git reset --hard &&
> -       git checkout lorem^0 &&

In other cases, you insert the setup_temporary_branch() invocation where the old code resided. Why the difference in this test (and others following)?

Show 14 quoted lines

>         git am rename.patch &&
>         test_path_is_missing .git/rebase-apply &&
>         git update-index --refresh &&
> @@ -349,11 +335,9 @@ test_expect_success 'am -3 -q is quiet' '
>  '
>
>  test_expect_success 'am pauses on conflict' '
> -       rm -fr .git/rebase-apply &&
> -       git reset --hard &&
> -       git checkout lorem2^^ &&
> +       setup_temporary_branch lorem2^^ &&
>         test_must_fail git am lorem-move.patch &&
> -       test -d .git/rebase-apply
> +       test_path_is_dir .git/rebase-apply

Sneaking in unrelated change. This sort of cleanup should go in a patch of its own.

>  '
>
>  test_expect_success 'am --skip works' '