patch, 12 partsImprove git-am test coverage

> Since a078f73 (git-am: add --message-id/--no-message-id, 2014-11-25),
> the am.messageid setting determines whether the --message-id option is
> set by default.
> 
> Add a test for this.
> 
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Paul Tan <pyokagan@gmail.com>
> ---
>  t/t4150-am.sh | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/t/t4150-am.sh b/t/t4150-am.sh
> index b822a39..3f54bdf 100755
> --- a/t/t4150-am.sh
> +++ b/t/t4150-am.sh
> @@ -563,6 +563,17 @@ test_expect_success 'am --message-id really adds the message id' '
>  	test_cmp expected actual
>  '
>  
> +test_expect_success 'am.messageid really adds the message id' '
> +	rm -fr .git/rebase-apply &&
> +	git checkout -f HEAD^ &&
> +	test_config am.messageid true &&
> +	git am patch1.eml &&
> +	test_path_is_missing .git/rebase-apply &&
> +	git cat-file commit HEAD | tail -n1 >actual &&
> +	grep Message-Id patch1.eml >expected &&
> +	test_cmp expected actual
> +'
> +
>  test_expect_success 'am --message-id -s signs off after the message id' '
>  	rm -fr .git/rebase-apply &&
>  	git reset --hard &&
> 

> diff --git a/t/t4150-am.sh b/t/t4150-am.sh
> index 3f54bdf..0a19136 100755
> --- a/t/t4150-am.sh
> +++ b/t/t4150-am.sh
> @@ -154,6 +154,17 @@ test_expect_success 'am applies patch correctly' '
>  	test "$(git rev-parse second^)" = "$(git rev-parse HEAD^)"
>  '
>  
> +test_expect_success 'am fails if index is dirty' '
> +	test_when_finished "rm -fr dirtyfile" &&

> +	rm -fr .git/rebase-apply &&
> +	git checkout -f first &&
> +	echo dirtyfile >dirtyfile &&
> +	git add dirtyfile &&
> +	test_must_fail git am patch1 &&
> +	test_path_is_dir .git/rebase-apply &&
> +	test_cmp_rev first HEAD

> On 2015-07-02 20:16, Paul Tan wrote:
>
>> diff --git a/t/t4150-am.sh b/t/t4150-am.sh
>> index 3f54bdf..0a19136 100755
>> --- a/t/t4150-am.sh
>> +++ b/t/t4150-am.sh
>> @@ -154,6 +154,17 @@ test_expect_success 'am applies patch correctly' '
>>       test "$(git rev-parse second^)" = "$(git rev-parse HEAD^)"
>>  '
>>
>> +test_expect_success 'am fails if index is dirty' '
>> +     test_when_finished "rm -fr dirtyfile" &&
>
> Seeing as you `git add` that file further down, how about `git rm -f dirtfile` here?

> +test_expect_success 'am with applypatch-msg hook' '
> +	test_when_finished "rm -f .git/hooks/applypatch-msg" &&
> +	rm -fr .git/rebase-apply &&
> +	git reset --hard &&
> +	git checkout first &&
> +	mkdir -p .git/hooks &&
> +	cat >.git/hooks/applypatch-msg <<-\EOF &&
> +	#!/bin/sh
> +	cat "$1" >actual-msg &&
> +	echo hook-message >"$1"
> +	EOF
> +	chmod +x .git/hooks/applypatch-msg &&

> +	git am patch1 &&
> +	test_path_is_missing .git/rebase-apply &&
> +	git diff --exit-code second &&
> +	echo hook-message >expected &&
> +	git log -1 --format=format:%B >actual &&
> +	test_cmp expected actual &&
> +	git log -1 --format=format:%B second >expected &&
> +	test_cmp expected actual-msg
> +'
> +
> +test_expect_success 'am with failing applypatch-msg hook' '
> +	test_when_finished "rm -f .git/hooks/applypatch-msg" &&
> +	rm -fr .git/rebase-apply &&
> +	git reset --hard &&
> +	git checkout first &&
> +	mkdir -p .git/hooks &&
> +	cat >.git/hooks/applypatch-msg <<-\EOF &&
> +	#!/bin/sh
> +	exit 1
> +	EOF
> +	chmod +x .git/hooks/applypatch-msg &&
> +	test_must_fail git am patch1 &&
> +	test_path_is_dir .git/rebase-apply &&
> +	git diff --exit-code first &&
> +	test_cmp_rev first HEAD
> +'
> +
>  test_expect_success 'setup: new author and committer' '
>  	GIT_AUTHOR_NAME="Another Thor" &&
>  	GIT_AUTHOR_EMAIL="a.thor@example.com" &&

