[PATCH 0/5] Tests and fixes for merge-recursive rename options

[PATCH 0/5] Tests and fixes for merge-recursive rename options

[Felipe Gonçalves Assis]

This builds on the work in 1b47ad160b55f50a7a98c180e18d80f0f8f17a67
(merge-recursive: more consistent interface, 2016-02-17).

Add tests for the merge-recursive rename options, as suggested by Eric Sunshine.
Also fixes an inconsistency in the behaviour of find-renames and a typo.

The first two commits contain fixes.

The tests were divided in the latter three, reproducing the chronological
history of the features:
  3. --rename-threshold only
  4. --rename-threshold and --no-renames
  5. --find-renames, --no-renames and deprecated --rename-threshold

Felipe Gonçalves Assis (5):
  merge-recursive: find-renames resets threshold
  merge-strategies.txt: fix typo
  merge-recursive: test rename threshold option
  merge-recursive: test option to disable renames
  merge-recursive: test more consistent interface

 Documentation/merge-strategies.txt                 |   4 +-
 merge-recursive.c                                  |   4 +-
 ...s.sh => t3032-merge-recursive-space-options.sh} |   2 +-
 t/t3034-merge-recursive-rename-options.sh          | 200 +++++++++++++++++++++
 4 files changed, 206 insertions(+), 4 deletions(-)
 rename t/{t3032-merge-recursive-options.sh => t3032-merge-recursive-space-options.sh} (99%)
 create mode 100755 t/t3034-merge-recursive-rename-options.sh

-- 
2.7.1.492.gc9722f8

[PATCH 1/5] merge-recursive: find-renames resets threshold

[Felipe Gonçalves Assis]

Make the find-renames option follow the behaviour in git-diff, where it
resets the threshold when none is given. So, for instance,
"--find-renames=25 --find-renames" should result in the default
threshold (50%) instead of 25%.

Signed-off-by: Felipe Gonçalves Assis <felipegassis@gmail.com>
---
 merge-recursive.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/merge-recursive.c b/merge-recursive.c
index 7bff5be..b880ae5 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -2094,8 +2094,10 @@ int parse_merge_opt(struct merge_options *o, const char *s)
 		o->renormalize = 0;
 	else if (!strcmp(s, "no-renames"))
 		o->detect_rename = 0;
