[1/1] t3206-range-diff.sh: cover single-patch case
diff mbox series

Message ID 58347a962438852be0d37c3957686ea5000b2dbd.1536697263.git.gitgitgadget@gmail.com
State New
Headers show
Series
  • Add coverage for 'git format-patch --range-diff' single-patch case
Related show

Commit Message

James via GitGitGadget Sept. 11, 2018, 8:21 p.m. UTC
From: Derrick Stolee <dstolee@microsoft.com>

The commit 40ce4160 "format-patch: allow --range-diff to apply to
a lone-patch" added the ability to see a range-diff as commentary
after the commit message of a single patch series (i.e. [PATCH]
instead of [PATCH X/N]). However, this functionality was not
covered by a test case.

Add a simple test case that checks that a range-diff is written as
commentary to the patch.

Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
---
 t/t3206-range-diff.sh | 5 +++++
 1 file changed, 5 insertions(+)

Comments

Junio C Hamano Sept. 11, 2018, 8:58 p.m. UTC | #1
"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Derrick Stolee <dstolee@microsoft.com>
>
> The commit 40ce4160 "format-patch: allow --range-diff to apply to
> a lone-patch" added the ability to see a range-diff as commentary
> after the commit message of a single patch series (i.e. [PATCH]
> instead of [PATCH X/N]). However, this functionality was not
> covered by a test case.
>
> Add a simple test case that checks that a range-diff is written as
> commentary to the patch.
>
> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> ---
>  t/t3206-range-diff.sh | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh
> index 3d7a2d8a4d..05ef3263d2 100755
> --- a/t/t3206-range-diff.sh
> +++ b/t/t3206-range-diff.sh
> @@ -154,4 +154,9 @@ do
>  	'
>  done
>  
> +test_expect_success 'format-patch --range-diff as commentary' '
> +	git format-patch --stdout --range-diff=HEAD~1 HEAD~1 >actual &&
> +	grep -A 1 -e "\-\-\-" actual | grep "Range-diff:"

Isn't "grep -A" GNUism?
Stefan Beller Sept. 11, 2018, 9:09 p.m. UTC | #2
On Tue, Sep 11, 2018 at 1:21 PM Derrick Stolee via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> From: Derrick Stolee <dstolee@microsoft.com>
>
> The commit 40ce4160 "format-patch: allow --range-diff to apply to
> a lone-patch" added the ability to see a range-diff as commentary
> after the commit message of a single patch series (i.e. [PATCH]
> instead of [PATCH X/N]). However, this functionality was not
> covered by a test case.
>
> Add a simple test case that checks that a range-diff is written as
> commentary to the patch.
>
> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> ---
>  t/t3206-range-diff.sh | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh
> index 3d7a2d8a4d..05ef3263d2 100755
> --- a/t/t3206-range-diff.sh
> +++ b/t/t3206-range-diff.sh
> @@ -154,4 +154,9 @@ do
>         '
>  done
>
> +test_expect_success 'format-patch --range-diff as commentary' '
> +       git format-patch --stdout --range-diff=HEAD~1 HEAD~1 >actual &&

This is an interesting use of range-diff, as it basically tells us
"Range-diff: This is a new patch", but it works to make sure
there is a range diff section. (I shortly wondered if we would
ever omit the range diff for "obvious" cases or word it differently)

> +       grep -A 1 -e "\-\-\-" actual | grep "Range-diff:"

So the first grep finds the three dashes, presumably those
after the commit message/ but others as well, e.g. in
    --- a/<path>
    +++ b/<path>
and then the second grep should find the string "Range-diff".
By having the greps chained with a pipe, only one return
code can be delivered to the test suite, and as we get the last
commands return code, we get reported if we found the string
in the preselected part.

I was wondering if we could get away with just one command to
check for that multi line pattern

    sed -n -e '/---/,/^Range/p' actual

seems to detect that pattern, and prints from there on to the
rest of the file.


> +'
> +
>  test_done
> --
> gitgitgadget
Junio C Hamano Sept. 11, 2018, 9:11 p.m. UTC | #3
Junio C Hamano <gitster@pobox.com> writes:

>> +test_expect_success 'format-patch --range-diff as commentary' '
>> +	git format-patch --stdout --range-diff=HEAD~1 HEAD~1 >actual &&
>> +	grep -A 1 -e "\-\-\-" actual | grep "Range-diff:"
>
> Isn't "grep -A" GNUism?

Sorry for short-write(2) X-<.

Perhaps

	sed -ne "/^---$/,+1/s/^Range-diff:/&/p"

