{"thread":{"id":"54928","subject":"[RFC PATCH] gitk: Activate --find-copies-harder","startedAt":"2021-01-04T19:55:41Z","lastAt":"2021-01-11T21:01:25Z","messageCount":7,"participants":["Robert Pollak","Laszlo Ersek","Pratyush Yadav","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"413385","messageId":"b12574f0-3ebc-95c0-9def-555150257e46@posteo.net","threadId":"54928","inReplyTo":null,"subject":"[RFC PATCH] gitk: Activate --find-copies-harder","fromName":"Robert Pollak","fromEmail":"robert.pollak@posteo.net","sentAt":"2021-01-04T19:54:36Z","receivedAt":"2021-01-04T19:55:41Z","isPatch":true,"sender":{"key":"robert.pollak@posteo.net","avatar":null},"body":"Naively forward the diff arguments to make --find-copies-harder work.\n\nSigned-off-by: Robert Pollak <robert.pollak@posteo.net>\n---\nDear Paul Mackerras,\n\nThis patch is an obviously naive attempt to make gitk observe\n--find-copies-harder. I am a gitk user with many small projects, so I am\ncurrently using this patch to get better diffs.\n\nWith this, gitk displays the copy in the second commit of my test\nrepository [1] as desired:\n\nsimilarity index 100%\ncopy from a\ncopy to b\n\n\nI see the following problems with my patch:\n\n1) It is totally untested with all the other args that are collected in\ndiffargs, like e.g. \"-O<orderfile>\", since I didn't need them yet.\n\n2) Even if --find-copies-harder is the only diff argument used, there\nmight be unintended side effects, since the modified procedure 'diffcmd'\nis called in several places. I did not systematically test all these\ncode paths.\n\n\nTo deal with 1), I could rename the variable 'vdflags' to\n'vdflags_ignored' and continue using 'vdflags' only for\n--find-copies-harder. Later, other flags could be moved over, after\ntheir harmlessness has been proven. Would this be good?\n\nAd 2), maybe someone with more code knowledge can tell whether this\na real risk? Also, would it be preferable to add the new flag(s) only to\nthe arguments of the diffcmd call in 'getblobdiffs'?\n(as in:\ndiff --git a/gitk b/gitk\nindex 23d9dd1..da6b372 100755\n--- a/gitk\n+++ b/gitk\n@@ -8017,7 +8017,7 @@ proc initblobdiffvars {} {\n proc getblobdiffs {ids} {\n     global blobdifffd diffids env\n     global treediffs\n-    global diffcontext\n+    global vdflags diffcontext\n     global ignorespace\n     global worddiff\n     global limitdiffs vfilelimit curview\n@@ -8031,7 +8031,7 @@ proc getblobdiffs {ids} {\n     if {[package vcompare $git_version \"1.6.6\"] >= 0} {\n         set submodule \"--submodule\"\n     }\n-    set cmd [diffcmd $ids \"-p $textconv $submodule  -C --cc --no-commit-id -U$diffcontext\"]\n+    set cmd [diffcmd $ids \"$vdflags($curview) -p $textconv $submodule  -C --cc --no-commit-id -U$diffcontext\"]\n     if {$ignorespace} {\n         append cmd \" -w\"\n     }\n)\nFor my test case, this also works.\n\n\nI'd be happy to prepare an updated patch incorporating your feedback.\n\nHaving this functionality in gitk will hopefully make some people stop\ncrafting their git history for copy detection, like described e.g. in\nhttps://devblogs.microsoft.com/oldnewthing/20190919-00/?p=102904 .\n\nI am CCing Laszlo Ersek, because he expressed interest in a similar\ntopic a year ago:\n'gitk feature requests: (1) \"diff.orderFile\" and (2) \"--function-context\"',\nhttps://public-inbox.org/git/d972c1f1-c49a-f644-ab1c-6a3e26c43ee3@redhat.com/\n.\n\n-- Robert\n\n[1] My minimal test case:\n> git init\n> echo \"a file\" > a\n> git add a\n> git commit -m \"a file\"\n> cp a b\n> git add b\n> git commit -m \"a copy\"\n\n\n gitk | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/gitk b/gitk\nindex 23d9dd1..eb6ba9a 100755\n--- a/gitk\n+++ b/gitk\n@@ -7869,7 +7869,7 @@ proc addtocflist {ids} {\n }\n  proc diffcmd {ids flags} {\n-    global log_showroot nullid nullid2 git_version\n+    global log_showroot nullid nullid2 git_version vdflags curview\n      set i [lsearch -exact $ids $nullid]\n     set j [lsearch -exact $ids $nullid2]\n@@ -7909,7 +7909,7 @@ proc diffcmd {ids flags} {\n         if {$log_showroot} {\n             lappend flags --root\n         }\n-        set cmd [concat | git diff-tree -r $flags $ids]\n+        set cmd [concat | git diff-tree -r $vdflags($curview) $flags $ids]\n     }\n     return $cmd\n }\n-- \n2.17.1\n\n"},{"id":"413586","messageId":"46693c60-98ee-b6c9-df8e-12216622ddf9@redhat.com","threadId":"54928","inReplyTo":"b12574f0-3ebc-95c0-9def-555150257e46@posteo.net","subject":"Re: [RFC PATCH] gitk: Activate --find-copies-harder","fromName":"Laszlo Ersek","fromEmail":"lersek@redhat.com","sentAt":"2021-01-06T15:58:17Z","receivedAt":"2021-01-06T15:59:53Z","isPatch":true,"sender":{"key":"lersek@redhat.com","avatar":null},"body":"On 01/04/21 20:54, Robert Pollak wrote:\n> Naively forward the diff arguments to make --find-copies-harder work.\n> \n> Signed-off-by: Robert Pollak <robert.pollak@posteo.net>\n> ---\n> Dear Paul Mackerras,\n> \n> This patch is an obviously naive attempt to make gitk observe\n> --find-copies-harder. I am a gitk user with many small projects, so I am\n> currently using this patch to get better diffs.\n> \n> With this, gitk displays the copy in the second commit of my test\n> repository [1] as desired:\n> \n> similarity index 100%\n> copy from a\n> copy to b\n> \n> \n> I see the following problems with my patch:\n> \n> 1) It is totally untested with all the other args that are collected in\n> diffargs, like e.g. \"-O<orderfile>\", since I didn't need them yet.\n\nIt would be really great if gitk supported both \"-O<orderfile>\" and\n--find-copies-harder!\n\nI can't offer a review, but I very much support the use case.\n\nThanks,\nLaszlo\n\n> \n> 2) Even if --find-copies-harder is the only diff argument used, there\n> might be unintended side effects, since the modified procedure 'diffcmd'\n> is called in several places. I did not systematically test all these\n> code paths.\n> \n> \n> To deal with 1), I could rename the variable 'vdflags' to\n> 'vdflags_ignored' and continue using 'vdflags' only for\n> --find-copies-harder. Later, other flags could be moved over, after\n> their harmlessness has been proven. Would this be good?\n> \n> Ad 2), maybe someone with more code knowledge can tell whether this\n> a real risk? Also, would it be preferable to add the new flag(s) only to\n> the arguments of the diffcmd call in 'getblobdiffs'?\n> (as in:\n> diff --git a/gitk b/gitk\n> index 23d9dd1..da6b372 100755\n> --- a/gitk\n> +++ b/gitk\n> @@ -8017,7 +8017,7 @@ proc initblobdiffvars {} {\n>  proc getblobdiffs {ids} {\n>      global blobdifffd diffids env\n>      global treediffs\n> -    global diffcontext\n> +    global vdflags diffcontext\n>      global ignorespace\n>      global worddiff\n>      global limitdiffs vfilelimit curview\n> @@ -8031,7 +8031,7 @@ proc getblobdiffs {ids} {\n>      if {[package vcompare $git_version \"1.6.6\"] >= 0} {\n>          set submodule \"--submodule\"\n>      }\n> -    set cmd [diffcmd $ids \"-p $textconv $submodule  -C --cc --no-commit-id -U$diffcontext\"]\n> +    set cmd [diffcmd $ids \"$vdflags($curview) -p $textconv $submodule  -C --cc --no-commit-id -U$diffcontext\"]\n>      if {$ignorespace} {\n>          append cmd \" -w\"\n>      }\n> )\n> For my test case, this also works.\n> \n> \n> I'd be happy to prepare an updated patch incorporating your feedback.\n> \n> Having this functionality in gitk will hopefully make some people stop\n> crafting their git history for copy detection, like described e.g. in\n> https://devblogs.microsoft.com/oldnewthing/20190919-00/?p=102904 .\n> \n> I am CCing Laszlo Ersek, because he expressed interest in a similar\n> topic a year ago:\n> 'gitk feature requests: (1) \"diff.orderFile\" and (2) \"--function-context\"',\n> https://public-inbox.org/git/d972c1f1-c49a-f644-ab1c-6a3e26c43ee3@redhat.com/\n> .\n> \n> -- Robert\n> \n> [1] My minimal test case:\n>> git init\n>> echo \"a file\" > a\n>> git add a\n>> git commit -m \"a file\"\n>> cp a b\n>> git add b\n>> git commit -m \"a copy\"\n> \n> \n>  gitk | 4 ++--\n>  1 file changed, 2 insertions(+), 2 deletions(-)\n> \n> diff --git a/gitk b/gitk\n> index 23d9dd1..eb6ba9a 100755\n> --- a/gitk\n> +++ b/gitk\n> @@ -7869,7 +7869,7 @@ proc addtocflist {ids} {\n>  }\n>   proc diffcmd {ids flags} {\n> -    global log_showroot nullid nullid2 git_version\n> +    global log_showroot nullid nullid2 git_version vdflags curview\n>       set i [lsearch -exact $ids $nullid]\n>      set j [lsearch -exact $ids $nullid2]\n> @@ -7909,7 +7909,7 @@ proc diffcmd {ids flags} {\n>          if {$log_showroot} {\n>              lappend flags --root\n>          }\n> -        set cmd [concat | git diff-tree -r $flags $ids]\n> +        set cmd [concat | git diff-tree -r $vdflags($curview) $flags $ids]\n>      }\n>      return $cmd\n>  }\n> \n\n"},{"id":"413965","messageId":"b0c0a630-25a9-8ef8-2270-fa40404c2897@posteo.net","threadId":"54928","inReplyTo":"46693c60-98ee-b6c9-df8e-12216622ddf9@redhat.com","subject":"Re: [RFC PATCH] gitk: Activate --find-copies-harder","fromName":"Robert Pollak","fromEmail":"robert.pollak@posteo.net","sentAt":"2021-01-10T12:59:00Z","receivedAt":"2021-01-10T13:00:16Z","isPatch":true,"sender":{"key":"robert.pollak@posteo.net","avatar":null},"body":"On 2021-01-06 16:58, Laszlo Ersek wrote:\n> On 01/04/21 20:54, Robert Pollak wrote:\n[...]\n>> I see the following problems with my patch:\n>>\n>> 1) It is totally untested with all the other args that are collected in\n>> diffargs, like e.g. \"-O<orderfile>\", since I didn't need them yet.\n> \n> It would be really great if gitk supported both \"-O<orderfile>\" and\n> --find-copies-harder!\n\nCan you please test these options with my patch and report back?\n\n-- Robert\n"},{"id":"414023","messageId":"23445a9c-46b2-0232-845b-c8d5fe36d506@redhat.com","threadId":"54928","inReplyTo":"b0c0a630-25a9-8ef8-2270-fa40404c2897@posteo.net","subject":"Re: [RFC PATCH] gitk: Activate --find-copies-harder","fromName":"Laszlo Ersek","fromEmail":"lersek@redhat.com","sentAt":"2021-01-11T09:21:25Z","receivedAt":"2021-01-11T09:23:14Z","isPatch":true,"sender":{"key":"lersek@redhat.com","avatar":null},"body":"On 01/10/21 13:59, Robert Pollak wrote:\n> On 2021-01-06 16:58, Laszlo Ersek wrote:\n>> On 01/04/21 20:54, Robert Pollak wrote:\n> [...]\n>>> I see the following problems with my patch:\n>>>\n>>> 1) It is totally untested with all the other args that are collected in\n>>> diffargs, like e.g. \"-O<orderfile>\", since I didn't need them yet.\n>>\n>> It would be really great if gitk supported both \"-O<orderfile>\" and\n>> --find-copies-harder!\n>\n> Can you please test these options with my patch and report back?\n>\n> -- Robert\n>\n\nThe patch doesn't apply with git-am (I'm trying on top of 72c4083ddf91):\n\n> Applying: gitk: Activate --find-copies-harder\n> error: corrupt patch at line 100\n> Patch failed at 0001 gitk: Activate --find-copies-harder\n> hint: Use 'git am --show-current-patch' to see the failed patch\n> When you have resolved this problem, run \"git am --continue\".\n> If you prefer to skip this patch, run \"git am --skip\" instead.\n> To restore the original branch and stop patching, run \"git am --abort\".\n\nOne problem could be the embedded diff in the notes section (I guess it\ncould confuse git-am).\n\nAlso, \"gitk\" has existed at \"gitk-git/gitk\" since commit 62ba5143ec2a\n(\"Move gitk to its own subdirectory\", 2007-11-18), so the pathname\nheaders in the patch look wrong.\n\nThanks\nLaszlo\n\n"},{"id":"414041","messageId":"87mtxfwt3e.fsf@yadavpratyush.com","threadId":"54928","inReplyTo":"23445a9c-46b2-0232-845b-c8d5fe36d506@redhat.com","subject":"Re: [RFC PATCH] gitk: Activate --find-copies-harder","fromName":"Pratyush Yadav","fromEmail":"me@yadavpratyush.com","sentAt":"2021-01-11T12:33:57Z","receivedAt":"2021-01-11T12:34:43Z","isPatch":true,"sender":{"key":"me@yadavpratyush.com","avatar":"https://avatars.githubusercontent.com/u/8817931?v=4"},"body":"On Mon, Jan 11 2021, Laszlo Ersek wrote:\n\n> On 01/10/21 13:59, Robert Pollak wrote:\n>> On 2021-01-06 16:58, Laszlo Ersek wrote:\n>>> On 01/04/21 20:54, Robert Pollak wrote:\n>> [...]\n>>>> I see the following problems with my patch:\n>>>>\n>>>> 1) It is totally untested with all the other args that are collected in\n>>>> diffargs, like e.g. \"-O<orderfile>\", since I didn't need them yet.\n>>>\n>>> It would be really great if gitk supported both \"-O<orderfile>\" and\n>>> --find-copies-harder!\n>>\n>> Can you please test these options with my patch and report back?\n>>\n>> -- Robert\n>>\n>\n> The patch doesn't apply with git-am (I'm trying on top of 72c4083ddf91):\n>\n>> Applying: gitk: Activate --find-copies-harder\n>> error: corrupt patch at line 100\n>> Patch failed at 0001 gitk: Activate --find-copies-harder\n>> hint: Use 'git am --show-current-patch' to see the failed patch\n>> When you have resolved this problem, run \"git am --continue\".\n>> If you prefer to skip this patch, run \"git am --skip\" instead.\n>> To restore the original branch and stop patching, run \"git am --abort\".\n>\n> One problem could be the embedded diff in the notes section (I guess it\n> could confuse git-am).\n>\n> Also, \"gitk\" has existed at \"gitk-git/gitk\" since commit 62ba5143ec2a\n> (\"Move gitk to its own subdirectory\", 2007-11-18), so the pathname\n> headers in the patch look wrong.\n\ngitk is maintained as a separate repo by Paul Mackerras at\ngit://ozlabs.org/~paulus/gitk, and then is pulled into the main Git repo\nfrom time to time using a subtree merge. That's how gitk changes end up\nin gitk-git/. Patches for gitk should be based on the gitk repo to make\nit easier for Paul to apply them. In short, the paths are fine.\n\n> Thanks\n> Laszlo\n>\n\n-- \nRegards,\nPratyush Yadav\n"},{"id":"414059","messageId":"d1f1691b-2fdc-1e6e-b346-31afb6aa5532@redhat.com","threadId":"54928","inReplyTo":"87mtxfwt3e.fsf@yadavpratyush.com","subject":"Re: [RFC PATCH] gitk: Activate --find-copies-harder","fromName":"Laszlo Ersek","fromEmail":"lersek@redhat.com","sentAt":"2021-01-11T16:28:24Z","receivedAt":"2021-01-11T16:29:58Z","isPatch":true,"sender":{"key":"lersek@redhat.com","avatar":null},"body":"On 01/11/21 13:33, Pratyush Yadav wrote:\n> On Mon, Jan 11 2021, Laszlo Ersek wrote:\n> \n>> On 01/10/21 13:59, Robert Pollak wrote:\n>>> On 2021-01-06 16:58, Laszlo Ersek wrote:\n>>>> On 01/04/21 20:54, Robert Pollak wrote:\n>>> [...]\n>>>>> I see the following problems with my patch:\n>>>>>\n>>>>> 1) It is totally untested with all the other args that are collected in\n>>>>> diffargs, like e.g. \"-O<orderfile>\", since I didn't need them yet.\n>>>>\n>>>> It would be really great if gitk supported both \"-O<orderfile>\" and\n>>>> --find-copies-harder!\n>>>\n>>> Can you please test these options with my patch and report back?\n>>>\n>>> -- Robert\n>>>\n>>\n>> The patch doesn't apply with git-am (I'm trying on top of 72c4083ddf91):\n>>\n>>> Applying: gitk: Activate --find-copies-harder\n>>> error: corrupt patch at line 100\n>>> Patch failed at 0001 gitk: Activate --find-copies-harder\n>>> hint: Use 'git am --show-current-patch' to see the failed patch\n>>> When you have resolved this problem, run \"git am --continue\".\n>>> If you prefer to skip this patch, run \"git am --skip\" instead.\n>>> To restore the original branch and stop patching, run \"git am --abort\".\n>>\n>> One problem could be the embedded diff in the notes section (I guess it\n>> could confuse git-am).\n>>\n>> Also, \"gitk\" has existed at \"gitk-git/gitk\" since commit 62ba5143ec2a\n>> (\"Move gitk to its own subdirectory\", 2007-11-18), so the pathname\n>> headers in the patch look wrong.\n> \n> gitk is maintained as a separate repo by Paul Mackerras at\n> git://ozlabs.org/~paulus/gitk, and then is pulled into the main Git repo\n> from time to time using a subtree merge. That's how gitk changes end up\n> in gitk-git/. Patches for gitk should be based on the gitk repo to make\n> it easier for Paul to apply them.\n\nThank you for explaining!\n\n(I guess I'm more familiar with submodules than the subtree merge\nstrategy in general; I've had to look up the latter on the web now.)\n\n> In short, the paths are fine.\n\nI tried applying the patch again, now on top of 6cd80496e9d3 (\"gitk:\nResize panes correctly when reducing window size\", 2020-10-03) -- it\nturns out I'd actually had a clone of the stand-alone gitk repo you\nmention, I just needed to pull.\n\nBut: git-am fails the same way.\n\nThanks\nLaszlo\n\n"},{"id":"414076","messageId":"xmqqmtxf6vfd.fsf@gitster.c.googlers.com","threadId":"54928","inReplyTo":"d1f1691b-2fdc-1e6e-b346-31afb6aa5532@redhat.com","subject":"Re: [RFC PATCH] gitk: Activate --find-copies-harder","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-01-11T21:00:22Z","receivedAt":"2021-01-11T21:01:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Laszlo Ersek <lersek@redhat.com> writes:\n\n> On 01/11/21 13:33, Pratyush Yadav wrote:\n>> On Mon, Jan 11 2021, Laszlo Ersek wrote:\n>> \n>>> On 01/10/21 13:59, Robert Pollak wrote:\n>>>> On 2021-01-06 16:58, Laszlo Ersek wrote:\n>>>>> On 01/04/21 20:54, Robert Pollak wrote:\n>>>> [...]\n>>>>>> I see the following problems with my patch:\n>>>>>>\n>>>>>> 1) It is totally untested with all the other args that are collected in\n>>>>>> diffargs, like e.g. \"-O<orderfile>\", since I didn't need them yet.\n>>>>>\n>>>>> It would be really great if gitk supported both \"-O<orderfile>\" and\n>>>>> --find-copies-harder!\n>>>>\n>>>> Can you please test these options with my patch and report back?\n>>>>\n>>>> -- Robert\n>>>>\n>>>\n>>> The patch doesn't apply with git-am (I'm trying on top of 72c4083ddf91):\n>>>\n>>>> Applying: gitk: Activate --find-copies-harder\n>>>> error: corrupt patch at line 100\n>>>> Patch failed at 0001 gitk: Activate --find-copies-harder\n>>>> hint: Use 'git am --show-current-patch' to see the failed patch\n>>>> When you have resolved this problem, run \"git am --continue\".\n>>>> If you prefer to skip this patch, run \"git am --skip\" instead.\n>>>> To restore the original branch and stop patching, run \"git am --abort\".\n>>>\n>>> One problem could be the embedded diff in the notes section (I guess it\n>>> could confuse git-am).\n>>>\n>>> Also, \"gitk\" has existed at \"gitk-git/gitk\" since commit 62ba5143ec2a\n>>> (\"Move gitk to its own subdirectory\", 2007-11-18), so the pathname\n>>> headers in the patch look wrong.\n>> \n>> gitk is maintained as a separate repo by Paul Mackerras at\n>> git://ozlabs.org/~paulus/gitk, and then is pulled into the main Git repo\n>> from time to time using a subtree merge. That's how gitk changes end up\n>> in gitk-git/. Patches for gitk should be based on the gitk repo to make\n>> it easier for Paul to apply them.\n>\n> Thank you for explaining!\n>\n> (I guess I'm more familiar with submodules than the subtree merge\n> strategy in general; I've had to look up the latter on the web now.)\n>\n>> In short, the paths are fine.\n>\n> I tried applying the patch again, now on top of 6cd80496e9d3 (\"gitk:\n> Resize panes correctly when reducing window size\", 2020-10-03) -- it\n> turns out I'd actually had a clone of the stand-alone gitk repo you\n> mention, I just needed to pull.\n>\n> But: git-am fails the same way.\n\nThat is because it has a patch-looking material that is not to be\napplied before the real patch.  It is customery to indent such \"diff\"\nthat is meant to be read by humans as part of reading log message\nand justification of the patch, not the patch itself.\n"}]}