> diff --git a/t/t4150-am.sh b/t/t4150-am.sh
> index dd6fe81..62b678c 100755
> --- a/t/t4150-am.sh
> +++ b/t/t4150-am.sh
> @@ -275,6 +275,48 @@ test_expect_success 'am with failing pre-applypatch hook' '
>  	test_cmp_rev first HEAD
>  '
>  
> +test_expect_success 'am with post-applypatch hook' '
> +	test_when_finished "rm -f .git/hooks/post-applypatch" &&
> +	rm -fr .git/rebase-apply &&
> +	git reset --hard &&
> +	git checkout first &&
> +	mkdir -p .git/hooks &&
> +	cat >.git/hooks/post-applypatch <<-\EOF &&
> +	#!/bin/sh
> +	git rev-parse HEAD >head.actual
> +	git diff second >diff.actual
> +	exit 0
> +	EOF
> +	chmod +x .git/hooks/post-applypatch &&
> +	git am patch1 &&
> +	test_path_is_missing .git/rebase-apply &&
> +	test_cmp_rev second HEAD &&
> +	git rev-parse second >head.expected &&
> +	test_cmp head.expected head.actual &&
> +	git diff second >diff.expected &&
> +	test_cmp diff.expected diff.actual
> +'
> +
> +test_expect_success 'am with failing post-applypatch hook' '
> +	test_when_finished "rm -f .git/hooks/post-applypatch" &&
> +	rm -fr .git/rebase-apply &&
> +	git reset --hard &&
> +	git checkout first &&
> +	mkdir -p .git/hooks &&
> +	cat >.git/hooks/post-applypatch <<-\EOF &&
> +	#!/bin/sh
> +	git rev-parse HEAD >head.actual
> +	exit 1
> +	EOF
> +	chmod +x .git/hooks/post-applypatch &&
> +	git am patch1 &&
> +	test_path_is_missing .git/rebase-apply &&
> +	git diff --exit-code second &&
> +	test_cmp_rev second HEAD &&
> +	git rev-parse second >head.expected &&
> +	test_cmp head.expected head.actual
> +'

> On 2015-07-02 20:16, Paul Tan wrote:
>
>> diff --git a/t/t4150-am.sh b/t/t4150-am.sh
>> index dd6fe81..62b678c 100755
>> --- a/t/t4150-am.sh
>> +++ b/t/t4150-am.sh
>> @@ -275,6 +275,48 @@ test_expect_success 'am with failing pre-applypatch hook' '
>>       test_cmp_rev first HEAD
>>  '
>>
>> +test_expect_success 'am with post-applypatch hook' '
>> +     test_when_finished "rm -f .git/hooks/post-applypatch" &&
>> +     rm -fr .git/rebase-apply &&
>> +     git reset --hard &&
>> +     git checkout first &&
>> +     mkdir -p .git/hooks &&
>> +     cat >.git/hooks/post-applypatch <<-\EOF &&
>> +     #!/bin/sh
>> +     git rev-parse HEAD >head.actual
>> +     git diff second >diff.actual
>> +     exit 0
>> +     EOF
>> +     chmod +x .git/hooks/post-applypatch &&
>> +     git am patch1 &&
>> +     test_path_is_missing .git/rebase-apply &&
>> +     test_cmp_rev second HEAD &&
>> +     git rev-parse second >head.expected &&
>> +     test_cmp head.expected head.actual &&
>> +     git diff second >diff.expected &&
>> +     test_cmp diff.expected diff.actual
>> +'
>> +
>> +test_expect_success 'am with failing post-applypatch hook' '
>> +     test_when_finished "rm -f .git/hooks/post-applypatch" &&
>> +     rm -fr .git/rebase-apply &&
>> +     git reset --hard &&
>> +     git checkout first &&
>> +     mkdir -p .git/hooks &&
>> +     cat >.git/hooks/post-applypatch <<-\EOF &&
>> +     #!/bin/sh
>> +     git rev-parse HEAD >head.actual
>> +     exit 1
>> +     EOF
>> +     chmod +x .git/hooks/post-applypatch &&
>> +     git am patch1 &&
>> +     test_path_is_missing .git/rebase-apply &&
>> +     git diff --exit-code second &&
>> +     test_cmp_rev second HEAD &&
>> +     git rev-parse second >head.expected &&
>> +     test_cmp head.expected head.actual
>> +'
>
> These 2 tests as well as the previous patches look to me as if they could be refactored (the paradigm is the same: add a certain hook after resetting and then apply the patch, verify that the hook ran/failed)... do you think there is a chance for that?

