> -----Ursprüngliche Nachricht-----
> Von: Johannes Sixt <j6t@xxxxxxxx>
> Gesendet: Dienstag, 6. Mai 2025 21:39
> An: ToBoMi via GitGitGadget <gitgitgadget@xxxxxxxxx>; git@xxxxxxxxxxxxxxx
> Cc: Boesch, Tobias <tobias.boesch@xxxxxxxxx>
> 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@xxxxxxxxx>
> >
> > 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@xxxxxxxxx>
>
> 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?
I believe this could go wrong, because I think one can scroll after selecting
the file and then right click on it or even another file to execute the external diff.
Then the filenames are no longer at the top top of the text field.
I got around this by finding a git command that delivers the full paths of the original
and the renamed file. Since git commands are executed in the codebase already I
hope this is okay and maybe this is even more efficient than parsing the ctext.
>
> Would it make sense to support also copied files?
I don't know how why this is a benefit and I believe that git currently does not handle
copied files the way it does with renamed or moved files. That's why I would like to skip this.
>
> > 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?
True. Now that a git command is used that emits the full path of the files
This regex is no longer necessary and is removed.
>
> > + 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'.
done
>
> Please avoid the extra set of parentheses, even around && (below). Also, in
> this code base, we do not have spaces around the condition inside {}.
done
>
> > + 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...
done
>
> > 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
-------------------------------------------------------------------------------------------------
imperial-Werke oHG, Sitz Bünde, Registergericht Bad Oeynhausen - HRA 4825
