[PATCH] git-mergetool: add support for ediff

[PATCH] git-mergetool: add support for ediff

[Sam Vilain]

There was emerge already but I much prefer this mode.

Signed-off-by: Sam Vilain <sam.vilain@catalyst.net.nz>
---
 Documentation/config.txt        |    3 ++-
 Documentation/git-mergetool.txt |    3 ++-
 git-mergetool.sh                |   19 ++++++++++++++-----
 3 files changed, 18 insertions(+), 7 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 50503e8..4661e24 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -550,7 +550,8 @@ merge.summary::
 merge.tool::
 	Controls which merge resolution program is used by
 	gitlink:git-mergetool[l].  Valid values are: "kdiff3", "tkdiff",
-	"meld", "xxdiff", "emerge", "vimdiff", "gvimdiff", and "opendiff".
+	"meld", "xxdiff", "emerge", "ediff", "vimdiff", "gvimdiff", and
+	"opendiff".
 
 merge.verbosity::
 	Controls the amount of output shown by the recursive merge
diff --git a/Documentation/git-mergetool.txt b/Documentation/git-mergetool.txt
index 6c32c6d..1efe6e4 100644
--- a/Documentation/git-mergetool.txt
+++ b/Documentation/git-mergetool.txt
@@ -25,7 +25,8 @@ OPTIONS
 -t or --tool=<tool>::
 	Use the merge resolution program specified by <tool>.
 	Valid merge tools are:
-	kdiff3, tkdiff, meld, xxdiff, emerge, vimdiff, gvimdiff, and opendiff
+	kdiff3, tkdiff, meld, xxdiff, emerge, ediff, vimdiff, gvimdiff,
+	and opendiff
 +
 If a merge resolution program is not specified, 'git mergetool'
 will use the configuration variable merge.tool.  If the
diff --git a/git-mergetool.sh b/git-mergetool.sh
index 7b66309..6fda8af 100755
--- a/git-mergetool.sh
+++ b/git-mergetool.sh
@@ -258,6 +258,15 @@ merge_file () {
 	    status=$?
 	    save_backup
 	    ;;
+	ediff)
+	    if base_present ; then
+		emacs --eval "(ediff-merge-files-with-ancestor \"$LOCAL\" \"$REMOTE\" \"$BASE\" nil \"$path\")"
+	    else
+		emacs --eval "(ediff-merge-files \"$LOCAL\" \"$REMOTE\" nil \"$path\")"
+	    fi
+	    status=$?
+	    save_backup
+	    ;;
     esac
     if test "$status" -ne 0; then
 	echo "merge of $path failed" 1>&2
@@ -299,7 +308,7 @@ done
 if test -z "$merge_tool"; then
     merge_tool=`git-config merge.tool`
     case "$merge_tool" in
-	kdiff3 | tkdiff | xxdiff | meld | opendiff | emerge | vimdiff | gvimdiff | "")
+	kdiff3 | tkdiff | xxdiff | meld | opendiff | emerge | ediff | vimdiff | gvimdiff | "")
 	    ;; # happy
 	*)
 	    echo >&2 "git config option merge.tool set to unknown tool: $merge_tool"
@@ -320,15 +329,15 @@ if test -z "$merge_tool" ; then
         fi
     fi
     if echo "${VISUAL:-$EDITOR}" | grep 'emacs' > /dev/null 2>&1; then
-        merge_tool_candidates="$merge_tool_candidates emerge"
+        merge_tool_candidates="$merge_tool_candidates emerge ediff"
     fi
     if echo "${VISUAL:-$EDITOR}" | grep 'vim' > /dev/null 2>&1; then
         merge_tool_candidates="$merge_tool_candidates vimdiff"
     fi
-    merge_tool_candidates="$merge_tool_candidates opendiff emerge vimdiff"
+    merge_tool_candidates="$merge_tool_candidates opendiff ediff emerge vimdiff"
     echo "merge tool candidates: $merge_tool_candidates"
     for i in $merge_tool_candidates; do
-        if test $i = emerge ; then
+        if test $i = emerge || test $i = ediff ; then
             cmd=emacs
         else
             cmd=$i
@@ -351,7 +360,7 @@ case "$merge_tool" in
 	    exit 1
 	fi
 	;;
-    emerge)
+    emerge|ediff)
 	if ! type "emacs" > /dev/null 2>&1; then
 	    echo "Emacs is not available"
 	    exit 1
-- 
1.5.2.1.1131.g3b90

Re: [PATCH] git-mergetool: add support for ediff

[Jason Sewall]

On 6/28/07, Sam Vilain <sam.vilain@catalyst.net.nz> wrote:
> There was emerge already but I much prefer this mode.
>

I beat ya to it: http://marc.info/?l=git&m=118301192520295&w=2

But it looks like maybe you did a better job (updated docs, for
example). Other than that, it's almost exactly the same.

Ack.

Jason

P.S.

doing this:
>      if echo "${VISUAL:-$EDITOR}" | grep 'emacs' > /dev/null 2>&1; then
>         merge_tool_candidates="$merge_tool_candidates emerge ediff"
>      fi

and then this

>     merge_tool_candidates="$merge_tool_candidates opendiff ediff emerge vimdiff"

makes this

>      echo "merge tool candidates: $merge_tool_candidates"

print out emerge and ediff twice, presumably because we're adding it
in for both "visual" emacs and "regular" (i.e. -nw) emacs. I suck at
shell scripts, so I'm probably missing something but what why do we
have all of that testing for emacs + vim if we just add their tools
anyway right afterwards?

Re: [PATCH] git-mergetool: add support for ediff

[Theodore Tso]

On Thu, Jun 28, 2007 at 06:31:50PM -0700, Jason Sewall wrote:
> >     echo "merge tool candidates: $merge_tool_candidates"

This was a debugging echo that slipped by; I had never intended for it
to be kept.

> print out emerge and ediff twice, presumably because we're adding it
> in for both "visual" emacs and "regular" (i.e. -nw) emacs. I suck at
> shell scripts, so I'm probably missing something but what why do we
> have all of that testing for emacs + vim if we just add their tools
> anyway right afterwards?

