threads / patch / 21632

patchgitk: Use git-difftool for external diffs

Subject: [PATCH] gitk: Use git-difftool for external diffs

## tl;dr

13 messages between Nov 16, 2009 and Dec 31, 2009. Diffs are folded; open one to read it.

replies: 12people: 5as markdown or json

David Aguilar· Nov 16, 2009, 03:12 UTC · lore

This teaches gitk about git-difftool. A benefit of this change is that gitk's external diff now works with read-only repositories.

Signed-off-by: David Aguilar <davvid@gmail.com>
---
The diffstat alone makes me happy.
 gitk |   83 ++++--------------------------------------------------------------
 1 files changed, 5 insertions(+), 78 deletions(-)
Show changes to gitk +5 −78
diff --git a/gitk b/gitk
index db5ec54..881ef95 100755
--- a/gitk
+++ b/gitk
@@ -2520,7 +2520,7 @@ proc savestuff {w} {
     global viewname viewfiles viewargs viewargscmd viewperm nextviewnum
     global cmitmode wrapcomment datetimeformat limitdiffs
     global colors uicolor bgcolor fgcolor diffcolors diffcontext selectbgcolor
-    global autoselect extdifftool perfile_attrs markbgcolor
+    global autoselect perfile_attrs markbgcolor
     global hideremotes
 
     if {$stuffsaved} return
@@ -2554,7 +2554,6 @@ proc savestuff {w} {
 	puts $f [list set markbgcolor $markbgcolor]
 	puts $f [list set diffcontext $diffcontext]
 	puts $f [list set selectbgcolor $selectbgcolor]
-	puts $f [list set extdifftool $extdifftool]
 	puts $f [list set perfile_attrs $perfile_attrs]
 
 	puts $f "set geometry(main) [wm geometry .]"
@@ -3204,31 +3203,10 @@ proc save_file_from_commit {filename output what} {
     return $output
 }
 
-proc external_diff_get_one_file {diffid filename diffdir} {
-    global nullid nullid2 nullfile
-    global gitdir
-
-    if {$diffid == $nullid} {
-        set difffile [file join [file dirname $gitdir] $filename]
-	if {[file exists $difffile]} {
-	    return $difffile
-	}
-	return $nullfile
-    }
-    if {$diffid == $nullid2} {
-        set difffile [file join $diffdir "\[index\] [file tail $filename]"]
-        return [save_file_from_commit :$filename $difffile index]
-    }
-    set difffile [file join $diffdir "\[$diffid\] [file tail $filename]"]
-    return [save_file_from_commit $diffid:$filename $difffile \
-	       "revision $diffid"]
-}
-
 proc external_diff {} {
     global nullid nullid2
     global flist_menu_file
     global diffids
-    global extdifftool
 
     if {[llength $diffids] == 1} {
         # no reference commit given
@@ -3249,23 +3227,9 @@ proc external_diff {} {
         set diffidto [lindex $diffids 1]
     }
 
-    # make sure that several diffs wont collide
-    set diffdir [gitknewtmpdir]
-    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]
-
-    if {$difffromfile ne {} && $difftofile ne {}} {
-        set cmd [list [shellsplit $extdifftool] $difffromfile $difftofile]
-        if {[catch {set fl [open |$cmd r]} err]} {
-            file delete -force $diffdir
-            error_popup "$extdifftool: [mc "command failed:"] $err"
-        } else {
-            fconfigure $fl -blocking 0
-            filerun $fl [list delete_at_eof $fl $diffdir]
-        }
+    set cmd {git difftool --no-prompt $diffidfrom $diffidto -- $flist_menu_file}
+    if {[catch {eval exec $cmd &} err]} {
+        error_popup "[mc "git difftool: command failed:"] $err"
     }
 }
 
@@ -3557,19 +3521,6 @@ proc read_line_source {fd inst} {
     return 0
 }
 
-# delete $dir when we see eof on $f (presumably because the child has exited)
-proc delete_at_eof {f dir} {
-    while {[gets $f line] >= 0} {}
-    if {[eof $f]} {
-	if {[catch {close $f} err]} {
-	    error_popup "[mc "External diff viewer failed:"] $err"
-	}
-	file delete -force $dir
-	return 0
-    }
-    return 1
-}
-
 # Functions for adding and removing shell-type quoting
 
 proc shellquote {str} {
@@ -10446,7 +10397,7 @@ proc doprefs {} {
     global maxwidth maxgraphpct
     global oldprefs prefstop showneartags showlocalchanges
     global uicolor bgcolor fgcolor ctext diffcolors selectbgcolor markbgcolor
-    global tabstop limitdiffs autoselect extdifftool perfile_attrs
+    global tabstop limitdiffs autoselect perfile_attrs
     global hideremotes
 
     set top .gitkprefs
@@ -10498,15 +10449,6 @@ proc doprefs {} {
 	-font optionfont -variable perfile_attrs
     grid x $top.lattr -sticky w
 
-    entry $top.extdifft -textvariable extdifftool
-    frame $top.extdifff
-    label $top.extdifff.l -text [mc "External diff tool" ] -font optionfont \
-	-padx 10
-    button $top.extdifff.b -text [mc "Choose..."] -font optionfont \
-	-command choose_extdiff
-    pack $top.extdifff.l $top.extdifff.b -side left
-    grid x $top.extdifff $top.extdifft -sticky w
-
     label $top.cdisp -text [mc "Colors: press to choose"]
     grid $top.cdisp - -sticky w -pady 10
     label $top.ui -padx 40 -relief sunk -background $uicolor
@@ -10566,15 +10508,6 @@ proc doprefs {} {
     bind $top <Visibility> "focus $top.buts.ok"
 }
 
-proc choose_extdiff {} {
-    global extdifftool
-
-    set prog [tk_getOpenFile -title [mc "External diff tool"] -multiple false]
-    if {$prog ne {}} {
-	set extdifftool $prog
-    }
-}
-
 proc choosecolor {v vi w x cmd} {
     global $v
 
@@ -11100,12 +11033,6 @@ set datetimeformat "%Y-%m-%d %H:%M:%S"
 set autoselect 1
 set perfile_attrs 0
 
-if {[tk windowingsystem] eq "aqua"} {
-    set extdifftool "opendiff"
-} else {
-    set extdifftool "meld"
-}
-
 set colors {green red blue magenta darkgrey brown orange}
 set uicolor grey85
 set bgcolor white

-- 
1.6.5.2.180.gc5b3e
Paul Mackerras· Nov 19, 2009, 09:03 UTC · re: David Aguilar · lore

Re: [PATCH] gitk: Use git-difftool for external diffs

David Aguilar writes:
> This teaches gitk about git-difftool.  A benefit of this change is
> that gitk's external diff now works with read-only repositories.

What version of git does git difftool first appear in? I prefer not to introduce hard requirements on very recent versions of git into gitk.

Paul.
David Aguilar· Nov 19, 2009, 19:39 UTC · re: Paul Mackerras · lore

Re: [PATCH] gitk: Use git-difftool for external diffs

On Thu, Nov 19, 2009 at 08:03:31PM +1100, Paul Mackerras wrote:
Show 10 quoted lines
> David Aguilar writes:
> 
> > This teaches gitk about git-difftool.  A benefit of this change is
> > that gitk's external diff now works with read-only repositories.
> 
> What version of git does git difftool first appear in?  I prefer not
> to introduce hard requirements on very recent versions of git into
> gitk.
> 
> Paul.
git-difftool appeared in git 1.6.3.

If this patch is not going in then how do you suggest we fix the read-only repository bug?

My immediate thought is to harden the $TMPDIR patch so that the filenames used by gitk are much less predictable (I bailed on it once Peff noted the $TMPDIR vulnerabilities and used git-difftool instead since it is known to be safe).

Does hardening the $TMPDIR patch have a better chance of being accepted?

In defense of difftool: there are more benefits to using git-difftool than just read-only repositories. The current external diff code does not work if 'meld' is not installed whereas using git-difftool works by default in more environments. It also makes things match users' expectations when they have already gone through the trouble of configuring a diff or merge tool.

Is there a particular time frame in which such a patch could go in?

My primary concern is getting gitk to work with read-only repositories. git-difftool was just one way of getting there. If keeping backwards compatibility is a must-have then I can hold onto the git-difftool patch until some time in the future when it is more appropriate. Hopefully something along the lines of a hardened $TMPDIR patch can stand in for the final fix in the meantime.

What do you think?
-- 
		David
Paul Mackerras· Nov 19, 2009, 22:21 UTC · re: David Aguilar · lore

Re: [PATCH] gitk: Use git-difftool for external diffs

David Aguilar writes:
Show 16 quoted lines
> On Thu, Nov 19, 2009 at 08:03:31PM +1100, Paul Mackerras wrote:
> > David Aguilar writes:
> > 
> > > This teaches gitk about git-difftool.  A benefit of this change is
> > > that gitk's external diff now works with read-only repositories.
> > 
> > What version of git does git difftool first appear in?  I prefer not
> > to introduce hard requirements on very recent versions of git into
> > gitk.
> > 
> > Paul.
> 
> git-difftool appeared in git 1.6.3.
> 
> If this patch is not going in then how do you suggest we fix the
> read-only repository bug?

I have no problem with using git difftool if the underlying git is new enough, I just don't want gitk to explode when it isn't.

If the underlying git isn't new enough then we should probably make a directory under $TMPDIR with a reasonably unpredictable name.

Also, I don't think we should remove the ability for the user to choose which external diff tool to use; if we're using git difftool then we should pass the selected tool with the -t option of git difftool. Maybe we need a "use default from git config" option as well, though.

Paul.
Junio C Hamano· Nov 20, 2009, 07:53 UTC · re: Paul Mackerras · lore

Re: [PATCH] gitk: Use git-difftool for external diffs

Paul Mackerras <paulus@samba.org> writes:
> I have no problem with using git difftool if the underlying git is new
> enough, I just don't want gitk to explode when it isn't.

I somehow doubt there are many users who use pre 1.6.3 git but keep their gitk separately updated to very recent version, so personally I wouldn't worry too much about this.

> Also, I don't think we should remove the ability for the user to
> choose which external diff tool to use.

This is a larger concern. Does "difftool" allow use of an arbitrary tool like how gitk does (I have a suspicion that it is not as flexible)?

David Aguilar· Nov 20, 2009, 18:55 UTC · re: Junio C Hamano · lore

Re: [PATCH] gitk: Use git-difftool for external diffs

On Thu, Nov 19, 2009 at 11:53:37PM -0800, Junio C Hamano wrote:
Show 14 quoted lines
> Paul Mackerras <paulus@samba.org> writes:
> 
> > I have no problem with using git difftool if the underlying git is new
> > enough, I just don't want gitk to explode when it isn't.
> 
> I somehow doubt there are many users who use pre 1.6.3 git but keep their
> gitk separately updated to very recent version, so personally I wouldn't
> worry too much about this.
> 
> > Also, I don't think we should remove the ability for the user to
> > choose which external diff tool to use.
> 
> This is a larger concern.  Does "difftool" allow use of an arbitrary tool
> like how gitk does (I have a suspicion that it is not as flexible)?

difftool supports arbitrary tools through configuration. For arbitrary tools we set difftool.$name.cmd and diff.tool = $name.

If gitk were to have a difftool paradigm then it might be nice to have a text field mapped to the diff.tool variable. The patch didn't address the diff.tool configuration variable so that's a concern if we are to pursue difftool further.

The argument in favor of difftool is one of user expectations. From a user's POV it ~seems~ desirable for gitk to honor difftool configurations. The easiest way for gitk to do that is to use git-difftool and not worry about the details of how arbitrary tools are setup.

Later work could be done to provide UI for setting up arbitrary tools inside of gitk. My gut feeling is that gitk isn't a git config editor.

This is why the difftool patch took the minimalist approach. It trusts the user to have things configured to their liking and trusts git-difftool to do the right thing when no such configuration exists.

I think it's a question of whether this is something we'd want to change. It certainly doesn't bother me either way.

I might be the only person who'd notice such small details so my vote-in-code is with my v2 $TMPDIR patch until someone else thinks that using difftool inside of gitk is a good idea.

At which point I would probably pursue it with the minimalist approach first and then gradually add UI for the pertinent git config variables.

Just my .02,
-- 
		David
Junio C Hamano· Nov 20, 2009, 20:51 UTC · re: David Aguilar · lore

Re: [PATCH] gitk: Use git-difftool for external diffs

David Aguilar <davvid@gmail.com> writes:
Show 9 quoted lines
>> > Also, I don't think we should remove the ability for the user to
>> > choose which external diff tool to use.
>> 
>> This is a larger concern.  Does "difftool" allow use of an arbitrary tool
>> like how gitk does (I have a suspicion that it is not as flexible)?
>
> difftool supports arbitrary tools through configuration.
> For arbitrary tools we set difftool.$name.cmd and
> diff.tool = $name.

So it is not as flexible as just giving a command line template for one-shot invocation, but that is Ok.

Don't get me wrong. I think the longer term direction should be to reduce code from gitk without making lifes harder to existing users, and just like built-in "diff --cc" stripped the combined-diff implemented in Tcl from gitk, use of "difftool" would be a good way to unify the "diffing" experience for a user who uses both command line and gitk at the same time.

I do not read Tcl very well but I am guessing that in gitk you specify what tool to run (e.g. "frobanodiff -z"), gitk feeds you two temporary files on the filesystem to compare (e.g. "frobanodiff -z $tmp1 $tmp2"), and your command line is responsible for giving satisfying diff experience to the end user.

I see three possible approaches:
 * Teach "git-difftool" a mode to compare two arbitrary files on the
   filesytem, and set that as "External Diff" command that takes the
   filenames as extra two parameters, just like any other "External Diff"
   programs given to gitk does.  This is the least palatable, as it won't
   solve the read-only repository issue at all (it only allows you the
   logic to choose the configured difftool backend program).
 * Instead of disabling the traditional "External Diff" and taking it over
   like your patch did, add a new codepath for "Difftool" that feeds the
   commit IDs and paths the way git-difftool expects.  The user can use
   both, and the issue of read-only repository is solved when "Difftool"
   is used (but not "External Diff").
 * Take over "External Diff" codepath exactly like your patch did, but
   teach "git-difftool" a new command line option to name an unconfigured
   external program that takes two filenames.  When "External Diff"
   program is *not* configured in gitk, the command line to invoke
   difftool would be exactly as in your patch, i.e. "difftool --no-prompt
   $from $to -- $path".  Otherwise, when gitk is configured to use an
   external program, e.g. "frobanodiff -z", for "External Diff", you pass
   that command line to "git-difftool" via that new option, e.g.
     difftool --no-prompt --extcmd="frobanodiff -z" $from $to -- $path
   Then difftool is responsible for preparing the two necessary temporary
   files out of the given information ($from/$to/$path) and feeding them
   to "frobanodiff -z" command line.
   Maybe such --extcmd support already exists in difftool, in which case
   my earlier suspicion that difftool is not as flexible would be false.
I think the last one would be the way to go in the longer term.
David Aguilar· Nov 21, 2009, 21:47 UTC · re: Junio C Hamano · lore

Re: [PATCH] gitk: Use git-difftool for external diffs

On Nov 20, 2009, at 12:51 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 54 quoted lines
> I do not read Tcl very well but I am guessing that in gitk you specify
>>>
>>
> what tool to run (e.g. "frobanodiff -z"), gitk feeds you two temporary
> files on the filesystem to compare (e.g. "frobanodiff -z $tmp1  
> $tmp2"),
> and your command line is responsible for giving satisfying diff  
> experience
> to the end user.
>
> I see three possible approaches:
>
> * Teach "git-difftool" a mode to compare two arbitrary files on the
>   filesytem, and set that as "External Diff" command that takes the
>   filenames as extra two parameters, just like any other "External  
> Diff"
>   programs given to gitk does.  This is the least palatable, as it  
> won't
>   solve the read-only repository issue at all (it only allows you the
>   logic to choose the configured difftool backend program).
>
> * Instead of disabling the traditional "External Diff" and taking it  
> over
>   like your patch did, add a new codepath for "Difftool" that feeds  
> the
>   commit IDs and paths the way git-difftool expects.  The user can use
>   both, and the issue of read-only repository is solved when  
> "Difftool"
>   is used (but not "External Diff").
>
> * Take over "External Diff" codepath exactly like your patch did, but
>   teach "git-difftool" a new command line option to name an  
> unconfigured
>   external program that takes two filenames.  When "External Diff"
>   program is *not* configured in gitk, the command line to invoke
>   difftool would be exactly as in your patch, i.e. "difftool --no- 
> prompt
>   $from $to -- $path".  Otherwise, when gitk is configured to use an
>   external program, e.g. "frobanodiff -z", for "External Diff", you  
> pass
>   that command line to "git-difftool" via that new option, e.g.
>
>     difftool --no-prompt --extcmd="frobanodiff -z" $from $to -- $path
>
>   Then difftool is responsible for preparing the two necessary  
> temporary
>   files out of the given information ($from/$to/$path) and feeding  
> them
>   to "frobanodiff -z" command line.
>
>   Maybe such --extcmd support already exists in difftool, in which  
> case
>   my earlier suspicion that difftool is not as flexible would be  
> false.

Sounds good. Adding --extcmd should be nice and straightforward. Markus mentioned the need for a diff.guitool variable that would be tested for in the gitk case so it sounds like having a --gui option to let difftool know to do that should then cover all the bases.

It's looking like we'll have ourselves a small difftool patch series soon. Thanks all,

-- 
         David
Markus Heidelberg· Nov 20, 2009, 23:33 UTC · re: David Aguilar · lore

Re: [PATCH] gitk: Use git-difftool for external diffs

David Aguilar, 2009-11-20:
> The argument in favor of difftool is one of user
> expectations.  From a user's POV it ~seems~ desirable for
> gitk to honor difftool configurations.

In general I absolutely like the idea. There are however problems you can encounter. I use vimdiff for git-difftool and gvimdiff for gitk. When testing gitk with your patch, I wondered where the editor is, because nothing popped up. I then found vim on the terminal from where I started gitk. Even worse: when started with "gitk &", the vim process runs somewhere in the background and gitk freezes.

To overcome this problem maybe we need a second config variable like diff.guitool.

Markus
Junio C Hamano· Dec 30, 2009, 07:49 UTC · re: Nanako Shiraishi · lore

Re: [PATCH] gitk: Use git-difftool for external diffs

Nanako Shiraishi <nanako3@lavabit.com> writes:
> Junio, could you tell us what happened to this thread?
See http://thread.gmane.org/gmane.comp.version-control.git/132983/focus=133414

In short, the particular way to call difftool this patch implements was found to be inadequate to support existing external diff support by gitk and a small difftool update will happen first, followed by a patch to gitk to use the updated difftool, to avoid regression.

David Aguilar· Dec 31, 2009, 07:16 UTC · re: Junio C Hamano · lore

Re: [PATCH] gitk: Use git-difftool for external diffs

On Tue, Dec 29, 2009 at 11:49:52PM -0800, Junio C Hamano wrote:
Show 10 quoted lines
> Nanako Shiraishi <nanako3@lavabit.com> writes:
> 
> > Junio, could you tell us what happened to this thread?
> 
> See http://thread.gmane.org/gmane.comp.version-control.git/132983/focus=133414
> 
> In short, the particular way to call difftool this patch implements was
> found to be inadequate to support existing external diff support by gitk
> and a small difftool update will happen first, followed by a patch to gitk
> to use the updated difftool, to avoid regression.
I started the first step:

http://thread.gmane.org/gmane.comp.version-control.git/135613 http://thread.gmane.org/gmane.comp.version-control.git/135613/focus=135612

The 2nd patch implements the the --gui option which Markus pointed out would be needed to avoid issues such as calling "vimdiff" from a console-less gitk:

http://article.gmane.org/gmane.comp.version-control.git/133386

I marked the --gui patch as "RFC" since it introduced a new config variable and I want to make sure that we agreed on its name. I didn't get any feedback about that patch (my fault-- we were in RC freeze and I forgot to CC: Markus).

If that looks like a good first step then we can do the next step which would be to introduce the --extcmd= option as mentioned here:

http://thread.gmane.org/gmane.comp.version-control.git/132983/focus=133386

I will try and prepare the changes for --extcmd= within the next week assuming the existing --gui patch is ok.

On a related note, Jay Soffian recently submitted a git-mergetool--lib patch adding support for "diffmerge". It made it clear that there were parts of git-mergetool--lib that could use some refactoring:

http://thread.gmane.org/gmane.comp.version-control.git/134906

Jay did mention that he'd give it a shot at the time, though it does seems like the refactoring could wait until we see how --extcmd= fits into the world.

Thank you for following up on this thread, Nanako.
-- 
		David
Junio C Hamano· Dec 31, 2009, 20:37 UTC · re: David Aguilar · lore

Re: [PATCH] gitk: Use git-difftool for external diffs

David Aguilar <davvid@gmail.com> writes:
> I started the first step:
>
> http://thread.gmane.org/gmane.comp.version-control.git/135613
> http://thread.gmane.org/gmane.comp.version-control.git/135613/focus=135612
Thanks.
Show 10 quoted lines
> The 2nd patch implements the the --gui option which Markus
> pointed out would be needed to avoid issues such as calling
> "vimdiff" from a console-less gitk:
>
> http://article.gmane.org/gmane.comp.version-control.git/133386
>
> I marked the --gui patch as "RFC" since it introduced a new
> config variable and I want to make sure that we agreed on its
> name.  I didn't get any feedback about that patch
> (my fault-- we were in RC freeze and I forgot to CC: Markus).
I don't think "diff.guitool" would hurt.  However,...

I think the "--gui" patch is a more or less independent issue to "gitk runs external diff through difftool", because difftool/mergetool already have a built-in way to auto-guess which backend to use depending on what its environment looks like (e.g. do we have $DISPLAY etc.).

The "--gui" patch is about giving a more explicit way for the caller to control that backend picking decision process and it is more like icing than a prerequisite for the issue. IOW, I think the end result will be usable by gitk users even if they do not configure "diff.guitool".

← back to recent threads