Re: [PATCH 5/5] merge-recursive: test more consistent interface

2016-02-21 Thread Eric Sunshine 
On Sun, Feb 21, 2016 at 2:55 PM, Felipe Gonçalves Assis
 wrote:
> On 21 February 2016 at 15:40, Eric Sunshine  wrote:
>> Taking the above and review comments from earlier patches into
>> account, it might make sense to re-order the series as follows:
>>
>> 1/5: add --find-renames & --find-renames= tests (including "last wins")
>> 2/5: add --find-renames= / --rename-threshold= aliases tests
>> 3/5: add --no-renames tests (including "last wins")
>> 4/5: fix --find-renames to reset threshold to default (including test)
>> 5/5: fix merge-strategies.txt typo
>>
>> The position of the typo fix patch isn't significant; I just
>> arbitrarily plopped it at the end. Also, the order of patches 2 & 3 is
>> arbitrary.
>
> Fair enough. [...]
>
> I am currently working on the following order, which follows your constraints.
> 1/5: fix typo (I don't like typos)
> 2/5: tests involving --find-renames

Presumably 2/5 also tests --find-renames=...

> 3/5: tests involving --no-renames
> 4/5: tests involving --rename-threshold (this represents what would be
> reverted if the feature was discontinued)
> 5/5: fix --find-renames + test

Sounds reasonable.

>>> +test_expect_success 'last wins in --no-renames --find-renames' '
>>> +   git read-tree --reset -u HEAD &&
>>> +   test_must_fail git merge-recursive --no-renames --find-renames 
>>> HEAD^ -- HEAD master &&
>>> +   check_find_renames_50
>>> +'
>>> +
>>> +test_expect_success 'last wins in --find-renames --no-renames' '
>>> +   git read-tree --reset -u HEAD &&
>>> +   git merge-recursive --find-renames --no-renames HEAD^ -- HEAD 
>>> master &&
>>> +   check_no_renames
>>> +'
>>> +
>>> +test_expect_success 'rename-threshold= is a synonym for 
>>> find-renames=' '
>>> +   git read-tree --reset -u HEAD &&
>>> +   test_must_fail git merge-recursive --rename-threshold=25 HEAD^ -- 
>>> HEAD master &&
>>> +   check_find_renames_25
>>> +'
>>
>> I rather expected to see this test come first, as all the others are
>> rather subordinate to it.
>
> But it already is the first test involving "rename-threshold". The
> preceding tests verify the rename detection functionality with the
> recommended interface. Then we have tests for the deprecated option.
> This tail is exactly what we would remove if it was discontinued.
> What did you mean?

It's probably just a minor viewpoint issue, as I interpreted the patch
as primarily testing that --find-renames= and --rename-threshold= are
aliases, in which case checking that condition would be done first.
However, that's effectively moot given the proposed revised patch
ordering since this test would be in patch 4/5, and the two above it
would belong in 3/5.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 5/5] merge-recursive: test more consistent interface

2016-02-21 Thread Felipe Gonçalves Assis 
On 21 February 2016 at 15:40, Eric Sunshine  wrote:
> On Sun, Feb 21, 2016 at 10:09 AM, Felipe Gonçalves Assis
>  wrote:
>> merge-recursive: test more consistent interface
>
> The real meat of this patch (it seems) is that you're adding tests to
> verify that --find-renames= and --rename-threshold= are aliases, so it
> might make sense for the summary line to state that.
>
> t3034: test that --find-renames= and --rename-threshold= are aliases
>
>> Update basic tests to use the new option find-renames instead of
>> rename-threshold. Add tests to verify that rename-threshold= behaves
>> as a synonym for find-renames=. Test that find-renames resets
>> threshold.
>
> Likewise, the order of these sentences seems wrong. The important bit
> should be mentioned first, which is that the one is an alias for the
> other.
>
> (In fact, if you take advice given below in the actual patch content,
> then this paragraph can probably be dropped altogether since the other
> two bits don't really belong in this patch.)
>
>> Signed-off-by: Felipe Gonçalves Assis 
>> ---
>> diff --git a/t/t3034-merge-recursive-rename-options.sh 
>> b/t/t3034-merge-recursive-rename-options.sh
>> @@ -115,25 +115,25 @@ test_expect_success 'the default similarity index is 
>> 50%' '
>>
>>  test_expect_success 'low rename threshold' '
>> git read-tree --reset -u HEAD &&
>> -   test_must_fail git merge-recursive --rename-threshold=25 HEAD^ -- 
>> HEAD master &&
>> +   test_must_fail git merge-recursive --find-renames=25 HEAD^ -- HEAD 
>> master &&
>
> Since you're building this series atop 10ae752 (merge-recursive:
> option to specify rename threshold, 2010-09-27) in 'next', the
> --find-renames= option already exists, so these tests, which were
> added in 3/5, can instead use --find-renames= from the start, thus
> making this patch (5/5) much less noisy since this change and several
> below will disappear altogether.
>
> Taking the above and review comments from earlier patches into
> account, it might make sense to re-order the series as follows:
>
> 1/5: add --find-renames & --find-renames= tests (including "last wins")
> 2/5: add --find-renames= / --rename-threshold= aliases tests
> 3/5: add --no-renames tests (including "last wins")
> 4/5: fix --find-renames to reset threshold to default (including test)
> 5/5: fix merge-strategies.txt typo
>
> The position of the typo fix patch isn't significant; I just
> arbitrarily plopped it at the end. Also, the order of patches 2 & 3 is
> arbitrary.
>

