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

[Eric Sunshine <sunshine@sunshineco.com>]

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

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

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

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

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