On Sat, Feb 16, 2008 at 12:04:26PM -0800, Junio C Hamano wrote: > Charles Bailey <charles@xxxxxxxxxxxxx> 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. > > 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 --- > 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. - To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
