git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] gitk: Use git-difftool for external diffs
@ 2009-11-16  3:12 David Aguilar
  2009-11-19  9:03 ` Paul Mackerras
  2009-12-30  3:13 ` Nanako Shiraishi
  0 siblings, 2 replies; 14+ messages in thread
From: David Aguilar @ 2009-11-16  3:12 UTC (permalink / raw)
  To: paulus; +Cc: peff, sam, git, 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

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH] gitk: Use git-difftool for external diffs
  2009-11-16  3:12 [PATCH] gitk: Use git-difftool for external diffs David Aguilar
@ 2009-11-19  9:03 ` Paul Mackerras
  2009-11-19 19:39   ` David Aguilar
  2009-12-30  3:13 ` Nanako Shiraishi
  1 sibling, 1 reply; 14+ messages in thread
From: Paul Mackerras @ 2009-11-19  9:03 UTC (permalink / raw)
  To: David Aguilar; +Cc: peff, sam, git

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.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] gitk: Use git-difftool for external diffs
  2009-11-19  9:03 ` Paul Mackerras
@ 2009-11-19 19:39   ` David Aguilar
  2009-11-19 22:21     ` Paul Mackerras
  0 siblings, 1 reply; 14+ messages in thread
From: David Aguilar @ 2009-11-19 19:39 UTC (permalink / raw)
  To: Paul Mackerras; +Cc: peff, sam, git

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] gitk: Use git-difftool for external diffs
  2009-11-19 19:39   ` David Aguilar
@ 2009-11-19 22:21     ` Paul Mackerras
  2009-11-20  7:53       ` Junio C Hamano
  0 siblings, 1 reply; 14+ messages in thread
From: Paul Mackerras @ 2009-11-19 22:21 UTC (permalink / raw)
  To: David Aguilar; +Cc: peff, sam, git

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.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] gitk: Use git-difftool for external diffs
  2009-11-19 22:21     ` Paul Mackerras
@ 2009-11-20  7:53       ` Junio C Hamano
  2009-11-20 18:55         ` David Aguilar
  0 siblings, 1 reply; 14+ messages in thread
From: Junio C Hamano @ 2009-11-20  7:53 UTC (permalink / raw)
  To: Paul Mackerras; +Cc: David Aguilar, peff, sam, git

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] gitk: Use git-difftool for external diffs
  2009-11-20  7:53       ` Junio C Hamano
@ 2009-11-20 18:55         ` David Aguilar
  2009-11-20 20:51           ` Junio C Hamano
  2009-11-20 23:33           ` Markus Heidelberg
  0 siblings, 2 replies; 14+ messages in thread
From: David Aguilar @ 2009-11-20 18:55 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Paul Mackerras, peff, sam, git

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] gitk: Use git-difftool for external diffs
  2009-11-20 18:55         ` David Aguilar
@ 2009-11-20 20:51           ` Junio C Hamano
  2009-11-21 21:47             ` David Aguilar
  2009-11-20 23:33           ` Markus Heidelberg
  1 sibling, 1 reply; 14+ messages in thread
From: Junio C Hamano @ 2009-11-20 20:51 UTC (permalink / raw)
  To: David Aguilar; +Cc: Paul Mackerras, peff, sam, git

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.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] gitk: Use git-difftool for external diffs
  2009-11-20 18:55         ` David Aguilar
  2009-11-20 20:51           ` Junio C Hamano
@ 2009-11-20 23:33           ` Markus Heidelberg
  1 sibling, 0 replies; 14+ messages in thread
From: Markus Heidelberg @ 2009-11-20 23:33 UTC (permalink / raw)
  To: David Aguilar; +Cc: Junio C Hamano, Paul Mackerras, peff, sam, git

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] gitk: Use git-difftool for external diffs
  2009-11-20 20:51           ` Junio C Hamano
@ 2009-11-21 21:47             ` David Aguilar
  0 siblings, 0 replies; 14+ messages in thread
From: David Aguilar @ 2009-11-21 21:47 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: Paul Mackerras, peff@peff.net, sam@vilain.net,
	git@vger.kernel.org, markus.heidelberg@web.de

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 
  

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] gitk: Use git-difftool for external diffs
  2009-11-16  3:12 [PATCH] gitk: Use git-difftool for external diffs David Aguilar
  2009-11-19  9:03 ` Paul Mackerras
@ 2009-12-30  3:13 ` Nanako Shiraishi
  2009-12-30  7:49   ` Junio C Hamano
  1 sibling, 1 reply; 14+ messages in thread
From: Nanako Shiraishi @ 2009-12-30  3:13 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: David Aguilar, peff, sam, git, paulus

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] gitk: Use git-difftool for external diffs
  2009-12-30  3:13 ` Nanako Shiraishi
@ 2009-12-30  7:49   ` Junio C Hamano
  2009-12-31  7:16     ` David Aguilar
  0 siblings, 1 reply; 14+ messages in thread
From: Junio C Hamano @ 2009-12-30  7:49 UTC (permalink / raw)
  To: Nanako Shiraishi; +Cc: David Aguilar, peff, sam, git, paulus

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.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] gitk: Use git-difftool for external diffs
  2009-12-30  7:49   ` Junio C Hamano
@ 2009-12-31  7:16     ` David Aguilar
  2009-12-31 20:37       ` Junio C Hamano
  0 siblings, 1 reply; 14+ messages in thread
From: David Aguilar @ 2009-12-31  7:16 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: Nanako Shiraishi, peff, sam, git, paulus, Markus Heidelberg,
	Jay Soffian

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] gitk: Use git-difftool for external diffs
  2009-12-31  7:16     ` David Aguilar
@ 2009-12-31 20:37       ` Junio C Hamano
  0 siblings, 0 replies; 14+ messages in thread
From: Junio C Hamano @ 2009-12-31 20:37 UTC (permalink / raw)
  To: David Aguilar
  Cc: Nanako Shiraishi, peff, sam, git, paulus, Markus Heidelberg,
	Jay Soffian

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH] gitk: Use git-difftool for external diffs
@ 2010-03-27 21:45 David Aguilar
  0 siblings, 0 replies; 14+ messages in thread
From: David Aguilar @ 2010-03-27 21:45 UTC (permalink / raw)
  To: Paul Mackerras; +Cc: Markus Heidelberg, Junio C Hamano, Nanako Shiraishi, git

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

^ permalink raw reply related	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2010-03-27 21:45 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2009-11-16  3:12 [PATCH] gitk: Use git-difftool for external diffs David Aguilar
2009-11-19  9:03 ` Paul Mackerras
2009-11-19 19:39   ` David Aguilar
2009-11-19 22:21     ` Paul Mackerras
2009-11-20  7:53       ` Junio C Hamano
2009-11-20 18:55         ` David Aguilar
2009-11-20 20:51           ` Junio C Hamano
2009-11-21 21:47             ` David Aguilar
2009-11-20 23:33           ` Markus Heidelberg
2009-12-30  3:13 ` Nanako Shiraishi
2009-12-30  7:49   ` Junio C Hamano
2009-12-31  7:16     ` David Aguilar
2009-12-31 20:37       ` Junio C Hamano
  -- strict thread matches above, loose matches on Subject: below --
2010-03-27 21:45 David Aguilar

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).