-	else if (!strcmp(s, "find-renames"))
+	else if (!strcmp(s, "find-renames")) {
 		o->detect_rename = 1;
+		o->rename_score = 0;
+	}
 	else if (skip_prefix(s, "find-renames=", &arg) ||
 		 skip_prefix(s, "rename-threshold=", &arg)) {
 		if ((o->rename_score = parse_rename_score(&arg)) == -1 || *arg != 0)
-- 
2.7.1.492.gc9722f8

[PATCH 2/5] merge-strategies.txt: fix typo

[Felipe Gonçalves Assis]

Signed-off-by: Felipe Gonçalves Assis <felipegassis@gmail.com>
---
 Documentation/merge-strategies.txt | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Documentation/merge-strategies.txt b/Documentation/merge-strategies.txt
index ff359b6..2eb92b9 100644
--- a/Documentation/merge-strategies.txt
+++ b/Documentation/merge-strategies.txt
@@ -86,8 +86,8 @@ no-renames;;
 	See also linkgit:git-diff[1] `--no-renames`.
 
 find-renames[=<n>];;
-	Turn on rename detection, optionally setting the the similarity
-	threshold. This is the default.
+	Turn on rename detection, optionally setting the similarity
+	threshold.  This is the default.
 	See also linkgit:git-diff[1] `--find-renames`.
 
 rename-threshold=<n>;;
-- 
2.7.1.492.gc9722f8

[PATCH 3/5] merge-recursive: test rename threshold option

[Felipe Gonçalves Assis]

Commit 10ae7526bebb505ddddba01f76ec97d5f7b5e0e5 introduced this feature,
but did not include any tests. This commit fixes this.

Signed-off-by: Felipe Gonçalves Assis <felipegassis@gmail.com>
---
 t/t3034-merge-recursive-rename-threshold.sh | 146 ++++++++++++++++++++++++++++
 1 file changed, 146 insertions(+)
 create mode 100755 t/t3034-merge-recursive-rename-threshold.sh

diff --git a/t/t3034-merge-recursive-rename-threshold.sh b/t/t3034-merge-recursive-rename-threshold.sh
new file mode 100755
index 0000000..f0b3f44
--- /dev/null
+++ b/t/t3034-merge-recursive-rename-threshold.sh
@@ -0,0 +1,146 @@
+#!/bin/sh
+
+test_description='merge-recursive rename threshold option
+
+Test rename detection by examining rename/delete conflicts.
+
+Similarity index:
+R100 a-old a-new
+R075 b-old b-new
+R050 c-old c-new
+R025 d-old d-new
+'
+
+. ./test-lib.sh
+
+test_expect_success setup '
+	get_expected_stages () {
+		git checkout rename -- $1-new &&
+		git ls-files --stage $1-new > expected-stages-undetected-$1
+		sed "s/ 0	/ 2	/
+		" < expected-stages-undetected-$1 > expected-stages-detected-$1
+		git read-tree -u --reset HEAD
+	} &&
+
+	rename_detected () {
+		git ls-files --stage $1-old $1-new > stages-actual-$1 &&
+		test_cmp expected-stages-detected-$1 stages-actual-$1
+	} &&
+
+	rename_undetected () {
+		git ls-files --stage $1-old $1-new > stages-actual-$1 &&
+		test_cmp expected-stages-undetected-$1 stages-actual-$1
+	} &&
+
+	check_common () {
+		git ls-files --stage > stages-actual &&
+		test $(wc -l < stages-actual) -eq 4
+	} &&
+
+	check_find_renames_25 () {
+		check_common &&
+		rename_detected a &&
+		rename_detected b &&
+		rename_detected c &&
+		rename_detected d
+	} &&
+
+	check_find_renames_50 () {
+		check_common
+		rename_detected a &&
+		rename_detected b &&
+		rename_detected c &&
+		rename_undetected d
+	} &&
+
+	check_find_renames_75 () {
+		check_common
+		rename_detected a &&
+		rename_detected b &&
+		rename_undetected c &&
+		rename_undetected d
+	} &&
+
+	check_find_renames_100 () {
+		check_common
+		rename_detected a &&
+		rename_undetected b &&
+		rename_undetected c &&
+		rename_undetected d
+	} &&
+
+	check_no_renames () {
+		check_common
+		rename_undetected a &&
+		rename_undetected b &&
+		rename_undetected c &&
+		rename_undetected d
+	} &&
+
+	cat <<-\EOF > a-old &&
+	aa1
+	aa2
+	aa3
+	aa4
+	EOF
+	sed s/aa/bb/ < a-old > b-old &&
+	sed s/aa/cc/ < a-old > c-old &&
+	sed s/aa/dd/ < a-old > d-old &&
+	git add [a-d]-old &&
+	test_tick &&
+	git commit -m base &&
+	git rm [a-d]-old &&
+	test_tick &&
+	git commit -m delete &&
+	git checkout -b rename HEAD^ &&
+	cp a-old a-new &&
+	sed 1,1s/./x/ < b-old > b-new &&
+	sed 1,2s/./x/ < c-old > c-new &&
+	sed 1,3s/./x/ < d-old > d-new &&
+	git add [a-d]-new &&
+	git rm [a-d]-old &&
+	test_tick &&
+	git commit -m rename &&
+	get_expected_stages a &&
+	get_expected_stages b &&
+	get_expected_stages c &&
+	get_expected_stages d
+'
+
+test_expect_success 'the default similarity index is 50%' '
+	git read-tree --reset -u HEAD &&
+	test_must_fail git merge-recursive HEAD^ -- HEAD master &&
+	check_find_renames_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 &&
+	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 &&
+	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 &&
+	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 &&
+	check_find_renames_100
+'
+
+test_expect_success 'last wins in --rename-threshold=<m> --rename-threshold=<n>' '
+	git read-tree --reset -u HEAD &&
+	test_must_fail git merge-recursive --rename-threshold=25 --rename-threshold=75 HEAD^ -- HEAD master &&
+	check_find_renames_75
+'
+
+test_done
-- 
2.7.1.492.gc9722f8

[PATCH 4/5] merge-recursive: test option to disable renames

[Felipe Gonçalves Assis]

Also update name and description of tests for consistency:
"merge-recursive options"          -> "merge-recursive space options"
"merge-recursive rename threshold" -> "merge-recursive rename options"

Signed-off-by: Felipe Gonçalves Assis <felipegassis@gmail.com>
---
 ...ons.sh => t3032-merge-recursive-space-options.sh} |  2 +-
 ...ld.sh => t3034-merge-recursive-rename-options.sh} | 20 +++++++++++++++++++-
 2 files changed, 20 insertions(+), 2 deletions(-)
 rename t/{t3032-merge-recursive-options.sh => t3032-merge-recursive-space-options.sh} (99%)
 rename t/{t3034-merge-recursive-rename-threshold.sh => t3034-merge-recursive-rename-options.sh} (83%)