> On Sun, Jul 5, 2015 at 11:58 PM, Johannes Schindelin
> <johannes.schindelin@gmx.de> wrote:
>> On 2015-07-02 20:16, Paul Tan wrote:
>>
>>> diff --git a/t/t4150-am.sh b/t/t4150-am.sh
>>> index dd6fe81..62b678c 100755
>>> --- a/t/t4150-am.sh
>>> +++ b/t/t4150-am.sh
>>> @@ -275,6 +275,48 @@ test_expect_success 'am with failing pre-applypatch hook' '
>>>       test_cmp_rev first HEAD
>>>  '
>>>
>>> +test_expect_success 'am with post-applypatch hook' '
>>> +     test_when_finished "rm -f .git/hooks/post-applypatch" &&
>>> +     rm -fr .git/rebase-apply &&
>>> +     git reset --hard &&
>>> +     git checkout first &&
>>> +     mkdir -p .git/hooks &&
>>> +     cat >.git/hooks/post-applypatch <<-\EOF &&
>>> +     #!/bin/sh
>>> +     git rev-parse HEAD >head.actual
>>> +     git diff second >diff.actual
>>> +     exit 0
>>> +     EOF
>>> +     chmod +x .git/hooks/post-applypatch &&
>>> +     git am patch1 &&
>>> +     test_path_is_missing .git/rebase-apply &&
>>> +     test_cmp_rev second HEAD &&
>>> +     git rev-parse second >head.expected &&
>>> +     test_cmp head.expected head.actual &&
>>> +     git diff second >diff.expected &&
>>> +     test_cmp diff.expected diff.actual
>>> +'
>>> +
>>> +test_expect_success 'am with failing post-applypatch hook' '
>>> +     test_when_finished "rm -f .git/hooks/post-applypatch" &&
>>> +     rm -fr .git/rebase-apply &&
>>> +     git reset --hard &&
>>> +     git checkout first &&
>>> +     mkdir -p .git/hooks &&
>>> +     cat >.git/hooks/post-applypatch <<-\EOF &&
>>> +     #!/bin/sh
>>> +     git rev-parse HEAD >head.actual
>>> +     exit 1
>>> +     EOF
>>> +     chmod +x .git/hooks/post-applypatch &&
>>> +     git am patch1 &&
>>> +     test_path_is_missing .git/rebase-apply &&
>>> +     git diff --exit-code second &&
>>> +     test_cmp_rev second HEAD &&
>>> +     git rev-parse second >head.expected &&
>>> +     test_cmp head.expected head.actual
>>> +'
>>
>> These 2 tests as well as the previous patches look to me as if they could be refactored (the paradigm is the same: add a certain hook after resetting and then apply the patch, verify that the hook ran/failed)... do you think there is a chance for that?
> 
> I had a look, but I think that while it is true that the overall
> sequence of each test is the same, the details differ enough that
> there's no obvious way to refactor the tests sensibly. For example,
> the contents of the hook scripts are not the same, as we need to check
> that the hooks are run at the correct stage of git-am execution. And
> as such, the verification tests are also different as well.

