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

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

[David Aguilar]

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(-)

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

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

[Paul Mackerras]

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.

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

[David Aguilar]

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?

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

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

[Paul Mackerras]

David Aguilar writes:

> 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.

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

[Junio C Hamano]

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)?

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

[David Aguilar]

On Thu, Nov 19, 2009 at 11:53:37PM -0800, Junio C Hamano wrote:
> 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

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

[Junio C Hamano]

David Aguilar <davvid@gmail.com> writes:

>> > 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.

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

[Markus Heidelberg]

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

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

[David Aguilar]

On Nov 20, 2009, at 12:51 PM, Junio C Hamano <gitster@pobox.com> wrote:

> 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 
  

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

[Nanako Shiraishi]

Junio, could you tell us what happened to this thread?

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

[Junio C Hamano]

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.

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

[David Aguilar]

On Tue, Dec 29, 2009 at 11:49:52PM -0800, Junio C Hamano wrote:
> 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

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

[Junio C Hamano]

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.

> 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".

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

[David Aguilar]

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

Signed-off-by: David Aguilar <davvid@gmail.com>
---

This is still not the final result but it does get us
to a better place (having gitk work in read-only repos).

We may later want to add a radio button with "difftool"
as a choice so that the configured difftool is used
instead of the one specified being specified in --extcmd.

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

An even older attempt to fix the tempdir problem:
http://thread.gmane.org/gmane.comp.version-control.git/133277

This diffstat alone still makes me happy.

 gitk |   59 ++++++++++-------------------------------------------------
 1 files changed, 10 insertions(+), 49 deletions(-)

diff --git a/gitk b/gitk
index 1f36a3e..7e114da 100755
--- a/gitk
+++ b/gitk
@@ -3317,39 +3317,6 @@ proc gitknewtmpdir {} {
     return $diffdir
 }
 
-proc save_file_from_commit {filename output what} {
-    global nullfile
-
-    if {[catch {exec git show $filename -- > $output} err]} {
-	if {[string match "fatal: bad revision *" $err]} {
-	    return $nullfile
-	}
-	error_popup "[mc "Error getting \"%s\" from %s:" $filename $what] $err"
-	return {}
-    }
-    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
@@ -3375,23 +3342,17 @@ 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]
+    if {$flist_menu_file ne {}} {
+        set cmd [list "git" "difftool" "--no-prompt" "--gui"]
+        lappend cmd "--extcmd" $extdifftool
+        if {$diffidfrom ne $nullid && $diffidfrom ne $nullid2} {
+            lappend cmd $diffidfrom
+        }
+        if {$diffidto ne $nullid && $diffidto ne $nullid2} {
+            lappend cmd $diffidto
         }
+        lappend cmd "--" $flist_menu_file
+        eval exec $cmd &
     }
 }
 
-- 
1.7.0.3.291.g5e4f6

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

[David Aguilar]

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

Signed-off-by: David Aguilar <davvid@gmail.com>
---

Differences since the first patch:
This one doesn't pass "--gui" to difftool.

"--gui" was mistakenly included in the first patch and
consequently uncovered a bug in difftool.

 gitk |   58 +++++++++-------------------------------------------------
 1 files changed, 9 insertions(+), 49 deletions(-)

diff --git a/gitk b/gitk
index 1f36a3e..46c103e 100755
--- a/gitk
+++ b/gitk
@@ -3317,39 +3317,6 @@ proc gitknewtmpdir {} {
     return $diffdir
 }
 
-proc save_file_from_commit {filename output what} {
-    global nullfile
-
-    if {[catch {exec git show $filename -- > $output} err]} {
-	if {[string match "fatal: bad revision *" $err]} {
-	    return $nullfile
-	}
-	error_popup "[mc "Error getting \"%s\" from %s:" $filename $what] $err"
-	return {}
-    }
-    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
@@ -3375,23 +3342,16 @@ 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]
+    if {$flist_menu_file ne {}} {
+        set cmd [list "git" "difftool" "--no-prompt" "--extcmd" $extdifftool]
+        if {$diffidfrom ne $nullid && $diffidfrom ne $nullid2} {
+            lappend cmd $diffidfrom
+        }
+        if {$diffidto ne $nullid && $diffidto ne $nullid2} {
+            lappend cmd $diffidto
         }
+        lappend cmd "--" $flist_menu_file
+        eval exec $cmd &
     }
 }
 
