All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2 0/5] Tests and fixes for merge-recursive rename options
@ 2016-02-21 22:59 Felipe Gonçalves Assis
  2016-02-21 22:59 ` [PATCH v2 1/5] merge-strategies.txt: fix typo Felipe Gonçalves Assis
                   ` (5 more replies)
  0 siblings, 6 replies; 11+ messages in thread
From: Felipe Gonçalves Assis @ 2016-02-21 22:59 UTC (permalink / raw)
  To: git; +Cc: Johannes.Schindelin, gitster, sunshine, Felipe Gonçalves Assis

This is a reorganisation of the previous series, bundling the test for the fix
along with the commit itself, as suggested by Eric. It also includes many fixes
and improvements pointed out by the same reviewer, whom I thank.

The typo fix is the same as before.

In "add rename threshold tests", I include tests involving --find-renames,
except for one that depends on the fix.

"test option to disable renames" adds tests involving --rename-threshold.

"test deprecated interface" tests the aliasing --rename-threshold.

In "find-renames resets threshold", the specific test for the feature was
bundled along.

To Junio: Please pay special attention to the test of threshold truncation.
Given that it seems to be an undocumented feature, I am not sure whether it
should be included or not.

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

 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          | 235 +++++++++++++++++++++
 4 files changed, 241 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.gd821b20

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH v2 1/5] merge-strategies.txt: fix typo
  2016-02-21 22:59 [PATCH v2 0/5] Tests and fixes for merge-recursive rename options Felipe Gonçalves Assis
@ 2016-02-21 22:59 ` Felipe Gonçalves Assis
  2016-02-21 22:59 ` [PATCH v2 2/5] t3034: add rename threshold tests Felipe Gonçalves Assis
                   ` (4 subsequent siblings)
  5 siblings, 0 replies; 11+ messages in thread
From: Felipe Gonçalves Assis @ 2016-02-21 22:59 UTC (permalink / raw)
  To: git; +Cc: Johannes.Schindelin, gitster, sunshine, 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.gd821b20

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH v2 2/5] t3034: add rename threshold tests
  2016-02-21 22:59 [PATCH v2 0/5] Tests and fixes for merge-recursive rename options Felipe Gonçalves Assis
  2016-02-21 22:59 ` [PATCH v2 1/5] merge-strategies.txt: fix typo Felipe Gonçalves Assis
@ 2016-02-21 22:59 ` Felipe Gonçalves Assis
  2016-02-22 19:03   ` Junio C Hamano
  2016-02-21 22:59 ` [PATCH v2 3/5] t3034: test option to disable renames Felipe Gonçalves Assis
                   ` (3 subsequent siblings)
  5 siblings, 1 reply; 11+ messages in thread
From: Felipe Gonçalves Assis @ 2016-02-21 22:59 UTC (permalink / raw)
  To: git; +Cc: Johannes.Schindelin, gitster, sunshine, Felipe Gonçalves Assis

10ae752 (merge-recursive: option to specify rename threshold,
2010-09-27) introduced this feature but did not include any tests.

The tests use the new option --find-renames, which replaces the then
introduced and now deprecated option --rename-threshold.

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

Signed-off-by: Felipe Gonçalves Assis <felipegassis@gmail.com>
---

Now this already introduces the final names and descriptions for the tests.

The test for threshold truncation was kept for now. Please confirm whether it is
desirable.

