{"thread":{"id":"41460","subject":"[PATCH v2 0/5] Tests and fixes for merge-recursive rename options","startedAt":"2016-02-21T22:59:00Z","lastAt":"2016-02-22T22:37:32Z","messageCount":11,"participants":["Felipe Gonçalves Assis","Junio C Hamano"],"isPatch":true,"patchVersion":2,"patchTotal":5},"messages":[{"id":"278784","messageId":"1456095545-20201-1-git-send-email-felipegassis@gmail.com","threadId":"41460","inReplyTo":null,"subject":"[PATCH v2 0/5] Tests and fixes for merge-recursive rename options","fromName":"Felipe Gonçalves Assis","fromEmail":"felipeg.assis@gmail.com","sentAt":"2016-02-21T22:59:00Z","receivedAt":"2016-02-21T22:59:00Z","isPatch":true,"sender":{"key":"felipeg.assis@gmail.com","avatar":null},"body":"This is a reorganisation of the previous series, bundling the test for the fix\nalong with the commit itself, as suggested by Eric. It also includes many fixes\nand improvements pointed out by the same reviewer, whom I thank.\n\nThe typo fix is the same as before.\n\nIn \"add rename threshold tests\", I include tests involving --find-renames,\nexcept for one that depends on the fix.\n\n\"test option to disable renames\" adds tests involving --rename-threshold.\n\n\"test deprecated interface\" tests the aliasing --rename-threshold.\n\nIn \"find-renames resets threshold\", the specific test for the feature was\nbundled along.\n\nTo Junio: Please pay special attention to the test of threshold truncation.\nGiven that it seems to be an undocumented feature, I am not sure whether it\nshould be included or not.\n\nFelipe Gonçalves Assis (5):\n merge-strategies.txt: fix typo\n t3034: add rename threshold tests\n t3034: test option to disable renames\n t3034: test deprecated interface\n merge-recursive: find-renames resets threshold\n\n Documentation/merge-strategies.txt | 4 +-\n merge-recursive.c | 4 +-\n ...s.sh => t3032-merge-recursive-space-options.sh} | 2 +-\n t/t3034-merge-recursive-rename-options.sh | 235 +++++++++++++++++++++\n 4 files changed, 241 insertions(+), 4 deletions(-)\n rename t/{t3032-merge-recursive-options.sh => t3032-merge-recursive-space-options.sh} (99%)\n create mode 100755 t/t3034-merge-recursive-rename-options.sh\n\n-- \n2.7.1.492.gd821b20\n"},{"id":"278787","messageId":"1456095545-20201-2-git-send-email-felipegassis@gmail.com","threadId":"41460","inReplyTo":"1456095545-20201-1-git-send-email-felipegassis@gmail.com","subject":"[PATCH v2 1/5] merge-strategies.txt: fix typo","fromName":"Felipe Gonçalves Assis","fromEmail":"felipeg.assis@gmail.com","sentAt":"2016-02-21T22:59:01Z","receivedAt":"2016-02-21T22:59:01Z","isPatch":true,"sender":{"key":"felipeg.assis@gmail.com","avatar":null},"body":"Signed-off-by: Felipe Gonçalves Assis \n---\n Documentation/merge-strategies.txt | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/merge-strategies.txt b/Documentation/merge-strategies.txt\nindex ff359b6..2eb92b9 100644\n--- a/Documentation/merge-strategies.txt\n+++ b/Documentation/merge-strategies.txt\n@@ -86,8 +86,8 @@ no-renames;;\n \tSee also linkgit:git-diff[1] `--no-renames`.\n \n find-renames[=];;\n-\tTurn on rename detection, optionally setting the the similarity\n-\tthreshold. This is the default.\n+\tTurn on rename detection, optionally setting the similarity\n+\tthreshold. This is the default.\n \tSee also linkgit:git-diff[1] `--find-renames`.\n \n rename-threshold=;;\n-- \n2.7.1.492.gd821b20\n"},{"id":"278786","messageId":"1456095545-20201-3-git-send-email-felipegassis@gmail.com","threadId":"41460","inReplyTo":"1456095545-20201-1-git-send-email-felipegassis@gmail.com","subject":"[PATCH v2 2/5] t3034: add rename threshold tests","fromName":"Felipe Gonçalves Assis","fromEmail":"felipeg.assis@gmail.com","sentAt":"2016-02-21T22:59:02Z","receivedAt":"2016-02-21T22:59:02Z","isPatch":true,"sender":{"key":"felipeg.assis@gmail.com","avatar":null},"body":"10ae752 (merge-recursive: option to specify rename threshold,\n2010-09-27) introduced this feature but did not include any tests.\n\nThe tests use the new option --find-renames, which replaces the then\nintroduced and now deprecated option --rename-threshold.\n\nAlso update name and description of t3032 for consistency:\n\"merge-recursive options\" -> \"merge-recursive space options\"\n\nSigned-off-by: Felipe Gonçalves Assis \n---\n\nNow this already introduces the final names and descriptions for the tests.\n\nThe test for threshold truncation was kept for now. Please confirm whether it is\ndesirable.\n\nNew tests were added at the end to check that invalid arguments to find-renames=\nsuch as negative and non-numbers error out.\n\n ...s.sh => t3032-merge-recursive-space-options.sh} | 2 +-\n t/t3034-merge-recursive-rename-options.sh | 159 +++++++++++++++++++++\n 2 files changed, 160 insertions(+), 1 deletion(-)\n rename t/{t3032-merge-recursive-options.sh => t3032-merge-recursive-space-options.sh} (99%)\n create mode 100755 t/t3034-merge-recursive-rename-options.sh\n\ndiff --git a/t/t3032-merge-recursive-options.sh b/t/t3032-merge-recursive-space-options.sh\nsimilarity index 99%\nrename from t/t3032-merge-recursive-options.sh\nrename to t/t3032-merge-recursive-space-options.sh\nindex 4029c9c..b56180e 100755\n--- a/t/t3032-merge-recursive-options.sh\n+++ b/t/t3032-merge-recursive-space-options.sh\n@@ -1,6 +1,6 @@\n #!/bin/sh\n \n-test_description='merge-recursive options\n+test_description='merge-recursive space options\n \n * [master] Clarify\n ! [remote] Remove cruft\ndiff --git a/t/t3034-merge-recursive-rename-options.sh b/t/t3034-merge-recursive-rename-options.sh\nnew file mode 100755\nindex 0000000..7ae7f83\n--- /dev/null\n+++ b/t/t3034-merge-recursive-rename-options.sh\n@@ -0,0 +1,159 @@\n+#!/bin/sh\n+\n+test_description='merge-recursive rename options\n+\n+Test rename detection by examining rename/delete conflicts.\n+\n+Similarity index:\n+R100 a-old a-new\n+R075 b-old b-new\n+R050 c-old c-new\n+R025 d-old d-new\n+'\n+\n+. ./test-lib.sh\n+\n+get_expected_stages () {\n+\tgit checkout rename -- $1-new &&\n+\tgit ls-files --stage $1-new >expected-stages-undetected-$1 &&\n+\tsed \"s/ 0\t/ 2\t/\" expected-stages-detected-$1 &&\n+\tgit read-tree -u --reset HEAD\n+}\n+\n+rename_detected () {\n+\tgit ls-files --stage $1-old $1-new >stages-actual-$1 &&\n+\ttest_cmp expected-stages-detected-$1 stages-actual-$1\n+}\n+\n+rename_undetected () {\n+\tgit ls-files --stage $1-old $1-new >stages-actual-$1 &&\n+\ttest_cmp expected-stages-undetected-$1 stages-actual-$1\n+}\n+\n+check_common () {\n+\tgit ls-files --stage >stages-actual &&\n+\ttest_line_count = 4 stages-actual\n+}\n+\n+check_find_renames_25 () {\n+\tcheck_common &&\n+\trename_detected a &&\n+\trename_detected b &&\n+\trename_detected c &&\n+\trename_detected d\n+}\n+\n+check_find_renames_50 () {\n+\tcheck_common &&\n+\trename_detected a &&\n+\trename_detected b &&\n+\trename_detected c &&\n+\trename_undetected d\n+}\n+\n+check_find_renames_75 () {\n+\tcheck_common &&\n+\trename_detected a &&\n+\trename_detected b &&\n+\trename_undetected c &&\n+\trename_undetected d\n+}\n+\n+check_find_renames_100 () {\n+\tcheck_common &&\n+\trename_detected a &&\n+\trename_undetected b &&\n+\trename_undetected c &&\n+\trename_undetected d\n+}\n+\n+test_expect_success setup '\n+\tcat <<-\\EOF >a-old &&\n+\taa1\n+\taa2\n+\taa3\n+\taa4\n+\tEOF\n+\tsed s/aa/bb/ b-old &&\n+\tsed s/aa/cc/ c-old &&\n+\tsed s/aa/dd/ d-old &&\n+\tgit add [a-d]-old &&\n+\tgit commit -m base &&\n+\tgit rm [a-d]-old &&\n+\tgit commit -m delete &&\n+\tgit checkout -b rename HEAD^ &&\n+\tcp a-old a-new &&\n+\tsed 1,1s/./x/ b-new &&\n+\tsed 1,2s/./x/ c-new &&\n+\tsed 1,3s/./x/ d-new &&\n+\tgit add [a-d]-new &&\n+\tgit rm [a-d]-old &&\n+\tgit commit -m rename &&\n+\tget_expected_stages a &&\n+\tget_expected_stages b &&\n+\tget_expected_stages c &&\n+\tget_expected_stages d\n+'\n+\n+test_expect_success 'default similarity threshold is 50%' '\n+\tgit read-tree --reset -u HEAD &&\n+\ttest_must_fail git merge-recursive HEAD^ -- HEAD master &&\n+\tcheck_find_renames_50\n+'\n+\n+test_expect_success 'low rename threshold' '\n+\tgit read-tree --reset -u HEAD &&\n+\ttest_must_fail git merge-recursive --find-renames=25 HEAD^ -- HEAD master &&\n+\tcheck_find_renames_25\n+'\n+\n+test_expect_success 'high rename threshold' '\n+\tgit read-tree --reset -u HEAD &&\n+\ttest_must_fail git merge-recursive --find-renames=75 HEAD^ -- HEAD master &&\n+\tcheck_find_renames_75\n+'\n+\n+test_expect_success 'exact renames only' '\n+\tgit read-tree --reset -u HEAD &&\n+\ttest_must_fail git merge-recursive --find-renames=100% HEAD^ -- HEAD master &&\n+\tcheck_find_renames_100\n+'\n+\n+test_expect_success 'rename threshold is truncated' '\n+\tgit read-tree --reset -u HEAD &&\n+\ttest_must_fail git merge-recursive --find-renames=200% HEAD^ -- HEAD master &&\n+\tcheck_find_renames_100\n+'\n+\n+test_expect_success 'last wins in --find-renames= --find-renames=' '\n+\tgit read-tree --reset -u HEAD &&\n+\ttest_must_fail git merge-recursive --find-renames=25 --find-renames=75 HEAD^ -- HEAD master &&\n+\tcheck_find_renames_75\n+'\n+\n+test_expect_success 'assumption for further tests: trivial merge succeeds' '\n+\tgit read-tree --reset -u HEAD &&\n+\tgit merge-recursive HEAD -- HEAD HEAD &&\n+\tgit diff --quiet --cached &&\n+\tgit merge-recursive --find-renames=25 HEAD -- HEAD HEAD &&\n+\tgit diff --quiet --cached &&\n+\tgit merge-recursive --find-renames=75 HEAD -- HEAD HEAD &&\n+\tgit diff --quiet --cached &&\n+\tgit merge-recursive --find-renames=100% HEAD -- HEAD HEAD &&\n+\tgit diff --quiet --cached\n+'\n+\n+test_expect_success '--find-renames rejects negative argument' '\n+\tgit read-tree --reset -u HEAD &&\n+\ttest_must_fail git merge-recursive --find-renames=-25 HEAD -- HEAD HEAD &&\n+\tgit diff --quiet --cached\n+'\n+\n+test_expect_success '--find-renames rejects non-numbers' '\n+\tgit read-tree --reset -u HEAD &&\n+\ttest_must_fail git merge-recursive --find-renames=0xf HEAD -- HEAD HEAD &&\n+\tgit diff --quiet --cached\n+'\n+\n+test_done\n-- \n2.7.1.492.gd821b20\n"},{"id":"278785","messageId":"1456095545-20201-4-git-send-email-felipegassis@gmail.com","threadId":"41460","inReplyTo":"1456095545-20201-1-git-send-email-felipegassis@gmail.com","subject":"[PATCH v2 3/5] t3034: test option to disable renames","fromName":"Felipe Gonçalves Assis","fromEmail":"felipeg.assis@gmail.com","sentAt":"2016-02-21T22:59:03Z","receivedAt":"2016-02-21T22:59:03Z","isPatch":true,"sender":{"key":"felipeg.assis@gmail.com","avatar":null},"body":"Signed-off-by: Felipe Gonçalves Assis \n---\n t/t3034-merge-recursive-rename-options.sh | 28 ++++++++++++++++++++++++++++\n 1 file changed, 28 insertions(+)\n\ndiff --git a/t/t3034-merge-recursive-rename-options.sh b/t/t3034-merge-recursive-rename-options.sh\nindex 7ae7f83..a459236 100755\n--- a/t/t3034-merge-recursive-rename-options.sh\n+++ b/t/t3034-merge-recursive-rename-options.sh\n@@ -68,6 +68,14 @@ check_find_renames_100 () {\n \trename_undetected d\n }\n \n+check_no_renames () {\n+\tcheck_common &&\n+\trename_undetected a &&\n+\trename_undetected b &&\n+\trename_undetected c &&\n+\trename_undetected d\n+}\n+\n test_expect_success setup '\n \tcat <<-\\EOF >a-old &&\n \taa1\n@@ -126,12 +134,30 @@ test_expect_success 'rename threshold is truncated' '\n \tcheck_find_renames_100\n '\n \n+test_expect_success 'disabled rename detection' '\n+\tgit read-tree --reset -u HEAD &&\n+\tgit merge-recursive --no-renames HEAD^ -- HEAD master &&\n+\tcheck_no_renames\n+'\n+\n test_expect_success 'last wins in --find-renames= --find-renames=' '\n \tgit read-tree --reset -u HEAD &&\n \ttest_must_fail git merge-recursive --find-renames=25 --find-renames=75 HEAD^ -- HEAD master &&\n \tcheck_find_renames_75\n '\n \n+test_expect_success 'last wins in --no-renames --find-renames' '\n+\tgit read-tree --reset -u HEAD &&\n+\ttest_must_fail git merge-recursive --no-renames --find-renames HEAD^ -- HEAD master &&\n+\tcheck_find_renames_50\n+'\n+\n+test_expect_success 'last wins in --find-renames --no-renames' '\n+\tgit read-tree --reset -u HEAD &&\n+\tgit merge-recursive --find-renames --no-renames HEAD^ -- HEAD master &&\n+\tcheck_no_renames\n+'\n+\n test_expect_success 'assumption for further tests: trivial merge succeeds' '\n \tgit read-tree --reset -u HEAD &&\n \tgit merge-recursive HEAD -- HEAD HEAD &&\n@@ -141,6 +167,8 @@ test_expect_success 'assumption for further tests: trivial merge succeeds' '\n \tgit merge-recursive --find-renames=75 HEAD -- HEAD HEAD &&\n \tgit diff --quiet --cached &&\n \tgit merge-recursive --find-renames=100% HEAD -- HEAD HEAD &&\n+\tgit diff --quiet --cached &&\n+\tgit merge-recursive --no-renames HEAD -- HEAD HEAD &&\n \tgit diff --quiet --cached\n '\n \n-- \n2.7.1.492.gd821b20\n"},{"id":"278788","messageId":"1456095545-20201-5-git-send-email-felipegassis@gmail.com","threadId":"41460","inReplyTo":"1456095545-20201-1-git-send-email-felipegassis@gmail.com","subject":"[PATCH v2 4/5] t3034: test deprecated interface","fromName":"Felipe Gonçalves Assis","fromEmail":"felipeg.assis@gmail.com","sentAt":"2016-02-21T22:59:04Z","receivedAt":"2016-02-21T22:59:04Z","isPatch":true,"sender":{"key":"felipeg.assis@gmail.com","avatar":null},"body":"--find-renames= and --rename-threshold= should be aliases.\n\nSigned-off-by: Felipe Gonçalves Assis \n---\n\nNow includes tests with invalid arguments to --rename-threshold=\n\n t/t3034-merge-recursive-rename-options.sh | 42 +++++++++++++++++++++++++++++++\n 1 file changed, 42 insertions(+)\n\ndiff --git a/t/t3034-merge-recursive-rename-options.sh b/t/t3034-merge-recursive-rename-options.sh\nindex a459236..d4f9742 100755\n--- a/t/t3034-merge-recursive-rename-options.sh\n+++ b/t/t3034-merge-recursive-rename-options.sh\n@@ -184,4 +184,46 @@ test_expect_success '--find-renames rejects non-numbers' '\n \tgit diff --quiet --cached\n '\n \n+test_expect_success 'rename-threshold= is a synonym for find-renames=' '\n+\tgit read-tree --reset -u HEAD &&\n+\ttest_must_fail git merge-recursive --rename-threshold=25 HEAD^ -- HEAD master &&\n+\tcheck_find_renames_25\n+'\n+\n+test_expect_success 'last wins in --no-renames --rename-threshold=' '\n+\tgit read-tree --reset -u HEAD &&\n+\ttest_must_fail git merge-recursive --no-renames --rename-threshold=25 HEAD^ -- HEAD master &&\n+\tcheck_find_renames_25\n+'\n+\n+test_expect_success 'last wins in --rename-threshold= --no-renames' '\n+\tgit read-tree --reset -u HEAD &&\n+\tgit merge-recursive --rename-threshold=25 --no-renames HEAD^ -- HEAD master &&\n+\tcheck_no_renames\n+'\n+\n+test_expect_success '--rename-threshold= rejects negative argument' '\n+\tgit read-tree --reset -u HEAD &&\n+\ttest_must_fail git merge-recursive --rename-threshold=-25 HEAD -- HEAD HEAD &&\n+\tgit diff --quiet --cached\n+'\n+\n+test_expect_success '--rename-threshold= rejects non-numbers' '\n+\tgit read-tree --reset -u HEAD &&\n+\ttest_must_fail git merge-recursive --rename-threshold=0xf HEAD -- HEAD HEAD &&\n+\tgit diff --quiet --cached\n+'\n+\n+test_expect_success 'last wins in --rename-threshold= --find-renames=' '\n+\tgit read-tree --reset -u HEAD &&\n+\ttest_must_fail git merge-recursive --rename-threshold=25 --find-renames=75 HEAD^ -- HEAD master &&\n+\tcheck_find_renames_75\n+'\n+\n+test_expect_success 'last wins in --find-renames= --rename-threshold=' '\n+\tgit read-tree --reset -u HEAD &&\n+\ttest_must_fail git merge-recursive --find-renames=75 --rename-threshold=25 HEAD^ -- HEAD master &&\n+\tcheck_find_renames_25\n+'\n+\n test_done\n-- \n2.7.1.492.gd821b20\n"},{"id":"278789","messageId":"1456095545-20201-6-git-send-email-felipegassis@gmail.com","threadId":"41460","inReplyTo":"1456095545-20201-1-git-send-email-felipegassis@gmail.com","subject":"[PATCH v2 5/5] merge-recursive: find-renames resets threshold","fromName":"Felipe Gonçalves Assis","fromEmail":"felipeg.assis@gmail.com","sentAt":"2016-02-21T22:59:05Z","receivedAt":"2016-02-21T22:59:05Z","isPatch":true,"sender":{"key":"felipeg.assis@gmail.com","avatar":null},"body":"Make the find-renames option follow the behaviour in git-diff, where it\nresets the threshold when none is given. So, for instance,\n\"--find-renames=25 --find-renames\" should result in the default\nthreshold (50%) instead of 25%.\n\nAdd corresponding test.\n\nSigned-off-by: Felipe Gonçalves Assis \n---\n\nNow bundles the relevant test.\n\n merge-recursive.c | 4 +++-\n t/t3034-merge-recursive-rename-options.sh | 6 ++++++\n 2 files changed, 9 insertions(+), 1 deletion(-)\n\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex 7bff5be..b880ae5 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -2094,8 +2094,10 @@ int parse_merge_opt(struct merge_options *o, const char *s)\n \t\to->renormalize = 0;\n \telse if (!strcmp(s, \"no-renames\"))\n \t\to->detect_rename = 0;\n-\telse if (!strcmp(s, \"find-renames\"))\n+\telse if (!strcmp(s, \"find-renames\")) {\n \t\to->detect_rename = 1;\n+\t\to->rename_score = 0;\n+\t}\n \telse if (skip_prefix(s, \"find-renames=\", &arg) ||\n \t\t skip_prefix(s, \"rename-threshold=\", &arg)) {\n \t\tif ((o->rename_score = parse_rename_score(&arg)) == -1 || *arg != 0)\ndiff --git a/t/t3034-merge-recursive-rename-options.sh b/t/t3034-merge-recursive-rename-options.sh\nindex d4f9742..54d5f1b 100755\n--- a/t/t3034-merge-recursive-rename-options.sh\n+++ b/t/t3034-merge-recursive-rename-options.sh\n@@ -146,6 +146,12 @@ test_expect_success 'last wins in --find-renames= --find-renames=' '\n \tcheck_find_renames_75\n '\n \n+test_expect_success '--find-renames resets threshold' '\n+\tgit read-tree --reset -u HEAD &&\n+\ttest_must_fail git merge-recursive --find-renames=25 --find-renames HEAD^ -- HEAD master &&\n+\tcheck_find_renames_50\n+'\n+\n test_expect_success 'last wins in --no-renames --find-renames' '\n \tgit read-tree --reset -u HEAD &&\n \ttest_must_fail git merge-recursive --no-renames --find-renames HEAD^ -- HEAD master &&\n-- \n2.7.1.492.gd821b20\n"},{"id":"278894","messageId":"xmqqh9h07ejt.fsf@gitster.mtv.corp.google.com","threadId":"41460","inReplyTo":"1456095545-20201-3-git-send-email-felipegassis@gmail.com","subject":"Re: [PATCH v2 2/5] t3034: add rename threshold tests","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-02-22T19:03:50Z","receivedAt":"2016-02-22T19:03:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Felipe Gonçalves Assis\" writes:\n\n> 10ae752 (merge-recursive: option to specify rename threshold,\n> 2010-09-27) introduced this feature but did not include any tests.\n>\n> The tests use the new option --find-renames, which replaces the then\n> introduced and now deprecated option --rename-threshold.\n> ...\n> diff --git a/t/t3034-merge-recursive-rename-options.sh b/t/t3034-merge-recursive-rename-options.sh\n> new file mode 100755\n> index 0000000..7ae7f83\n> --- /dev/null\n> +++ b/t/t3034-merge-recursive-rename-options.sh\n> @@ -0,0 +1,159 @@\n> +#!/bin/sh\n> +\n> +test_description='merge-recursive rename options\n> +\n> +Test rename detection by examining rename/delete conflicts.\n> +\n> +Similarity index:\n> +R100 a-old a-new\n> +R075 b-old b-new\n> +R050 c-old c-new\n> +R025 d-old d-new\n> +'\n> ...\n> +test_expect_success setup '\n> +\tcat <<-\\EOF >a-old &&\n> +\taa1\n> +\taa2\n> +\taa3\n> +\taa4\n> +\tEOF\n> +\tsed s/aa/bb/ b-old &&\n> +\tsed s/aa/cc/ c-old &&\n> +\tsed s/aa/dd/ d-old &&\n> +\tgit add [a-d]-old &&\n> +\tgit commit -m base &&\n> +\tgit rm [a-d]-old &&\n> +\tgit commit -m delete &&\n> +\tgit checkout -b rename HEAD^ &&\n> +\tcp a-old a-new &&\n> +\tsed 1,1s/./x/ b-new &&\n> +\tsed 1,2s/./x/ c-new &&\n> +\tsed 1,3s/./x/ d-new &&\n> +\tgit add [a-d]-new &&\n> +\tgit rm [a-d]-old &&\n> +\tgit commit -m rename &&\n> +\tget_expected_stages a &&\n> +\tget_expected_stages b &&\n> +\tget_expected_stages c &&\n> +\tget_expected_stages d\n> +'\n\nI somehow doubt that it is wise to make the similarity index\ncomputed by the current heuristics as a hard promise like this test\ndoes.\n\nThis test specifies that turning the original bb1/bb2/bb3/bb4 into\nupdated xb1/bb2/bb3/bb4 _MUST_ get 75% similarity, but that is not\nsomething we want to guarantee, ever. We may later update the\nalgorithm and tweak such a change to register 70% or 78%, but such a\nchange would break the expectation by this test. This test script\nhowever should not be interested in the exact similarity index\nassignment for a given filepair--it only wants to make sure that the\noption chooses the filepair with similarity index above the given\nvalue.\n\nThe test for --find-renames= should instead only validate that\nthe code works on the value given from the command line relative to\nthe similarity index the code computed. I.e. first measure what the\nsimilarity index going from b-old to b-new is (e.g. it may say 76%,\nor 74%, depending on the version of Git being tested), then choose a\nvalue that is higher (or lower) than that similarity to give to\nthe --find-renames= option and ensure that merge-recursive does\nwhat is expected.\n\nI am not very happy with this one.\n"},{"id":"278902","messageId":"xmqqpovo5ul2.fsf@gitster.mtv.corp.google.com","threadId":"41460","inReplyTo":"1456095545-20201-1-git-send-email-felipegassis@gmail.com","subject":"Re: [PATCH v2 0/5] Tests and fixes for merge-recursive rename options","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-02-22T21:00:25Z","receivedAt":"2016-02-22T21:00:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Felipe Gonçalves Assis\" writes:\n\n> This is a reorganisation of the previous series, bundling the test for the fix\n> along with the commit itself, as suggested by Eric. It also includes many fixes\n> and improvements pointed out by the same reviewer, whom I thank.\n>\n> The typo fix is the same as before.\n>\n> In \"add rename threshold tests\", I include tests involving --find-renames,\n> except for one that depends on the fix.\n>\n> \"test option to disable renames\" adds tests involving --rename-threshold.\n>\n> \"test deprecated interface\" tests the aliasing --rename-threshold.\n>\n> In \"find-renames resets threshold\", the specific test for the feature was\n> bundled along.\n>\n> To Junio: Please pay special attention to the test of threshold truncation.\n> Given that it seems to be an undocumented feature, I am not sure whether it\n> should be included or not.\n>\n> Felipe Gonçalves Assis (5):\n> merge-strategies.txt: fix typo\n> t3034: add rename threshold tests\n> t3034: test option to disable renames\n> t3034: test deprecated interface\n> merge-recursive: find-renames resets threshold\n\nAs I said, I am reluctant to take the 25%/50%/75% tests in their\ncurrent form. Let me take the first one and a half of the last one\n(i.e. excluding the test) for now.\n\nThanks.\n"},{"id":"278910","messageId":"CALMa68qmGie+AHOWZ=BKHZOeSK9K4djiHRuooCz2h72rNZg7_A@mail.gmail.com","threadId":"41460","inReplyTo":"xmqqpovo5ul2.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2 0/5] Tests and fixes for merge-recursive rename options","fromName":"Felipe Gonçalves Assis","fromEmail":"felipeg.assis@gmail.com","sentAt":"2016-02-22T22:16:09Z","receivedAt":"2016-02-22T22:16:09Z","isPatch":true,"sender":{"key":"felipeg.assis@gmail.com","avatar":null},"body":"On 22 February 2016 at 18:00, Junio C Hamano wrote:\n> \"Felipe Gonçalves Assis\" writes:\n>\n>> This is a reorganisation of the previous series, bundling the test for the fix\n>> along with the commit itself, as suggested by Eric. It also includes many fixes\n>> and improvements pointed out by the same reviewer, whom I thank.\n>>\n>> The typo fix is the same as before.\n>>\n>> In \"add rename threshold tests\", I include tests involving --find-renames,\n>> except for one that depends on the fix.\n>>\n>> \"test option to disable renames\" adds tests involving --rename-threshold.\n>>\n>> \"test deprecated interface\" tests the aliasing --rename-threshold.\n>>\n>> In \"find-renames resets threshold\", the specific test for the feature was\n>> bundled along.\n>>\n>> To Junio: Please pay special attention to the test of threshold truncation.\n>> Given that it seems to be an undocumented feature, I am not sure whether it\n>> should be included or not.\n>>\n>> Felipe Gonçalves Assis (5):\n>> merge-strategies.txt: fix typo\n>> t3034: add rename threshold tests\n>> t3034: test option to disable renames\n>> t3034: test deprecated interface\n>> merge-recursive: find-renames resets threshold\n>\n> As I said, I am reluctant to take the 25%/50%/75% tests in their\n> current form. Let me take the first one and a half of the last one\n> (i.e. excluding the test) for now.\n>\n> Thanks.\n\nOk, should I post a new version of the patch without the tests while I\nrework them or does that mean that you have already filtered them out\nlocally?\n"},{"id":"278916","messageId":"xmqqwppw4bvw.fsf@gitster.mtv.corp.google.com","threadId":"41460","inReplyTo":"CALMa68qmGie+AHOWZ=BKHZOeSK9K4djiHRuooCz2h72rNZg7_A@mail.gmail.com","subject":"Re: [PATCH v2 0/5] Tests and fixes for merge-recursive rename options","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-02-22T22:29:39Z","receivedAt":"2016-02-22T22:29:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Felipe Gonçalves Assis writes:\n\n>> As I said, I am reluctant to take the 25%/50%/75% tests in their\n>> current form. Let me take the first one and a half of the last one\n>> (i.e. excluding the test) for now.\n>>\n>> Thanks.\n>\n> Ok, should I post a new version of the patch without the tests while I\n> rework them or does that mean that you have already filtered them out\n> locally?\n\nI already have and queued them tentatively as \n\n c443d39 merge-recursive: find-renames resets threshold\n 83837ec merge-strategies.txt: fix typo\n\nbut they haven't been merged to 'next', so it is up to you whether\nyou rebuild the remainder on top of c443d39 or redo these 5 patches\naltogether (just tell me to drop these two if you go the latter\nroute).\n\nThanks.\n"},{"id":"278920","messageId":"CALMa68pYL+x62zOKKrwS9VtH8Qm=-hbzARHN1kJ+4t-Hz+TpAA@mail.gmail.com","threadId":"41460","inReplyTo":"xmqqwppw4bvw.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2 0/5] Tests and fixes for merge-recursive rename options","fromName":"Felipe Gonçalves Assis","fromEmail":"felipeg.assis@gmail.com","sentAt":"2016-02-22T22:37:32Z","receivedAt":"2016-02-22T22:37:32Z","isPatch":true,"sender":{"key":"felipeg.assis@gmail.com","avatar":null},"body":"On 22 February 2016 at 19:29, Junio C Hamano wrote:\n> Felipe Gonçalves Assis writes:\n>\n>>> As I said, I am reluctant to take the 25%/50%/75% tests in their\n>>> current form. Let me take the first one and a half of the last one\n>>> (i.e. excluding the test) for now.\n>>>\n>>> Thanks.\n>>\n>> Ok, should I post a new version of the patch without the tests while I\n>> rework them or does that mean that you have already filtered them out\n>> locally?\n>\n> I already have and queued them tentatively as\n>\n> c443d39 merge-recursive: find-renames resets threshold\n> 83837ec merge-strategies.txt: fix typo\n>\n> but they haven't been merged to 'next', so it is up to you whether\n> you rebuild the remainder on top of c443d39 or redo these 5 patches\n> altogether (just tell me to drop these two if you go the latter\n> route).\n>\n\nMy original patch started with exactly those commits and then added\nthe tests, so it is fine for me.\n\nThe motivation for the reorganisation was bundling the relevant test\nwith the fix, but then it might be better to publish the fix soon,\nright?\n"}]}