From: Junio C Hamano Date: Thu, 10 Dec 2009 00:08:55 GMT Subject: Re: [PATCH] mergetool--lib: add diffmerge as a pre-configured mergetool option Message-ID: <7vaaxrn10o.fsf@alter.siamese.dyndns.org> In-Reply-To: <1260302477-49412-1-git-send-email-jaysoffian@gmail.com> Jay Soffian writes: > diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh > index 5b62785..5b29fef 100644 > --- a/git-mergetool--lib.sh > +++ b/git-mergetool--lib.sh > @@ -46,7 +46,8 @@ check_unchanged () { > valid_tool () { > case "$1" in > kdiff3 | tkdiff | xxdiff | meld | opendiff | \ > - emerge | vimdiff | gvimdiff | ecmerge | diffuse | araxis | p4merge) > + emerge | vimdiff | gvimdiff | ecmerge | diffuse | araxis | p4merge | \ > + diffmerge) > ;; # happy > tortoisemerge) > if ! merge_mode; then As we are in pre-release feature freeze, it doesn't matter very much if I take this patch now, so I'll let it sit in the list archive for now. But I have to wonder about the maintainability of this file, if we have to add every time somebody finds yet another diff/merge backends that could be used, even a closed-source one. There are only a handful of entry points that mergetool--lib defines, and by overriding what should happen when these entry points are called, an end user should be able to tell mergetool/difftool to use a new backend. Perhaps it is a better approach to first eject bulk of code for the backends we currently support under these case statements into separate files, one per backend, move them to mergetool/ subdirectory in the source tree, install them as "$(share)/git-core/mergetool/$toolname", and at runtime source them? That way, a patch to add a new backend can be as simple as adding a new file in mergetool/ and doing nothing else. Also an end user can privately add support to a new backend much more easily. Anybody want to try that approach?