Re: [PATCH] mergetools/diffmerge: support DiffMerge as a git mergetool
Stefan Saasen ssaa...@atlassian.com wrote: 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. 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. 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 -- 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
Re: [PATCH] mergetools/diffmerge: support DiffMerge as a git mergetool
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
Re: [PATCH] mergetools/diffmerge: support DiffMerge as a git mergetool
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 000..85ac720
--- /dev/null
+++ b/mergetools/diffmerge
@@ -0,0 +1,15 @@
+diff_cmd () {
+ $merge_tool_path $LOCAL $REMOTE /dev/null 21
+}
+
+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