New tests were added at the end to check that invalid arguments to find-renames=
such as negative and non-numbers error out.

 ...s.sh => t3032-merge-recursive-space-options.sh} |   2 +-
 t/t3034-merge-recursive-rename-options.sh          | 159 +++++++++++++++++++++
 2 files changed, 160 insertions(+), 1 deletion(-)
 rename t/{t3032-merge-recursive-options.sh => t3032-merge-recursive-space-options.sh} (99%)
 create mode 100755 t/t3034-merge-recursive-rename-options.sh

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-options.sh b/t/t3034-merge-recursive-rename-options.sh
new file mode 100755
index 0000000..7ae7f83
--- /dev/null
+++ b/t/t3034-merge-recursive-rename-options.sh
@@ -0,0 +1,159 @@
+#!/bin/sh
+
+test_description='merge-recursive rename options
+
+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
+
+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_line_count = 4 stages-actual
+}
+
+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
+}
+
+test_expect_success setup '
+	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 &&
+	git commit -m base &&
+	git rm [a-d]-old &&
+	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 &&
+	git commit -m rename &&
+	get_expected_stages a &&
+	get_expected_stages b &&
+	get_expected_stages c &&
+	get_expected_stages d
+'
+
+test_expect_success 'default similarity threshold 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 --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 --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 --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 --find-renames=200% HEAD^ -- HEAD master &&
+	check_find_renames_100
+'
+
+test_expect_success 'last wins in --find-renames=<m> --find-renames=<n>' '
+	git read-tree --reset -u HEAD &&
+	test_must_fail git merge-recursive --find-renames=25 --find-renames=75 HEAD^ -- HEAD master &&
+	check_find_renames_75
+'
+
+test_expect_success 'assumption for further tests: trivial merge succeeds' '
+	git read-tree --reset -u HEAD &&
+	git merge-recursive HEAD -- HEAD HEAD &&
+	git diff --quiet --cached &&
+	git merge-recursive --find-renames=25 HEAD -- HEAD HEAD &&
+	git diff --quiet --cached &&
+	git merge-recursive --find-renames=75 HEAD -- HEAD HEAD &&
+	git diff --quiet --cached &&
+	git merge-recursive --find-renames=100% HEAD -- HEAD HEAD &&
+	git diff --quiet --cached
+'
+
+test_expect_success '--find-renames rejects negative argument' '
+	git read-tree --reset -u HEAD &&
+	test_must_fail git merge-recursive --find-renames=-25 HEAD -- HEAD HEAD &&
+	git diff --quiet --cached
+'
+
+test_expect_success '--find-renames rejects non-numbers' '
+	git read-tree --reset -u HEAD &&
+	test_must_fail git merge-recursive --find-renames=0xf HEAD -- HEAD HEAD &&
+	git diff --quiet --cached
+'
+
+test_done
-- 
2.7.1.492.gd821b20

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH v2 3/5] t3034: test option to disable renames
  2016-02-21 22:59 [PATCH v2 0/5] Tests and fixes for merge-recursive rename options Felipe Gonçalves Assis
  2016-02-21 22:59 ` [PATCH v2 1/5] merge-strategies.txt: fix typo Felipe Gonçalves Assis
  2016-02-21 22:59 ` [PATCH v2 2/5] t3034: add rename threshold tests Felipe Gonçalves Assis
