AW: [PATCH v4] gitk: add external diff file rename detection

> -----Ursprüngliche Nachricht-----
> Von: Johannes Sixt <j6t@kdbg.org>
> Gesendet: Dienstag, 6. Mai 2025 21:39
> An: ToBoMi via GitGitGadget <gitgitgadget@gmail.com>; git@vger.kernel.org
> Cc: Boesch, Tobias <tobias.boesch@miele.com>
> Betreff: Re: [PATCH v4] gitk: add external diff file rename detection
>
> Am 28.04.25 um 10:47 schrieb ToBoMi via GitGitGadget:
> > From: Tobias Boesch <tobias.boesch@miele.com>
> >
> > If a file is renamed between commits and an external diff is started
> > through gitk on the original or the renamed file name, gitk is unable
> > to open the renamed file in the external diff editor.
> > It fails to fetch the renamed file from git, because it fetches it
> > using its original path in contrast to using the renamed path of the
> > file.
> > Detect the rename and open the external diff with the original and the
> > renamed file instead of no file (fetch the renamed file path and name
> > from git) no matter if the original or the renamed file is selected in
> > gitk.
> > Since moved or renamed file are handled the same way do this also for
> > moved files.
> >
> > Signed-off-by: Tobias Boesch <tobias.boesch@miele.com>
>
> Thank you. Sorry for taking so long to respond.
>
> In general, I like the goal of this patch.
>
> I am not familar, yet, how renamed files are represented in Gitk.
>
> I wonder whether it is necessary to parse diff text to find renamed file names.
> When you click on a renamed file in the file list, the diff panel jumps to the
> corresponding text for both the original file name and the renamed file name.
> Is the information about those two names not already available?

>
> >  gitk-git/gitk | 45 +++++++++++++++++++++++++++++++++++++++++++--
> >  1 file changed, 43 insertions(+), 2 deletions(-)
> >
> > diff --git a/gitk-git/gitk b/gitk-git/gitk index
> > bc9efa18566..ddbe60398f2 100755
> > --- a/gitk-git/gitk
> > +++ b/gitk-git/gitk
> > @@ -3806,6 +3806,39 @@ proc external_diff_get_one_file {diffid filename
> diffdir} {
> >                 "revision $diffid"]
> >  }
> >
> > +proc check_for_renames_in_diff {filepath} {
> > +    global ctext
> > +
> > +    set renamed_filenames [list {}]
> > +    set filename [file tail $filepath]
> > +    set rename_from_text_identifier_length 12
> > +    set rename_to_text_identifier_length 10
> > +    set reg_expr_rename_from {^rename from (.*$filename)}
>
> $filename can certainly have characters that are special for a regular
> expression, such as the fullstop, right? They need to be escaped or this will
> find the wrong file if one at all.
>
> If this search wants to find one side of the rename, why does it ignore the
> directories?

>
> > +    set reg_expr_rename_from [subst -nobackslashes -nocommands
> $reg_expr_rename_from]
> > +    set rename_from_text_index [$ctext search -elide -regexp --
> $reg_expr_rename_from 0.0]
> > +    if { ($rename_from_text_index != {})} {
>
> Here and elsewhere in this patch we have a string comparison that uses '!='. It
> should use 'ne'.

>
> > +        set reg_expr_rename_to {^rename to (.*)}
> > +        set rename_to_text_index [$ctext search -elide -regexp --
> $reg_expr_rename_to $rename_from_text_index]
> > +        if { ($rename_from_text_index != {}) && ($rename_to_text_index != {})
> } {
> > +            lappend renamed_filenames [$ctext get "$rename_from_text_index
> + $rename_from_text_identifier_length chars" "$rename_from_text_index
> lineend"]
> > +            lappend renamed_filenames [$ctext get "$rename_to_text_index +
> $rename_to_text_identifier_length chars" "$rename_to_text_index lineend"]
> > +        }
> > +        return $renamed_filenames
> > +    }
> > +    set reg_expr_rename_to {^rename to (.*$filename)}
> > +    set reg_expr_rename_to [subst -nobackslashes -nocommands
> $reg_expr_rename_to]
> > +    set rename_to_text_index [$ctext search -elide -regexp --
> $reg_expr_rename_to 0.0]
> > +    if { ($rename_to_text_index != {})} {
> > +        set reg_expr_rename_from {^rename from (.*)}
> > +        set rename_from_text_index [$ctext search -backwards -elide -regexp -
> - $reg_expr_rename_from $rename_to_text_index]
> > +        if { ($rename_to_text_index != {}) && ($rename_from_text_index != {})
> } {
> > +            lappend renamed_filenames [$ctext get "$rename_from_text_index
> + $rename_from_text_identifier_length chars" "$rename_from_text_index
> lineend"]
> > +            lappend renamed_filenames [$ctext get "$rename_to_text_index +
> $rename_to_text_identifier_length chars" "$rename_to_text_index lineend"]
> > +        }
> > +        return $renamed_filenames
> > +    }
> > +}
> > +
>
> Can we please have shorter variable names? They are all local variables.
> I have to spend so mucht time to find the end of the variable names before I
> can understand what the lines do...

>
> >  proc external_diff {} {
> >      global nullid nullid2
> >      global flist_menu_file
> > @@ -3836,8 +3869,16 @@ proc external_diff {} {
> >      if {$diffdir eq {}} return
> >
> >      # gather files to diff
> > -    set difffromfile [external_diff_get_one_file $diffidfrom $flist_menu_file
> $diffdir]
> > -    set difftofile [external_diff_get_one_file $diffidto $flist_menu_file $diffdir]
> > +    set renamed_filenames [check_for_renames_in_diff $flist_menu_file]
> > +    set rename_from_filename [lindex $renamed_filenames 1]
> > +    set rename_to_filename [lindex $renamed_filenames 2]
> > +    if { ($rename_from_filename != {}) && ($rename_to_filename != {}) } {
> > +        set difffromfile [external_diff_get_one_file $diffidfrom
> $rename_from_filename $diffdir]
> > +        set difftofile [external_diff_get_one_file $diffidto $rename_to_filename
> $diffdir]
> > +    } else {
> > +        set difffromfile [external_diff_get_one_file $diffidfrom $flist_menu_file
> $diffdir]
> > +        set difftofile [external_diff_get_one_file $diffidto $flist_menu_file
> $diffdir]
> > +    }
> >
> >      if {$difffromfile ne {} && $difftofile ne {}} {
> >          set cmd [list [shellsplit $extdifftool] $difffromfile
> > $difftofile]
> >
> > base-commit: 5b97a56fa0e7d580dc8865b73107407c9b3f0eff
>
> -- Hannes