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@xxxxxxxxxx>
> ---
> 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@xxxxxxxxxx/
> .
>
> -- 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
> }
>