-- 
1.7.0.3.292.gbeff

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

[David Aguilar]

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

Signed-off-by: David Aguilar <davvid@gmail.com>
---

Differences since v1 and v2:

v1: Do not pass "--gui" to difftool
v2: Do not needlessly check if $flist_menu_file is non-empty

 gitk |   58 ++++++++--------------------------------------------------
 1 files changed, 8 insertions(+), 50 deletions(-)

diff --git a/gitk b/gitk
index 1f36a3e..637f8f9 100755
--- a/gitk
+++ b/gitk
@@ -3317,39 +3317,6 @@ proc gitknewtmpdir {} {
     return $diffdir
 }
 
-proc save_file_from_commit {filename output what} {
-    global nullfile
-
-    if {[catch {exec git show $filename -- > $output} err]} {
-	if {[string match "fatal: bad revision *" $err]} {
-	    return $nullfile
-	}
-	error_popup "[mc "Error getting \"%s\" from %s:" $filename $what] $err"
-	return {}
-    }
-    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
@@ -3375,24 +3342,15 @@ 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 [list "git" "difftool" "--no-prompt" "--extcmd" $extdifftool]
+    if {$diffidfrom ne $nullid && $diffidfrom ne $nullid2} {
+        lappend cmd $diffidfrom
+    }
+    if {$diffidto ne $nullid && $diffidto ne $nullid2} {
+        lappend cmd $diffidto
     }
