patchgit-mergetool: add support for ediff

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

On 6/28/07, Sam Vilain <sam.vilain@catalyst.net.nz> wrote:

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:

and then this

makes this

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?

On Thu, Jun 28, 2007 at 06:31:50PM -0700, Jason Sewall wrote:

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

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

On Fri, Jun 29, 2007 at 12:03:28AM -0400, 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. 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

Theodore Tso <tytso@mit.edu> writes:

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.

On Sun, Jul 01, 2007 at 07:32:59PM -0700, Junio C Hamano wrote:

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

Theodore Tso <tytso@mit.edu> writes:

I hope not.

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.

On Sun, Jul 01, 2007 at 09:49:10PM -0700, Junio C Hamano wrote:

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

Theodore Tso <tytso@mit.edu> writes:

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

I think that is sane.

Theodore Tso wrote:

  [...]

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.

On Tue, Jul 03, 2007 at 09:32:34AM +1200, Sam Vilain wrote:

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

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

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

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.

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

Theodore Tso wrote:

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.

[Picking up an old thread]

Theodore Tso <tytso@mit.edu> writes:

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.

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

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.

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

Theodore Tso <tytso@mit.edu> writes:

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.

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.

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.

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

$EDITOR --version

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.

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

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

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

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.

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.