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

From: Paul Tan <pyokagan@gmail.com>
Date: May 28, 2015, 13:10 UTC
Message-ID: <CACRoPnQEJHPfoz0LjSs2X1CrW-TdVGb54XSrg6VWXP2tdyu5Uw@mail.gmail.com>
In-Reply-To: <1432675975-11020-1-git-send-email-remi.lespinet@ensimag.grenoble-inp.fr>

Hi,

Take these comments/suggestions with a pinch of salt because I'm not that experienced with the code base as well ;-).

On Wed, May 27, 2015 at 5:32 AM, 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

Agreed. This removes a lot of boilerplate from the tests. Another positive effect is that we can be sure that any commits made on that throwaway branch will not affect the other parts of the test suite, which makes understanding and editing the test suite much easier.

>  - setup_fixed_branch: creates a fixed branch, which can be re-used
>    in later tests

Looking at the patch, I see that setup_fixed_branch() is only used in 2 places, so I don't think it is a common pattern that would require its own function. Also, see below.

Show 14 quoted lines

> Signed-off-by: Remi Lespinet <remi.lespinet@ensimag.grenoble-inp.fr>
> ---
>  t/t4150-am.sh | 138 ++++++++++++++++++++--------------------------------------
>  1 file changed, 47 insertions(+), 91 deletions(-)
>
> 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 () {

Maybe name this checkout_temp_branch() or something, since it more or less is just a wrapper around git-checkout.

> +       tmp_name=${2-"temporary"}

I don't think the quotes are required. Also, I don't feel good about swapping the order of the arguments to git-checkout. (or making $2 an optional argument). As the patch stands, if I see

setup_temporary_branch lorem

I will think: so we are creating a temporary branch "lorem". But nope, we are creating a temporary branch "temporary" that branches from "lorem". I don't think writing setup_temporary_branch "temporary" "lorem" explicitly is that bad.

This is just personal preference though.

> +       git reset --hard &&
> +       rm -fr .git/rebase-apply &&
> +       test_when_finished "git checkout $1 && git branch -D $tmp_name" &&

I think you should use "git checkout -f $1" as if the working tree is dirty the test will fail at cleanup with no error message at all, which is annoying for debugging. Furthermore, the test_when_finished should be shifted below the git checkout -b below, as git branch -D will fail if the branch does not exist.

> +       git checkout -b "$tmp_name" "$1"

If you use git checkout -f here then there's no need for the git reset --hard above.

Show 6 quoted lines

> +}
> +
> +setup_fixed_branch () {
> +       git reset --hard &&
> +       rm -fr .git/rebase-apply &&
> +       git checkout -b "$1" "$2"

Again, by using git checkout -f we can drop the git reset --hard. But that reduces the function to 2 lines, and thus I don't think that this usage pattern needs its own function.

Show 7 quoted lines

> +}
> +
>  test_expect_success 'setup: messages' '
>         cat >msg <<-\EOF &&
>         second
> @@ -143,9 +157,7 @@ test_expect_success setup '
>  '

I haven't looked at the rest of the patch in detail because I'm not that well-acquainted with t4150 yet, but it looks okay.

Thanks, Paul