From: Ramkumar Ramachandra <artagnon@gmail.com>
Date: Fri, 09 Dec 2011 18:51:58 GMT
Subject: Re: [PATCH 1/2] t3401: modernize style
Message-ID: <CALkWK0mip_pzUDQO=YLxaVwsWEAUdrk_EKcNG94Xr5+N+kzBtw@mail.gmail.com>
In-Reply-To: <1323449952-14161-1-git-send-email-martin.von.zweigbergk@gmail.com>

Hi Martin,

No cover letter, so I'm assuming these are just two random one-off
patches. The motivation is unclear: lazy afternoon? :P

Martin von Zweigbergk wrote:
> [...]
> diff --git a/t/t3401-rebase-partial.sh b/t/t3401-rebase-partial.sh
> index aea6685..d7c874c 100755
> --- a/t/t3401-rebase-partial.sh
> +++ b/t/t3401-rebase-partial.sh
> @@ -11,51 +11,50 @@ local branch.
>  '
>  . ./test-lib.sh
>
> -test_expect_success \
> -    'prepare repository with topic branch' \
> -    'echo First > A &&
> -     git update-index --add A &&
> -     git commit -m "Add A." &&
> +test_expect_success 'prepare repository with topic branch' '
> +       echo First > A &&
> +       git update-index --add A &&
> +       git commit -m "Add A." &&

Style nit: >[^ ] is prevalent FWIW.

$ git grep '> [^ ]' t/ | wc -l
3091
$ git grep '>[^ ]' t/ | wc -l
9271

Sure, the regular expressions aren't tailored to make sure that only
redirections are caught, but I suppose it's safe to assume that one
number is significantly larger than the other.

> [...]
> -test_expect_success \
> -    'rebase topic branch against new master and check git am did not get halted' \
> -    'git rebase master && test ! -d .git/rebase-apply'
> +test_expect_success 'rebase topic branch against new master and check git am did not get halted' '
> +       git rebase master &&
> +       test ! -d .git/rebase-apply
> +'

While at it, why not change this "test ! -d" to
"test_path_is_missing"?  My rationale is that you're touching the file
anyway to do a generic cleanup; might as well finish it.

Thanks.

-- Ram