Some things get added twice but in a different order because the
search order matters.  But in terms of adding emerge and ediff, yes,
there's no point, since they always get added in the same order.  

I'll have to look at the two and see why people like one over the
other, and then we'll have to pick which one should be the default.
Although as I've said, past a certain point people should just put
their personal preference in .gitconfig.

						- Ted

[PATCH] git-mergetool: add support for ediff

[Sam Vilain]

There was emerge already but I much prefer this mode.

Signed-off-by: Sam Vilain <sam.vilain@catalyst.net.nz>
---
 Documentation/config.txt        |    3 ++-
 Documentation/git-mergetool.txt |    3 ++-
 git-mergetool.sh                |   19 ++++++++++++++-----
 3 files changed, 18 insertions(+), 7 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 50503e8..4661e24 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -550,7 +550,8 @@ merge.summary::
 merge.tool::
 	Controls which merge resolution program is used by
 	gitlink:git-mergetool[l].  Valid values are: "kdiff3", "tkdiff",
-	"meld", "xxdiff", "emerge", "vimdiff", "gvimdiff", and "opendiff".
+	"meld", "xxdiff", "emerge", "ediff", "vimdiff", "gvimdiff", and
+	"opendiff".
 
 merge.verbosity::
 	Controls the amount of output shown by the recursive merge
diff --git a/Documentation/git-mergetool.txt b/Documentation/git-mergetool.txt
index 6c32c6d..1efe6e4 100644
--- a/Documentation/git-mergetool.txt
+++ b/Documentation/git-mergetool.txt
@@ -25,7 +25,8 @@ OPTIONS
 -t or --tool=<tool>::
 	Use the merge resolution program specified by <tool>.
 	Valid merge tools are:
-	kdiff3, tkdiff, meld, xxdiff, emerge, vimdiff, gvimdiff, and opendiff
+	kdiff3, tkdiff, meld, xxdiff, emerge, ediff, vimdiff, gvimdiff,
+	and opendiff
 +
 If a merge resolution program is not specified, 'git mergetool'
 will use the configuration variable merge.tool.  If the
diff --git a/git-mergetool.sh b/git-mergetool.sh
index 7b66309..6fda8af 100755
--- a/git-mergetool.sh
+++ b/git-mergetool.sh
@@ -258,6 +258,15 @@ merge_file () {
 	    status=$?
 	    save_backup
 	    ;;
+	ediff)
+	    if base_present ; then
+		emacs --eval "(ediff-merge-files-with-ancestor \"$LOCAL\" \"$REMOTE\" \"$BASE\" nil \"$path\")"
+	    else
+		emacs --eval "(ediff-merge-files \"$LOCAL\" \"$REMOTE\" nil \"$path\")"
+	    fi
+	    status=$?
+	    save_backup
+	    ;;
     esac
     if test "$status" -ne 0; then
 	echo "merge of $path failed" 1>&2
@@ -299,7 +308,7 @@ done
 if test -z "$merge_tool"; then
     merge_tool=`git-config merge.tool`
     case "$merge_tool" in
-	kdiff3 | tkdiff | xxdiff | meld | opendiff | emerge | vimdiff | gvimdiff | "")
+	kdiff3 | tkdiff | xxdiff | meld | opendiff | emerge | ediff | vimdiff | gvimdiff | "")
 	    ;; # happy
 	*)
 	    echo >&2 "git config option merge.tool set to unknown tool: $merge_tool"
@@ -320,15 +329,15 @@ if test -z "$merge_tool" ; then
         fi
     fi
     if echo "${VISUAL:-$EDITOR}" | grep 'emacs' > /dev/null 2>&1; then
-        merge_tool_candidates="$merge_tool_candidates emerge"
+        merge_tool_candidates="$merge_tool_candidates emerge ediff"
     fi
     if echo "${VISUAL:-$EDITOR}" | grep 'vim' > /dev/null 2>&1; then
         merge_tool_candidates="$merge_tool_candidates vimdiff"
     fi
-    merge_tool_candidates="$merge_tool_candidates opendiff emerge vimdiff"
+    merge_tool_candidates="$merge_tool_candidates opendiff ediff emerge vimdiff"
     echo "merge tool candidates: $merge_tool_candidates"
     for i in $merge_tool_candidates; do
-        if test $i = emerge ; then
+        if test $i = emerge || test $i = ediff ; then
             cmd=emacs
         else
             cmd=$i
@@ -351,7 +360,7 @@ case "$merge_tool" in
 	    exit 1
 	fi
 	;;
-    emerge)
+    emerge|ediff)
 	if ! type "emacs" > /dev/null 2>&1; then
 	    echo "Emacs is not available"
 	    exit 1
-- 
1.5.2.1.1131.g3b90

Re: [PATCH] git-mergetool: add support for ediff

[Junio C Hamano]

Sam Vilain <sam.vilain@catalyst.net.nz> writes:

> There was emerge already but I much prefer this mode.

I thought Ted said he'll look into clearning this up, so I won't
apply it yet at this moment to my tree, but have one comment...

> @@ -320,15 +329,15 @@ if test -z "$merge_tool" ; then
>          fi
>      fi
>      if echo "${VISUAL:-$EDITOR}" | grep 'emacs' > /dev/null 2>&1; then
> -        merge_tool_candidates="$merge_tool_candidates emerge"
> +        merge_tool_candidates="$merge_tool_candidates emerge ediff"
>      fi
>      if echo "${VISUAL:-$EDITOR}" | grep 'vim' > /dev/null 2>&1; then
>          merge_tool_candidates="$merge_tool_candidates vimdiff"
>      fi
> -    merge_tool_candidates="$merge_tool_candidates opendiff emerge vimdiff"
> +    merge_tool_candidates="$merge_tool_candidates opendiff ediff emerge vimdiff"
>      echo "merge tool candidates: $merge_tool_candidates"

So by default outside X environment, if your $EDITOR is emacs,
you would use emerge and not ediff, but if your $EDITOR is unset
and have emacs in your $PATH you would use ediff not emerge?