> Since d1c5f2a (Add git-am, applymbox replacement., 2005-10-07), git-am
> supported the --utf8 and --no-utf8 options, and if set, would pass the
> -u flag and the -k flag respectively.
>
> git mailinfo -u will re-code the commit log message and authorship info
> in the charset specified by i18n.commitencoding setting, while
> git mailinfo -n will disable the re-coding.
>
> Since d84029b (--utf8 is now default for 'git-am', 2007-01-08), --utf8
> is set by default in git-am.
>
> Add various encoding conversion tests to t3901 to test git-mailinfo's
> encoding conversion. In addition, add a test for --no-utf8 to check that
> no encoding conversion will occur if that option is set.
>
> Cc: Junio C Hamano <gitster@pobox.com>
> Signed-off-by: Paul Tan <pyokagan@gmail.com>
> ---
>   t/t3901-i18n-patch.sh | 62 +++++++++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 62 insertions(+)
>
> diff --git a/t/t3901-i18n-patch.sh b/t/t3901-i18n-patch.sh
> index 75cf3ff..b49bdb7 100755
> --- a/t/t3901-i18n-patch.sh
> +++ b/t/t3901-i18n-patch.sh
> @@ -251,4 +251,66 @@ test_expect_success 'rebase --merge (L/U)' '
>   	check_encoding 2 8859
>   '
>
> +test_expect_success 'am (U/U)' '
> +	# Apply UTF-8 patches with UTF-8 commitencoding
> +	git config i18n.commitencoding UTF-8 &&
> +	. "$TEST_DIRECTORY"/t3901-utf8.txt &&
> +
> +	git reset --hard master &&
> +	git am out-u1 out-u2 &&
> +
> +	check_encoding 2
> +'
> +
> +test_expect_success 'am (L/L)' '
> +	# Apply ISO-8859-1 patches with ISO-8859-1 commitencoding
> +	git config i18n.commitencoding ISO8859-1 &&
> +	. "$TEST_DIRECTORY"/t3901-8859-1.txt &&
> +
> +	git reset --hard master &&
> +	git am out-l1 out-l2 &&
> +
> +	check_encoding 2 8859
> +'

> +
> +test_expect_success 'am (U/L)' '
> +	# Apply ISO-8859-1 patches with UTF-8 commitencoding
> +	git config i18n.commitencoding UTF-8 &&
> +	. "$TEST_DIRECTORY"/t3901-utf8.txt &&
> +	git reset --hard master &&
> +
> +	# am specifies --utf8 by default.
> +	git am out-l1 out-l2 &&
> +
> +	check_encoding 2
> +'
> +
> +test_expect_success 'am --no-utf8 (U/L)' '
> +	# Apply ISO-8859-1 patches with UTF-8 commitencoding
> +	git config i18n.commitencoding UTF-8 &&
> +	. "$TEST_DIRECTORY"/t3901-utf8.txt &&
> +
> +	git reset --hard master &&
> +	git am --no-utf8 out-l1 out-l2 2>err &&
> +
> +	# commit-tree will warn that the commit message does not contain valid UTF-8
> +	# as mailinfo did not convert it
> +	grep "did not conform" err &&
> +
> +	check_encoding 2
> +'
> +
> +test_expect_success !MINGW 'am (L/U)' '
> +	# Apply UTF-8 patches with ISO-8859-1 commitencoding
> +	git config i18n.commitencoding ISO8859-1 &&
> +	. "$TEST_DIRECTORY"/t3901-8859-1.txt &&
> +
> +	git reset --hard master &&
> +	# mailinfo will re-code the commit message to the charset specified by
> +	# i18n.commitencoding
> +	git am out-u1 out-u2 &&
> +
> +	check_encoding 2 8859
> +'
> +
>   test_done
>

