{"thread":{"id":"41455","subject":"Re: [PATCH 1/5] merge-recursive: find-renames resets threshold","startedAt":"2016-02-21T15:09:13Z","lastAt":"2016-02-21T23:15:26Z","messageCount":16,"participants":["Eric Sunshine","Felipe Gonçalves Assis","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":5},"messages":[{"id":"278765","messageId":"1456067358-19781-1-git-send-email-felipegassis@gmail.com","threadId":"41455","inReplyTo":null,"subject":"[PATCH 0/5] Tests and fixes for merge-recursive rename options","fromName":"Felipe Gonçalves Assis","fromEmail":"felipeg.assis@gmail.com","sentAt":"2016-02-21T15:09:13Z","receivedAt":"2016-02-21T15:09:13Z","isPatch":true,"sender":{"key":"felipeg.assis@gmail.com","avatar":null},"body":"This builds on the work in 1b47ad160b55f50a7a98c180e18d80f0f8f17a67\n(merge-recursive: more consistent interface, 2016-02-17).\n\nAdd tests for the merge-recursive rename options, as suggested by Eric Sunshine.\nAlso fixes an inconsistency in the behaviour of find-renames and a typo.\n\nThe first two commits contain fixes.\n\nThe tests were divided in the latter three, reproducing the chronological\nhistory of the features:\n 3. --rename-threshold only\n 4. --rename-threshold and --no-renames\n 5. --find-renames, --no-renames and deprecated --rename-threshold\n\nFelipe Gonçalves Assis (5):\n merge-recursive: find-renames resets threshold\n merge-strategies.txt: fix typo\n merge-recursive: test rename threshold option\n merge-recursive: test option to disable renames\n merge-recursive: test more consistent interface\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 | 200 +++++++++++++++++++++\n 4 files changed, 206 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.gc9722f8\n"},{"id":"278771","messageId":"1456067358-19781-2-git-send-email-felipegassis@gmail.com","threadId":"41455","inReplyTo":"1456067358-19781-1-git-send-email-felipegassis@gmail.com","subject":"[PATCH 1/5] merge-recursive: find-renames resets threshold","fromName":"Felipe Gonçalves Assis","fromEmail":"felipeg.assis@gmail.com","sentAt":"2016-02-21T15:09:14Z","receivedAt":"2016-02-21T15:09:14Z","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\nSigned-off-by: Felipe Gonçalves Assis \n---\n merge-recursive.c | 4 +++-\n 1 file changed, 3 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)\n-- \n2.7.1.492.gc9722f8\n"},{"id":"278772","messageId":"1456067358-19781-3-git-send-email-felipegassis@gmail.com","threadId":"41455","inReplyTo":"1456067358-19781-1-git-send-email-felipegassis@gmail.com","subject":"[PATCH 2/5] merge-strategies.txt: fix typo","fromName":"Felipe Gonçalves Assis","fromEmail":"felipeg.assis@gmail.com","sentAt":"2016-02-21T15:09:15Z","receivedAt":"2016-02-21T15:09:15Z","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.gc9722f8\n"},{"id":"278764","messageId":"1456067358-19781-4-git-send-email-felipegassis@gmail.com","threadId":"41455","inReplyTo":"1456067358-19781-1-git-send-email-felipegassis@gmail.com","subject":"[PATCH 3/5] merge-recursive: test rename threshold option","fromName":"Felipe Gonçalves Assis","fromEmail":"felipeg.assis@gmail.com","sentAt":"2016-02-21T15:09:16Z","receivedAt":"2016-02-21T15:09:16Z","isPatch":true,"sender":{"key":"felipeg.assis@gmail.com","avatar":null},"body":"Commit 10ae7526bebb505ddddba01f76ec97d5f7b5e0e5 introduced this feature,\nbut did not include any tests. This commit fixes this.\n\nSigned-off-by: Felipe Gonçalves Assis \n---\n t/t3034-merge-recursive-rename-threshold.sh | 146 ++++++++++++++++++++++++++++\n 1 file changed, 146 insertions(+)\n create mode 100755 t/t3034-merge-recursive-rename-threshold.sh\n\ndiff --git a/t/t3034-merge-recursive-rename-threshold.sh b/t/t3034-merge-recursive-rename-threshold.sh\nnew file mode 100755\nindex 0000000..f0b3f44\n--- /dev/null\n+++ b/t/t3034-merge-recursive-rename-threshold.sh\n@@ -0,0 +1,146 @@\n+#!/bin/sh\n+\n+test_description='merge-recursive rename threshold option\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+test_expect_success setup '\n+\tget_expected_stages () {\n+\t\tgit checkout rename -- $1-new &&\n+\t\tgit ls-files --stage $1-new > expected-stages-undetected-$1\n+\t\tsed \"s/ 0\t/ 2\t/\n+\t\t\" < expected-stages-undetected-$1 > expected-stages-detected-$1\n+\t\tgit read-tree -u --reset HEAD\n+\t} &&\n+\n+\trename_detected () {\n+\t\tgit ls-files --stage $1-old $1-new > stages-actual-$1 &&\n+\t\ttest_cmp expected-stages-detected-$1 stages-actual-$1\n+\t} &&\n+\n+\trename_undetected () {\n+\t\tgit ls-files --stage $1-old $1-new > stages-actual-$1 &&\n+\t\ttest_cmp expected-stages-undetected-$1 stages-actual-$1\n+\t} &&\n+\n+\tcheck_common () {\n+\t\tgit ls-files --stage > stages-actual &&\n+\t\ttest $(wc -l < stages-actual) -eq 4\n+\t} &&\n+\n+\tcheck_find_renames_25 () {\n+\t\tcheck_common &&\n+\t\trename_detected a &&\n+\t\trename_detected b &&\n+\t\trename_detected c &&\n+\t\trename_detected d\n+\t} &&\n+\n+\tcheck_find_renames_50 () {\n+\t\tcheck_common\n+\t\trename_detected a &&\n+\t\trename_detected b &&\n+\t\trename_detected c &&\n+\t\trename_undetected d\n+\t} &&\n+\n+\tcheck_find_renames_75 () {\n+\t\tcheck_common\n+\t\trename_detected a &&\n+\t\trename_detected b &&\n+\t\trename_undetected c &&\n+\t\trename_undetected d\n+\t} &&\n+\n+\tcheck_find_renames_100 () {\n+\t\tcheck_common\n+\t\trename_detected a &&\n+\t\trename_undetected b &&\n+\t\trename_undetected c &&\n+\t\trename_undetected d\n+\t} &&\n+\n+\tcheck_no_renames () {\n+\t\tcheck_common\n+\t\trename_undetected a &&\n+\t\trename_undetected b &&\n+\t\trename_undetected c &&\n+\t\trename_undetected d\n+\t} &&\n+\n+\tcat <<-\\EOF > a-old &&\n+\taa1\n+\taa2\n+\taa3\n+\taa4\n+\tEOF\n+\tsed s/aa/bb/ < a-old > b-old &&\n+\tsed s/aa/cc/ < a-old > c-old &&\n+\tsed s/aa/dd/ < a-old > d-old &&\n+\tgit add [a-d]-old &&\n+\ttest_tick &&\n+\tgit commit -m base &&\n+\tgit rm [a-d]-old &&\n+\ttest_tick &&\n+\tgit commit -m delete &&\n+\tgit checkout -b rename HEAD^ &&\n+\tcp a-old a-new &&\n+\tsed 1,1s/./x/ < b-old > b-new &&\n+\tsed 1,2s/./x/ < c-old > c-new &&\n+\tsed 1,3s/./x/ < d-old > d-new &&\n+\tgit add [a-d]-new &&\n+\tgit rm [a-d]-old &&\n+\ttest_tick &&\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 'the default similarity index 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 --rename-threshold=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 --rename-threshold=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 --rename-threshold=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 --rename-threshold=200% HEAD^ -- HEAD master &&\n+\tcheck_find_renames_100\n+'\n+\n+test_expect_success 'last wins in --rename-threshold= --rename-threshold=' '\n+\tgit read-tree --reset -u HEAD &&\n+\ttest_must_fail git merge-recursive --rename-threshold=25 --rename-threshold=75 HEAD^ -- HEAD master &&\n+\tcheck_find_renames_75\n+'\n+\n+test_done\n-- \n2.7.1.492.gc9722f8\n"},{"id":"278768","messageId":"1456067358-19781-5-git-send-email-felipegassis@gmail.com","threadId":"41455","inReplyTo":"1456067358-19781-1-git-send-email-felipegassis@gmail.com","subject":"[PATCH 4/5] merge-recursive: test option to disable renames","fromName":"Felipe Gonçalves Assis","fromEmail":"felipeg.assis@gmail.com","sentAt":"2016-02-21T15:09:17Z","receivedAt":"2016-02-21T15:09:17Z","isPatch":true,"sender":{"key":"felipeg.assis@gmail.com","avatar":null},"body":"Also update name and description of tests for consistency:\n\"merge-recursive options\" -> \"merge-recursive space options\"\n\"merge-recursive rename threshold\" -> \"merge-recursive rename options\"\n\nSigned-off-by: Felipe Gonçalves Assis \n---\n ...ons.sh => t3032-merge-recursive-space-options.sh} | 2 +-\n ...ld.sh => t3034-merge-recursive-rename-options.sh} | 20 +++++++++++++++++++-\n 2 files changed, 20 insertions(+), 2 deletions(-)\n rename t/{t3032-merge-recursive-options.sh => t3032-merge-recursive-space-options.sh} (99%)\n rename t/{t3034-merge-recursive-rename-threshold.sh => t3034-merge-recursive-rename-options.sh} (83%)\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-threshold.sh b/t/t3034-merge-recursive-rename-options.sh\nsimilarity index 83%\nrename from t/t3034-merge-recursive-rename-threshold.sh\nrename to t/t3034-merge-recursive-rename-options.sh\nindex f0b3f44..2f10fa7 100755\n--- a/t/t3034-merge-recursive-rename-threshold.sh\n+++ b/t/t3034-merge-recursive-rename-options.sh\n@@ -1,6 +1,6 @@\n #!/bin/sh\n \n-test_description='merge-recursive rename threshold option\n+test_description='merge-recursive rename options\n \n Test rename detection by examining rename/delete conflicts.\n \n@@ -137,10 +137,28 @@ 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 --rename-threshold= --rename-threshold=' '\n \tgit read-tree --reset -u HEAD &&\n \ttest_must_fail git merge-recursive --rename-threshold=25 --rename-threshold=75 HEAD^ -- HEAD master &&\n \tcheck_find_renames_75\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_done\n-- \n2.7.1.492.gc9722f8\n"},{"id":"278769","messageId":"1456067358-19781-6-git-send-email-felipegassis@gmail.com","threadId":"41455","inReplyTo":"1456067358-19781-1-git-send-email-felipegassis@gmail.com","subject":"[PATCH 5/5] merge-recursive: test more consistent interface","fromName":"Felipe Gonçalves Assis","fromEmail":"felipeg.assis@gmail.com","sentAt":"2016-02-21T15:09:18Z","receivedAt":"2016-02-21T15:09:18Z","isPatch":true,"sender":{"key":"felipeg.assis@gmail.com","avatar":null},"body":"Update basic tests to use the new option find-renames instead of\nrename-threshold. Add tests to verify that rename-threshold= behaves\nas a synonym for find-renames=. Test that find-renames resets\nthreshold.\n\nSigned-off-by: Felipe Gonçalves Assis \n---\n t/t3034-merge-recursive-rename-options.sh | 48 +++++++++++++++++++++++++++----\n 1 file changed, 42 insertions(+), 6 deletions(-)\n\ndiff --git a/t/t3034-merge-recursive-rename-options.sh b/t/t3034-merge-recursive-rename-options.sh\nindex 2f10fa7..7fea7bd 100755\n--- a/t/t3034-merge-recursive-rename-options.sh\n+++ b/t/t3034-merge-recursive-rename-options.sh\n@@ -115,25 +115,25 @@ test_expect_success 'the default similarity index is 50%' '\n \n test_expect_success 'low rename threshold' '\n \tgit read-tree --reset -u HEAD &&\n-\ttest_must_fail git merge-recursive --rename-threshold=25 HEAD^ -- HEAD master &&\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 --rename-threshold=75 HEAD^ -- HEAD master &&\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 --rename-threshold=100% HEAD^ -- HEAD master &&\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 --rename-threshold=200% HEAD^ -- HEAD master &&\n+\ttest_must_fail git merge-recursive --find-renames=200% HEAD^ -- HEAD master &&\n \tcheck_find_renames_100\n '\n \n@@ -143,12 +143,36 @@ test_expect_success 'disabled rename detection' '\n \tcheck_no_renames\n '\n \n-test_expect_success 'last wins in --rename-threshold= --rename-threshold=' '\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 --rename-threshold=25 --rename-threshold=75 HEAD^ -- HEAD master &&\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 '--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+\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 '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@@ -161,4 +185,16 @@ test_expect_success 'last wins in --rename-threshold= --no-renames' '\n \tcheck_no_renames\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 HEAD^ -- HEAD master &&\n+\tcheck_find_renames_50\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 --rename-threshold=25 HEAD^ -- HEAD master &&\n+\tcheck_find_renames_25\n+'\n+\n test_done\n-- \n2.7.1.492.gc9722f8\n"},{"id":"278751","messageId":"CAPig+cQa0t3DuXEP=zW1=2bSMro632c7DfMHcLo_iN9xhGqLqg@mail.gmail.com","threadId":"41455","inReplyTo":"1456067358-19781-2-git-send-email-felipegassis@gmail.com","subject":"Re: [PATCH 1/5] merge-recursive: find-renames resets threshold","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-02-21T17:18:47Z","receivedAt":"2016-02-21T17:18:47Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Feb 21, 2016 at 10:09 AM, Felipe Gonçalves Assis\n wrote:\n> Make the find-renames option follow the behaviour in git-diff, where it\n> resets the threshold when none is given. So, for instance,\n> \"--find-renames=25 --find-renames\" should result in the default\n> threshold (50%) instead of 25%.\n\nMakes sense. I'd have expected to see the corresponding tests bundled\nalong with this patch rather than included in a separate patch (5/5)\nwhose title doesn't indicate any direct relation to this change\n(although the corresponding test is mentioned briefly in the 5/5\ncommit message).\n\nYou might consider re-ordering the patches so that the patch (3/5)\nwhich adds missing tests for existing features instead comes first.\nThen, bundle the tests for this \"fix\" directly into this patch.\n\n> Signed-off-by: Felipe Gonçalves Assis \n> ---\n> merge-recursive.c | 4 +++-\n> 1 file changed, 3 insertions(+), 1 deletion(-)\n>\n> diff --git a/merge-recursive.c b/merge-recursive.c\n> index 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> o->renormalize = 0;\n> else if (!strcmp(s, \"no-renames\"))\n> o->detect_rename = 0;\n> - else if (!strcmp(s, \"find-renames\"))\n> + else if (!strcmp(s, \"find-renames\")) {\n> o->detect_rename = 1;\n> + o->rename_score = 0;\n> + }\n> else if (skip_prefix(s, \"find-renames=\", &arg) ||\n> skip_prefix(s, \"rename-threshold=\", &arg)) {\n> if ((o->rename_score = parse_rename_score(&arg)) == -1 || *arg != 0)\n> --\n> 2.7.1.492.gc9722f8\n>\n"},{"id":"278752","messageId":"CAPig+cTkTxGJAOy-CDFJHwOg=C3Mq9ZHNyqBZ_+S0wTXh_SHaA@mail.gmail.com","threadId":"41455","inReplyTo":"1456067358-19781-3-git-send-email-felipegassis@gmail.com","subject":"Re: [PATCH 2/5] merge-strategies.txt: fix typo","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-02-21T17:21:20Z","receivedAt":"2016-02-21T17:21:20Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Feb 21, 2016 at 10:09 AM, Felipe Gonçalves Assis\n wrote:\n> Signed-off-by: Felipe Gonçalves Assis \n> ---\n> diff --git a/Documentation/merge-strategies.txt b/Documentation/merge-strategies.txt\n> @@ -86,8 +86,8 @@ no-renames;;\n> find-renames[=];;\n> - Turn on rename detection, optionally setting the the similarity\n> - threshold. This is the default.\n> + Turn on rename detection, optionally setting the similarity\n> + threshold. This is the default.\n\nI'd probably have omitted the unimportant whitespace change on the\nsecond line from the patch since it's just noise which distracts from\nthe real fix, but this alone is not worth a re-roll.\n\n> See also linkgit:git-diff[1] `--find-renames`.\n"},{"id":"278762","messageId":"CAPig+cTpngfi6EF+MbD-fQ1nM_9TTeVZJ_fBviD5=4Q9uN-8aA@mail.gmail.com","threadId":"41455","inReplyTo":"1456067358-19781-4-git-send-email-felipegassis@gmail.com","subject":"Re: [PATCH 3/5] merge-recursive: test rename threshold option","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-02-21T17:52:07Z","receivedAt":"2016-02-21T17:52:07Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Feb 21, 2016 at 10:09 AM, Felipe Gonçalves Assis\n wrote:\n> merge-recursive: test rename threshold option\n\nMaybe rephrase this as:\n\n t3034: add rename threshold tests\n\n> Commit 10ae7526bebb505ddddba01f76ec97d5f7b5e0e5 introduced this feature,\n\nIn commit messages, it is helpful to provide more context about a\ncommit by quoting its \"subject\", like this:\n\n 10ae752 (merge-recursive: option to specify rename\n threshold, 2010-09-27) introduced this feature...\n\n> but did not include any tests. This commit fixes this.\n\n\"This commit fixes this.\" is redundant with the subject and could be dropped.\n\nThe above are superficial observation; see below for a bit more meaty stuff...\n\n> Signed-off-by: Felipe Gonçalves Assis \n> ---\n> diff --git a/t/t3034-merge-recursive-rename-threshold.sh b/t/t3034-merge-recursive-rename-threshold.sh\n> @@ -0,0 +1,146 @@\n> +#!/bin/sh\n> +\n> +test_description='merge-recursive rename threshold option\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> +test_expect_success setup '\n> + get_expected_stages () {\n\nThere is no particularly good reason to define these shell functions\nwithin the 'setup' test, and doing so makes the test itself more\ndifficult to read. More typical would be to define the functions\noutside any test.\n\n> + git checkout rename -- $1-new &&\n> + git ls-files --stage $1-new > expected-stages-undetected-$1\n\nBroken &&-chain.\n\nStyle: Omit space after redirection operators (here and elsewhere in\nthe script). outputfile\n\n> + sed \"s/ 0 / 2 /\n> + \" < expected-stages-undetected-$1 > expected-stages-detected-$1\n\nThis is difficult to read due to the way this is formatted with the\nnewline inside the quoted string. How about formatting it in a more\ntypical fashion, like this?\n\n sed \"s/ 0 / 2 /\" expected-stages-detected-$1\n\nAlso, broken &&-chain.\n\n> + git read-tree -u --reset HEAD\n> + } &&\n> +\n> + rename_detected () {\n> + git ls-files --stage $1-old $1-new > stages-actual-$1 &&\n> + test_cmp expected-stages-detected-$1 stages-actual-$1\n> + } &&\n> +\n> + rename_undetected () {\n> + git ls-files --stage $1-old $1-new > stages-actual-$1 &&\n> + test_cmp expected-stages-undetected-$1 stages-actual-$1\n> + } &&\n> +\n> + check_common () {\n> + git ls-files --stage > stages-actual &&\n> + test $(wc -l < stages-actual) -eq 4\n\nPerhaps use test_line_count() instead.\n\n> + } &&\n> +\n> + check_find_renames_25 () {\n> + check_common &&\n> + rename_detected a &&\n> + rename_detected b &&\n> + rename_detected c &&\n> + rename_detected d\n> + } &&\n> +\n> + check_find_renames_50 () {\n> + check_common\n\nBroken &&-chain.\n\n> + rename_detected a &&\n> + rename_detected b &&\n> + rename_detected c &&\n> + rename_undetected d\n> + } &&\n> +\n> + check_find_renames_75 () {\n> + check_common\n\nDitto.\n\n> + rename_detected a &&\n> + rename_detected b &&\n> + rename_undetected c &&\n> + rename_undetected d\n> + } &&\n> +\n> + check_find_renames_100 () {\n> + check_common\n\nDitto.\n\n> + rename_detected a &&\n> + rename_undetected b &&\n> + rename_undetected c &&\n> + rename_undetected d\n> + } &&\n> +\n> + check_no_renames () {\n> + check_common\n\nDitto.\n\n> + rename_undetected a &&\n> + rename_undetected b &&\n> + rename_undetected c &&\n> + rename_undetected d\n> + } &&\n> +\n> + cat <<-\\EOF > a-old &&\n> + aa1\n> + aa2\n> + aa3\n> + aa4\n> + EOF\n> + sed s/aa/bb/ < a-old > b-old &&\n> + sed s/aa/cc/ < a-old > c-old &&\n> + sed s/aa/dd/ < a-old > d-old &&\n> + git add [a-d]-old &&\n> + test_tick &&\n\nNothing in these tests depend upon these test_tick() invocations, so\nhaving them here distracts the reader by making him wonder if\nsomething unusual is going on to require them.\n\n> + git commit -m base &&\n> + git rm [a-d]-old &&\n> + test_tick &&\n> + git commit -m delete &&\n> + git checkout -b rename HEAD^ &&\n> + cp a-old a-new &&\n> + sed 1,1s/./x/ < b-old > b-new &&\n> + sed 1,2s/./x/ < c-old > c-new &&\n> + sed 1,3s/./x/ < d-old > d-new &&\n> + git add [a-d]-new &&\n> + git rm [a-d]-old &&\n> + test_tick &&\n> + git commit -m rename &&\n> + get_expected_stages a &&\n> + get_expected_stages b &&\n> + get_expected_stages c &&\n> + get_expected_stages d\n> +'\n> +\n> +test_expect_success 'the default similarity index is 50%' '\n\ns/the//\n\nAlso, do you mean s/index/threshold/ ?\n\n> + git read-tree --reset -u HEAD &&\n> + test_must_fail git merge-recursive HEAD^ -- HEAD master &&\n> + check_find_renames_50\n> +'\n> +\n> +test_expect_success 'low rename threshold' '\n> + git read-tree --reset -u HEAD &&\n> + test_must_fail git merge-recursive --rename-threshold=25 HEAD^ -- HEAD master &&\n> + check_find_renames_25\n> [...]\n> + check_find_renames_75\n> [...]\n> + check_find_renames_100\n> +'\n> +\n> +test_expect_success 'rename threshold is truncated' '\n> + git read-tree --reset -u HEAD &&\n> + test_must_fail git merge-recursive --rename-threshold=200% HEAD^ -- HEAD master &&\n> + check_find_renames_100\n> +'\n\nIs this truncation documented behavior or is it just a detail of the\ncurrent implementation. (Genuine question; I haven't checked the\ndocumentation or source.) If just an implementation detail, then it\nmight not be desirable to formalize it via a test.\n\n> +test_expect_success 'last wins in --rename-threshold= --rename-threshold=' '\n> + git read-tree --reset -u HEAD &&\n> + test_must_fail git merge-recursive --rename-threshold=25 --rename-threshold=75 HEAD^ -- HEAD master &&\n> + check_find_renames_75\n> +'\n\nWould it make sense to add tests checking that invalid\n--rename-threshold= arguments, such as negative and non-numbers,\ncorrectly error out?\n\n> +test_done\n"},{"id":"278770","messageId":"CAPig+cSaF73ff-RppOZWiS3Hm1skmqGj+tE9g6Y85hM1GnWn7A@mail.gmail.com","threadId":"41455","inReplyTo":"1456067358-19781-5-git-send-email-felipegassis@gmail.com","subject":"Re: [PATCH 4/5] merge-recursive: test option to disable renames","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-02-21T18:01:53Z","receivedAt":"2016-02-21T18:01:53Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Feb 21, 2016 at 10:09 AM, Felipe Gonçalves Assis\n wrote:\n> Also update name and description of tests for consistency:\n> \"merge-recursive options\" -> \"merge-recursive space options\"\n> \"merge-recursive rename threshold\" -> \"merge-recursive rename options\"\n>\n> Signed-off-by: Felipe Gonçalves Assis \n> ---\n> diff --git a/t/t3034-merge-recursive-rename-threshold.sh b/t/t3034-merge-recursive-rename-options.sh\n> similarity index 83%\n> rename from t/t3034-merge-recursive-rename-threshold.sh\n> rename to t/t3034-merge-recursive-rename-options.sh\n\nt3034 was entirely new in the previous patch (3/5) and could have been\ngiven the correct name at that time, so it's not clear why the\napproach of giving it a too-specific name and then immediately\nrenaming it with a more general purpose name in the following patch is\ndesirable. It should be perfectly fine to start with the general\npurpose name even if the patch initially contains only very specific\ntests (knowing that you will be adding more tests later).\n\n> --- a/t/t3034-merge-recursive-rename-threshold.sh\n> +++ b/t/t3034-merge-recursive-rename-options.sh\n> @@ -1,6 +1,6 @@\n> #!/bin/sh\n>\n> -test_description='merge-recursive rename threshold option\n> +test_description='merge-recursive rename options\n\nDitto. This could have been described with the more generic \"options\"\nfrom the start.\n\n> Test rename detection by examining rename/delete conflicts.\n>\n> @@ -137,10 +137,28 @@ test_expect_success 'rename threshold is truncated' '\n> check_find_renames_100\n> '\n>\n> +test_expect_success 'disabled rename detection' '\n> + git read-tree --reset -u HEAD &&\n> + git merge-recursive --no-renames HEAD^ -- HEAD master &&\n> + check_no_renames\n> +'\n> +\n> test_expect_success 'last wins in --rename-threshold= --rename-threshold=' '\n> git read-tree --reset -u HEAD &&\n> test_must_fail git merge-recursive --rename-threshold=25 --rename-threshold=75 HEAD^ -- HEAD master &&\n> check_find_renames_75\n> '\n>\n> +test_expect_success 'last wins in --no-renames --rename-threshold=' '\n> + git read-tree --reset -u HEAD &&\n> + test_must_fail git merge-recursive --no-renames --rename-threshold=25 HEAD^ -- HEAD master &&\n> + check_find_renames_25\n> +'\n> +\n> +test_expect_success 'last wins in --rename-threshold= --no-renames' '\n> + git read-tree --reset -u HEAD &&\n> + git merge-recursive --rename-threshold=25 --no-renames HEAD^ -- HEAD master &&\n> + check_no_renames\n> +'\n"},{"id":"278774","messageId":"CAPig+cSAWjqD7weNLyk4MrAU1Q7+R6z16GndKkByHo2Uc4yWqg@mail.gmail.com","threadId":"41455","inReplyTo":"1456067358-19781-6-git-send-email-felipegassis@gmail.com","subject":"Re: [PATCH 5/5] merge-recursive: test more consistent interface","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-02-21T18:40:26Z","receivedAt":"2016-02-21T18:40:26Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Feb 21, 2016 at 10:09 AM, Felipe Gonçalves Assis\n wrote:\n> merge-recursive: test more consistent interface\n\nThe real meat of this patch (it seems) is that you're adding tests to\nverify that --find-renames= and --rename-threshold= are aliases, so it\nmight make sense for the summary line to state that.\n\n t3034: test that --find-renames= and --rename-threshold= are aliases\n\n> Update basic tests to use the new option find-renames instead of\n> rename-threshold. Add tests to verify that rename-threshold= behaves\n> as a synonym for find-renames=. Test that find-renames resets\n> threshold.\n\nLikewise, the order of these sentences seems wrong. The important bit\nshould be mentioned first, which is that the one is an alias for the\nother.\n\n(In fact, if you take advice given below in the actual patch content,\nthen this paragraph can probably be dropped altogether since the other\ntwo bits don't really belong in this patch.)\n\n> Signed-off-by: Felipe Gonçalves Assis \n> ---\n> diff --git a/t/t3034-merge-recursive-rename-options.sh b/t/t3034-merge-recursive-rename-options.sh\n> @@ -115,25 +115,25 @@ test_expect_success 'the default similarity index is 50%' '\n>\n> test_expect_success 'low rename threshold' '\n> git read-tree --reset -u HEAD &&\n> - test_must_fail git merge-recursive --rename-threshold=25 HEAD^ -- HEAD master &&\n> + test_must_fail git merge-recursive --find-renames=25 HEAD^ -- HEAD master &&\n\nSince you're building this series atop 10ae752 (merge-recursive:\noption to specify rename threshold, 2010-09-27) in 'next', the\n--find-renames= option already exists, so these tests, which were\nadded in 3/5, can instead use --find-renames= from the start, thus\nmaking this patch (5/5) much less noisy since this change and several\nbelow will disappear altogether.\n\nTaking the above and review comments from earlier patches into\naccount, it might make sense to re-order the series as follows:\n\n1/5: add --find-renames & --find-renames= tests (including \"last wins\")\n2/5: add --find-renames= / --rename-threshold= aliases tests\n3/5: add --no-renames tests (including \"last wins\")\n4/5: fix --find-renames to reset threshold to default (including test)\n5/5: fix merge-strategies.txt typo\n\nThe position of the typo fix patch isn't significant; I just\narbitrarily plopped it at the end. Also, the order of patches 2 & 3 is\narbitrary.\n\nMore below...\n\n> check_find_renames_25\n> '\n>\n> test_expect_success 'high rename threshold' '\n> git read-tree --reset -u HEAD &&\n> - test_must_fail git merge-recursive --rename-threshold=75 HEAD^ -- HEAD master &&\n> + test_must_fail git merge-recursive --find-renames=75 HEAD^ -- HEAD master &&\n> check_find_renames_75\n> '\n>\n> test_expect_success 'exact renames only' '\n> git read-tree --reset -u HEAD &&\n> - test_must_fail git merge-recursive --rename-threshold=100% HEAD^ -- HEAD master &&\n> + test_must_fail git merge-recursive --find-renames=100% HEAD^ -- HEAD master &&\n> check_find_renames_100\n> '\n>\n> test_expect_success 'rename threshold is truncated' '\n> git read-tree --reset -u HEAD &&\n> - test_must_fail git merge-recursive --rename-threshold=200% HEAD^ -- HEAD master &&\n> + test_must_fail git merge-recursive --find-renames=200% HEAD^ -- HEAD master &&\n> check_find_renames_100\n> '\n>\n> @@ -143,12 +143,36 @@ test_expect_success 'disabled rename detection' '\n> check_no_renames\n> '\n>\n> -test_expect_success 'last wins in --rename-threshold= --rename-threshold=' '\n> +test_expect_success 'last wins in --find-renames= --find-renames=' '\n> git read-tree --reset -u HEAD &&\n> - test_must_fail git merge-recursive --rename-threshold=25 --rename-threshold=75 HEAD^ -- HEAD master &&\n> + test_must_fail git merge-recursive --find-renames=25 --find-renames=75 HEAD^ -- HEAD master &&\n> check_find_renames_75\n> '\n>\n> +test_expect_success '--find-renames resets threshold' '\n> + git read-tree --reset -u HEAD &&\n> + test_must_fail git merge-recursive --find-renames=25 --find-renames HEAD^ -- HEAD master &&\n> + check_find_renames_50\n> +'\n\nAs mentioned in my review of patch 1/5, this test really ought to be\nbundled with that patch.\n\n> +test_expect_success 'last wins in --no-renames --find-renames' '\n> + git read-tree --reset -u HEAD &&\n> + test_must_fail git merge-recursive --no-renames --find-renames HEAD^ -- HEAD master &&\n> + check_find_renames_50\n> +'\n> +\n> +test_expect_success 'last wins in --find-renames --no-renames' '\n> + git read-tree --reset -u HEAD &&\n> + git merge-recursive --find-renames --no-renames HEAD^ -- HEAD master &&\n> + check_no_renames\n> +'\n> +\n> +test_expect_success 'rename-threshold= is a synonym for find-renames=' '\n> + git read-tree --reset -u HEAD &&\n> + test_must_fail git merge-recursive --rename-threshold=25 HEAD^ -- HEAD master &&\n> + check_find_renames_25\n> +'\n\nI rather expected to see this test come first, as all the others are\nrather subordinate to it.\n\n> test_expect_success 'last wins in --no-renames --rename-threshold=' '\n> git read-tree --reset -u HEAD &&\n> test_must_fail git merge-recursive --no-renames --rename-threshold=25 HEAD^ -- HEAD master &&\n> @@ -161,4 +185,16 @@ test_expect_success 'last wins in --rename-threshold= --no-renames' '\n> check_no_renames\n> '\n>\n> +test_expect_success 'last wins in --rename-threshold= --find-renames' '\n> + git read-tree --reset -u HEAD &&\n> + test_must_fail git merge-recursive --rename-threshold=25 --find-renames HEAD^ -- HEAD master &&\n> + check_find_renames_50\n> +'\n> +\n> +test_expect_success 'last wins in --find-renames --rename-threshold=' '\n> + git read-tree --reset -u HEAD &&\n> + test_must_fail git merge-recursive --find-renames --rename-threshold=25 HEAD^ -- HEAD master &&\n> + check_find_renames_25\n> +'\n> +\n> test_done\n"},{"id":"278776","messageId":"CALMa68qhCBJ5MnT7i6UT989OKKzzOF_bTqo38thSPMwq0mbj6g@mail.gmail.com","threadId":"41455","inReplyTo":"CAPig+cTpngfi6EF+MbD-fQ1nM_9TTeVZJ_fBviD5=4Q9uN-8aA@mail.gmail.com","subject":"Re: [PATCH 3/5] merge-recursive: test rename threshold option","fromName":"Felipe Gonçalves Assis","fromEmail":"felipeg.assis@gmail.com","sentAt":"2016-02-21T18:55:07Z","receivedAt":"2016-02-21T18:55:07Z","isPatch":true,"sender":{"key":"felipeg.assis@gmail.com","avatar":null},"body":"On 21 February 2016 at 14:52, Eric Sunshine wrote:\n> On Sun, Feb 21, 2016 at 10:09 AM, Felipe Gonçalves Assis\n> wrote:\n>> merge-recursive: test rename threshold option\n>\n>> + git read-tree --reset -u HEAD &&\n>> + test_must_fail git merge-recursive HEAD^ -- HEAD master &&\n>> + check_find_renames_50\n>> +'\n>> +\n>> +test_expect_success 'low rename threshold' '\n>> + git read-tree --reset -u HEAD &&\n>> + test_must_fail git merge-recursive --rename-threshold=25 HEAD^ -- HEAD master &&\n>> + check_find_renames_25\n>> [...]\n>> + check_find_renames_75\n>> [...]\n>> + check_find_renames_100\n>> +'\n>> +\n>> +test_expect_success 'rename threshold is truncated' '\n>> + git read-tree --reset -u HEAD &&\n>> + test_must_fail git merge-recursive --rename-threshold=200% HEAD^ -- HEAD master &&\n>> + check_find_renames_100\n>> +'\n>\n> Is this truncation documented behavior or is it just a detail of the\n> current implementation. (Genuine question; I haven't checked the\n> documentation or source.) If just an implementation detail, then it\n> might not be desirable to formalize it via a test.\n>\n\nNot documented. I will remove this. If you prefer to have it\ndocumented and the test added back later, I can do that.\n\n>> +test_expect_success 'last wins in --rename-threshold= --rename-threshold=' '\n>> + git read-tree --reset -u HEAD &&\n>> + test_must_fail git merge-recursive --rename-threshold=25 --rename-threshold=75 HEAD^ -- HEAD master &&\n>> + check_find_renames_75\n>> +'\n>\n> Would it make sense to add tests checking that invalid\n> --rename-threshold= arguments, such as negative and non-numbers,\n> correctly error out?\n>\n\nI guess so. Can I ask you for a suggestion on how to check this?\n\nGiven that merges here usually fail anyway because of the conflicts,\nwhat is the best way of checking the effect of an argument rejection?\n1. Check that the merge fails but the index is not changed\n2. Check for a specific exit code\n3. Use another setup so that the merges succeed\n\n\nThanks for the detailed review. Already working on the other comments.\n"},{"id":"278778","messageId":"CALMa68qBtw+MdhDsOFofEDoySuTU5BJ2tQvEKrynQxcXKWkhGw@mail.gmail.com","threadId":"41455","inReplyTo":"CAPig+cSAWjqD7weNLyk4MrAU1Q7+R6z16GndKkByHo2Uc4yWqg@mail.gmail.com","subject":"Re: [PATCH 5/5] merge-recursive: test more consistent interface","fromName":"Felipe Gonçalves Assis","fromEmail":"felipeg.assis@gmail.com","sentAt":"2016-02-21T19:55:09Z","receivedAt":"2016-02-21T19:55:09Z","isPatch":true,"sender":{"key":"felipeg.assis@gmail.com","avatar":null},"body":"On 21 February 2016 at 15:40, Eric Sunshine wrote:\n> On Sun, Feb 21, 2016 at 10:09 AM, Felipe Gonçalves Assis\n> wrote:\n>> merge-recursive: test more consistent interface\n>\n> The real meat of this patch (it seems) is that you're adding tests to\n> verify that --find-renames= and --rename-threshold= are aliases, so it\n> might make sense for the summary line to state that.\n>\n> t3034: test that --find-renames= and --rename-threshold= are aliases\n>\n>> Update basic tests to use the new option find-renames instead of\n>> rename-threshold. Add tests to verify that rename-threshold= behaves\n>> as a synonym for find-renames=. Test that find-renames resets\n>> threshold.\n>\n> Likewise, the order of these sentences seems wrong. The important bit\n> should be mentioned first, which is that the one is an alias for the\n> other.\n>\n> (In fact, if you take advice given below in the actual patch content,\n> then this paragraph can probably be dropped altogether since the other\n> two bits don't really belong in this patch.)\n>\n>> Signed-off-by: Felipe Gonçalves Assis \n>> ---\n>> diff --git a/t/t3034-merge-recursive-rename-options.sh b/t/t3034-merge-recursive-rename-options.sh\n>> @@ -115,25 +115,25 @@ test_expect_success 'the default similarity index is 50%' '\n>>\n>> test_expect_success 'low rename threshold' '\n>> git read-tree --reset -u HEAD &&\n>> - test_must_fail git merge-recursive --rename-threshold=25 HEAD^ -- HEAD master &&\n>> + test_must_fail git merge-recursive --find-renames=25 HEAD^ -- HEAD master &&\n>\n> Since you're building this series atop 10ae752 (merge-recursive:\n> option to specify rename threshold, 2010-09-27) in 'next', the\n> --find-renames= option already exists, so these tests, which were\n> added in 3/5, can instead use --find-renames= from the start, thus\n> making this patch (5/5) much less noisy since this change and several\n> below will disappear altogether.\n>\n> Taking the above and review comments from earlier patches into\n> account, it might make sense to re-order the series as follows:\n>\n> 1/5: add --find-renames & --find-renames= tests (including \"last wins\")\n> 2/5: add --find-renames= / --rename-threshold= aliases tests\n> 3/5: add --no-renames tests (including \"last wins\")\n> 4/5: fix --find-renames to reset threshold to default (including test)\n> 5/5: fix merge-strategies.txt typo\n>\n> The position of the typo fix patch isn't significant; I just\n> arbitrarily plopped it at the end. Also, the order of patches 2 & 3 is\n> arbitrary.\n>\n\nFair enough. As I said, I ordered the three test commits so that the\nfirst one could be applied soon after the commit introducing\n\"rename-thresholds\", the second soon after the commit introducing\n\"no-renames\" and the third one soon after the fixup for the commit\nintroducing \"find-renames\" (which would ideally be correct from the\nstart), but then this is probably more aesthetic than practical.\n\nI am currently working on the following order, which follows your constraints.\n1/5: fix typo (I don't like typos)\n2/5: tests involving --find-renames\n3/5: tests involving --no-renames\n4/5: tests involving --rename-threshold (this represents what would be\nreverted if the feature was discontinued)\n5/5: fix --find-renames + test\n\n> More below...\n>\n>\n>> +test_expect_success 'last wins in --no-renames --find-renames' '\n>> + git read-tree --reset -u HEAD &&\n>> + test_must_fail git merge-recursive --no-renames --find-renames HEAD^ -- HEAD master &&\n>> + check_find_renames_50\n>> +'\n>> +\n>> +test_expect_success 'last wins in --find-renames --no-renames' '\n>> + git read-tree --reset -u HEAD &&\n>> + git merge-recursive --find-renames --no-renames HEAD^ -- HEAD master &&\n>> + check_no_renames\n>> +'\n>> +\n>> +test_expect_success 'rename-threshold= is a synonym for find-renames=' '\n>> + git read-tree --reset -u HEAD &&\n>> + test_must_fail git merge-recursive --rename-threshold=25 HEAD^ -- HEAD master &&\n>> + check_find_renames_25\n>> +'\n>\n> I rather expected to see this test come first, as all the others are\n> rather subordinate to it.\n>\n\nBut it already is the first test involving \"rename-threshold\". The\npreceding tests verify the rename detection functionality with the\nrecommended interface. Then we have tests for the deprecated option.\nThis tail is exactly what we would remove if it was discontinued.\n\nWhat did you mean?\n\n>> test_expect_success 'last wins in --no-renames --rename-threshold=' '\n>> git read-tree --reset -u HEAD &&\n>> test_must_fail git merge-recursive --no-renames --rename-threshold=25 HEAD^ -- HEAD master &&\n>> @@ -161,4 +185,16 @@ test_expect_success 'last wins in --rename-threshold= --no-renames' '\n>> check_no_renames\n>> '\n>>\n>> +test_expect_success 'last wins in --rename-threshold= --find-renames' '\n>> + git read-tree --reset -u HEAD &&\n>> + test_must_fail git merge-recursive --rename-threshold=25 --find-renames HEAD^ -- HEAD master &&\n>> + check_find_renames_50\n>> +'\n>> +\n>> +test_expect_success 'last wins in --find-renames --rename-threshold=' '\n>> + git read-tree --reset -u HEAD &&\n>> + test_must_fail git merge-recursive --find-renames --rename-threshold=25 HEAD^ -- HEAD master &&\n>> + check_find_renames_25\n>> +'\n>> +\n>> test_done\n"},{"id":"278779","messageId":"CAPig+cRgBCH7-EqEQ8q=kJwSf0nNNWUGRL6PA4Lp0KSE2agLSA@mail.gmail.com","threadId":"41455","inReplyTo":"CALMa68qBtw+MdhDsOFofEDoySuTU5BJ2tQvEKrynQxcXKWkhGw@mail.gmail.com","subject":"Re: [PATCH 5/5] merge-recursive: test more consistent interface","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-02-21T20:32:58Z","receivedAt":"2016-02-21T20:32:58Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Feb 21, 2016 at 2:55 PM, Felipe Gonçalves Assis\n wrote:\n> On 21 February 2016 at 15:40, Eric Sunshine wrote:\n>> Taking the above and review comments from earlier patches into\n>> account, it might make sense to re-order the series as follows:\n>>\n>> 1/5: add --find-renames & --find-renames= tests (including \"last wins\")\n>> 2/5: add --find-renames= / --rename-threshold= aliases tests\n>> 3/5: add --no-renames tests (including \"last wins\")\n>> 4/5: fix --find-renames to reset threshold to default (including test)\n>> 5/5: fix merge-strategies.txt typo\n>>\n>> The position of the typo fix patch isn't significant; I just\n>> arbitrarily plopped it at the end. Also, the order of patches 2 & 3 is\n>> arbitrary.\n>\n> Fair enough. [...]\n>\n> I am currently working on the following order, which follows your constraints.\n> 1/5: fix typo (I don't like typos)\n> 2/5: tests involving --find-renames\n\nPresumably 2/5 also tests --find-renames=...\n\n> 3/5: tests involving --no-renames\n> 4/5: tests involving --rename-threshold (this represents what would be\n> reverted if the feature was discontinued)\n> 5/5: fix --find-renames + test\n\nSounds reasonable.\n\n>>> +test_expect_success 'last wins in --no-renames --find-renames' '\n>>> + git read-tree --reset -u HEAD &&\n>>> + test_must_fail git merge-recursive --no-renames --find-renames HEAD^ -- HEAD master &&\n>>> + check_find_renames_50\n>>> +'\n>>> +\n>>> +test_expect_success 'last wins in --find-renames --no-renames' '\n>>> + git read-tree --reset -u HEAD &&\n>>> + git merge-recursive --find-renames --no-renames HEAD^ -- HEAD master &&\n>>> + check_no_renames\n>>> +'\n>>> +\n>>> +test_expect_success 'rename-threshold= is a synonym for find-renames=' '\n>>> + git read-tree --reset -u HEAD &&\n>>> + test_must_fail git merge-recursive --rename-threshold=25 HEAD^ -- HEAD master &&\n>>> + check_find_renames_25\n>>> +'\n>>\n>> I rather expected to see this test come first, as all the others are\n>> rather subordinate to it.\n>\n> But it already is the first test involving \"rename-threshold\". The\n> preceding tests verify the rename detection functionality with the\n> recommended interface. Then we have tests for the deprecated option.\n> This tail is exactly what we would remove if it was discontinued.\n> What did you mean?\n\nIt's probably just a minor viewpoint issue, as I interpreted the patch\nas primarily testing that --find-renames= and --rename-threshold= are\naliases, in which case checking that condition would be done first.\nHowever, that's effectively moot given the proposed revised patch\nordering since this test would be in patch 4/5, and the two above it\nwould belong in 3/5.\n"},{"id":"278780","messageId":"CAPig+cQ5h4hjOwO6-xXii6QQP0J8B_bHYUwWniR6HdQN2dB_1A@mail.gmail.com","threadId":"41455","inReplyTo":"CALMa68qhCBJ5MnT7i6UT989OKKzzOF_bTqo38thSPMwq0mbj6g@mail.gmail.com","subject":"Re: [PATCH 3/5] merge-recursive: test rename threshold option","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-02-21T20:45:32Z","receivedAt":"2016-02-21T20:45:32Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Feb 21, 2016 at 1:55 PM, Felipe Gonçalves Assis\n wrote:\n> On 21 February 2016 at 14:52, Eric Sunshine wrote:\n>> On Sun, Feb 21, 2016 at 10:09 AM, Felipe Gonçalves Assis\n>> wrote:\n>>> +test_expect_success 'rename threshold is truncated' '\n>>> + git read-tree --reset -u HEAD &&\n>>> + test_must_fail git merge-recursive --rename-threshold=200% HEAD^ -- HEAD master &&\n>>> + check_find_renames_100\n>>> +'\n>>\n>> Is this truncation documented behavior or is it just a detail of the\n>> current implementation. (Genuine question; I haven't checked the\n>> documentation or source.) If just an implementation detail, then it\n>> might not be desirable to formalize it via a test.\n>\n> Not documented. I will remove this. If you prefer to have it\n> documented and the test added back later, I can do that.\n\nLooking at the code itself and its history, this seems to be a\ndeliberate decision, so the test may be appropriate, however, I defer\nto Junio's judgment.\n\n>>> +test_expect_success 'last wins in --rename-threshold= --rename-threshold=' '\n>>> + git read-tree --reset -u HEAD &&\n>>> + test_must_fail git merge-recursive --rename-threshold=25 --rename-threshold=75 HEAD^ -- HEAD master &&\n>>> + check_find_renames_75\n>>> +'\n>>\n>> Would it make sense to add tests checking that invalid\n>> --rename-threshold= arguments, such as negative and non-numbers,\n>> correctly error out?\n>\n> I guess so. Can I ask you for a suggestion on how to check this?\n>\n> Given that merges here usually fail anyway because of the conflicts,\n> what is the best way of checking the effect of an argument rejection?\n> 1. Check that the merge fails but the index is not changed\n> 2. Check for a specific exit code\n> 3. Use another setup so that the merges succeed\n\nThe last option would be most straightforward, however, looking at\ndiff.c:parse_rename_score(), I see that it never actually errors out\n(by returning -1), so you may not (presently) be able to test these\ncases. It seems (presently) that merge-recursive.c:parse_merge_opt()\ncan only catch a usage error if nothing follows the '='.\n"},{"id":"278790","messageId":"xmqq37slac4x.fsf@gitster.mtv.corp.google.com","threadId":"41455","inReplyTo":"CAPig+cQ5h4hjOwO6-xXii6QQP0J8B_bHYUwWniR6HdQN2dB_1A@mail.gmail.com","subject":"Re: [PATCH 3/5] merge-recursive: test rename threshold option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-02-21T23:15:26Z","receivedAt":"2016-02-21T23:15:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n> On Sun, Feb 21, 2016 at 1:55 PM, Felipe Gonçalves Assis\n> wrote:\n>> On 21 February 2016 at 14:52, Eric Sunshine wrote:\n>>> On Sun, Feb 21, 2016 at 10:09 AM, Felipe Gonçalves Assis\n>>> wrote:\n>>>> +test_expect_success 'rename threshold is truncated' '\n>>>> + git read-tree --reset -u HEAD &&\n>>>> + test_must_fail git merge-recursive --rename-threshold=200% HEAD^ -- HEAD master &&\n>>>> + check_find_renames_100\n>>>> +'\n>>>\n>>> Is this truncation documented behavior or is it just a detail of the\n>>> current implementation. (Genuine question; I haven't checked the\n>>> documentation or source.) If just an implementation detail, then it\n>>> might not be desirable to formalize it via a test.\n>>\n>> Not documented. I will remove this. If you prefer to have it\n>> documented and the test added back later, I can do that.\n>\n> Looking at the code itself and its history, this seems to be a\n> deliberate decision, so the test may be appropriate, however, I defer\n> to Junio's judgment.\n\nThis is just us not caring enough about illogical input by crazy\nusers who ignore the fact that the rename scores are supposed to be\nbetween 0 and max.\n\nWe could have added code to reject out-of-range values when we added\nsupport for input with '%', but we instead just made sure we clip\nthe result within the range (probably because it was easier).\n\nI personally do not think it would break too many people if we\nchanged the parser to error out, but at the same time, I do not\nthink it is worth the code churn to do so, and it probably is not\nworth to document the clipping either.\n\n\n[Footnote]\n\n*1* With the original \"give us an integer, and we'll take it as a\nratio of that number out of the next power of ten\", there was no\nneed to worry about out-of-range values.\n"}]}