Re: [PATCH] git-mergetool: add support for ediff

[Sam Vilain]

Junio C Hamano wrote:
> I thought Ted said he'll look into clearning this up, so I won't
> apply it yet at this moment to my tree, but have one comment...

Yes, sorry I should have left this one out, it didn't get any changes.

Sam.

Re: [PATCH] git-mergetool: add support for ediff

[Theodore Tso]

On Fri, Jun 29, 2007 at 12:03:28AM -0400, Theodore Tso wrote:
> I'll have to look at the two and see why people like one over the
> other, and then we'll have to pick which one should be the default.
> Although as I've said, past a certain point people should just put
> their personal preference in .gitconfig.

After looking at ediff, it is definitely the more polished and
featureful compared to emerge --- except in one critical area, which
is calling as a mergeing tool from a shell script or command line.
Ediff fundamentally assumes that it fired off from inside an emacs
environment, whereas emerge is much friendly as an external merge
program. 

This can be shown in the relatively easy way emerge can be run from
the command-line:

	emacs -f emerge-files-with-ancestor-command "$LOCAL" "$REMOTE" "$BASE" "$path"

... where as with ediff, you have to run it this way:

	emacs --eval "(ediff-merge-files-with-ancestor \"$LOCAL\" \"$REMOTE\" \"$BASE\" nil \"$path\")"

Unfortunately, it's not enough.  Ediff doesn't have an "abort" command
which returns a non-zero exit status, and when you use the "quit"
command, it asks you a series of obnoxious questions:

Quit this Ediff session? (y or n)
File /usr/projects/git/test/testfile.c exists, overwrite? (y or n)
Merge buffer saved in /usr/projects/git/test/testfile.c
<delay for 3 annoying seconds>
Merge buffer saved.  Now kill the buffer? (y or n)

... and then it leaves you in the emacs window, and you have to type
^X^C by hand.

So while ediff is more featureful, its integration is so lacking that
it is incredibly annoying to use.

Which leaves us with the interesting question.  We could just
integrate it, but not make it the default (the above makes ediff just
far too annoying for a user who is not expecting it).  

Alternatively, we could patch around the problem.  The following emacs
lisp code fixes the ediff issues:

(defun ediff-write-merge-buffer ()
  (let ((file ediff-merge-store-file))
    (set-buffer ediff-buffer-C)
    (write-region (point-min) (point-max) file)
    (message "Merge buffer saved in: %s" file)
    (set-buffer-modified-p nil)
    (sit-for 1)))

