Hi,

On Thu, 28 May 2020, Đoàn Trần Công Danh wrote:

> On 2020-05-27 18:57:48+0100, Phillip Wood <phillip.wood123@gmail.com> wrote:
> > From: Phillip Wood <phillip.wood@dunelm.org.uk>
> >
> > Sorry I somehow forgot to commit this before sending the v4 patches,
> > it fixes up the final patch
> >
> > Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> > ---
> >  t/t3436-rebase-more-options.sh | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/t/t3436-rebase-more-options.sh b/t/t3436-rebase-more-options.sh
> > index 5ee193f333..ecfd68397f 100755
> > --- a/t/t3436-rebase-more-options.sh
> > +++ b/t/t3436-rebase-more-options.sh
> > @@ -196,7 +196,7 @@ test_expect_success '--ignore-date is an alias for --reset-author-date' '
> >  	git rebase --apply --ignore-date HEAD^ &&
> >  	git commit --allow-empty -m empty --date="$GIT_AUTHOR_DATE" &&
> >  	git rebase -m --ignore-date HEAD^ &&
> > -	git log -2 --pretty="format:%ai" >authortime &&
> > +	git log -2 --pretty=%ai >authortime &&
> >  	grep "+0000" authortime >output &&
> >  	test_line_count = 2 output
> >  '
>
> This version addressed all of my concerns, LGTM.
>
> Only the last
>
> 	test_line_count = 2 output
>
> puzzled me at first.
> Since it's the only usage of test_line_count in this version
> Turn out, it's equivalence with:
> -----------8<-----------
> diff --git a/t/t3436-rebase-more-options.sh b/t/t3436-rebase-more-options.sh
> index ecfd68397f..abe9af4d8c 100755
> --- a/t/t3436-rebase-more-options.sh
> +++ b/t/t3436-rebase-more-options.sh
> @@ -197,8 +197,7 @@ test_expect_success '--ignore-date is an alias for --reset-author-date' '
>  	git commit --allow-empty -m empty --date="$GIT_AUTHOR_DATE" &&
>  	git rebase -m --ignore-date HEAD^ &&
>  	git log -2 --pretty=%ai >authortime &&
> -	grep "+0000" authortime >output &&
> -	test_line_count = 2 output
> +	! grep -v "+0000" authortime
>  '
>
>  # This must be the last test in this file
> ------>8------

Good suggestion!

I've read through all 5 patches, and rather than repeating much of what I
said about 1/5 and 2/5 in 4/5, I'll just say it here that it applies
there, too: less repetitions in the test script, and I'd prefer the layer
where the `apply` vs `merge` options are set to be `cmd__rebase()` rather
than `run_am()` (and `get_replay_opts()`).

All in all, it was a pleasant read.

Thanks,
Dscho