Re: [RFC/PATCH] Teach git mergetool to use custom commands defined at config time
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Feb 16, 2008, 20:04 UTC
- Message-ID
- <7vzlu07jn9.fsf@gitster.siamese.dyndns.org>
- In-Reply-To
- <20080216185349.GA29177@hashpling.org>
Charles Bailey <charles@hashpling.org> writes:
Show 7 quoted lines
> 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 ;-).
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.
> 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.
> +mergetool.<tool>.keepBackup:: > + After performing a merge, the original file with conflict markers > + can be saved as a file with a `.orig` extension. If this variable > + is set to `false` then this file is not preserved.
I doubt this belongs to this patch, so does ...
Show 6 quoted lines
> + if test "$merge_tool_keep_backup" = "true"; then > + save_backup > + else > + remove_backup > + fi > + fi
... this part and anything else that deals with the backup files in the patch.
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.