(setq ediff-quit-hook 'kill-emacs
      ediff-quit-merge-hook 'ediff-write-merge-buffer)

But the only clean way of adding that to git-mergetool would be something like this:

	emacs --eval "(progn (defun ediff-write-merge-buffer () (let ((file ediff-merge-store-file)) (set-buffer ediff-buffer-C) (write-region (point-min) (point-max) file) (message \"Merge buffer saved in: %s\" file) (set-buffer-modified-p nil) (sit-for 1))) (setq ediff-quit-hook 'kill-emacs ediff-quit-merge-hook 'ediff-write-merge-buffer) (ediff-merge-files-with-ancestor \"$LOCAL\" \"$REMOTE\" \"$BASE\" nil \"$path\")"

But that seems too ugly to live, and it could break in the future if
ediff ever changes some of its internal variables.


Alternatively, we could file a bug report with the ediff folks, and
request that they add an 'ediff-files-with-ancestor-command and
'ediff-files-command just as emerge does.  The problem with that
approach is that ediff is shipped with emacs, and emacs has a release
cycle measured in **years**.


So my current thinking is that ediff will *not* be the default for
git-mergetool if emacs is present, and that emerge will be used for
now, because of these problems.

Comments?

						- Ted

Re: [PATCH] git-mergetool: add support for ediff

[Junio C Hamano]

Theodore Tso <tytso@mit.edu> writes:

> Unfortunately, it's not enough.  Ediff doesn't have an "abort" command
> which returns a non-zero exit status, and when you use the "quit"
> command, it asks you a series of obnoxious questions:
>
> ...
> Alternatively, we could patch around the problem.  The following emacs
> lisp code fixes the ediff issues:

But that would be changing the behaviour globally, and not
limited to the particular session invoked from git-mergetool,
wouldn't it?  If that is the case it would be a hard sell to
Emacs users, especially the ones that keep their Emacs running
forever and have emacsclient as their EDITOR, I would think.

Re: [PATCH] git-mergetool: add support for ediff

[Theodore Tso]

On Sun, Jul 01, 2007 at 07:32:59PM -0700, Junio C Hamano wrote:
> Theodore Tso <tytso@mit.edu> writes:
> 
> > Unfortunately, it's not enough.  Ediff doesn't have an "abort" command
> > which returns a non-zero exit status, and when you use the "quit"
> > command, it asks you a series of obnoxious questions:
> >
> > ...
> > Alternatively, we could patch around the problem.  The following emacs
> > lisp code fixes the ediff issues:
> 
> But that would be changing the behaviour globally, and not
> limited to the particular session invoked from git-mergetool,
> wouldn't it?  If that is the case it would be a hard sell to
> Emacs users, especially the ones that keep their Emacs running
> forever and have emacsclient as their EDITOR, I would think.

The emacs lisp code I gave there was the minimal necessary so it
could be passed on the command-line; I was trying to keep it small.

Obviously, the patch that would have to get sent to the ediff folks
would have to be much more generalized --- in fact, probably the right
thing to do is to send a full patch that actually implemented
ediff-merge-files-command and ediff-merge-files-with-ancestoers-commands.

As far as people using emacsclient as their editor, it would be simple
enough to have the emacs lisp code test to see if
server-buffer-clients is non-nill; if it is, then we know that this
merge request was trigered by emacsclient, and so (server-done) should
be called instead of (kill-emacs).  Emerge does not do this; arguably
this is a bug in emerge.

The other way we could deal with this problem is to fire up a separate
emacs even if EDITOR is emacsclient, on the theory that
EDITOR=emacsclient meants that the user prefers emacs, but it doesn't
necessarily mean that we have to *use* emacsclient, especially when
emerge currently doesn't DTRT with emacsclient.

One thing that did cross my mind is that we could put code which
patched ediff.el and emerge.el in /usr/share/git/lisp/... and then
passed called emacs with something like this "emacs -l
$sharedir/lisp/ediff-patches.el ...".  But this implies packaging
emacs lisp files with git, and I'm not at ALL sure we want to go
there.  Personally, I still like kdiff3 as my personal favorite
mergetool, and given that emacs starts up pretty fast these days, I've
given up on emacsclient, but I know there are certainly people who use
them.

(Mmmm...., I just pulled down an early emacs 23 snapshot with Xft
support enabled, so I can enjoy the anti-aliased font goodness.  Even
with all of the Gtk and Xft bloat, the emacs 23 snapshot is still
quick snappy to fire up.)

					- Ted

Re: [PATCH] git-mergetool: add support for ediff

[Junio C Hamano]

Theodore Tso <tytso@mit.edu> writes:

> One thing that did cross my mind is that we could put code which
> patched ediff.el and emerge.el in /usr/share/git/lisp/... and then
> passed called emacs with something like this "emacs -l
> $sharedir/lisp/ediff-patches.el ...".  But this implies packaging
> emacs lisp files with git, and I'm not at ALL sure we want to go
> there. ...

I hope not.

> ...  Personally, I still like kdiff3 as my personal favorite
> mergetool, and given that emacs starts up pretty fast these days, I've
> given up on emacsclient, but I know there are certainly people who use
> them.

The reason I personally use emacsclient is not about the
start-up delay, but with the access to existing buffers,
keyboard macros, Gnus buffers, ... IOW the access to the
"session" while editing.  I suspect people with long running
Emacs session use emacsclient for that reason.

Re: [PATCH] git-mergetool: add support for ediff

[Theodore Tso]

On Sun, Jul 01, 2007 at 09:49:10PM -0700, Junio C Hamano wrote:
> The reason I personally use emacsclient is not about the
> start-up delay, but with the access to existing buffers,
> keyboard macros, Gnus buffers, ... IOW the access to the
> "session" while editing.  I suspect people with long running
> Emacs session use emacsclient for that reason.

Sure, but do you need access to existing buffers, keyboard, macros,
etc., if you're simply firing up an emacs to handle a merge conflict?
If the goal is just to run a merge application, then firing up a
separate process makes a lot more sense.

One other thing which I just noticed is that emacs21's emacsclient
does NOT support the -f or -e option.  And a lot of people may still
be using emacs21.  So in any case, at the moment we are in fact using
to fire up a separate process when using emerge or ediff.  I suppose
we could try testing to see if the user is running emacs21 or emacs22
if EDITOR==emacsclient, but there's no easy way of doing this short of
doing something heavyweight such as firing up emacs and asking to eval
some lisp that prints the value of emacs-version to stdout.  And even
then we would have to fix emerge to do the right thing when invoked
via emacsclient.  Yuck...

This still leaves us with the question about whether the following to
fix ediff is acceptable:

   	  emacs --eval "(progn (defun ediff-write-merge-buffer () (let ((file ediff-merge-store-file)) (set-buffer ediff-buffer-C) (write-region (point-min) (point-max) file) (message \"Merge buffer saved in: %s\" file) (set-buffer-modified-p nil) (sit-for 1))) (setq ediff-quit-hook 'kill-emacs ediff-quit-merge-hook 'ediff-write-merge-buffer) (ediff-merge-files-with-ancestor \"$LOCAL\" \"$REMOTE\" \"$BASE\" nil \"$path\"))"

In my mind it's on the hairy edge. Alternatively we just never use
ediff by default, and assume that either expert users can hack their
.emacs.el file to have the right overrides will use ediff, or who are
willing to put up with ediff's user-hostile approach to quitting an
merge session.

						 -  Ted

Re: [PATCH] git-mergetool: add support for ediff

[Sam Vilain]

Theodore Tso wrote:
> After looking at ediff, it is definitely the more polished and
> featureful compared to emerge --- except in one critical area, which
> is calling as a mergeing tool from a shell script or command line.
  [...]
> 	emacs --eval "(ediff-merge-files-with-ancestor \"$LOCAL\" \"$REMOTE\" \"$BASE\" nil \"$path\")"
> 
> Unfortunately, it's not enough.  Ediff doesn't have an "abort" command
> which returns a non-zero exit status, and when you use the "quit"
> command, it asks you a series of obnoxious questions:
> 
> Quit this Ediff session? (y or n)
> File /usr/projects/git/test/testfile.c exists, overwrite? (y or n)
> Merge buffer saved in /usr/projects/git/test/testfile.c
> <delay for 3 annoying seconds>
> Merge buffer saved.  Now kill the buffer? (y or n)

Yeah, I normally just save the merged buffer and quit.  This skips all that.

But I will add your little snippet to my .emacs :)

Sam.

Re: [PATCH] git-mergetool: add support for ediff

[Theodore Tso]

On Tue, Jul 03, 2007 at 09:32:34AM +1200, Sam Vilain wrote:
> > Unfortunately, it's not enough.  Ediff doesn't have an "abort" command
> > which returns a non-zero exit status, and when you use the "quit"
> > command, it asks you a series of obnoxious questions:
> > 
> > Quit this Ediff session? (y or n)
> > File /usr/projects/git/test/testfile.c exists, overwrite? (y or n)
> > Merge buffer saved in /usr/projects/git/test/testfile.c
> > <delay for 3 annoying seconds>
> > Merge buffer saved.  Now kill the buffer? (y or n)
> 
> Yeah, I normally just save the merged buffer and quit.  This skips all that.
> 
> But I will add your little snippet to my .emacs :)

You probably don't want to just add that snippet to your .emacs, since
it changes the ediff 'quit' command to always cause emacs to
immediately exit, and that's probably not the right thing if you are
starting ediff from an emacs session.