diff --git a/t/t3032-merge-recursive-options.sh b/t/t3032-merge-recursive-space-options.sh
similarity index 99%
rename from t/t3032-merge-recursive-options.sh
rename to t/t3032-merge-recursive-space-options.sh
index 4029c9c..b56180e 100755
--- a/t/t3032-merge-recursive-options.sh
+++ b/t/t3032-merge-recursive-space-options.sh
@@ -1,6 +1,6 @@
 #!/bin/sh
 
-test_description='merge-recursive options
+test_description='merge-recursive space options
 
 * [master] Clarify
  ! [remote] Remove cruft
diff --git a/t/t3034-merge-recursive-rename-threshold.sh b/t/t3034-merge-recursive-rename-options.sh
similarity index 83%
rename from t/t3034-merge-recursive-rename-threshold.sh
rename to t/t3034-merge-recursive-rename-options.sh
index f0b3f44..2f10fa7 100755
--- a/t/t3034-merge-recursive-rename-threshold.sh
+++ b/t/t3034-merge-recursive-rename-options.sh
@@ -1,6 +1,6 @@
 #!/bin/sh
 
-test_description='merge-recursive rename threshold option
+test_description='merge-recursive rename options
 
 Test rename detection by examining rename/delete conflicts.
 
@@ -137,10 +137,28 @@ test_expect_success 'rename threshold is truncated' '
 	check_find_renames_100
 '
 
+test_expect_success 'disabled rename detection' '
+	git read-tree --reset -u HEAD &&
+	git merge-recursive --no-renames HEAD^ -- HEAD master &&
+	check_no_renames
+'
+
 test_expect_success 'last wins in --rename-threshold=<m> --rename-threshold=<n>' '
 	git read-tree --reset -u HEAD &&
 	test_must_fail git merge-recursive --rename-threshold=25 --rename-threshold=75 HEAD^ -- HEAD master &&
 	check_find_renames_75
 '
 
+test_expect_success 'last wins in --no-renames --rename-threshold=<n>' '
+	git read-tree --reset -u HEAD &&
+	test_must_fail git merge-recursive --no-renames --rename-threshold=25 HEAD^ -- HEAD master &&
+	check_find_renames_25
+'
+
+test_expect_success 'last wins in --rename-threshold=<n> --no-renames' '
+	git read-tree --reset -u HEAD &&
+	git merge-recursive --rename-threshold=25 --no-renames HEAD^ -- HEAD master &&
+	check_no_renames
+'
+
 test_done
-- 
2.7.1.492.gc9722f8

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