Fair enough. As I said, I ordered the three test commits so that the
first one could be applied soon after the commit introducing
"rename-thresholds", the second soon after the commit introducing
"no-renames" and the third one soon after the fixup for the commit
introducing "find-renames" (which would ideally be correct from the
start), but then this is probably more aesthetic than practical.

I am currently working on the following order, which follows your constraints.
1/5: fix typo (I don't like typos)
2/5: tests involving --find-renames
3/5: tests involving --no-renames
4/5: tests involving --rename-threshold (this represents what would be
reverted if the feature was discontinued)
5/5: fix --find-renames + test

> More below...
>
>
>> +test_expect_success 'last wins in --no-renames --find-renames' '
>> +   git read-tree --reset -u HEAD &&
>> +   test_must_fail git merge-recursive --no-renames --find-renames HEAD^ 
>> -- HEAD master &&
>> +   check_find_renames_50
>> +'
>> +
>> +test_expect_success 'last wins in --find-renames --no-renames' '
>> +   git read-tree --reset -u HEAD &&
>> +   git merge-recursive --find-renames --no-renames HEAD^ -- HEAD master 
>> &&
>> +   check_no_renames
>> +'
>> +
>> +test_expect_success 'rename-threshold= is a synonym for 
>> find-renames=' '
>> +   git read-tree --reset -u HEAD &&
>> +   test_must_fail git merge-recursive --rename-threshold=25 HEAD^ -- 
>> HEAD master &&
>> +   check_find_renames_25
>> +'
>
> I rather expected to see this test come first, as all the others are
> rather subordinate to it.
>

But it already is the first test involving "rename-threshold". The
preceding tests verify the rename detection functionality with the
recommended interface. Then we have tests for the deprecated option.
This tail is exactly what we would remove if it was discontinued.

What did you mean?

>>  test_expect_success 'last wins in --no-renames --rename-threshold=' '
>> git read-tree --reset -u HEAD &&
>> test_must_fail git merge-recursive --no-renames 
>> --rename-threshold=25 HEAD^ -- HEAD master &&
>> @@ -161,4 +185,16 @@ test_expect_success 'last wins in 
>> --rename-threshold= --no-renames' '
>> check_no_renames
>>  '
>>
>> +test_expect_success 'last wins in --rename-threshold= --find-renames' '
>> +   git read-tree --reset -u HEAD &&
>> +   test_must_fail

Re: [PATCH 5/5] merge-recursive: test more consistent interface

2016-02-21 Thread Eric Sunshine 
On Sun, Feb 21, 2016 at 10:09 AM, Felipe Gonçalves Assis
 wrote:
> merge-recursive: test more consistent interface

The real meat of this patch (it seems) is that you're adding tests to
verify that --find-renames= and --rename-threshold= are aliases, so it
might make sense for the summary line to state that.

t3034: test that --find-renames= and --rename-threshold= are aliases

> Update basic tests to use the new option find-renames instead of
> rename-threshold. Add tests to verify that rename-threshold= behaves
> as a synonym for find-renames=. Test that find-renames resets
> threshold.

Likewise, the order of these sentences seems wrong. The important bit
should be mentioned first, which is that the one is an alias for the
other.

(In fact, if you take advice given below in the actual patch content,
then this paragraph can probably be dropped altogether since the other
two bits don't really belong in this patch.)

> Signed-off-by: Felipe Gonçalves Assis 
> ---
> diff --git a/t/t3034-merge-recursive-rename-options.sh 
> b/t/t3034-merge-recursive-rename-options.sh
> @@ -115,25 +115,25 @@ test_expect_success 'the default similarity index is 
> 50%' '
>
>  test_expect_success 'low rename threshold' '
> git read-tree --reset -u HEAD &&
> -   test_must_fail git merge-recursive --rename-threshold=25 HEAD^ -- 
> HEAD master &&
> +   test_must_fail git merge-recursive --find-renames=25 HEAD^ -- HEAD 
> master &&

Since you're building this series atop 10ae752 (merge-recursive:
option to specify rename threshold, 2010-09-27) in 'next', the
--find-renames= option already exists, so these tests, which were
added in 3/5, can instead use --find-renames= from the start, thus
making this patch (5/5) much less noisy since this change and several
below will disappear altogether.

Taking the above and review comments from earlier patches into
account, it might make sense to re-order the series as follows:

1/5: add --find-renames & --find-renames= tests (including "last wins")
2/5: add --find-renames= / --rename-threshold= aliases tests
3/5: add --no-renames tests (including "last wins")
4/5: fix --find-renames to reset threshold to default (including test)
5/5: fix merge-strategies.txt typo

The position of the typo fix patch isn't significant; I just
arbitrarily plopped it at the end. Also, the order of patches 2 & 3 is
arbitrary.

More below...

> check_find_renames_25
>  '
>
>  test_expect_success 'high rename threshold' '
> git read-tree --reset -u HEAD &&
> -   test_must_fail git merge-recursive --rename-threshold=75 HEAD^ -- 
> HEAD master &&
> +   test_must_fail git merge-recursive --find-renames=75 HEAD^ -- HEAD 
> master &&
> check_find_renames_75
>  '
>
>  test_expect_success 'exact renames only' '
> git read-tree --reset -u HEAD &&
> -   test_must_fail git merge-recursive --rename-threshold=100% HEAD^ -- 
> HEAD master &&
> +   test_must_fail git merge-recursive --find-renames=100% HEAD^ -- HEAD 
> master &&
> check_find_renames_100
>  '
>
>  test_expect_success 'rename threshold is truncated' '
> git read-tree --reset -u HEAD &&
> -   test_must_fail git merge-recursive --rename-threshold=200% HEAD^ -- 
> HEAD master &&
> +   test_must_fail git merge-recursive --find-renames=200% HEAD^ -- HEAD 
> master &&
> check_find_renames_100
>  '
>
> @@ -143,12 +143,36 @@ test_expect_success 'disabled rename detection' '
> check_no_renames
>  '
>
> -test_expect_success 'last wins in --rename-threshold= 
> --rename-threshold=' '
> +test_expect_success 'last wins in --find-renames= --find-renames=' '
> git read-tree --reset -u HEAD &&
> -   test_must_fail git merge-recursive --rename-threshold=25 
> --rename-threshold=75 HEAD^ -- HEAD master &&
> +   test_must_fail git merge-recursive --find-renames=25 
> --find-renames=75 HEAD^ -- HEAD master &&
> check_find_renames_75
>  '
>
> +test_expect_success '--find-renames resets threshold' '
> +   git read-tree --reset -u HEAD &&
> +   test_must_fail git merge-recursive --find-renames=25 --find-renames 
> HEAD^ -- HEAD master &&
> +   check_find_renames_50
> +'

As mentioned in my review of patch 1/5, this test really ought to be
bundled with that patch.

> +test_expect_success 'last wins in --no-renames --find-renames' '
> +   git read-tree --reset -u HEAD &&
> +   test_must_fail git merge-recursive --no-renames --find-renames HEAD^ 
> -- HEAD master &&
> +   check_find_renames_50
> +'
> +
> +test_expect_success 'last wins in --find-renames --no-renames' '
> +   git read-tree --reset -u HEAD &&
> +   git merge-recursive --find-renames --no-renames HEAD^ -- HEAD master 
> &&
> +   check_no_renames
> +'
> +
> +test_expect_success 'rename-threshold= is a synonym for find-renames=' 
> '
> +   git read-tree --reset -u HEAD &&
> +   test_must_fail git

[PATCH 5/5] merge-recursive: test more consistent interface

2016-02-21 Thread Felipe Gonçalves Assis 
Update basic tests to use the new option find-renames instead of
rename-threshold. Add tests to verify that rename-threshold= behaves
as a synonym for find-renames=. Test that find-renames resets
threshold.

Signed-off-by: Felipe Gonçalves Assis 
---
 t/t3034-merge-recursive-rename-options.sh | 48 +++
 1 file changed, 42 insertions(+), 6 deletions(-)

diff --git a/t/t3034-merge-recursive-rename-options.sh 
b/t/t3034-merge-recursive-rename-options.sh
index 2f10fa7..7fea7bd 100755
--- a/t/t3034-merge-recursive-rename-options.sh
+++ b/t/t3034-merge-recursive-rename-options.sh
@@ -115,25 +115,25 @@ test_expect_success 'the default similarity index is 50%' 
'
 
 test_expect_success 'low rename threshold' '
git read-tree --reset -u HEAD &&
-   test_must_fail git merge-recursive --rename-threshold=25 HEAD^ -- HEAD 
master &&
+   test_must_fail git merge-recursive --find-renames=25 HEAD^ -- HEAD 
master &&
check_find_renames_25
 '
 
 test_expect_success 'high rename threshold' '
git read-tree --reset -u HEAD &&
-   test_must_fail git merge-recursive --rename-threshold=75 HEAD^ -- HEAD 
master &&
+   test_must_fail git merge-recursive --find-renames=75 HEAD^ -- HEAD 
master &&
check_find_renames_75
 '
 
 test_expect_success 'exact renames only' '
git read-tree --reset -u HEAD &&
-   test_must_fail git merge-recursive --rename-threshold=100% HEAD^ -- 
HEAD master &&
+   test_must_fail git merge-recursive --find-renames=100% HEAD^ -- HEAD 
master &&
check_find_renames_100
 '
 
 test_expect_success 'rename threshold is truncated' '
git read-tree --reset -u HEAD &&
-   test_must_fail git merge-recursive --rename-threshold=200% HEAD^ -- 
HEAD master &&
+   test_must_fail git merge-recursive --find-renames=200% HEAD^ -- HEAD 
master &&
check_find_renames_100
 '
 
@@ -143,12 +143,36 @@ test_expect_success 'disabled rename detection' '
check_no_renames
 '
 
-test_expect_success 'last wins in --rename-threshold= 
--rename-threshold=' '
+test_expect_success 'last wins in --find-renames= --find-renames=' '
git read-tree --reset -u HEAD &&
-   test_must_fail git merge-recursive --rename-threshold=25 
--rename-threshold=75 HEAD^ -- HEAD master &&
+   test_must_fail git merge-recursive --find-renames=25 --find-renames=75 
HEAD^ -- HEAD master &&
check_find_renames_75
 '
 
+test_expect_success '--find-renames resets threshold' '
+   git read-tree --reset -u HEAD &&
+   test_must_fail git merge-recursive --find-renames=25 --find-renames 
HEAD^ -- HEAD master &&
+   check_find_renames_50
+'
+
+test_expect_success 'last wins in --no-renames --find-renames' '
+   git read-tree --reset -u HEAD &&
+   test_must_fail git merge-recursive --no-renames --find-renames HEAD^ -- 
HEAD master &&
+   check_find_renames_50
+'
+
+test_expect_success 'last wins in --find-renames --no-renames' '
+   git read-tree --reset -u HEAD &&
+   git merge-recursive --find-renames --no-renames HEAD^ -- HEAD master &&
+   check_no_renames
+'
+
+test_expect_success 'rename-threshold= is a synonym for find-renames=' '
+   git read-tree --reset -u HEAD &&
+   test_must_fail git merge-recursive --rename-threshold=25 HEAD^ -- HEAD 
master &&
+   check_find_renames_25
+'
+
 test_expect_success 'last wins in --no-renames --rename-threshold=' '
git read-tree --reset -u HEAD &&
test_must_fail git merge-recursive --no-renames --rename-threshold=25 
HEAD^ -- HEAD master &&
@@ -161,4 +185,16 @@ test_expect_success 'last wins in --rename-threshold= 
--no-renames' '
check_no_renames
 '
 
+test_expect_success 'last wins in --rename-threshold= --find-renames' '
+   git read-tree --reset -u HEAD &&
+   test_must_fail git merge-recursive --rename-threshold=25 --find-renames 
HEAD^ -- HEAD master &&
+   check_find_renames_50
+'
+
+test_expect_success 'last wins in --find-renames --rename-threshold=' '
+   git read-tree --reset -u HEAD &&
+   test_must_fail git merge-recursive --find-renames --rename-threshold=25 
HEAD^ -- HEAD master &&
+   check_find_renames_25
+'
+
 test_done
-- 
2.7.1.492.gc9722f8

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html