> Am 02.07.2015 um 20:16 schrieb Paul Tan:
>> diff --git a/t/t3901-i18n-patch.sh b/t/t3901-i18n-patch.sh
>> index 75cf3ff..b49bdb7 100755
>> --- a/t/t3901-i18n-patch.sh
>> +++ b/t/t3901-i18n-patch.sh
>> @@ -251,4 +251,66 @@ test_expect_success 'rebase --merge (L/U)' '
>>         check_encoding 2 8859
>>   '
>>
>> +test_expect_success 'am (U/U)' '
>> +       # Apply UTF-8 patches with UTF-8 commitencoding
>> +       git config i18n.commitencoding UTF-8 &&
>> +       . "$TEST_DIRECTORY"/t3901-utf8.txt &&
>> +
>> +       git reset --hard master &&
>> +       git am out-u1 out-u2 &&
>> +
>> +       check_encoding 2
>> +'
>> +
>> +test_expect_success 'am (L/L)' '
>> +       # Apply ISO-8859-1 patches with ISO-8859-1 commitencoding
>> +       git config i18n.commitencoding ISO8859-1 &&
>> +       . "$TEST_DIRECTORY"/t3901-8859-1.txt &&
>> +
>> +       git reset --hard master &&
>> +       git am out-l1 out-l2 &&
>> +
>> +       check_encoding 2 8859
>> +'
>
>
> This test case must be protected by !MINGW, just like the last case below
> and other cases that are already in the file. See 32f4cb6cee for details.

> Increase test coverage of git-am.sh to help prevent regressions that could arise
> from the rewrite of git-am.sh to C. This patch series, along with
> pt/am-foreign, improved test coverage as measured by kcov from 56.5%[1] to
> 67.3%[2].
>
> No tests for git-am's interactive mode, though, as test_terminal does not seem
> to attach a pseudo-tty to stdin(?), thus making git-am's "test -t 0" check fail.
>
> This is part of my GSoC project to rewrite git-am.sh to a C builtin[3].

>
> [1] http://pyokagan.github.io/git/20150430132408-a75942b//kcov-merged/git-am.eb79278e.html
> [2] http://pyokagan.github.io/git/20150702173751-2fdae08//kcov-merged/git-am.eb79278e.html
> [3] https://gist.github.com/pyokagan/1b7b0d1f4dab6ba3cef1
>
>
> Paul Tan (12):
>   t4150: am.messageid really adds the message id
>   t4150: am fails if index is dirty
>   t4151: am --abort will keep dirty index intact
>   t4150: am refuses patches when paused
>   t4150: am --resolved fails if index has no changes
>   t4150: am --resolved fails if index has unmerged entries
>   t4150: am with applypatch-msg hook
>   t4150: am with pre-applypatch hook
>   t4150: am with post-applypatch hook
>   t4150: tests for am --[no-]scissors
>   t3418: non-interactive rebase --continue with rerere enabled
>   t3901: test git-am encoding conversion
>
>  t/t3418-rebase-continue.sh |  19 ++++
>  t/t3901-i18n-patch.sh      |  62 ++++++++++++
>  t/t4150-am.sh              | 228 +++++++++++++++++++++++++++++++++++++++++++++
>  t/t4151-am-abort.sh        |  15 +++
>  4 files changed, 324 insertions(+)
>
> --
> 2.5.0.rc1.81.gfe77482
>

> On Thu, Jul 2, 2015 at 11:16 AM, Paul Tan <pyokagan@gmail.com> wrote:
>> Increase test coverage of git-am.sh to help prevent regressions that could arise
>> from the rewrite of git-am.sh to C. This patch series, along with
>> pt/am-foreign, improved test coverage as measured by kcov from 56.5%[1] to
>> 67.3%[2].
>>
>> No tests for git-am's interactive mode, though, as test_terminal does not seem
>> to attach a pseudo-tty to stdin(?), thus making git-am's "test -t 0" check fail.
>>
>> This is part of my GSoC project to rewrite git-am.sh to a C builtin[3].
> 
> The whole series looks good to me.

> On 2015-07-03 18:24, Stefan Beller wrote:
>> On Thu, Jul 2, 2015 at 11:16 AM, Paul Tan <pyokagan@gmail.com> wrote:
>>> Increase test coverage of git-am.sh to help prevent regressions
>>> that could arise
>>> from the rewrite of git-am.sh to C. This patch series, along with
>>> pt/am-foreign, improved test coverage as measured by kcov from 56.5%[1] to
>>> 67.3%[2].
>>>
>>> No tests for git-am's interactive mode, though, as test_terminal does not seem
>>> to attach a pseudo-tty to stdin(?), thus making git-am's "test -t
>>> 0" check fail.
>>>
>>> This is part of my GSoC project to rewrite git-am.sh to a C builtin[3].
>> 
>> The whole series looks good to me.
>
> I concur (my two comments really are minor nit picks).
>
> Thanks,
> Dscho