@ 2016-02-21 22:59 ` Felipe Gonçalves Assis
  2016-02-21 22:59 ` [PATCH v2 4/5] t3034: test deprecated interface Felipe Gonçalves Assis
                   ` (2 subsequent siblings)
  5 siblings, 0 replies; 11+ messages in thread
From: Felipe Gonçalves Assis @ 2016-02-21 22:59 UTC (permalink / raw)
  To: git; +Cc: Johannes.Schindelin, gitster, sunshine, Felipe Gonçalves Assis

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

diff --git a/t/t3034-merge-recursive-rename-options.sh b/t/t3034-merge-recursive-rename-options.sh
index 7ae7f83..a459236 100755
--- a/t/t3034-merge-recursive-rename-options.sh
+++ b/t/t3034-merge-recursive-rename-options.sh
@@ -68,6 +68,14 @@ check_find_renames_100 () {
 	rename_undetected d
 }
 
+check_no_renames () {
+	check_common &&
+	rename_undetected a &&
+	rename_undetected b &&
+	rename_undetected c &&
+	rename_undetected d
+}
+
 test_expect_success setup '
 	cat <<-\EOF >a-old &&
 	aa1
@@ -126,12 +134,30 @@ 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 --find-renames=<m> --find-renames=<n>' '
 	git read-tree --reset -u HEAD &&
 	test_must_fail git merge-recursive --find-renames=25 --find-renames=75 HEAD^ -- HEAD master &&
 	check_find_renames_75
 '
 
+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 'assumption for further tests: trivial merge succeeds' '
 	git read-tree --reset -u HEAD &&
 	git merge-recursive HEAD -- HEAD HEAD &&
@@ -141,6 +167,8 @@ test_expect_success 'assumption for further tests: trivial merge succeeds' '
 	git merge-recursive --find-renames=75 HEAD -- HEAD HEAD &&
 	git diff --quiet --cached &&
 	git merge-recursive --find-renames=100% HEAD -- HEAD HEAD &&
+	git diff --quiet --cached &&
+	git merge-recursive --no-renames HEAD -- HEAD HEAD &&
 	git diff --quiet --cached
 '
 
-- 
2.7.1.492.gd821b20

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH v2 4/5] t3034: test deprecated interface
  2016-02-21 22:59 [PATCH v2 0/5] Tests and fixes for merge-recursive rename options Felipe Gonçalves Assis
                   ` (2 preceding siblings ...)
  2016-02-21 22:59 ` [PATCH v2 3/5] t3034: test option to disable renames Felipe Gonçalves Assis
@ 2016-02-21 22:59 ` Felipe Gonçalves Assis
  2016-02-21 22:59 ` [PATCH v2 5/5] merge-recursive: find-renames resets threshold Felipe Gonçalves Assis
  2016-02-22 21:00 ` [PATCH v2 0/5] Tests and fixes for merge-recursive rename options Junio C Hamano
  5 siblings, 0 replies; 11+ messages in thread
From: Felipe Gonçalves Assis @ 2016-02-21 22:59 UTC (permalink / raw)
  To: git; +Cc: Johannes.Schindelin, gitster, sunshine, Felipe Gonçalves Assis

--find-renames= and --rename-threshold= should be aliases.

Signed-off-by: Felipe Gonçalves Assis <felipegassis@gmail.com>
---

Now includes tests with invalid arguments to --rename-threshold=

 t/t3034-merge-recursive-rename-options.sh | 42 +++++++++++++++++++++++++++++++
 1 file changed, 42 insertions(+)

diff --git a/t/t3034-merge-recursive-rename-options.sh b/t/t3034-merge-recursive-rename-options.sh
index a459236..d4f9742 100755
--- a/t/t3034-merge-recursive-rename-options.sh
+++ b/t/t3034-merge-recursive-rename-options.sh
@@ -184,4 +184,46 @@ test_expect_success '--find-renames rejects non-numbers' '
 	git diff --quiet --cached
 '
 
+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 &&
+	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_expect_success '--rename-threshold=<n> rejects negative argument' '
+	git read-tree --reset -u HEAD &&
+	test_must_fail git merge-recursive --rename-threshold=-25 HEAD -- HEAD HEAD &&
+	git diff --quiet --cached
+'
+
+test_expect_success '--rename-threshold=<n> rejects non-numbers' '
+	git read-tree --reset -u HEAD &&
+	test_must_fail git merge-recursive --rename-threshold=0xf HEAD -- HEAD HEAD &&
+	git diff --quiet --cached
+'
+
+test_expect_success 'last wins in --rename-threshold=<m> --find-renames=<n>' '
+	git read-tree --reset -u HEAD &&
+	test_must_fail git merge-recursive --rename-threshold=25 --find-renames=75 HEAD^ -- HEAD master &&
+	check_find_renames_75
+'
+
+test_expect_success 'last wins in --find-renames=<m> --rename-threshold=<n>' '
+	git read-tree --reset -u HEAD &&
+	test_must_fail git merge-recursive --find-renames=75 --rename-threshold=25 HEAD^ -- HEAD master &&
+	check_find_renames_25
+'
+
 test_done
-- 
2.7.1.492.gd821b20

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH v2 5/5] merge-recursive: find-renames resets threshold
  2016-02-21 22:59 [PATCH v2 0/5] Tests and fixes for merge-recursive rename options Felipe Gonçalves Assis
                   ` (3 preceding siblings ...)
  2016-02-21 22:59 ` [PATCH v2 4/5] t3034: test deprecated interface Felipe Gonçalves Assis