The correct fix would involve stealing code from emerge's
emerge-merge-files-command function to parse the arguments from the
command-line --- and in fact, probably the simplest way of fixing
things for folks would be to write replacement emerge-*-command
functions which call ediff after patching the ediff hooks in the
emacs-lisp fragment I sent above.

In fact, maybe that's the right approach.  I don't think we want to
ship emacs lisp files which git-mergetool depends upon, but what if we
instead ship some emacs lisp code in the contrib directory which a
user could slip into their .emacs file which replaces the two
emerge-*-command functions which ones that call ediff instead?

That way we don't have all of this complexity added into git-mergetool.

		 	   	      	   - Ted

Re: [PATCH] git-mergetool: add support for ediff

[Theodore Tso]

OK, so I've hacked together the following emacs-lisp snippet, which I
propose would go in contrib/use-ediff-instead.el.  If placed in your
.emacs.el file, it will cause you to use ediff instead of emerge when
you call "git mergetool".  It does so by replacing the two functions
emerge-files-command and emerge-files-with-ancestor-comand with ones
that patch the necessary ediff hooks, and then calling the ediff
package instead of the emerge package.

With this .el file, no changes are needed to git-mergetool.sh.  Does
this meet your needs?

					- Ted

;; use-ediff-instead.el
;;
;; This emacs lisp snippet should be placed in your .emacs.el file in
;; order to use the ediff package instead of emerge for git-mergetool.
;; Ediff has more whiz-bang features, but unfortunately it doesn't
;; integrate well with shell scripts that try to invoke ediff from an
;; emacs shell invocation.

(defun ediff-write-merge-buffer ()
  (let ((file ediff-merge-store-file))
    (set-buffer ediff-buffer-C)
    (write-region (point-min) (point-max) file)
    (message "Merge buffer saved in: %s" file)
    (set-buffer-modified-p nil)
    (sit-for 1)))

