Re: [RFC PATCH] gitk: Activate --find-copies-harder

[Laszlo Ersek <lersek@redhat.com>]

On 01/04/21 20:54, Robert Pollak wrote:
> Naively forward the diff arguments to make --find-copies-harder work.
> 
> Signed-off-by: Robert Pollak <robert.pollak@posteo.net>
> ---
> Dear Paul Mackerras,
> 
> This patch is an obviously naive attempt to make gitk observe
> --find-copies-harder. I am a gitk user with many small projects, so I am
> currently using this patch to get better diffs.
> 
> With this, gitk displays the copy in the second commit of my test
> repository [1] as desired:
> 
> similarity index 100%
> copy from a
> copy to b
> 
> 
> I see the following problems with my patch:
> 
> 1) It is totally untested with all the other args that are collected in
> diffargs, like e.g. "-O<orderfile>", since I didn't need them yet.

It would be really great if gitk supported both "-O<orderfile>" and
--find-copies-harder!

I can't offer a review, but I very much support the use case.

Thanks,
Laszlo

> 
> 2) Even if --find-copies-harder is the only diff argument used, there
> might be unintended side effects, since the modified procedure 'diffcmd'
> is called in several places. I did not systematically test all these
> code paths.
> 
> 
> To deal with 1), I could rename the variable 'vdflags' to
> 'vdflags_ignored' and continue using 'vdflags' only for
> --find-copies-harder. Later, other flags could be moved over, after
> their harmlessness has been proven. Would this be good?
> 
> Ad 2), maybe someone with more code knowledge can tell whether this
> a real risk? Also, would it be preferable to add the new flag(s) only to
> the arguments of the diffcmd call in 'getblobdiffs'?
> (as in:
> diff --git a/gitk b/gitk
> index 23d9dd1..da6b372 100755
> --- a/gitk
> +++ b/gitk
> @@ -8017,7 +8017,7 @@ proc initblobdiffvars {} {
>  proc getblobdiffs {ids} {
>      global blobdifffd diffids env
>      global treediffs
> -    global diffcontext
> +    global vdflags diffcontext
>      global ignorespace
>      global worddiff
>      global limitdiffs vfilelimit curview
> @@ -8031,7 +8031,7 @@ proc getblobdiffs {ids} {
>      if {[package vcompare $git_version "1.6.6"] >= 0} {
>          set submodule "--submodule"
>      }
> -    set cmd [diffcmd $ids "-p $textconv $submodule  -C --cc --no-commit-id -U$diffcontext"]
> +    set cmd [diffcmd $ids "$vdflags($curview) -p $textconv $submodule  -C --cc --no-commit-id -U$diffcontext"]
>      if {$ignorespace} {
>          append cmd " -w"
>      }
> )
> For my test case, this also works.
> 
> 
> I'd be happy to prepare an updated patch incorporating your feedback.
> 
> Having this functionality in gitk will hopefully make some people stop
> crafting their git history for copy detection, like described e.g. in
> https://devblogs.microsoft.com/oldnewthing/20190919-00/?p=102904 .
> 
> I am CCing Laszlo Ersek, because he expressed interest in a similar
> topic a year ago:
> 'gitk feature requests: (1) "diff.orderFile" and (2) "--function-context"',
> https://public-inbox.org/git/d972c1f1-c49a-f644-ab1c-6a3e26c43ee3@redhat.com/
> .
> 
> -- Robert
> 
> [1] My minimal test case:
>> git init
>> echo "a file" > a
>> git add a
>> git commit -m "a file"
>> cp a b
>> git add b
>> git commit -m "a copy"
> 
> 
>  gitk | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/gitk b/gitk
> index 23d9dd1..eb6ba9a 100755
> --- a/gitk
> +++ b/gitk
> @@ -7869,7 +7869,7 @@ proc addtocflist {ids} {
>  }
>   proc diffcmd {ids flags} {
> -    global log_showroot nullid nullid2 git_version
> +    global log_showroot nullid nullid2 git_version vdflags curview
>       set i [lsearch -exact $ids $nullid]
>      set j [lsearch -exact $ids $nullid2]
> @@ -7909,7 +7909,7 @@ proc diffcmd {ids flags} {
>          if {$log_showroot} {
>              lappend flags --root
>          }
> -        set cmd [concat | git diff-tree -r $flags $ids]
> +        set cmd [concat | git diff-tree -r $vdflags($curview) $flags $ids]
>      }
>      return $cmd
>  }
>