@ 2016-02-21 22:59 ` Felipe Gonçalves Assis
  2016-02-22 21:00 ` [PATCH v2 0/5] Tests and fixes for merge-recursive rename options Junio C Hamano
  5 siblings, 0 replies; 11+ messages in thread
From: Felipe Gonçalves Assis @ 2016-02-21 22:59 UTC (permalink / raw)
  To: git; +Cc: Johannes.Schindelin, gitster, sunshine, 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%.

Add corresponding test.

Signed-off-by: Felipe Gonçalves Assis <felipegassis@gmail.com>
---

Now bundles the relevant test.

 merge-recursive.c                         | 4 +++-
 t/t3034-merge-recursive-rename-options.sh | 6 ++++++
 2 files changed, 9 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)
diff --git a/t/t3034-merge-recursive-rename-options.sh b/t/t3034-merge-recursive-rename-options.sh
index d4f9742..54d5f1b 100755
--- a/t/t3034-merge-recursive-rename-options.sh
+++ b/t/t3034-merge-recursive-rename-options.sh
@@ -146,6 +146,12 @@ test_expect_success 'last wins in --find-renames=<m> --find-renames=<n>' '
 	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 &&
-- 
2.7.1.492.gd821b20

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH v2 2/5] t3034: add rename threshold tests
  2016-02-21 22:59 ` [PATCH v2 2/5] t3034: add rename threshold tests Felipe Gonçalves Assis
@ 2016-02-22 19:03   ` Junio C Hamano
  0 siblings, 0 replies; 11+ messages in thread
From: Junio C Hamano @ 2016-02-22 19:03 UTC (permalink / raw)
  To: Felipe Gonçalves Assis
  Cc: git, Johannes.Schindelin, sunshine, Felipe Gonçalves Assis

"Felipe Gonçalves Assis"  <felipeg.assis@gmail.com> writes:

> 10ae752 (merge-recursive: option to specify rename threshold,
> 2010-09-27) introduced this feature but did not include any tests.
>
> The tests use the new option --find-renames, which replaces the then
> introduced and now deprecated option --rename-threshold.
> ...
> diff --git a/t/t3034-merge-recursive-rename-options.sh b/t/t3034-merge-recursive-rename-options.sh
> new file mode 100755
> index 0000000..7ae7f83
> --- /dev/null
> +++ b/t/t3034-merge-recursive-rename-options.sh
> @@ -0,0 +1,159 @@
> +#!/bin/sh
> +
> +test_description='merge-recursive rename options
> +
> +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_expect_success setup '
> +	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 &&
> +	git commit -m base &&
> +	git rm [a-d]-old &&
> +	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 &&
> +	git commit -m rename &&
> +	get_expected_stages a &&
> +	get_expected_stages b &&
> +	get_expected_stages c &&
> +	get_expected_stages d
> +'

I somehow doubt that it is wise to make the similarity index
computed by the current heuristics as a hard promise like this test
does.

This test specifies that turning the original bb1/bb2/bb3/bb4 into
updated xb1/bb2/bb3/bb4 _MUST_ get 75% similarity, but that is not
something we want to guarantee, ever.  We may later update the
algorithm and tweak such a change to register 70% or 78%, but such a
change would break the expectation by this test.  This test script
however should not be interested in the exact similarity index
assignment for a given filepair--it only wants to make sure that the
option chooses the filepair with similarity index above the given
value.

The test for --find-renames=<num> should instead only validate that
the code works on the value given from the command line relative to
the similarity index the code computed.  I.e. first measure what the
similarity index going from b-old to b-new is (e.g. it may say 76%,
or 74%, depending on the version of Git being tested), then choose a
value that is higher (or lower) than that similarity to give to
the --find-renames=<num> option and ensure that merge-recursive does
what is expected.

