Re: [RFC/PATCH] Teach git mergetool to use custom commands defined at config time

From: Charles Bailey <charles@hashpling.org>
Date: Feb 16, 2008, 20:20 UTC
Message-ID: <20080216202055.GA30146@hashpling.org>
In-Reply-To: <7vzlu07jn9.fsf@gitster.siamese.dyndns.org>

On Sat, Feb 16, 2008 at 12:04:26PM -0800, Junio C Hamano wrote:

Show 19 quoted lines

> Charles Bailey <charles@hashpling.org> writes:
> 
> > It follows filter-branch's 'eval a user shell snippet' philosophy to
> > provide the flexibility and here in lies an ugliness. It exposes
> > git-mergetool.sh's private variables to the user script. The variables
> > are BASE, REMOTE, LOCAL and path.
> >
> > My feeling is that we should give this consistent and documented
> > names, perhaps GIT_BASE, GIT_REMOTE, GIT_LOCAL, GIT_MERGED or similar.
> 
> I like the general idea and you are right that the external
> interface should not expose the variable names the current
> implementation happens to use.  I do not think it is necessary
> to rename them to GIT_BASE etc., but I do think we need to list
> the repertoire of variables that can be expected to be usable by
> custom script in any future version of mergetool (even after it
> is rewritten in C).  Anybody who uses a variable that is not in
> the documented set that the current implementation uses can be
> broken ;-).

OK, I'm fine with this, although I think we should use something other than path (in lower case) for the output. Either MERGED, RESULT or OUTPUT, say, I've no strong preferences.

> Also perhaps we would want to spawn the eval in a subprocess to
> make it clear that the custom script cannot affect the caller's
> variables.

Sub-shell, good plan.

Show 5 quoted lines

> > Also, does anyone know of any reason why the temporary files should
> > not be cleaned up after an unsuccessful merge?
> 
> I do not use mergetool myself, but presumably it would be to
> make the manual inspection easier.

I don't use it a lot (yet) either, but I think that this sort of change well help git get wider acceptance in the "but does it work with my favourite 3-way merge tool" community.

As far as it goes, though, I find that if I fire up a merge tool and it looks complex, I quite often want to abort and look at the easy files first. The .BASE/.LOCAL/.REMOTE files don't contain anything that isn't in the index anyway (the merge tool certainly should be updating them with intermediate state).

It's a separate issue to the main thrust of this patch, though.

> > +mergetool.<tool>.keepBackup::
>

--- snip ---

Show 13 quoted lines

> Shouldn't the handling of back-up files be the same across
> backends?
> 
> If the answer is yes, it makes mergetool.<tool>.keepBackup
> configuration a quite bogus variable, as it is not something you
> would configure per backend.
> 
> In the existing code, I see kdiff3 arm calls remove_backup while
> tkdiff arm and others call save_backup, which seems quite
> inconsistent.  Perhaps mergetool needs a command line option
> (and perhaps a single configuration variable independent from
> which backend is used) to tell what to do about them after a
> conflicting merge is resolved and/or resolution is aborted.

Yes, I think that you're right. I couldn't work out why kdiff3 was special, but I took the safe/wrong path of assuming that it should be configurable per backend. I think that it might be something that should be configurable, but the variable should probably me mergetool.keepBackup and the patch should be separate from this one.

Charles.