(defun emerge-files-command ()
  (let ((file-a (nth 0 command-line-args-left))
	(file-b (nth 1 command-line-args-left))
	(file-out (nth 2 command-line-args-left)))
    (setq command-line-args-left (nthcdr 3 command-line-args-left))
    (setq ediff-quit-hook 'kill-emacs
	  ediff-quit-merge-hook 'ediff-write-merge-buffer)
    (ediff-merge-files file-a file-b  nil file-out)))

(defun emerge-files-with-ancestor-command ()
  (let (file-a file-b file-anc file-out)
    ;; check for a -a flag, for filemerge compatibility
    (if (string= (car command-line-args-left) "-a")
	;; arguments are "-a ancestor file-a file-b file-out"
	(progn
	  (setq file-a (nth 2 command-line-args-left))
	  (setq file-b (nth 3 command-line-args-left))
	  (setq file-anc (nth 1 command-line-args-left))
	  (setq file-out (nth 4 command-line-args-left))
	  (setq command-line-args-left (nthcdr 5 command-line-args-left)))
        ;; arguments are "file-a file-b ancestor file-out"
        (setq file-a (nth 0 command-line-args-left))
        (setq file-b (nth 1 command-line-args-left))
        (setq file-anc (nth 2 command-line-args-left))
        (setq file-out (nth 3 command-line-args-left))
        (setq command-line-args-left (nthcdr 4 command-line-args-left)))
    (setq ediff-quit-hook 'kill-emacs
	  ediff-quit-merge-hook 'ediff-write-merge-buffer)
    (ediff-merge-files-with-ancestor file-a file-b file-anc nil file-out)))

;; End of use-ediff-instead.el

Re: [PATCH] git-mergetool: add support for ediff

[Junio C Hamano]

Theodore Tso <tytso@mit.edu> writes:

> On Sun, Jul 01, 2007 at 09:49:10PM -0700, Junio C Hamano wrote:
>> The reason I personally use emacsclient is not about the
>> start-up delay, but with the access to existing buffers,
>> keyboard macros, Gnus buffers, ... IOW the access to the
>> "session" while editing.  I suspect people with long running
>> Emacs session use emacsclient for that reason.
>
> Sure, but do you need access to existing buffers, keyboard, macros,
> etc., if you're simply firing up an emacs to handle a merge conflict?
>
> If the goal is just to run a merge application, then firing up a
> separate process makes a lot more sense.

Existing buffers may help somewhat as I am likely to have that
already loaded, but other than that probably not.

> In my mind it's on the hairy edge. Alternatively we just never use
> ediff by default, and assume that either expert users can hack their
> .emacs.el file to have the right overrides will use ediff, or who are
> willing to put up with ediff's user-hostile approach to quitting an
> merge session.

I think that is sane.

Re: [PATCH] git-mergetool: add support for ediff

[Sam Vilain]

Theodore Tso wrote:
> OK, so I've hacked together the following emacs-lisp snippet, which I
> propose would go in contrib/use-ediff-instead.el.  If placed in your
> .emacs.el file, it will cause you to use ediff instead of emerge when
> you call "git mergetool".  It does so by replacing the two functions
> emerge-files-command and emerge-files-with-ancestor-comand with ones
> that patch the necessary ediff hooks, and then calling the ediff
> package instead of the emerge package.
> 
> With this .el file, no changes are needed to git-mergetool.sh.  Does
> this meet your needs?
> 
> 					- Ted
> 
> ;; use-ediff-instead.el
 [...]

Thanks for that, it mostly works, however it doesn't seem to notice if I
abort without making the merge complete (on emacs21).  In my smartmerge
script (http://utsl.gen.nz/scripts/smartmerge) I detect this condition
based on the presence of merge markers, possibly dubious but pragmatic.

I still don't really understand why having to save the merged buffer and
exit is such a huge issue.  Already I have to select "-t emerge" to get
emerge.  I would have thought it would be better to just make the other
mode available, and let the user figure it out.

Sam.

Re: [PATCH] git-mergetool: add support for ediff

[Theodore Tso]

On Tue, Jul 03, 2007 at 11:19:49AM +1200, Sam Vilain wrote:
> Thanks for that, it mostly works, however it doesn't seem to notice if I
> abort without making the merge complete (on emacs21).  In my smartmerge
> script (http://utsl.gen.nz/scripts/smartmerge) I detect this condition
> based on the presence of merge markers, possibly dubious but pragmatic.

Hmm, well, here's a way of fixing it.  (See attached, below.)  It adds
a new command 'x', which when you hit it in the ediff control window,
exits with a error status of '1', indicating that the merge has
failed.  This is something which emerge, kdiff3, tkdiff, et. al all
support; but which ediff doesn't.

> I still don't really understand why having to save the merged buffer and
> exit is such a huge issue.  Already I have to select "-t emerge" to get
> emerge.  I would have thought it would be better to just make the other
> mode available, and let the user figure it out.

I'm just exploring alternatives.  Basically, it just seems interesting
that ediff has a lot of nice features, but also has some incredibly
user-hostile features.  The first time I tried using ediff, I indeed
tried saving the buffer and exiting it.  That's when I discovered that
after I changed the focus to the merge window and saved it, when I
tried typing ^X^C, the exit failed with the error message "Attempt to
delete a surrogate minibuffer frame".  That's the sort of thing that
will cause non-elisp programmers to run screaming off into the
distance.

So if you are going to save the merge the buffer and exit, you *have*
to use the 'q' command, and endure the loads of stupid questions
issued by ediff, OR, you can discover that ^X^C in the ediff control
window doesn't actually cause emacs to exit, but it does make the
ediff control window go away.  (Which is another insane bit of ediff's
UI design... why should ^X^C do something completely different in the
ediff control window?!?)

So yeah, we can add ediff as an optional support that people have to
explicitly request, but quite frankly, having played with it, I don't
know why anyone would use it without a huge number of fix ups, which
is why I was trying to make ediff actually be usable for someone who
doesn't mind typing ^X^C twice, for no good reason, after figuring out
that this illogical thing is what you actually need to do to exit
ediff.  (I actually read the help text first, so I got treated to the
really annoying ediff-quit behavior before I figured out the double
^X^C trick.)

						- Ted

;; use-ediff-instead.el
;;
;; This emacs lisp snippet should be placed in your .emacs.el file in
;; order to use the ediff package instead of emerge for git-mergetool.
;; Ediff has more whiz-bang features, but unfortunately it doesn't
;; integrate well with shell scripts that try to invoke ediff from an
;; emacs shell invocation.  This script tries to address these problems.

(defun ediff-write-merge-buffer ()
  (let ((file ediff-merge-store-file))
    (set-buffer ediff-buffer-C)
    (write-region (point-min) (point-max) file)
    (message "Merge buffer saved in: %s" file)
    (set-buffer-modified-p nil)
    (sit-for 1)))

(defun ediff-abort ()
  "Abort the ediff session without a non-zero exit status"
  (interactive)
  (kill-emacs 1))

(defun ediff-setup-abort ()
  (define-key ediff-mode-map "x" 'ediff-abort))

(defun emerge-files-command ()
  (let ((file-a (nth 0 command-line-args-left))
	(file-b (nth 1 command-line-args-left))
	(file-out (nth 2 command-line-args-left)))
    (setq command-line-args-left (nthcdr 3 command-line-args-left))
    (setq ediff-quit-hook 'kill-emacs
	  ediff-quit-merge-hook 'ediff-write-merge-buffer
	  ediff-keymap-setup-hook 'ediff-setup-abort)
    (ediff-merge-files file-a file-b  nil file-out)))

(defun emerge-files-with-ancestor-command ()
  (let (file-a file-b file-anc file-out)
    ;; check for a -a flag, for filemerge compatibility
    (if (string= (car command-line-args-left) "-a")
	;; arguments are "-a ancestor file-a file-b file-out"
	(progn
	  (setq file-a (nth 2 command-line-args-left))
	  (setq file-b (nth 3 command-line-args-left))
	  (setq file-anc (nth 1 command-line-args-left))
	  (setq file-out (nth 4 command-line-args-left))
	  (setq command-line-args-left (nthcdr 5 command-line-args-left)))
        ;; arguments are "file-a file-b ancestor file-out"
        (setq file-a (nth 0 command-line-args-left))
        (setq file-b (nth 1 command-line-args-left))
        (setq file-anc (nth 2 command-line-args-left))
        (setq file-out (nth 3 command-line-args-left))
        (setq command-line-args-left (nthcdr 4 command-line-args-left)))
    (setq ediff-quit-hook 'kill-emacs
	  ediff-quit-merge-hook 'ediff-write-merge-buffer
	  ediff-keymap-setup-hook 'ediff-setup-abort)
    (ediff-merge-files-with-ancestor file-a file-b file-anc nil file-out)))

;; End of use-ediff-instead.el

Re: [PATCH] git-mergetool: add support for ediff

[Sam Vilain]

Theodore Tso wrote:
> I'm just exploring alternatives.  Basically, it just seems interesting
> that ediff has a lot of nice features, but also has some incredibly
> user-hostile features.  The first time I tried using ediff, I indeed
> tried saving the buffer and exiting it.  That's when I discovered that
> after I changed the focus to the merge window and saved it, when I
> tried typing ^X^C, the exit failed with the error message "Attempt to
> delete a surrogate minibuffer frame".  That's the sort of thing that
> will cause non-elisp programmers to run screaming off into the
> distance.

Ouch.  Yes, I've never seen that before and no doubt if I had've I'd
feel the same way.  I just save the merge buffer and quit, and it is
pretty obedient for me.

However I guess it wouldn't be nice to have a merge mode that did not
work out of the box for a large number of users.

Your .el file certainly does the trick for me - I reckon throw it in
contrib/

Sam.

Re: [PATCH] git-mergetool: add support for ediff

[David Kastrup]

[Picking up an old thread]

Theodore Tso <tytso@mit.edu> writes:

> On Tue, Jul 03, 2007 at 11:19:49AM +1200, Sam Vilain wrote:
>
> Hmm, well, here's a way of fixing it.  (See attached, below.)  It
> adds a new command 'x', which when you hit it in the ediff control
> window, exits with a error status of '1', indicating that the merge
> has failed.  This is something which emerge, kdiff3, tkdiff, et. al
> all support; but which ediff doesn't.
>
>> I still don't really understand why having to save the merged buffer and
>> exit is such a huge issue.  Already I have to select "-t emerge" to get
>> emerge.  I would have thought it would be better to just make the other
>> mode available, and let the user figure it out.
>
> I'm just exploring alternatives.  Basically, it just seems
> interesting that ediff has a lot of nice features, but also has some
> incredibly user-hostile features.  The first time I tried using
> ediff, I indeed tried saving the buffer and exiting it.  That's when
> I discovered that after I changed the focus to the merge window and
> saved it, when I tried typing ^X^C, the exit failed with the error
> message "Attempt to delete a surrogate minibuffer frame".  That's
> the sort of thing that will cause non-elisp programmers to run
> screaming off into the distance.

Ted, I think you are somewhat missing the main audience here.  The
main audience are people who actually _use_ Emacs, and those will be
comfortable with the concept "save to have changes persist, don't save
if you don't want changes to persist, exit using C-x # or C-x C-c as
appropriate".  Basically, it would appear that you try figuring out
how to make ediff appeal to non-Emacs users.  But those would not have
emacs/emacsclient in their EDITOR variable in the first place.

I have been bitten by mergetool calling emacs rather than emacsclient,
resulting in a non-working merge (since the default directory was set
differently from what the call expected due to my use of the desktop
package), and mergetool afterwards assuming that the not-even-started
merge was successful.  A royal nuisance, and completely unworkable.

While it may be nice to have some Lisp preparation for people who
don't want to touch or learn Emacs _except_ for using it for merging
in git, I think we should first cater to people actually using Emacs
already.

-- 
David Kastrup, Kriemhildstr. 15, 44793 Bochum

Re: [PATCH] git-mergetool: add support for ediff

[Theodore Tso]

On Sat, Jul 28, 2007 at 11:22:43AM +0200, David Kastrup wrote:
> Ted, I think you are somewhat missing the main audience here.  The
> main audience are people who actually _use_ Emacs, and those will be
> comfortable with the concept "save to have changes persist, don't save
> if you don't want changes to persist, exit using C-x # or C-x C-c as
> appropriate".  Basically, it would appear that you try figuring out
> how to make ediff appeal to non-Emacs users.  But those would not have
> emacs/emacsclient in their EDITOR variable in the first place.
> 
> I have been bitten by mergetool calling emacs rather than emacsclient,
> resulting in a non-working merge (since the default directory was set
> differently from what the call expected due to my use of the desktop
> package), and mergetool afterwards assuming that the not-even-started
> merge was successful.  A royal nuisance, and completely unworkable.

Emacsclient is a completely different problem, or at least adds a
whole new dimention, compared to the ediff/emerge issue.  You can't
run either emerge or ediff using the emacsclient in emacs21, since it
lacks support for either the -e or the -f command-line option.  All
you can do in emacs21 when using eamcsclient is to request emacs to
edit a file.  

One of the problems with emacs is that it is so customizable that
people can set up emacs in such a way that different ways of launching
emacs may lead to surprises, thanks to their .emacs21.  This makes
supporting emacs based merging clients to be highly problematic.  Use
of the desktop package is one way in which things can be quite
surprising.  Worse yet, the desktop package is only in emacs22 and up.
(And emacs 22 was *just* released, not all that long ago; many people
may still be using emacs21).  So if we use emacs --no-desktop to
disable the desktop package, it will cause emacs21 to complain about
an unknown option.  Joy.  Which means that to avoid running into
problems with emacs22 users who are using the desktop package,
git-mergetool is going to have to find out in advance whether emacs21
or emacs22 (or an emacs development 23.0.0 snapshot) is in use; on a
debian system you can have 3 or 4 emacs installed simultaneously.  What fun.

In any case, the main issue is that there is an emerging (sorry)
standard about how merge tools are supposed to work, in terms of being
able to support 2-way or 3-way merges, about being able to specify
which file (and which file only, in the best case) should be used as
the output file as the result of the merge, and about how tools can
signal either a successful merge, or a request by the user to abort
the merge becuase things didn't work out for one reason or another.

The problem is that ediff doesn't really fit this model.  For people
who really want to live their life in emacs, and using emacs as their
desktop (not for me, but maybe for some folks), maybe it would be
better for those folks to simply build a git-mergetool.el that ran
100% in emacs, instead of trying to shift back and forth between the
command-line and emacs, would make everyone happier.  Right now
git-mergetool needs to ask questions about the disposition of
symlinks, permission changes, etc.  If it is done as a
git-mergetool.el which is tied into git.el and ediff, it could be a
lot more seamless.

> While it may be nice to have some Lisp preparation for people who
> don't want to touch or learn Emacs _except_ for using it for merging
> in git, I think we should first cater to people actually using Emacs
> already.

Catering to the hard-core Emacs folks is *hard*.  I knew someone who
had PDP-10 assembly language in their .emacs.el file, and one day his
custom emacs extension worked again when he started playing with the
KLH10 PDP-10 emulator, and reused his .emacs.el startup file there....
Of course, at some level folks like that will always need to fend for
themselves.

As I said earlier, I don't have a huge objection to support ediff in
some degraded mode (I think the UI is ghastly bad), if users
explicitly request it, but I would *not* want to make it the default
and spring it on some unsuspecting user.  Quite frankly, right now the
KDE and GNOME tools are way better either emerge or ediff, so they are
only really useful as a default in the terminal-only case.

     	    	      		       - Ted

Re: [PATCH] git-mergetool: add support for ediff

[David Kastrup]

Theodore Tso <tytso@mit.edu> writes:

> On Sat, Jul 28, 2007 at 11:22:43AM +0200, David Kastrup wrote:

>> Ted, I think you are somewhat missing the main audience here.  The
>> main audience are people who actually _use_ Emacs, and those will
>> be comfortable with the concept "save to have changes persist,
>> don't save if you don't want changes to persist, exit using C-x #
>> or C-x C-c as appropriate".  Basically, it would appear that you
>> try figuring out how to make ediff appeal to non-Emacs users.  But
>> those would not have emacs/emacsclient in their EDITOR variable in
>> the first place.
>> 
>> I have been bitten by mergetool calling emacs rather than
>> emacsclient, resulting in a non-working merge (since the default
>> directory was set differently from what the call expected due to my
>> use of the desktop package), and mergetool afterwards assuming that
>> the not-even-started merge was successful.  A royal nuisance, and
>> completely unworkable.
>
> Emacsclient is a completely different problem, or at least adds a
> whole new dimention, compared to the ediff/emerge issue.  You can't
> run either emerge or ediff using the emacsclient in emacs21, since
> it lacks support for either the -e or the -f command-line option.

If the user asks for it, we should try giving it to him.  If
Emacsclient bombs out because of a non-understood option, one can
still fall back to calling a separate Emacs.

> All you can do in emacs21 when using eamcsclient is to request emacs
> to edit a file.

Yes, but emacsclient --version returns a version string, and
emacsclient will exit with an error if it can't get to understand the
command line options or to talk with Emacs.  So there are reasonably
ways to notice when to fallback.

> One of the problems with emacs is that it is so customizable that
> people can set up emacs in such a way that different ways of
> launching emacs may lead to surprises, thanks to their .emacs21.
> This makes supporting emacs based merging clients to be highly
> problematic.  Use of the desktop package is one way in which things
> can be quite surprising.  Worse yet, the desktop package is only in
> emacs22 and up.

The desktop package has already been in Emacs 21, so it is not exactly
a new problem but has been round for more than 7 years.

> (And emacs 22 was *just* released, not all that long ago; many
> people may still be using emacs21).  So if we use emacs --no-desktop
> to disable the desktop package, it will cause emacs21 to complain
> about an unknown option.  Joy.

Correct: the --no-desktop option is new.

> Which means that to avoid running into problems with emacs22 users
> who are using the desktop package, git-mergetool is going to have to
> find out in advance whether emacs21 or emacs22 (or an emacs
> development 23.0.0 snapshot) is in use; on a debian system you can
> have 3 or 4 emacs installed simultaneously.  What fun.

$EDITOR --version

> In any case, the main issue is that there is an emerging (sorry)
> standard about how merge tools are supposed to work, in terms of
> being able to support 2-way or 3-way merges, about being able to
> specify which file (and which file only, in the best case) should be
> used as the output file as the result of the merge, and about how
> tools can signal either a successful merge, or a request by the user
> to abort the merge becuase things didn't work out for one reason or
> another.
>
> The problem is that ediff doesn't really fit this model.

Emacs is an editor.  If we can't make an editor fit into merge
resolution, we have a design problem.  It is a matter of convenience
that the editor is called with some initial files and something like
ediff-whatever, but the end result clearly should be that the user
writes the file if he wants changes to persist, and doesn't if
doesn't.

> For people who really want to live their life in emacs, and using
> emacs as their desktop (not for me, but maybe for some folks), maybe
> it would be better for those folks to simply build a
> git-mergetool.el that ran 100% in emacs, instead of trying to shift
> back and forth between the command-line and emacs, would make
> everyone happier.  Right now git-mergetool needs to ask questions
> about the disposition of symlinks, permission changes, etc.  If it
> is done as a git-mergetool.el which is tied into git.el and ediff,
> it could be a lot more seamless.

But this is no reason not to fix the currently broken behavior.  If
you insist that "emerge" or "ediff" is _not_ to be used as an editor,
but rather as a special-purpose mergetool for the sake of git, then
the only logical conclusion can be to call it with "-q", bypassing any
user initialization.

I believe this would be a mistake at least when $EDITOR points to
Emacs, because this means the user is used to using Emacs/Emacsclient
as _editor_, and anything else would be _confusing_.

>> While it may be nice to have some Lisp preparation for people who
>> don't want to touch or learn Emacs _except_ for using it for
>> merging in git, I think we should first cater to people actually
>> using Emacs already.
>
> Catering to the hard-core Emacs folks is *hard*.

Saving a desktop session is not hard-core.  Using emacsclient is not
hard-core.  Those are standard, basic, use cases.

> I knew someone who had PDP-10 assembly language in their .emacs.el
> file, and one day his custom emacs extension worked again when he
> started playing with the KLH10 PDP-10 emulator, and reused his
> .emacs.el startup file there....

Can we get another strawman a bit closer to the main road, please?

> Of course, at some level folks like that will always need to fend
> for themselves.

Yes.  I am not talking about people breaking things by code of their
own.  I am talking about a _standard_ setup being broken by git.

> As I said earlier, I don't have a huge objection to support ediff in
> some degraded mode (I think the UI is ghastly bad), if users
> explicitly request it, but I would *not* want to make it the default
> and spring it on some unsuspecting user.  Quite frankly, right now
> the KDE and GNOME tools are way better either emerge or ediff, so
> they are only really useful as a default in the terminal-only case.

Again, you fall into the trap of not allowing others to have a life
and Emacs outside of git's preconceptions.  emerge and ediff might be
worse for people who would not use Emacs for anything but merging.
But one advantage of Emacs is that I can look at all sorts of other
files and buffers and information sources _while_ I am merging, and
declare the merge finished when _I_ want it (signaled by exiting
either Emacs or Emacsclient), not when some arbitrary command thinks
it finished.

Emacs is one of the most flexible tools ever.  Disallowing any editing
use not foreseen and sanctioned by git-mergetool is _always_ going to
lead to trouble.  If you do _anything_ like this, you _must_ call
Emacs -q in order to omit _any_ user initializations you did not
foresee.  But this will also kill user-specific major modes which he
might want to use for visualizing files.  It will be less onerous than
having all hell break lose because you can't cater for even standard
initializations or setup, but it will still be a nuisance.

So please don't try crippling Emacs into a git-only tool.  Call it
with the files in question, give it an appropriate initial command to
work with if possible, and leave the rest to the user.  He will save
and finish, or not save and finish, which is the way of an editor to
communicate with its environment.

-- 
David Kastrup, Kriemhildstr. 15, 44793 Bochum