{"thread":{"id":"47349","subject":"[PATCH on en/rename-progress] diffcore-rename: make diff-tree -l0 mean -l","startedAt":"2017-11-29T18:32:16Z","lastAt":"2017-11-29T23:40:59Z","messageCount":7,"participants":["Jonathan Tan","Elijah Newren","Jonathan Nieder"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"333793","messageId":"20171129183200.178183-1-jonathantanmy@google.com","threadId":"47349","inReplyTo":null,"subject":"[PATCH on en/rename-progress] diffcore-rename: make diff-tree -l0 mean -l","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2017-11-29T18:32:00Z","receivedAt":"2017-11-29T18:32:16Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"In the documentation of diff-tree, it is stated that the -l option\n\"prevents rename/copy detection from running if the number of\nrename/copy targets exceeds the specified number\". The documentation\ndoes not mention any special handling for the number 0, but the\nimplementation before commit b520abf (\"sequencer: warn when internal\nmerge may be suboptimal due to renameLimit\", 2017-11-14) treated 0 as a\nspecial value indicating that the rename limit is to be a very large\nnumber instead.\n\nThe commit b520abf changed that behavior, treating 0 as 0. Revert this\nbehavior to what it was previously. This allows existing scripts and\ntools that use \"-l0\" to continue working. The alternative (to allow\n\"-l0\") is probably much less useful, since users can just refrain from\nspecifying -M and/or -C to have the same effect.\n\nSigned-off-by: Jonathan Tan \n---\nNote that this patch is built on en/rename-progress.\n\nWe noticed this through an automated test for an internal tool - the\ntool uses git diff-tree with -l0, and no longer produces the same\nresults as before.\n---\n diffcore-rename.c | 2 ++\n t/t4001-diff-rename.sh | 15 +++++++++++++++\n 2 files changed, 17 insertions(+)\n\ndiff --git a/diffcore-rename.c b/diffcore-rename.c\nindex 9ca0eaec7..245e999fe 100644\n--- a/diffcore-rename.c\n+++ b/diffcore-rename.c\n@@ -392,6 +392,8 @@ static int too_many_rename_candidates(int num_create,\n \t *\n \t * num_create * num_src > rename_limit * rename_limit\n \t */\n+\tif (rename_limit <= 0)\n+\t\trename_limit = 32767;\n \tif ((num_create <= rename_limit || num_src <= rename_limit) &&\n \t ((uint64_t)num_create * (uint64_t)num_src\n \t <= (uint64_t)rename_limit * (uint64_t)rename_limit))\ndiff --git a/t/t4001-diff-rename.sh b/t/t4001-diff-rename.sh\nindex 0d1fa45d2..eadf4f624 100755\n--- a/t/t4001-diff-rename.sh\n+++ b/t/t4001-diff-rename.sh\n@@ -230,4 +230,19 @@ test_expect_success 'rename pretty print common prefix and suffix overlap' '\n \ttest_i18ngrep \" d/f/{ => f}/e \" output\n '\n \n+test_expect_success 'diff-tree -l0 defaults to a big rename limit, not zero' '\n+\ttest_write_lines line1 line2 line3 >myfile &&\n+\tgit add myfile &&\n+\tgit commit -m x &&\n+\n+\ttest_write_lines line1 line2 line4 >myotherfile &&\n+\tgit rm myfile &&\n+\tgit add myotherfile &&\n+\tgit commit -m x &&\n+\n+\tgit diff-tree -M -l0 HEAD HEAD^ >actual &&\n+\t# Verify that a rename from myotherfile to myfile was detected\n+\tgrep \"myotherfile.*myfile\" actual\n+'\n+\n test_done\n-- \n2.15.0.173.g9268cf4a2.dirty\n\n"},{"id":"333795","messageId":"CABPp-BHEyXcjFzGvLHToz+2FRMM3g-mUTxn2uf8exQV61qff9w@mail.gmail.com","threadId":"47349","inReplyTo":"20171129183200.178183-1-jonathantanmy@google.com","subject":"Re: [PATCH on en/rename-progress] diffcore-rename: make diff-tree -l0 mean -l","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2017-11-29T18:51:20Z","receivedAt":"2017-11-29T18:51:31Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Wed, Nov 29, 2017 at 10:32 AM, Jonathan Tan wrote:\n> In the documentation of diff-tree, it is stated that the -l option\n> \"prevents rename/copy detection from running if the number of\n> rename/copy targets exceeds the specified number\". The documentation\n> does not mention any special handling for the number 0, but the\n> implementation before commit b520abf (\"sequencer: warn when internal\n> merge may be suboptimal due to renameLimit\", 2017-11-14) treated 0 as a\n> special value indicating that the rename limit is to be a very large\n> number instead.\n>\n> The commit b520abf changed that behavior, treating 0 as 0. Revert this\n> behavior to what it was previously. This allows existing scripts and\n> tools that use \"-l0\" to continue working. The alternative (to allow\n> \"-l0\") is probably much less useful, since users can just refrain from\n> specifying -M and/or -C to have the same effect.\n>\n> Signed-off-by: Jonathan Tan \n> ---\n> Note that this patch is built on en/rename-progress.\n>\n> We noticed this through an automated test for an internal tool - the\n> tool uses git diff-tree with -l0, and no longer produces the same\n> results as before.\n\nThanks for testing that version and sending along the fix.\n\nI suspect the commit referenced twice in the commit message should\nhave been 9f7e4bfa3b (\"diff: remove silent clamp of renameLimit\",\n2017-11-13) rather than b520abf (\"sequencer: warn when internal merge\nmay be suboptimal due to renameLimit\", 2017-11-14).\n\nOther than that minor issue, patch and test looks good to me.\n"},{"id":"333796","messageId":"20171129105407.d4232fdb6fce2b99ba75ab9b@google.com","threadId":"47349","inReplyTo":"CABPp-BHEyXcjFzGvLHToz+2FRMM3g-mUTxn2uf8exQV61qff9w@mail.gmail.com","subject":"Re: [PATCH on en/rename-progress] diffcore-rename: make diff-tree -l0 mean -l","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2017-11-29T18:54:07Z","receivedAt":"2017-11-29T18:54:13Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"On Wed, 29 Nov 2017 10:51:20 -0800\nElijah Newren wrote:\n\n> Thanks for testing that version and sending along the fix.\n> \n> I suspect the commit referenced twice in the commit message should\n> have been 9f7e4bfa3b (\"diff: remove silent clamp of renameLimit\",\n> 2017-11-13) rather than b520abf (\"sequencer: warn when internal merge\n> may be suboptimal due to renameLimit\", 2017-11-14).\n\nAh...yes, you're right. I'll update the commit message if I need to send\nout a new version or if Junio requests it (so that it's easier for him\nto merge it in).\n\n> Other than that minor issue, patch and test looks good to me.\n\nThanks.\n"},{"id":"333806","messageId":"20171129200421.GB15098@aiede.mtv.corp.google.com","threadId":"47349","inReplyTo":"CABPp-BHEyXcjFzGvLHToz+2FRMM3g-mUTxn2uf8exQV61qff9w@mail.gmail.com","subject":"Re: [PATCH on en/rename-progress] diffcore-rename: make diff-tree -l0 mean -l","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-11-29T20:04:21Z","receivedAt":"2017-11-29T20:04:28Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Elijah Newren wrote:\n> On Wed, Nov 29, 2017 at 10:32 AM, Jonathan Tan wrote:\n\n>> In the documentation of diff-tree, it is stated that the -l option\n>> \"prevents rename/copy detection from running if the number of\n>> rename/copy targets exceeds the specified number\". The documentation\n>> does not mention any special handling for the number 0, but the\n>> implementation before commit b520abf (\"sequencer: warn when internal\n>> merge may be suboptimal due to renameLimit\", 2017-11-14) treated 0 as a\n>> special value indicating that the rename limit is to be a very large\n>> number instead.\n>>\n>> The commit b520abf changed that behavior, treating 0 as 0. Revert this\n>> behavior to what it was previously. This allows existing scripts and\n>> tools that use \"-l0\" to continue working. The alternative (to allow\n>> \"-l0\") is probably much less useful, since users can just refrain from\n\nI think in the parenthesis you mean 'to allow \"-l0\" to suppress rename\ndetection', since this patch is all about allowing '-l0' already.\n\n>> specifying -M and/or -C to have the same effect.\n>>\n>> Signed-off-by: Jonathan Tan \n>> ---\n>> Note that this patch is built on en/rename-progress.\n>>\n>> We noticed this through an automated test for an internal tool - the\n>> tool uses git diff-tree with -l0, and no longer produces the same\n>> results as before.\n>\n> Thanks for testing that version and sending along the fix.\n>\n> I suspect the commit referenced twice in the commit message should\n> have been 9f7e4bfa3b (\"diff: remove silent clamp of renameLimit\",\n> 2017-11-13) rather than b520abf (\"sequencer: warn when internal merge\n> may be suboptimal due to renameLimit\", 2017-11-14).\n>\n> Other than that minor issue, patch and test looks good to me.\n\nThanks, both. Looking at that patch, the fix is obviously correct.\n\nWith Elijah's commit message tweak,\nReviewed-by: Jonathan Nieder \n"},{"id":"333808","messageId":"20171129201154.192379-1-jonathantanmy@google.com","threadId":"47349","inReplyTo":"20171129183200.178183-1-jonathantanmy@google.com","subject":"[PATCH on en/rename-progress v2] diffcore-rename: make diff-tree -l0 mean -l","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2017-11-29T20:11:54Z","receivedAt":"2017-11-29T20:12:16Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"In the documentation of diff-tree, it is stated that the -l option\n\"prevents rename/copy detection from running if the number of\nrename/copy targets exceeds the specified number\". The documentation\ndoes not mention any special handling for the number 0, but the\nimplementation before commit 9f7e4bfa3b (\"diff: remove silent clamp of\nrenameLimit\", 2017-11-13) treated 0 as a special value indicating that\nthe rename limit is to be a very large number instead.\n\nThe commit 9f7e4bfa3b changed that behavior, treating 0 as 0. Revert\nthis behavior to what it was previously. This allows existing scripts\nand tools that use \"-l0\" to continue working. The alternative (to have\n\"-l0\" suppress rename detection) is probably much less useful, since\nusers can just refrain from specifying -M and/or -C to have the same\neffect.\n\nSigned-off-by: Jonathan Tan \n---\nThere are multiple issues with the commit message, so I am sending this\nnew version out.\n\nv2 is exactly the same as previously, except that the commit message is\nchanged following Elijah Newren's and Jonathan Nieder's comments.\n---\n diffcore-rename.c | 2 ++\n t/t4001-diff-rename.sh | 15 +++++++++++++++\n 2 files changed, 17 insertions(+)\n\ndiff --git a/diffcore-rename.c b/diffcore-rename.c\nindex 9ca0eaec7..245e999fe 100644\n--- a/diffcore-rename.c\n+++ b/diffcore-rename.c\n@@ -392,6 +392,8 @@ static int too_many_rename_candidates(int num_create,\n \t *\n \t * num_create * num_src > rename_limit * rename_limit\n \t */\n+\tif (rename_limit <= 0)\n+\t\trename_limit = 32767;\n \tif ((num_create <= rename_limit || num_src <= rename_limit) &&\n \t ((uint64_t)num_create * (uint64_t)num_src\n \t <= (uint64_t)rename_limit * (uint64_t)rename_limit))\ndiff --git a/t/t4001-diff-rename.sh b/t/t4001-diff-rename.sh\nindex 0d1fa45d2..eadf4f624 100755\n--- a/t/t4001-diff-rename.sh\n+++ b/t/t4001-diff-rename.sh\n@@ -230,4 +230,19 @@ test_expect_success 'rename pretty print common prefix and suffix overlap' '\n \ttest_i18ngrep \" d/f/{ => f}/e \" output\n '\n \n+test_expect_success 'diff-tree -l0 defaults to a big rename limit, not zero' '\n+\ttest_write_lines line1 line2 line3 >myfile &&\n+\tgit add myfile &&\n+\tgit commit -m x &&\n+\n+\ttest_write_lines line1 line2 line4 >myotherfile &&\n+\tgit rm myfile &&\n+\tgit add myotherfile &&\n+\tgit commit -m x &&\n+\n+\tgit diff-tree -M -l0 HEAD HEAD^ >actual &&\n+\t# Verify that a rename from myotherfile to myfile was detected\n+\tgrep \"myotherfile.*myfile\" actual\n+'\n+\n test_done\n-- \n2.15.0.173.g9268cf4a2.dirty\n\n"},{"id":"333809","messageId":"20171129202305.GC15098@aiede.mtv.corp.google.com","threadId":"47349","inReplyTo":"20171129201154.192379-1-jonathantanmy@google.com","subject":"Re: [PATCH on en/rename-progress v2] diffcore-rename: make diff-tree -l0 mean -l","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-11-29T20:23:05Z","receivedAt":"2017-11-29T20:23:29Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jonathan Tan wrote:\n\n> In the documentation of diff-tree, it is stated that the -l option\n> \"prevents rename/copy detection from running if the number of\n> rename/copy targets exceeds the specified number\". The documentation\n> does not mention any special handling for the number 0, but the\n> implementation before commit 9f7e4bfa3b (\"diff: remove silent clamp of\n> renameLimit\", 2017-11-13) treated 0 as a special value indicating that\n> the rename limit is to be a very large number instead.\n>\n> The commit 9f7e4bfa3b changed that behavior, treating 0 as 0. Revert\n> this behavior to what it was previously. This allows existing scripts\n> and tools that use \"-l0\" to continue working. The alternative (to have\n> \"-l0\" suppress rename detection) is probably much less useful, since\n> users can just refrain from specifying -M and/or -C to have the same\n> effect.\n>\n> Signed-off-by: Jonathan Tan \n> ---\n> v2 is exactly the same as previously, except that the commit message is\n> changed following Elijah Newren's and Jonathan Nieder's comments.\n>\n> diffcore-rename.c | 2 ++\n> t/t4001-diff-rename.sh | 15 +++++++++++++++\n> 2 files changed, 17 insertions(+)\n\nReviewed-by: Jonathan Nieder \n\nThanks again.\n\n> diff --git a/diffcore-rename.c b/diffcore-rename.c\n> index 9ca0eaec7..245e999fe 100644\n> --- a/diffcore-rename.c\n> +++ b/diffcore-rename.c\n> @@ -392,6 +392,8 @@ static int too_many_rename_candidates(int num_create,\n> \t *\n> \t * num_create * num_src > rename_limit * rename_limit\n> \t */\n> +\tif (rename_limit <= 0)\n> +\t\trename_limit = 32767;\n> \tif ((num_create <= rename_limit || num_src <= rename_limit) &&\n> \t ((uint64_t)num_create * (uint64_t)num_src\n> \t <= (uint64_t)rename_limit * (uint64_t)rename_limit))\n> diff --git a/t/t4001-diff-rename.sh b/t/t4001-diff-rename.sh\n> index 0d1fa45d2..eadf4f624 100755\n> --- a/t/t4001-diff-rename.sh\n> +++ b/t/t4001-diff-rename.sh\n> @@ -230,4 +230,19 @@ test_expect_success 'rename pretty print common prefix and suffix overlap' '\n> \ttest_i18ngrep \" d/f/{ => f}/e \" output\n> '\n> \n> +test_expect_success 'diff-tree -l0 defaults to a big rename limit, not zero' '\n> +\ttest_write_lines line1 line2 line3 >myfile &&\n> +\tgit add myfile &&\n> +\tgit commit -m x &&\n> +\n> +\ttest_write_lines line1 line2 line4 >myotherfile &&\n> +\tgit rm myfile &&\n> +\tgit add myotherfile &&\n> +\tgit commit -m x &&\n> +\n> +\tgit diff-tree -M -l0 HEAD HEAD^ >actual &&\n> +\t# Verify that a rename from myotherfile to myfile was detected\n> +\tgrep \"myotherfile.*myfile\" actual\n> +'\n> +\n> test_done\n"},{"id":"333819","messageId":"CABPp-BH3FWQjv_uD+ddUgKd7mFo+VHEGWy2LBBNDD0C0nSYVjA@mail.gmail.com","threadId":"47349","inReplyTo":"20171129201154.192379-1-jonathantanmy@google.com","subject":"Re: [PATCH on en/rename-progress v2] diffcore-rename: make diff-tree -l0 mean -l","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2017-11-29T23:40:53Z","receivedAt":"2017-11-29T23:40:59Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Wed, Nov 29, 2017 at 12:11 PM, Jonathan Tan wrote:\n> In the documentation of diff-tree, it is stated that the -l option\n> \"prevents rename/copy detection from running if the number of\n> rename/copy targets exceeds the specified number\". The documentation\n> does not mention any special handling for the number 0, but the\n> implementation before commit 9f7e4bfa3b (\"diff: remove silent clamp of\n> renameLimit\", 2017-11-13) treated 0 as a special value indicating that\n> the rename limit is to be a very large number instead.\n>\n> The commit 9f7e4bfa3b changed that behavior, treating 0 as 0. Revert\n> this behavior to what it was previously. This allows existing scripts\n> and tools that use \"-l0\" to continue working. The alternative (to have\n> \"-l0\" suppress rename detection) is probably much less useful, since\n> users can just refrain from specifying -M and/or -C to have the same\n> effect.\n>\n> Signed-off-by: Jonathan Tan \n> ---\n> There are multiple issues with the commit message, so I am sending this\n> new version out.\n>\n> v2 is exactly the same as previously, except that the commit message is\n> changed following Elijah Newren's and Jonathan Nieder's comments.\n> ---\n> diffcore-rename.c | 2 ++\n> t/t4001-diff-rename.sh | 15 +++++++++++++++\n> 2 files changed, 17 insertions(+)\n\nReviewed-by: Elijah Newren \n\nThanks.\n"}]}