Re: [PATCH] mergetools/diffmerge: support DiffMerge as a git mergetool

Stefan Saasen [off-list ref] wrote:

Thanks for the review David, much appreciated.

quoted

I think this line was already too long in its current form.  Would

you mind

quoted

splitting up this long line?

I've updated the patch and had a look at how to avoid repeating the
list of
available merge/difftools.

quoted

... follow it up with a change that generalizes the "list
of tools" thing so that it can be reused here, possibly.  The
show_tool_help() function, as used by "git difftool --tool-help" and
"git mergetool --tool-help", might be a good place to look for
inspiration.

quoted

We were able to eliminate duplication in the docs (see the handling
for $(mergetools_txt) in Documentation/Makefile) so it'd be nice if

we

quoted

could do the same for git-completion.bash, somehow.

I can think of a number of approaches and I would like to get some
feedback.

Firstly I think a similar solution to how the duplication is avoided in
the
documentation can't be easily applied to the completion script. Looking
at the
script itself (and/or usage docs like
http://git-scm.com/book/en/Git-Basics-Tips-and-Tricks) the recommended
way of
using it is by copying the script as-is. That means there won't be a
build step
we could rely on unless I've overlooked something?

That leaves a different approach (run- vs. build time) where I can
think of two
possible solutions.
The first would be similar to what is being done at the moment by
looking at
the MERGE_TOOLS_DIR and in addition considering any custom merge tools
configured. I'm working with the premise that it is a reasonable
assumption
that users of the git completion script have a git installation
available even
though they may have gotten the script by other means.
For users to still be able to install the script by simply copying it
to any location
on the filesystem the list generation function(s) would either have to
be sourced
from the git installation or duplicated. I suppose the former would
need to
take into account that the completion script doesn't necessarily
matches the
installed version of git with some potential brittleness around
relying on external
files and directories. The latter doesn't buy us anything as it
duplicates even
more code than the current list of available mergetools.

The second approach would be to do something similar to resolving the
merge
strategies (in __git_list_merge_strategies) by parsing the output of
the `git
merge tool --tools-help` option with a very similar disadvantage that
it relies
on the textual output of the help command and doesn't work outside of a
git
repository.


I'm currently leaning towards the last approach as it seems less
reliant on
implementation details but it doesn't look ideal either and I may be
missing
another approach that would be better suited.

I agree that this seems like the way to go. Perhaps we can add git mergetool/difftool --list-tools which can print the available tools so that the completion can use it. 

quoted

It might be worth leaving the git-completion.bash bits alone in this
first patch and follow it up with a change that generalizes the "list
of tools" thing so that it can be reused here, possibly.

To decouple this and adding the diffmerge merge tool option, I'd rather
keep the
git-completion change part of the patch. That way the patch is self
contained
and covers the change including the completion using the current
approach and
doesn't rely on the duplication change. Any concerns around that,
otherwise I'll
resend the patch with only the long line fixed?

That sounds good, we can keep these as separate patches. 

Thanks,

-- 
David