Thanks for the review David, much appreciated. > I think this line was already too long in its current form. Would you mind > 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. > ... 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. > 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 > 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. > 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? Cheers, Stefan On 6 October 2013 14:21, David Aguilar <dav...@gmail.com> wrote: > > On Sat, Oct 5, 2013 at 1:29 AM, Stefan Saasen <ssaa...@atlassian.com> wrote: > > DiffMerge is a non-free (but gratis) tool that supports OS X, Windows and > > Linux. > > > > See http://www.sourcegear.com/diffmerge/ > > > > DiffMerge includes a script `/usr/bin/diffmerge` that can be used to launch > > the > > graphical compare tool. > > > > This change adds mergetool support for DiffMerge and adds 'diffmerge' as an > > option to the mergetool help. > > > > Signed-off-by: Stefan Saasen <ssaa...@atlassian.com> > > --- > > contrib/completion/git-completion.bash | 2 +- > > git-mergetool--lib.sh | 2 +- > > mergetools/diffmerge | 15 +++++++++++++++ > > 3 files changed, 17 insertions(+), 2 deletions(-) > > create mode 100644 mergetools/diffmerge > > > > diff --git a/contrib/completion/git-completion.bash > > b/contrib/completion/git-completion.bash > > index e1b7313..07b0ba5 100644 > > --- a/contrib/completion/git-completion.bash > > +++ b/contrib/completion/git-completion.bash > > @@ -1188,7 +1188,7 @@ _git_diff () > > __git_complete_revlist_file > > } > > > > -__git_mergetools_common="diffuse ecmerge emerge kdiff3 meld opendiff > > +__git_mergetools_common="diffuse diffmerge ecmerge emerge kdiff3 meld > > opendiff > > tkdiff vimdiff gvimdiff xxdiff araxis p4merge bc3 > > codecompare > > " > > It's a little unfortunate that we have to keep repeating ourselves > here. mergetool--lib has a list_merge_tool_candidate() that populates > $tools and help us avoid having to maintain these lists in separate > files. > > 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. 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. > > 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 > could do the same for git-completion.bash, somehow. > > > diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh > > index feee6a4..6d0fa3b 100644 > > --- a/git-mergetool--lib.sh > > +++ b/git-mergetool--lib.sh > > @@ -250,7 +250,7 @@ list_merge_tool_candidates () { > > else > > tools="opendiff kdiff3 tkdiff xxdiff meld $tools" > > fi > > - tools="$tools gvimdiff diffuse ecmerge p4merge araxis bc3 > > codecompare" > > + tools="$tools gvimdiff diffuse diffmerge ecmerge p4merge > > araxis bc3 codecompare" > > I think this line was already too long in its current form. Would you > mind splitting up this long line? > > > fi > > case "${VISUAL:-$EDITOR}" in > > *vim*) > > diff --git a/mergetools/diffmerge b/mergetools/diffmerge > > new file mode 100644 > > index 0000000..85ac720 > > --- /dev/null > > +++ b/mergetools/diffmerge > > @@ -0,0 +1,15 @@ > > +diff_cmd () { > > + "$merge_tool_path" "$LOCAL" "$REMOTE" >/dev/null 2>&1 > > +} > > + > > +merge_cmd () { > > + if $base_present > > + then > > + "$merge_tool_path" --merge --result="$MERGED" \ > > + "$LOCAL" "$BASE" "$REMOTE" > > + else > > + "$merge_tool_path" --merge \ > > + --result="$MERGED" "$LOCAL" "$REMOTE" > > + fi > > + status=$? > > +} > > Other than those two minor notes, this looks good to me. > > Thanks, > -- > David -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