I am not very happy with this one.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2 0/5] Tests and fixes for merge-recursive rename options
  2016-02-21 22:59 [PATCH v2 0/5] Tests and fixes for merge-recursive rename options Felipe Gonçalves Assis
                   ` (4 preceding siblings ...)
  2016-02-21 22:59 ` [PATCH v2 5/5] merge-recursive: find-renames resets threshold Felipe Gonçalves Assis
@ 2016-02-22 21:00 ` Junio C Hamano
  2016-02-22 22:16   ` Felipe Gonçalves Assis
  5 siblings, 1 reply; 11+ messages in thread
From: Junio C Hamano @ 2016-02-22 21:00 UTC (permalink / raw)
  To: Felipe Gonçalves Assis
  Cc: git, Johannes.Schindelin, sunshine, Felipe Gonçalves Assis

"Felipe Gonçalves Assis"  <felipeg.assis@gmail.com> writes:

> This is a reorganisation of the previous series, bundling the test for the fix
> along with the commit itself, as suggested by Eric. It also includes many fixes
> and improvements pointed out by the same reviewer, whom I thank.
>
> The typo fix is the same as before.
>
> In "add rename threshold tests", I include tests involving --find-renames,
> except for one that depends on the fix.
>
> "test option to disable renames" adds tests involving --rename-threshold.
>
> "test deprecated interface" tests the aliasing --rename-threshold.
>
> In "find-renames resets threshold", the specific test for the feature was
> bundled along.
>
> To Junio: Please pay special attention to the test of threshold truncation.
> Given that it seems to be an undocumented feature, I am not sure whether it
> should be included or not.
>
> Felipe Gonçalves Assis (5):
>   merge-strategies.txt: fix typo
>   t3034: add rename threshold tests
>   t3034: test option to disable renames
>   t3034: test deprecated interface
>   merge-recursive: find-renames resets threshold

As I said, I am reluctant to take the 25%/50%/75% tests in their
current form.  Let me take the first one and a half of the last one
(i.e. excluding the test) for now.

Thanks.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2 0/5] Tests and fixes for merge-recursive rename options
  2016-02-22 21:00 ` [PATCH v2 0/5] Tests and fixes for merge-recursive rename options Junio C Hamano
@ 2016-02-22 22:16   ` Felipe Gonçalves Assis
  2016-02-22 22:29     ` Junio C Hamano
  0 siblings, 1 reply; 11+ messages in thread
From: Felipe Gonçalves Assis @ 2016-02-22 22:16 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: Git List, Johannes Schindelin, Eric Sunshine,
	Felipe Gonçalves Assis

On 22 February 2016 at 18:00, Junio C Hamano <gitster@pobox.com> wrote:
> "Felipe Gonçalves Assis"  <felipeg.assis@gmail.com> writes:
>
>> This is a reorganisation of the previous series, bundling the test for the fix
>> along with the commit itself, as suggested by Eric. It also includes many fixes
>> and improvements pointed out by the same reviewer, whom I thank.
>>
>> The typo fix is the same as before.
>>
>> In "add rename threshold tests", I include tests involving --find-renames,
>> except for one that depends on the fix.
>>
>> "test option to disable renames" adds tests involving --rename-threshold.
>>
>> "test deprecated interface" tests the aliasing --rename-threshold.
>>
>> In "find-renames resets threshold", the specific test for the feature was
>> bundled along.
>>
>> To Junio: Please pay special attention to the test of threshold truncation.
>> Given that it seems to be an undocumented feature, I am not sure whether it
>> should be included or not.
>>
>> Felipe Gonçalves Assis (5):
>>   merge-strategies.txt: fix typo
>>   t3034: add rename threshold tests
>>   t3034: test option to disable renames
>>   t3034: test deprecated interface
>>   merge-recursive: find-renames resets threshold
>
> As I said, I am reluctant to take the 25%/50%/75% tests in their
> current form.  Let me take the first one and a half of the last one
> (i.e. excluding the test) for now.
>
> Thanks.

Ok, should I post a new version of the patch without the tests while I
rework them or does that mean that you have already filtered them out
locally?

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2 0/5] Tests and fixes for merge-recursive rename options
  2016-02-22 22:16   ` Felipe Gonçalves Assis