+    lappend cmd "--" $flist_menu_file
+    eval exec $cmd &
 }
 
 proc find_hunk_blamespec {base line} {
-- 
1.7.0.3.292.gbeff

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

[Markus Heidelberg]

David Aguilar, 2010-03-28 01:20:
> +    set cmd [list "git" "difftool" "--no-prompt" "--extcmd" $extdifftool]

According to "man gitcli" this should be "--extcmd=$extdifftool".

Besides this, works for me.

Markus

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

[David Aguilar]

On Sun, Mar 28, 2010 at 11:59:06AM +0100, Markus Heidelberg wrote:
> David Aguilar, 2010-03-28 01:20:
> > +    set cmd [list "git" "difftool" "--no-prompt" "--extcmd" $extdifftool]
> 
> According to "man gitcli" this should be "--extcmd=$extdifftool".
> 
> Besides this, works for me.
> 
> Markus

I'll send a v4 with this change.
Thanks for the review.

-- 
		David

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

[David Aguilar]

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

Signed-off-by: David Aguilar <davvid@gmail.com>
---
 gitk |   58 ++++++++--------------------------------------------------
 1 files changed, 8 insertions(+), 50 deletions(-)

diff --git a/gitk b/gitk
index 1f36a3e..71cb501 100755
--- a/gitk
+++ b/gitk
@@ -3317,39 +3317,6 @@ proc gitknewtmpdir {} {
     return $diffdir
 }
 
-proc save_file_from_commit {filename output what} {
-    global nullfile
-
-    if {[catch {exec git show $filename -- > $output} err]} {
-	if {[string match "fatal: bad revision *" $err]} {
-	    return $nullfile
-	}
-	error_popup "[mc "Error getting \"%s\" from %s:" $filename $what] $err"
-	return {}
-    }
-    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
@@ -3375,24 +3342,15 @@ 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 [list "git" "difftool" "--no-prompt" "--extcmd=$extdifftool"]
+    if {$diffidfrom ne $nullid && $diffidfrom ne $nullid2} {
+        lappend cmd $diffidfrom
+    }
+    if {$diffidto ne $nullid && $diffidto ne $nullid2} {
+        lappend cmd $diffidto
     }
+    lappend cmd "--" $flist_menu_file
+    eval exec $cmd &
 }
 
 proc find_hunk_blamespec {base line} {
-- 
1.7.0.3.313.g87b3c

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

[Markus Heidelberg]

David Aguilar, 2010-03-31 04:09:
> This teaches gitk about git-difftool.  A benefit of this change
> is that gitk's external diff feature now works with read-only
> repositories.
> 
> Signed-off-by: David Aguilar <davvid@gmail.com>

Tested-by: Markus Heidelberg <markus.heidelberg@web.de>

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

[David Aguilar]

On Fri, Apr 02, 2010 at 12:32:53PM +0100, Markus Heidelberg wrote:
> David Aguilar, 2010-03-31 04:09:
> > This teaches gitk about git-difftool.  A benefit of this change
> > is that gitk's external diff feature now works with read-only
> > repositories.
> > 
> > Signed-off-by: David Aguilar <davvid@gmail.com>
> 
> Tested-by: Markus Heidelberg <markus.heidelberg@web.de>

Paul, you haven't replied yet but I do remember you mentioning
that you like to keep gitk backwards-compat with older git.
I have a newer version of this patch that checks for the
existence of difftool --extcmd=<cmd> before trying to use it.
It'll be PATCH v5.

-- 
		David

[PATCH v5] gitk: Use git-difftool for external diffs when git >= 1.7.0

[David Aguilar]

git-difftool is used instead of the built-in external diff
code when git is >= 1.7.0.  git-difftool's '--extcmd=frotz'
feature was first introduced in 1.7.0 and is the mechanism
through which we launch the configured difftool.

A benefit of this change is that gitk's external diff feature
no longer needs write-access to the current directory.

Signed-off-by: David Aguilar <davvid@gmail.com>
---

Unlike previous versions, this one checks the git version at runtime
and gracefully falls back to the old code when git is older.

 gitk |   14 ++++++++++++++
 1 files changed, 14 insertions(+), 0 deletions(-)

diff --git a/gitk b/gitk
index 1f36a3e..a49eecc 100755
--- a/gitk
+++ b/gitk
@@ -3355,6 +3355,7 @@ proc external_diff {} {
     global flist_menu_file
     global diffids
     global extdifftool
+    global git_version
 
     if {[llength $diffids] == 1} {
         # no reference commit given
@@ -3375,6 +3376,19 @@ proc external_diff {} {
         set diffidto [lindex $diffids 1]
     }
 
+    if {[package vcompare $git_version "1.7.0"] >= 0} {
+        set cmd [list "git" "difftool" "--no-prompt" "--extcmd=$extdifftool"]
+        if {$diffidfrom ne $nullid && $diffidfrom ne $nullid2} {
+            lappend cmd $diffidfrom
+        }
+        if {$diffidto ne $nullid && $diffidto ne $nullid2} {
+            lappend cmd $diffidto
+        }
+        lappend cmd "--" $flist_menu_file
+        eval exec $cmd &
+        return
+    }
+
     # make sure that several diffs wont collide
     set diffdir [gitknewtmpdir]
     if {$diffdir eq {}} return
-- 
1.7.0.3.313.g87b3c

Re: [PATCH v5] gitk: Use git-difftool for external diffs when git >= 1.7.0

[Paul Mackerras]

On Thu, Apr 08, 2010 at 02:08:10AM -0700, David Aguilar wrote:

> git-difftool is used instead of the built-in external diff
> code when git is >= 1.7.0.  git-difftool's '--extcmd=frotz'
> feature was first introduced in 1.7.0 and is the mechanism
> through which we launch the configured difftool.
> 
> A benefit of this change is that gitk's external diff feature
> no longer needs write-access to the current directory.

I applied this one, and in testing it I noticed that if the git
difftool invocation fails, for example because the user doesn't have
meld installed, there is no notification via the GUI.  Instead an
error message is printed to stderr.  We need to do something more like
what we do in the old-git case -- use open rather than exec -- and pop
up an error dialog with error_popup on error so that the user knows
what the problem is.

Another minor problem is that the file names that meld (or other diff
tool) displays are less informative now.  For example I see
"KyaZ5d_gitk : 8iucke_gitk" as the tab label in meld instead of the
"[87d24ec87abc...e236e0833ff] gitk" label that we got before, which
was a little more informative, though the e236e0833ff is the end of
the second SHA1 rather than the beginning.

So I backed it out.  The error handling needs to be fixed using
something like what delete_at_eof does (except that obviously we don't
have any files to delete).

Paul.

Re: [PATCH v5] gitk: Use git-difftool for external diffs when git >= 1.7.0

[David Aguilar]

On Sat, Apr 17, 2010 at 06:52:30PM +1000, Paul Mackerras wrote:
> On Thu, Apr 08, 2010 at 02:08:10AM -0700, David Aguilar wrote:
> 
> > git-difftool is used instead of the built-in external diff
> > code when git is >= 1.7.0.  git-difftool's '--extcmd=frotz'
> > feature was first introduced in 1.7.0 and is the mechanism
> > through which we launch the configured difftool.
> > 
> > A benefit of this change is that gitk's external diff feature
> > no longer needs write-access to the current directory.
> 
> I applied this one, and in testing it I noticed that if the git
> difftool invocation fails, for example because the user doesn't have
> meld installed, there is no notification via the GUI.  Instead an
> error message is printed to stderr.  We need to do something more like
> what we do in the old-git case -- use open rather than exec -- and pop
> up an error dialog with error_popup on error so that the user knows
> what the problem is.
> 
> Another minor problem is that the file names that meld (or other diff
> tool) displays are less informative now.  For example I see
> "KyaZ5d_gitk : 8iucke_gitk" as the tab label in meld instead of the
> "[87d24ec87abc...e236e0833ff] gitk" label that we got before, which
> was a little more informative, though the e236e0833ff is the end of
> the second SHA1 rather than the beginning.
> 
> So I backed it out.  The error handling needs to be fixed using
> something like what delete_at_eof does (except that obviously we don't
> have any files to delete).
> 
> Paul.

I'll fix the error handling and add a few more notes to the
final commit message.

The title display is tool-specific so there's something we can
do about it on a tool-by-tool basis.  This'll have to wait until
we can break out the tool-dependent parts of git-mergetool--lib
as was discussed in the past:

http://lists-archives.org/git/707440-mergetool-lib-add-diffmerge-as-a-pre-configured-mergetool-option.html
(sorry, couldn't find it on gmane... :-/)

Jay, you mentioned wanting to give Junio's approach a try as
well as looking into what mercurial did with mergetools.
Do you have any thoughts about it since then?  I can help
factor out the backends if you don't think you'll have time to
get to it soon.

Once we factor out the backends we'll should have an easier
time working in additional variables for doing user-friendly
things like passing the --label= flag to meld.
git-difftool should be able to do it since its GIT_EXTERNAL_DIFF
helper is passed the sha1s.

Thanks all,

-- 
		David

Re: [PATCH v5] gitk: Use git-difftool for external diffs when git >= 1.7.0

[Jay Soffian]

On Sat, Apr 17, 2010 at 6:45 PM, David Aguilar <davvid@gmail.com> wrote:
> Jay, you mentioned wanting to give Junio's approach a try as
> well as looking into what mercurial did with mergetools.
> Do you have any thoughts about it since then?  I can help
> factor out the backends if you don't think you'll have time to
> get to it soon.

Wow, I need to stop starting new patches and start finishing some of
the ones on my todo list.

But sorry, no, I haven't had a chance to look at this and I'm not
likely to soon. So if you've got the time and inclination, I'm happy
for you to do the work. :-)

j.

[PATCH v6] gitk: Use git-difftool for external diffs when available

[David Aguilar]

git-difftool's '--extcmd=frotz' was added in 1.7.0 and
is the mechanism through which gitk launches the
configured 'extdifftool'.  When 'extdifftool' is
misconfigured an error dialog is used to display
git-difftool's stdout and stderr.

The existing implementation moved into 'proc gitkextdiff'
for use with git < 1.7.0.

One benefit of this change is that gitk's external diff
no longer requires write-access to the current directory.

Signed-off-by: David Aguilar <davvid@gmail.com>
---

Changes since last time:

* Errors are shown using 'proc error_popup'
* The existing code moved into a tidy function

 gitk |   25 +++++++++++++++++++++++++
 1 files changed, 25 insertions(+), 0 deletions(-)

diff --git a/gitk b/gitk
index 1b0e09a..0533baf 100755
--- a/gitk
+++ b/gitk
@@ -3361,6 +3361,7 @@ proc external_diff {} {
     global flist_menu_file
     global diffids
     global extdifftool
+    global git_version
 
     if {[llength $diffids] == 1} {
         # no reference commit given
@@ -3380,6 +3381,30 @@ proc external_diff {} {
         set diffidfrom [lindex $diffids 0]
         set diffidto [lindex $diffids 1]
     }
+    if {[package vcompare $git_version "1.7.0"] < 0} {
+        gitkextdiff $diffidfrom $diffidto
+        return
+    }
+
+    set cmd [list "git" "difftool" "--no-prompt" "--extcmd=$extdifftool"]
+    if {$diffidfrom ne $nullid && $diffidfrom ne $nullid2} {
+        lappend cmd $diffidfrom
+    }
+    if {$diffidto ne $nullid && $diffidto ne $nullid2} {
+        lappend cmd $diffidto
+    }
+    lappend cmd "--" $flist_menu_file
+
+    set pipe [open |$cmd r]
+    set stdout [read $pipe]
+    if {[catch {close $pipe} stderr] != 0} {
+        error_popup "git-difftool: $stdout $stderr"
+    }
+}
+
+proc gitkextdiff {diffidfrom diffidto} {
+    global flist_menu_file
+    global extdifftool
 
     # make sure that several diffs wont collide
     set diffdir [gitknewtmpdir]
-- 
1.7.1.rc2.5.gddd02

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

[David Aguilar]

Hello,

Is there anything else (besides sending this email) that I can
do to help move this patch along?

I got another "gitk read-only repo broken" email this week,
which is what reminded me... ;-)

It's been a while.  I just rebased the patch against the latest
master and it didn't have any conflicts.  Resend?

On Tue, Apr 20, 2010 at 01:11:19AM -0700, David Aguilar wrote:
> git-difftool's '--extcmd=frotz' was added in 1.7.0 and
> is the mechanism through which gitk launches the
> configured 'extdifftool'.  When 'extdifftool' is
> misconfigured an error dialog is used to display
> git-difftool's stdout and stderr.
> 
> The existing implementation moved into 'proc gitkextdiff'
> for use with git < 1.7.0.
> 
> One benefit of this change is that gitk's external diff
> no longer requires write-access to the current directory.
> 
> Signed-off-by: David Aguilar <davvid@gmail.com>
> ---
> 
> Changes since last time:
> 
> * Errors are shown using 'proc error_popup'
> * The existing code moved into a tidy function
> 
>  gitk |   25 +++++++++++++++++++++++++
>  1 files changed, 25 insertions(+), 0 deletions(-)
> 
> diff --git a/gitk b/gitk
> index 1b0e09a..0533baf 100755
> --- a/gitk
> +++ b/gitk
> @@ -3361,6 +3361,7 @@ proc external_diff {} {
>      global flist_menu_file
>      global diffids
>      global extdifftool
> +    global git_version
>  
>      if {[llength $diffids] == 1} {
>          # no reference commit given
> @@ -3380,6 +3381,30 @@ proc external_diff {} {
>          set diffidfrom [lindex $diffids 0]
>          set diffidto [lindex $diffids 1]
>      }
> +    if {[package vcompare $git_version "1.7.0"] < 0} {
> +        gitkextdiff $diffidfrom $diffidto
> +        return
> +    }
> +
> +    set cmd [list "git" "difftool" "--no-prompt" "--extcmd=$extdifftool"]
> +    if {$diffidfrom ne $nullid && $diffidfrom ne $nullid2} {
> +        lappend cmd $diffidfrom
> +    }
> +    if {$diffidto ne $nullid && $diffidto ne $nullid2} {
> +        lappend cmd $diffidto
> +    }
> +    lappend cmd "--" $flist_menu_file
> +
> +    set pipe [open |$cmd r]
> +    set stdout [read $pipe]
> +    if {[catch {close $pipe} stderr] != 0} {
> +        error_popup "git-difftool: $stdout $stderr"
> +    }
> +}
> +
> +proc gitkextdiff {diffidfrom diffidto} {
> +    global flist_menu_file
> +    global extdifftool
>  
>      # make sure that several diffs wont collide
>      set diffdir [gitknewtmpdir]
> -- 
> 1.7.1.rc2.5.gddd02
> 

-- 
		David