Hi dscho and Danh On 29/05/2020 03:59, Johannes Schindelin wrote: > 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@xxxxxxxxx> wrote: >>> From: Phillip Wood <phillip.wood@xxxxxxxxxxxxx> >>> >>> 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@xxxxxxxxxxxxx> >>> --- >>> 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! Yes thanks Danh I'll update with that > 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, I'll look at adding a helper for to do the checks > 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()`). Yeah that would make it nicer. > All in all, it was a pleasant read. Thanks for your comments on this series Phillip > Thanks, > Dscho >