[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=<n> behaves
as a synonym for find-renames=<n>. Test that find-renames resets
threshold.

Signed-off-by: Felipe Gonçalves Assis <felipegassis@gmail.com>
---
 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=<m> --rename-threshold=<n>' '
+test_expect_success 'last wins in --find-renames=<m> --find-renames=<n>' '
 	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=<n> is a synonym for find-renames=<n>' '
+	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=<n>' '
 	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=<n> --no-renames' '
 	check_no_renames
 '
 
+test_expect_success 'last wins in --rename-threshold=<n> --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=<n>' '
+	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

Re: [PATCH 1/5] merge-recursive: find-renames resets threshold

[Eric Sunshine]

On Sun, Feb 21, 2016 at 10:09 AM, Felipe Gonçalves Assis
<felipeg.assis@gmail.com> wrote:
> Make the find-renames option follow the behaviour in git-diff, where it
> resets the threshold when none is given. So, for instance,
> "--find-renames=25 --find-renames" should result in the default
> threshold (50%) instead of 25%.

Makes sense. I'd have expected to see the corresponding tests bundled
along with this patch rather than included in a separate patch (5/5)
whose title doesn't indicate any direct relation to this change
(although the corresponding test is mentioned briefly in the 5/5
commit message).

You might consider re-ordering the patches so that the patch (3/5)
which adds missing tests for existing features instead comes first.
Then, bundle the tests for this "fix" directly into this patch.

> Signed-off-by: Felipe Gonçalves Assis <felipegassis@gmail.com>
> ---
>  merge-recursive.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/merge-recursive.c b/merge-recursive.c
> index 7bff5be..b880ae5 100644
> --- a/merge-recursive.c
> +++ b/merge-recursive.c
> @@ -2094,8 +2094,10 @@ int parse_merge_opt(struct merge_options *o, const char *s)
>                 o->renormalize = 0;
>         else if (!strcmp(s, "no-renames"))
>                 o->detect_rename = 0;
> -       else if (!strcmp(s, "find-renames"))
> +       else if (!strcmp(s, "find-renames")) {
>                 o->detect_rename = 1;
> +               o->rename_score = 0;
> +       }
>         else if (skip_prefix(s, "find-renames=", &arg) ||
>                  skip_prefix(s, "rename-threshold=", &arg)) {
>                 if ((o->rename_score = parse_rename_score(&arg)) == -1 || *arg != 0)
> --
> 2.7.1.492.gc9722f8
>

Re: [PATCH 2/5] merge-strategies.txt: fix typo

[Eric Sunshine]

On Sun, Feb 21, 2016 at 10:09 AM, Felipe Gonçalves Assis
<felipeg.assis@gmail.com> wrote:
> Signed-off-by: Felipe Gonçalves Assis <felipegassis@gmail.com>
> ---
> diff --git a/Documentation/merge-strategies.txt b/Documentation/merge-strategies.txt
> @@ -86,8 +86,8 @@ no-renames;;
>  find-renames[=<n>];;
> -       Turn on rename detection, optionally setting the the similarity
> -       threshold. This is the default.
> +       Turn on rename detection, optionally setting the similarity
> +       threshold.  This is the default.

I'd probably have omitted the unimportant whitespace change on the
second line from the patch since it's just noise which distracts from
the real fix, but this alone is not worth a re-roll.

>         See also linkgit:git-diff[1] `--find-renames`.

Re: [PATCH 3/5] merge-recursive: test rename threshold option

[Eric Sunshine]

On Sun, Feb 21, 2016 at 10:09 AM, Felipe Gonçalves Assis
<felipeg.assis@gmail.com> wrote:
> merge-recursive: test rename threshold option

Maybe rephrase this as:

    t3034: add rename threshold tests

> Commit 10ae7526bebb505ddddba01f76ec97d5f7b5e0e5 introduced this feature,

In commit messages, it is helpful to provide more context about a
commit by quoting its "subject", like this:

    10ae752 (merge-recursive: option to specify rename
    threshold, 2010-09-27) introduced this feature...

> but did not include any tests. This commit fixes this.

"This commit fixes this." is redundant with the subject and could be dropped.

The above are superficial observation; see below for a bit more meaty stuff...

> Signed-off-by: Felipe Gonçalves Assis <felipegassis@gmail.com>
> ---
> diff --git a/t/t3034-merge-recursive-rename-threshold.sh b/t/t3034-merge-recursive-rename-threshold.sh
> @@ -0,0 +1,146 @@
> +#!/bin/sh
> +
> +test_description='merge-recursive rename threshold option
> +
> +Test rename detection by examining rename/delete conflicts.
> +
> +Similarity index:
> +R100 a-old a-new
> +R075 b-old b-new
> +R050 c-old c-new
> +R025 d-old d-new
> +'
> +
> +. ./test-lib.sh
> +
> +test_expect_success setup '
> +       get_expected_stages () {

There is no particularly good reason to define these shell functions
within the 'setup' test, and doing so makes the test itself more
difficult to read. More typical would be to define the functions
outside any test.

> +               git checkout rename -- $1-new &&
> +               git ls-files --stage $1-new > expected-stages-undetected-$1

Broken &&-chain.

Style: Omit space after redirection operators (here and elsewhere in
the script). <inputfile >outputfile

> +               sed "s/ 0       / 2     /
> +               " < expected-stages-undetected-$1 > expected-stages-detected-$1

This is difficult to read due to the way this is formatted with the
newline inside the quoted string. How about formatting it in a more
typical fashion, like this?

    sed "s/ 0       / 2     /" <expected-stages-undetected-$1 \
        >expected-stages-detected-$1

Also, broken &&-chain.

> +               git read-tree -u --reset HEAD
> +       } &&
> +
> +       rename_detected () {
> +               git ls-files --stage $1-old $1-new > stages-actual-$1 &&
> +               test_cmp expected-stages-detected-$1 stages-actual-$1
> +       } &&
> +
> +       rename_undetected () {
> +               git ls-files --stage $1-old $1-new > stages-actual-$1 &&
> +               test_cmp expected-stages-undetected-$1 stages-actual-$1
> +       } &&
> +
> +       check_common () {
> +               git ls-files --stage > stages-actual &&
> +               test $(wc -l < stages-actual) -eq 4

Perhaps use test_line_count() instead.

> +       } &&
> +
> +       check_find_renames_25 () {
> +               check_common &&
> +               rename_detected a &&
> +               rename_detected b &&
> +               rename_detected c &&
> +               rename_detected d
> +       } &&
> +
> +       check_find_renames_50 () {
> +               check_common

Broken &&-chain.

> +               rename_detected a &&
> +               rename_detected b &&
> +               rename_detected c &&
> +               rename_undetected d
> +       } &&
> +
> +       check_find_renames_75 () {
> +               check_common

Ditto.

> +               rename_detected a &&
> +               rename_detected b &&
> +               rename_undetected c &&
> +               rename_undetected d
> +       } &&
> +
> +       check_find_renames_100 () {
> +               check_common

Ditto.

> +               rename_detected a &&
> +               rename_undetected b &&
> +               rename_undetected c &&
> +               rename_undetected d
> +       } &&
> +
> +       check_no_renames () {
> +               check_common

Ditto.

> +               rename_undetected a &&
> +               rename_undetected b &&
> +               rename_undetected c &&
> +               rename_undetected d
> +       } &&
> +
> +       cat <<-\EOF > a-old &&
> +       aa1
> +       aa2
> +       aa3
> +       aa4
> +       EOF
> +       sed s/aa/bb/ < a-old > b-old &&
> +       sed s/aa/cc/ < a-old > c-old &&
> +       sed s/aa/dd/ < a-old > d-old &&
> +       git add [a-d]-old &&
> +       test_tick &&

Nothing in these tests depend upon these test_tick() invocations, so
having them here distracts the reader by making him wonder if
something unusual is going on to require them.

> +       git commit -m base &&
> +       git rm [a-d]-old &&
> +       test_tick &&
> +       git commit -m delete &&
> +       git checkout -b rename HEAD^ &&
> +       cp a-old a-new &&
> +       sed 1,1s/./x/ < b-old > b-new &&
> +       sed 1,2s/./x/ < c-old > c-new &&
> +       sed 1,3s/./x/ < d-old > d-new &&
> +       git add [a-d]-new &&
> +       git rm [a-d]-old &&
> +       test_tick &&
> +       git commit -m rename &&
> +       get_expected_stages a &&
> +       get_expected_stages b &&
> +       get_expected_stages c &&
> +       get_expected_stages d
> +'
> +
> +test_expect_success 'the default similarity index is 50%' '

s/the//

Also, do you mean s/index/threshold/ ?

> +       git read-tree --reset -u HEAD &&
> +       test_must_fail git merge-recursive HEAD^ -- HEAD master &&
> +       check_find_renames_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 &&
> +       check_find_renames_25
> [...]
> +       check_find_renames_75
> [...]
> +       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 &&
> +       check_find_renames_100
> +'

Is this truncation documented behavior or is it just a detail of the
current implementation. (Genuine question; I haven't checked the
documentation or source.) If just an implementation detail, then it
might not be desirable to formalize it via a test.

> +test_expect_success 'last wins in --rename-threshold=<m> --rename-threshold=<n>' '
> +       git read-tree --reset -u HEAD &&
> +       test_must_fail git merge-recursive --rename-threshold=25 --rename-threshold=75 HEAD^ -- HEAD master &&
> +       check_find_renames_75
> +'

Would it make sense to add tests checking that invalid
--rename-threshold= arguments, such as negative and non-numbers,
correctly error out?

> +test_done

Re: [PATCH 4/5] merge-recursive: test option to disable renames

[Eric Sunshine]

On Sun, Feb 21, 2016 at 10:09 AM, Felipe Gonçalves Assis
<felipeg.assis@gmail.com> wrote:
> Also update name and description of tests for consistency:
> "merge-recursive options"          -> "merge-recursive space options"
> "merge-recursive rename threshold" -> "merge-recursive rename options"
>
> Signed-off-by: Felipe Gonçalves Assis <felipegassis@gmail.com>
> ---
> diff --git a/t/t3034-merge-recursive-rename-threshold.sh b/t/t3034-merge-recursive-rename-options.sh
> similarity index 83%
> rename from t/t3034-merge-recursive-rename-threshold.sh
> rename to t/t3034-merge-recursive-rename-options.sh

t3034 was entirely new in the previous patch (3/5) and could have been
given the correct name at that time, so it's not clear why the
approach of giving it a too-specific name and then immediately
renaming it with a more general purpose name in the following patch is
desirable. It should be perfectly fine to start with the general
purpose name even if the patch initially contains only very specific
tests (knowing that you will be adding more tests later).

> --- a/t/t3034-merge-recursive-rename-threshold.sh
> +++ b/t/t3034-merge-recursive-rename-options.sh
> @@ -1,6 +1,6 @@
>  #!/bin/sh
>
> -test_description='merge-recursive rename threshold option
> +test_description='merge-recursive rename options

Ditto. This could have been described with the more generic "options"
from the start.

>  Test rename detection by examining rename/delete conflicts.
>
> @@ -137,10 +137,28 @@ test_expect_success 'rename threshold is truncated' '
>         check_find_renames_100
>  '
>
> +test_expect_success 'disabled rename detection' '
> +       git read-tree --reset -u HEAD &&
> +       git merge-recursive --no-renames HEAD^ -- HEAD master &&
> +       check_no_renames
> +'
> +
>  test_expect_success 'last wins in --rename-threshold=<m> --rename-threshold=<n>' '
>         git read-tree --reset -u HEAD &&
>         test_must_fail git merge-recursive --rename-threshold=25 --rename-threshold=75 HEAD^ -- HEAD master &&
>         check_find_renames_75
>  '
>
> +test_expect_success 'last wins in --no-renames --rename-threshold=<n>' '
> +       git read-tree --reset -u HEAD &&
> +       test_must_fail git merge-recursive --no-renames --rename-threshold=25 HEAD^ -- HEAD master &&
> +       check_find_renames_25
> +'
> +
> +test_expect_success 'last wins in --rename-threshold=<n> --no-renames' '
> +       git read-tree --reset -u HEAD &&
> +       git merge-recursive --rename-threshold=25 --no-renames HEAD^ -- HEAD master &&
> +       check_no_renames
> +'

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

[Eric Sunshine]

On Sun, Feb 21, 2016 at 10:09 AM, Felipe Gonçalves Assis
<felipeg.assis@gmail.com> 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=<n> behaves
> as a synonym for find-renames=<n>. 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 <felipegassis@gmail.com>
> ---
> 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=<m> --rename-threshold=<n>' '
> +test_expect_success 'last wins in --find-renames=<m> --find-renames=<n>' '
>         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=<n> is a synonym for find-renames=<n>' '
> +       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.

>  test_expect_success 'last wins in --no-renames --rename-threshold=<n>' '
>         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=<n> --no-renames' '
>         check_no_renames
>  '
>
> +test_expect_success 'last wins in --rename-threshold=<n> --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=<n>' '
> +       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

Re: [PATCH 3/5] merge-recursive: test rename threshold option

[Felipe Gonçalves Assis]

On 21 February 2016 at 14:52, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Sun, Feb 21, 2016 at 10:09 AM, Felipe Gonçalves Assis
> <felipeg.assis@gmail.com> wrote:
>> merge-recursive: test rename threshold option
>
>> +       git read-tree --reset -u HEAD &&
>> +       test_must_fail git merge-recursive HEAD^ -- HEAD master &&
>> +       check_find_renames_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 &&
>> +       check_find_renames_25
>> [...]
>> +       check_find_renames_75
>> [...]
>> +       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 &&
>> +       check_find_renames_100
>> +'
>
> Is this truncation documented behavior or is it just a detail of the
> current implementation. (Genuine question; I haven't checked the
> documentation or source.) If just an implementation detail, then it
> might not be desirable to formalize it via a test.
>

Not documented. I will remove this. If you prefer to have it
documented and the test added back later, I can do that.

>> +test_expect_success 'last wins in --rename-threshold=<m> --rename-threshold=<n>' '
>> +       git read-tree --reset -u HEAD &&
>> +       test_must_fail git merge-recursive --rename-threshold=25 --rename-threshold=75 HEAD^ -- HEAD master &&
>> +       check_find_renames_75
>> +'
>
> Would it make sense to add tests checking that invalid
> --rename-threshold= arguments, such as negative and non-numbers,
> correctly error out?
>

I guess so. Can I ask you for a suggestion on how to check this?

Given that merges here usually fail anyway because of the conflicts,
what is the best way of checking the effect of an argument rejection?
1. Check that the merge fails but the index is not changed
2. Check for a specific exit code
3. Use another setup so that the merges succeed


Thanks for the detailed review. Already working on the other comments.

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

[Felipe Gonçalves Assis]

On 21 February 2016 at 15:40, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Sun, Feb 21, 2016 at 10:09 AM, Felipe Gonçalves Assis
> <felipeg.assis@gmail.com> 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=<n> behaves
>> as a synonym for find-renames=<n>. 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 <felipegassis@gmail.com>
>> ---
>> 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=<n> is a synonym for find-renames=<n>' '
>> +       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=<n>' '
>>         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=<n> --no-renames' '
>>         check_no_renames
>>  '
>>
>> +test_expect_success 'last wins in --rename-threshold=<n> --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=<n>' '
>> +       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

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

[Eric Sunshine]

On Sun, Feb 21, 2016 at 2:55 PM, Felipe Gonçalves Assis
<felipeg.assis@gmail.com> wrote:
> On 21 February 2016 at 15:40, Eric Sunshine <sunshine@sunshineco.com> 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=<n> is a synonym for find-renames=<n>' '
>>> +       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.

Re: [PATCH 3/5] merge-recursive: test rename threshold option

[Eric Sunshine]

On Sun, Feb 21, 2016 at 1:55 PM, Felipe Gonçalves Assis
<felipeg.assis@gmail.com> wrote:
> On 21 February 2016 at 14:52, Eric Sunshine <sunshine@sunshineco.com> wrote:
>> On Sun, Feb 21, 2016 at 10:09 AM, Felipe Gonçalves Assis
>> <felipeg.assis@gmail.com> wrote:
>>> +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 &&
>>> +       check_find_renames_100
>>> +'
>>
>> Is this truncation documented behavior or is it just a detail of the
>> current implementation. (Genuine question; I haven't checked the
>> documentation or source.) If just an implementation detail, then it
>> might not be desirable to formalize it via a test.
>
> Not documented. I will remove this. If you prefer to have it
> documented and the test added back later, I can do that.

Looking at the code itself and its history, this seems to be a
deliberate decision, so the test may be appropriate, however, I defer
to Junio's judgment.

>>> +test_expect_success 'last wins in --rename-threshold=<m> --rename-threshold=<n>' '
>>> +       git read-tree --reset -u HEAD &&
>>> +       test_must_fail git merge-recursive --rename-threshold=25 --rename-threshold=75 HEAD^ -- HEAD master &&
>>> +       check_find_renames_75
>>> +'
>>
>> Would it make sense to add tests checking that invalid
>> --rename-threshold= arguments, such as negative and non-numbers,
>> correctly error out?
>
> I guess so. Can I ask you for a suggestion on how to check this?
>
> Given that merges here usually fail anyway because of the conflicts,
> what is the best way of checking the effect of an argument rejection?
> 1. Check that the merge fails but the index is not changed
> 2. Check for a specific exit code
> 3. Use another setup so that the merges succeed

The last option would be most straightforward, however, looking at
diff.c:parse_rename_score(), I see that it never actually errors out
(by returning -1), so you may not (presently) be able to test these
cases. It seems (presently) that merge-recursive.c:parse_merge_opt()
can only catch a usage error if nothing follows the '='.

Re: [PATCH 3/5] merge-recursive: test rename threshold option

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> On Sun, Feb 21, 2016 at 1:55 PM, Felipe Gonçalves Assis
> <felipeg.assis@gmail.com> wrote:
>> On 21 February 2016 at 14:52, Eric Sunshine <sunshine@sunshineco.com> wrote:
>>> On Sun, Feb 21, 2016 at 10:09 AM, Felipe Gonçalves Assis
>>> <felipeg.assis@gmail.com> wrote:
>>>> +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 &&
>>>> +       check_find_renames_100
>>>> +'
>>>
>>> Is this truncation documented behavior or is it just a detail of the
>>> current implementation. (Genuine question; I haven't checked the
>>> documentation or source.) If just an implementation detail, then it
>>> might not be desirable to formalize it via a test.
>>
>> Not documented. I will remove this. If you prefer to have it
>> documented and the test added back later, I can do that.
>
> Looking at the code itself and its history, this seems to be a
> deliberate decision, so the test may be appropriate, however, I defer
> to Junio's judgment.

This is just us not caring enough about illogical input by crazy
users who ignore the fact that the rename scores are supposed to be
between 0 and max.

We could have added code to reject out-of-range values when we added
support for input with '%', but we instead just made sure we clip
the result within the range (probably because it was easier).

I personally do not think it would break too many people if we
changed the parser to error out, but at the same time, I do not
think it is worth the code churn to do so, and it probably is not
worth to document the clipping either.


[Footnote]

*1* With the original "give us an integer, and we'll take it as a
ratio of that number out of the next power of ten", there was no
need to worry about out-of-range values.