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

[Paul Tan <pyokagan@gmail.com>]

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.

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

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

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