@ 2016-02-22 22:29     ` Junio C Hamano
  2016-02-22 22:37       ` Felipe Gonçalves Assis
  0 siblings, 1 reply; 11+ messages in thread
From: Junio C Hamano @ 2016-02-22 22:29 UTC (permalink / raw)
  To: Felipe Gonçalves Assis
  Cc: Git List, Johannes Schindelin, Eric Sunshine,
	Felipe Gonçalves Assis

Felipe Gonçalves Assis <felipeg.assis@gmail.com> writes:

>> As I said, I am reluctant to take the 25%/50%/75% tests in their
>> current form.  Let me take the first one and a half of the last one
>> (i.e. excluding the test) for now.
>>
>> Thanks.
>
> Ok, should I post a new version of the patch without the tests while I
> rework them or does that mean that you have already filtered them out
> locally?

I already have and queued them tentatively as 

    c443d39 merge-recursive: find-renames resets threshold
    83837ec merge-strategies.txt: fix typo

but they haven't been merged to 'next', so it is up to you whether
you rebuild the remainder on top of c443d39 or redo these 5 patches
altogether (just tell me to drop these two if you go the latter
route).

Thanks.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2 0/5] Tests and fixes for merge-recursive rename options
  2016-02-22 22:29     ` Junio C Hamano
@ 2016-02-22 22:37       ` Felipe Gonçalves Assis
  0 siblings, 0 replies; 11+ messages in thread
From: Felipe Gonçalves Assis @ 2016-02-22 22:37 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: Git List, Johannes Schindelin, Eric Sunshine,
	Felipe Gonçalves Assis

On 22 February 2016 at 19:29, Junio C Hamano <gitster@pobox.com> wrote:
> Felipe Gonçalves Assis <felipeg.assis@gmail.com> writes:
>
>>> As I said, I am reluctant to take the 25%/50%/75% tests in their
>>> current form.  Let me take the first one and a half of the last one
>>> (i.e. excluding the test) for now.
>>>
>>> Thanks.
>>
>> Ok, should I post a new version of the patch without the tests while I
>> rework them or does that mean that you have already filtered them out
>> locally?
>
> I already have and queued them tentatively as
>
>     c443d39 merge-recursive: find-renames resets threshold
>     83837ec merge-strategies.txt: fix typo
>
> but they haven't been merged to 'next', so it is up to you whether
> you rebuild the remainder on top of c443d39 or redo these 5 patches
> altogether (just tell me to drop these two if you go the latter
> route).
>

My original patch started with exactly those commits and then added
the tests, so it is fine for me.

The motivation for the reorganisation was bundling the relevant test
with the fix, but then it might be better to publish the fix soon,
right?

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2016-02-22 22:37 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-02-21 22:59 [PATCH v2 0/5] Tests and fixes for merge-recursive rename options Felipe Gonçalves Assis
2016-02-21 22:59 ` [PATCH v2 1/5] merge-strategies.txt: fix typo Felipe Gonçalves Assis
2016-02-21 22:59 ` [PATCH v2 2/5] t3034: add rename threshold tests Felipe Gonçalves Assis
2016-02-22 19:03   ` Junio C Hamano
2016-02-21 22:59 ` [PATCH v2 3/5] t3034: test option to disable renames Felipe Gonçalves Assis
2016-02-21 22:59 ` [PATCH v2 4/5] t3034: test deprecated interface Felipe Gonçalves Assis
2016-02-21 22:59 ` [PATCH v2 5/5] merge-recursive: find-renames resets threshold Felipe Gonçalves Assis
2016-02-22 21:00 ` [PATCH v2 0/5] Tests and fixes for merge-recursive rename options Junio C Hamano
2016-02-22 22:16   ` Felipe Gonçalves Assis
2016-02-22 22:29     ` Junio C Hamano
2016-02-22 22:37       ` Felipe Gonçalves Assis

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.