Re: [PATCH 1/2] format-patch: add a more exhaustive --range-diff test
On Sat, Nov 24 2018, Junio C Hamano wrote:
> Ævar Arnfjörð Bjarmason writes:
>
>> Change the narrow test added in 31e2617a5f ("format-patch: add
>> --range-diff option to embed diff in cover letter", 2018-07-22) to
>> test the full output. This test would have spotted a regression in the
>> output if it wasn't beating around the bush and tested the full
>> output, let's do that.
>
> This looks wrong, given that we are declaring that showing the
> broken --stat in the output is wrong. It makes us to expect a
> known-wrong output from the command.
>
> If the theme of the topic were "what we are giving by default is a
> sensible output when --stat is asked for, but it is rather too noisy
> and our default should be quieter---let's tone it down", then this
> patch in 1/2 and then a behaviour-change patch with updated
> expectation would make sense, but I do not think that is what you
> are aiming for with this series.
>
> Perhaps squash this into the real "fix" to show what the desired
> output should look like with the same patch?
I see you did that in 'pu'. I don't mind, looks good to me.
FWIW I split this up for ease of review, i.e. showing what the output
was before and what effect the code change had, but maybe that was
overdoing it and it's simpler just to have this all in one go.
>> Signed-off-by: Ævar Arnfjörð Bjarmason
>> ---
>> t/t3206-range-diff.sh | 27 ++-
>> 1 file changed, 22 insertions(+), 5 deletions(-)
>>
>> diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh
>> index e497c1358f..0235c038be 100755
>> --- a/t/t3206-range-diff.sh
>> +++ b/t/t3206-range-diff.sh
>> @@ -249,11 +249,28 @@ for prev in topic master..topic
>> do
>> test_expect_success "format-patch --range-diff=$prev" '
>> git format-patch --stdout --cover-letter --range-diff=$prev \
>> -master..unmodified >actual &&
>> -grep "= 1: .* s/5/A" actual &&
>> -grep "= 2: .* s/4/A" actual &&
>> -grep "= 3: .* s/11/B" actual &&
>> -grep "= 4: .* s/12/B" actual
>> +master..unmodified >actual.raw &&
>> +sed -e "s|^:||" -e "s|:$||" >expect <<-\EOF &&
>> +:1: 4de457d = 1: 35b9b25 s/5/A/
>> +: a => b | 0
>> +: 1 file changed, 0 insertions(+), 0 deletions(-)
>> +::
>> +:2: fccce22 = 2: de345ab s/4/A/
>> +: a => b | 0
>> +: 1 file changed, 0 insertions(+), 0 deletions(-)
>> +::
>> +:3: 147e64e = 3: 9af6654 s/11/B/
>> +: a => b | 0
>> +: 1 file changed, 0 insertions(+), 0 deletions(-)
>> +::
>> +:4: a63e992 = 4: 2901f77 s/12/B/
>> +: a => b | 0
>> +: 1 file changed, 0 insertions(+), 0 deletions(-)
>> +::
>> +:-- :
>> +EOF
>> +sed -ne "/^1:/,/^--/p" actual &&
>> +test_cmp expect actual
>> '
>> done
Re: [PATCH 1/2] format-patch: add a more exhaustive --range-diff test
Ævar Arnfjörð Bjarmason writes:
> Change the narrow test added in 31e2617a5f ("format-patch: add
> --range-diff option to embed diff in cover letter", 2018-07-22) to
> test the full output. This test would have spotted a regression in the
> output if it wasn't beating around the bush and tested the full
> output, let's do that.
This looks wrong, given that we are declaring that showing the
broken --stat in the output is wrong. It makes us to expect a
known-wrong output from the command.
If the theme of the topic were "what we are giving by default is a
sensible output when --stat is asked for, but it is rather too noisy
and our default should be quieter---let's tone it down", then this
patch in 1/2 and then a behaviour-change patch with updated
expectation would make sense, but I do not think that is what you
are aiming for with this series.
Perhaps squash this into the real "fix" to show what the desired
output should look like with the same patch?
> Signed-off-by: Ævar Arnfjörð Bjarmason
> ---
> t/t3206-range-diff.sh | 27 ++-
> 1 file changed, 22 insertions(+), 5 deletions(-)
>
> diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh
> index e497c1358f..0235c038be 100755
> --- a/t/t3206-range-diff.sh
> +++ b/t/t3206-range-diff.sh
> @@ -249,11 +249,28 @@ for prev in topic master..topic
> do
> test_expect_success "format-patch --range-diff=$prev" '
> git format-patch --stdout --cover-letter --range-diff=$prev \
> - master..unmodified >actual &&
> - grep "= 1: .* s/5/A" actual &&
> - grep "= 2: .* s/4/A" actual &&
> - grep "= 3: .* s/11/B" actual &&
> - grep "= 4: .* s/12/B" actual
> + master..unmodified >actual.raw &&
> + sed -e "s|^:||" -e "s|:$||" >expect <<-\EOF &&
> + :1: 4de457d = 1: 35b9b25 s/5/A/
> + : a => b | 0
> + : 1 file changed, 0 insertions(+), 0 deletions(-)
> + ::
> + :2: fccce22 = 2: de345ab s/4/A/
> + : a => b | 0
> + : 1 file changed, 0 insertions(+), 0 deletions(-)
> + ::
> + :3: 147e64e = 3: 9af6654 s/11/B/
> + : a => b | 0
> + : 1 file changed, 0 insertions(+), 0 deletions(-)
> + ::
> + :4: a63e992 = 4: 2901f77 s/12/B/
> + : a => b | 0
> + : 1 file changed, 0 insertions(+), 0 deletions(-)
> + ::
> + :-- :
> + EOF
> + sed -ne "/^1:/,/^--/p" actual &&
> + test_cmp expect actual
> '
> done
[PATCH 1/2] format-patch: add a more exhaustive --range-diff test
Change the narrow test added in 31e2617a5f ("format-patch: add
--range-diff option to embed diff in cover letter", 2018-07-22) to
test the full output. This test would have spotted a regression in the
output if it wasn't beating around the bush and tested the full
output, let's do that.
Signed-off-by: Ævar Arnfjörð Bjarmason
---
t/t3206-range-diff.sh | 27 ++-
1 file changed, 22 insertions(+), 5 deletions(-)
diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh
index e497c1358f..0235c038be 100755
--- a/t/t3206-range-diff.sh
+++ b/t/t3206-range-diff.sh
@@ -249,11 +249,28 @@ for prev in topic master..topic
do
test_expect_success "format-patch --range-diff=$prev" '
git format-patch --stdout --cover-letter --range-diff=$prev \
- master..unmodified >actual &&
- grep "= 1: .* s/5/A" actual &&
- grep "= 2: .* s/4/A" actual &&
- grep "= 3: .* s/11/B" actual &&
- grep "= 4: .* s/12/B" actual
+ master..unmodified >actual.raw &&
+ sed -e "s|^:||" -e "s|:$||" >expect <<-\EOF &&
+ :1: 4de457d = 1: 35b9b25 s/5/A/
+ : a => b | 0
+ : 1 file changed, 0 insertions(+), 0 deletions(-)
+ ::
+ :2: fccce22 = 2: de345ab s/4/A/
+ : a => b | 0
+ : 1 file changed, 0 insertions(+), 0 deletions(-)
+ ::
+ :3: 147e64e = 3: 9af6654 s/11/B/
+ : a => b | 0
+ : 1 file changed, 0 insertions(+), 0 deletions(-)
+ ::
+ :4: a63e992 = 4: 2901f77 s/12/B/
+ : a => b | 0
+ : 1 file changed, 0 insertions(+), 0 deletions(-)
+ ::
+ :-- :
+ EOF
+ sed -ne "/^1:/,/^--/p" actual &&
+ test_cmp expect actual
'
done
--
2.20.0.rc0.387.gc7a69e6b6c