or something along that line.
Eric Sunshine Sept. 11, 2018, 9:34 p.m. UTC | #4
On Tue, Sep 11, 2018 at 4:26 PM Derrick Stolee via GitGitGadget
<gitgitgadget@gmail.com> wrote:
> The commit 40ce4160 "format-patch: allow --range-diff to apply to
> a lone-patch" added the ability to see a range-diff as commentary
> after the commit message of a single patch series (i.e. [PATCH]
> instead of [PATCH X/N]). However, this functionality was not
> covered by a test case.
>
> Add a simple test case that checks that a range-diff is written as
> commentary to the patch.
>
> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> ---
> diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh
> @@ -154,4 +154,9 @@ do
> +test_expect_success 'format-patch --range-diff as commentary' '
> +       git format-patch --stdout --range-diff=HEAD~1 HEAD~1 >actual &&
> +       grep -A 1 -e "\-\-\-" actual | grep "Range-diff:"
> +'

Aside from Junio's and Stefan's comments...

Patch 6/14 [1], in addition to checking that a solo patch contains an
interdiff, takes the extra step of checking that individual patches
_don't_ contain an interdiff when --cover-letter is used. I wonder if
the same should be done here, though I don't feel too strongly about
it. If you do go that route, it might make sense to move this test to
t4014 as neighbor to the --interdiff tests. The reason 10/14 [2] added
the "git format-patch --range-diff" test to t3206 instead of t4014 was
so it could do a thorough check of the embedded range-diff by re-using
the specially crafted test repo set up by t3206. Your new test is much
looser, thus could be moved alongside the --interdiff tests. Not a big
deal, though. Either way is fine. Thanks for working on this.

[1]: https://public-inbox.org/git/20180722095717.17912-7-sunshine@sunshineco.com/
[2]: https://public-inbox.org/git/20180722095717.17912-11-sunshine@sunshineco.com/
Derrick Stolee Sept. 12, 2018, 2:20 p.m. UTC | #5
On 9/11/2018 5:34 PM, Eric Sunshine wrote:
> On Tue, Sep 11, 2018 at 4:26 PM Derrick Stolee via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
>> The commit 40ce4160 "format-patch: allow --range-diff to apply to
>> a lone-patch" added the ability to see a range-diff as commentary
>> after the commit message of a single patch series (i.e. [PATCH]
>> instead of [PATCH X/N]). However, this functionality was not
>> covered by a test case.
>>
>> Add a simple test case that checks that a range-diff is written as
>> commentary to the patch.
>>
>> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
>> ---
>> diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh
>> @@ -154,4 +154,9 @@ do
>> +test_expect_success 'format-patch --range-diff as commentary' '
>> +       git format-patch --stdout --range-diff=HEAD~1 HEAD~1 >actual &&
>> +       grep -A 1 -e "\-\-\-" actual | grep "Range-diff:"
>> +'
> Aside from Junio's and Stefan's comments...
>
> Patch 6/14 [1], in addition to checking that a solo patch contains an
> interdiff, takes the extra step of checking that individual patches
> _don't_ contain an interdiff when --cover-letter is used. I wonder if
> the same should be done here, though I don't feel too strongly about
> it. If you do go that route, it might make sense to move this test to
> t4014 as neighbor to the --interdiff tests. The reason 10/14 [2] added
> the "git format-patch --range-diff" test to t3206 instead of t4014 was
> so it could do a thorough check of the embedded range-diff by re-using
> the specially crafted test repo set up by t3206. Your new test is much
> looser, thus could be moved alongside the --interdiff tests. Not a big
> deal, though. Either way is fine. Thanks for working on this.
>
> [1]: https://public-inbox.org/git/20180722095717.17912-7-sunshine@sunshineco.com/
> [2]: https://public-inbox.org/git/20180722095717.17912-11-sunshine@sunshineco.com/
Thanks for these links! In particular, [2] uses this line to test the 
inter-diff appears:

+    test_i18ngrep "^Interdiff:$" 0001-fleep.patch &&

That's a better way to test, especially with the translation. It would 
be enough for my needs.

Thanks,

-Stolee

P.S. Resending because apparently I had HTML in the last response

Patch
diff mbox series

diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh
index 3d7a2d8a4d..05ef3263d2 100755
--- a/t/t3206-range-diff.sh
+++ b/t/t3206-range-diff.sh
@@ -154,4 +154,9 @@  do
 	'
 done
 
+test_expect_success 'format-patch --range-diff as commentary' '
+	git format-patch --stdout --range-diff=HEAD~1 HEAD~1 >actual &&
+	grep -A 1 -e "\-\-\-" actual | grep "Range-diff:"
+'
+
 test_done