Re: [Untested! proposal] git-mergetool.sh: introduce ediff option

From: Theodore Tso <tytso@mit.edu>
Date: Jul 29, 2007, 20:52 UTC
Message-ID: <20070729205232.GA10148@thunk.org>
In-Reply-To: <85hcnnwblu.fsf@lola.goethe.zz>

On Sun, Jul 29, 2007 at 03:51:34PM +0200, David Kastrup wrote:

Show 5 quoted lines

> 
> Most actual Emacs users prefer ediff to emerge concerning the
> consolidation of versions.  In general, people habitually using Emacs
> will have this preference reflected in the EDITOR/VISUAL environment
> variables.

Proof, please? Do you have any polls? What evidence do you have? For the past two decades, I have EDITOR set to emacs, but I am not an ediff fan. Yes, that's anecdotal evidence, but so are your assertions.

> If such a preference can be found there, ediff will be used/offered in
> preference of emerge (which retains its previous behavior).

Ediff is currently far more confusing for someone who just uses emacs as an editor. There are plenty of users who never learned the vi commands, but who use emacs as a reasonably easy-to-use text editor. Not everyone who uses emacs is a power-user.....

> In ediff mode, success or failure of the merge will be discerned by
> Emacs either having written or not written the merge buffer; no
> attempt of interpreting the exit code is made.

Sometimes resolving the merge file results in no changes. So the fact that ediff is buggy in that it doesn't return an exit code is a real problem. We could possibly work around the problem saving and then checking the modtime --- but only if ediff actually ends up rewriting the file.

> In order to bypass things like desktop files being loaded, emerge mode
> now passes the "-q" option to Emacs.  This will make it work in more
> situations likely to occur, at the price of excluding possibly
> harmless user customizations with the rest.

But that screws over users who want their customizations, but who don't use the desktop package. (And I have a news flash for you; the desktop package is *not* include as part of emacs21. It's not part of Debian's emacs21 package, version 21.4.) So do not believe your claim that emacs's desktop package is commonly used.

Probably a better choice is a config parameter which allows users to specify a set of options to be passed to emacs when git fires up an emacs program. That would allow some people to specfy --no-desktop if they are using a new enough emacs program that supports it. It would also allow users to use other emacs command-line options that they might like, i.e., -nw, or --title, etc.

Show 9 quoted lines

> +	ediff)
> +	    case "${EDITOR:-${VISUAL:-emacs}}" in
> +		*/emacs*|*/gnuclient*|*/xemacs*)
> +		    emacs_candidate="${EDITOR:-${VISUAL:-emacs}}";;
> +		*)
> +		    emacs_candidate=emacs;;
> +	    esac
> +	    if base_present ; then
> +		${emacs_candidate} --eval "(ediff-merge-files-with-ancestor (pop command-line-args-left) (pop command-line-args-left) (pop command-line-args-left) nil (pop-command-line-args-left))" "$LOCAL" "$REMOTE" "$BASE" "$path"

... and this will blow up if EMACS is set to emacsclient, and emacs version is 21. (And BTW, Debian stable and the current Ubuntu, Edgy Eft, are still shipping emacs21. So are a number of current major distro's. So if you think the vast majority of users are using emacs22, you are either on drugs, and have a very skewed view of what are "normal" emacs users.)

There is a reason why git-mergetool currently hardcodes the use of "emacs", instead of just blindly using the value of $EDITOR or $VISUAL. So what you're doing here in your patch is completely busted. If you insist on using emacs_candidate, we need to run emacs --version and parse the output, and only using the value of EMACS or VISUAL if the major version number of emacs is at least 22.

(It would probably be a good idea to do this once and cache the result, so we don't have to repeatedly for each file that git mergetool needs to process.)

Show 7 quoted lines

> -    if echo "${VISUAL:-$EDITOR}" | grep 'emacs' > /dev/null 2>&1; then
> -        merge_tool_candidates="$merge_tool_candidates emerge"
> -    fi
> +    case "${EDITOR:-${VISUAL}}" in
> +	*/emacs*|*/gnuclient*|*/xemacs*)
> +            merge_tool_candidates="$merge_tool_candidates ediff"
> +    esac

Changing the default from emerge to ediff is a non-starter, sorry. If you really want to use ediff, you can set a config parameter to explicitly request it.

						- Ted