Taylor Blau <me@xxxxxxxxxxxx> writes:
> This version is looking good. The main functionality is well-reasoned
> and straightforwardly implemented. One minor suggestion that you could
> consider squashing in is some test clean-up like so:
>
> --- 8< ---
> diff --git a/t/t3501-revert-cherry-pick.sh b/t/t3501-revert-cherry-pick.sh
> index 7011e3a421..4dee71d6d5 100755
> --- a/t/t3501-revert-cherry-pick.sh
> +++ b/t/t3501-revert-cherry-pick.sh
> @@ -176,29 +176,27 @@ test_expect_success 'advice from failed revert' '
> test_cmp expected actual
> '
>
> +test_expect_commit_msg () {
> + echo "$@" >expect &&
> + git log -1 --pretty=%s >actual &&
> + test_cmp expect actual
> +}
> +
> test_expect_success 'title of fresh reverts' '
> test_commit --no-tag A file1 &&
> test_commit --no-tag B file1 &&
> git revert --no-edit HEAD &&
> - echo "Revert \"B\"" >expect &&
> - git log -1 --pretty=%s >actual &&
> - test_cmp expect actual &&
> + test_expect_commit_msg "Revert \"B\"" &&
> git revert --no-edit HEAD &&
> - echo "Reapply \"B\"" >expect &&
> - git log -1 --pretty=%s >actual &&
> - test_cmp expect actual &&
> + test_expect_commit_msg "Reapply \"B\"" &&
> git revert --no-edit HEAD &&
> - echo "Revert \"Reapply \"B\"\"" >expect &&
> - git log -1 --pretty=%s >actual &&
> - test_cmp expect actual
> + test_expect_commit_msg "Revert \"Reapply \"B\"\""
> '
>
> test_expect_success 'title of legacy double revert' '
> test_commit --no-tag "Revert \"Revert \"B\"\"" file1 &&
> git revert --no-edit HEAD &&
> - echo "Revert \"Revert \"Revert \"B\"\"\"" >expect &&
> - git log -1 --pretty=%s >actual &&
> - test_cmp expect actual
> + test_expect_commit_msg "Revert \"Revert \"Revert \"B\"\"\""
> '
>
> test_expect_success 'identification of reverted commit (default)' '
> --- >8 ---
>
> To my eyes, it makes checking the subject of our revert commit against
> an expected value more readable by factoring out the echo, git log,
> test_cmp pattern.
Yeah it does make the test more concise and what is expected stand
out more clearly. Good suggestion.
