Various merge / diff tool related minor clean-ups and improvements
cover Various merge / diff tool related minor clean-ups and improvements
This series introduce various minor clean-ups and improvements to the merge / diff tool scripts and documentation.
Sebastian Schuberth (4):
Use variables for the lists of tools that support merging / diffing Explicitly list all valid diff tools and document --tool-help as an
option
Make sure to use Araxis' "compare" and not e.g. ImageMagick's Add a few more code comments and blank lines in guess_merge_tool
Documentation/git-difftool.txt | 9 ++++++--- contrib/completion/git-completion.bash | 11 +++++++++-- git-mergetool--lib.sh | 6 ++++++ mergetools/araxis | 8 +++++++- 4 files changed, 28 insertions(+), 6 deletions(-)
Signature · 1 line
1.7.11.msysgit.2
v2 cover Various merge / diff tool related minor clean-ups and improvements
This series introduces various minor clean-ups and improvements to the merge / diff tool scripts and documentation.
Sorry, the first version was missing a patch.
Sebastian Schuberth (5):
Sort the list of tools that support both merging and diffing
alphabetically
Use variables for the lists of tools that support merging / diffing Explicitly list all valid diff tools and document --tool-help as an
option
Make sure to use Araxis' "compare" and not e.g. ImageMagick's Add a few more code comments and blank lines in guess_merge_tool
Documentation/git-difftool.txt | 9 ++++++--- contrib/completion/git-completion.bash | 15 +++++++++++---- git-mergetool--lib.sh | 6 ++++++ mergetools/araxis | 8 +++++++- 4 files changed, 30 insertions(+), 8 deletions(-)
Signature · 1 line
1.7.11.msysgit.2
PATCH v2 1/5 Sort the list of tools that support both merging and diffing alphabetically
--- contrib/completion/git-completion.bash | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
contrib/completion/git-completion.bash +2 -2
@@ -1325,8 +1325,8 @@ _git_diff () __git_complete_revlist_file } -__git_mergetools_common="diffuse ecmerge emerge kdiff3 meld opendiff - tkdiff vimdiff gvimdiff xxdiff araxis p4merge bc3 +__git_mergetools_common="araxis bc3 diffuse ecmerge emerge gvimdiff + kdiff3 meld opendiff p4merge tkdiff vimdiff xxdiff " _git_difftool ()
Signature · 1 line
1.7.11.msysgit.2
PATCH v2 2/5 Use variables for the lists of tools that support merging / diffing
In reply to Sebastian SchuberthAlso, add a few comments that clarify the meaning of these variables.
--- contrib/completion/git-completion.bash | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-)
contrib/completion/git-completion.bash +9 -2
@@ -1325,17 +1325,24 @@ _git_diff () __git_complete_revlist_file } +# Tools that support both merging and diffing. __git_mergetools_common="araxis bc3 diffuse ecmerge emerge gvimdiff kdiff3 meld opendiff p4merge tkdiff vimdiff xxdiff " +# Tools that support diffing. +__git_difftools="$__git_mergetools_common kcompare" + +# Tools that support merging. +__git_mergetools="$__git_mergetools_common tortoisemerge" + _git_difftool () { __git_has_doubledash && return case "$cur" in --tool=*) - __gitcomp "$__git_mergetools_common kompare" "" "${cur##--tool=}" + __gitcomp "$__git_difftools" "" "${cur##--tool=}" return ;; --*)
@@ -1623,7 +1630,7 @@ _git_mergetool () { case "$cur" in --tool=*) - __gitcomp "$__git_mergetools_common tortoisemerge" "" "${cur##--tool=}" + __gitcomp "$__git_mergetools" "" "${cur##--tool=}" return ;; --*)
Signature · 1 line
1.7.11.msysgit.2
Re: [PATCH v2 2/5] Use variables for the lists of tools that support merging / diffing
Quoted 12 lines · Sebastian Schuberth
Also, add a few comments that clarify the meaning of these variables.
Signed-off-by Sebastian Schuberth <[email protected]>--- contrib/completion/git-completion.bash | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-)contrib/completion/git-completion.bash +0
-0
contrib/completion/git-completion.bash _git_diff () · line 1331
Quoted 7 lines
contrib/completion/git-completion.bash +1
-0@@ -1325,17 +1325,24 @@ _git_diff () __git_complete_revlist_file } +# Tools that support both merging and diffing. __git_mergetools_common="araxis bc3 diffuse ecmerge emerge gvimdiff kdiff3 meld opendiff p4merge tkdiff vimdiff xxdiff "
As the set of merge capable tools is not a superset of diff capable tools (tortoise can only merge but not diff), perhaps rename this to __git_diffmerge_tools or something?
contrib/completion/git-completion.bash _git_diff () · line 1338
Quoted 6 lines
contrib/completion/git-completion.bash +6
-0+# Tools that support diffing. +__git_difftools="$__git_mergetools_common kcompare" + +# Tools that support merging. +__git_mergetools="$__git_mergetools_common tortoisemerge" +
This patch makes sense to me, but at the same time makes [PATCH 1/5] a "Meh", methinks.
Quoted 20 lines
contrib/completion/git-completion.bash +2
-2_git_difftool () { __git_has_doubledash && return case "$cur" in --tool=*) - __gitcomp "$__git_mergetools_common kompare" "" "${cur##--tool=}" + __gitcomp "$__git_difftools" "" "${cur##--tool=}" return ;; --*)@@ -1623,7 +1630,7 @@ _git_mergetool () { case "$cur" in --tool=*) - __gitcomp "$__git_mergetools_common tortoisemerge" "" "${cur##--tool=}" + __gitcomp "$__git_mergetools" "" "${cur##--tool=}" return ;; --*)
Re: [PATCH v2 2/5] Use variables for the lists of tools that support merging / diffing
Quoted 8 lines · Junio C Hamano
Quoted 4 lines
+# Tools that support both merging and diffing.
__git_mergetools_common="araxis bc3 diffuse ecmerge emerge gvimdiff
kdiff3 meld opendiff p4merge tkdiff vimdiff xxdiff
"As the set of merge capable tools is not a superset of diff capable tools (tortoise can only merge but not diff), perhaps rename this to
__git_diffmerge_toolsor something?
Makes perfect sense.
Quoted 2 lines
This patch makes sense to me, but at the same time makes [PATCH 1/5] a "Meh", methinks.
Yeah, I can see why. So I've renamed __git_mergetools_common to __git_diffmerge_tools and squashed with [PATCH 1/5] to make it less "Meh" as it does not stand on its own.
Signature · 1 line
Sebastian
PATCH 1/4 Use variables for the lists of tools that support merging / diffing
Also, add a few comments that clarify the meaning of these variables and sort the list of tools alphabetically.
--- contrib/completion/git-completion.bash | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-)
contrib/completion/git-completion.bash +11 -4
@@ -1325,17 +1325,24 @@ _git_diff () __git_complete_revlist_file } -__git_mergetools_common="diffuse ecmerge emerge kdiff3 meld opendiff - tkdiff vimdiff gvimdiff xxdiff araxis p4merge bc3 +# Tools that support both merging and diffing. +__git_diffmerge_tools="araxis bc3 diffuse ecmerge emerge gvimdiff + kdiff3 meld opendiff p4merge tkdiff vimdiff xxdiff " +# Tools that support diffing. +__git_difftools="$__git_diffmerge_tools kcompare" + +# Tools that support merging. +__git_mergetools="$__git_diffmerge_tools tortoisemerge" + _git_difftool () { __git_has_doubledash && return case "$cur" in --tool=*) - __gitcomp "$__git_mergetools_common kompare" "" "${cur##--tool=}" + __gitcomp "$__git_difftools" "" "${cur##--tool=}" return ;; --*)
@@ -1623,7 +1630,7 @@ _git_mergetool () { case "$cur" in --tool=*) - __gitcomp "$__git_mergetools_common tortoisemerge" "" "${cur##--tool=}" + __gitcomp "$__git_mergetools" "" "${cur##--tool=}" return ;; --*)
Signature · 1 line
1.7.11.msysgit.2
Re: [PATCH v2 2/5] Use variables for the lists of tools that support merging / diffing
In reply to Sebastian SchuberthQuoted 6 lines · Sebastian Schuberth
Quoted 2 lines
This patch makes sense to me, but at the same time makes [PATCH 1/5] a "Meh", methinks.
Yeah, I can see why. So I've renamed
__git_mergetools_commonto__git_diffmerge_toolsand squashed with [PATCH 1/5] to make it less "Meh" as it does not stand on its own.
As you append kcompare or tortoise _after_ the common list, any code that uses the variable cannot assume that the list is sorted, and needs to sort the elements if it wants to give a sorted output, so squashing does not make the Meh-ness go away.
By the way, would it make sense to remove these three variables from the completion code, and instead ask "git mergetool --tool-help" when it needs the list of supported tools for the first time? It would be trivial to introduce --tool-list that gives a one tool per line output to both "git difftool" and "git mergetool" and we would remove the risk of separately maintained list drifting away over time.
Re: [PATCH v2 2/5] Use variables for the lists of tools that support merging / diffing
Quoted 11 lines · Junio C Hamano
Quoted 6 lines
Quoted 2 lines
This patch makes sense to me, but at the same time makes [PATCH 1/5] a "Meh", methinks.
Yeah, I can see why. So I've renamed
__git_mergetools_commonto__git_diffmerge_toolsand squashed with [PATCH 1/5] to make it less "Meh" as it does not stand on its own.As you append kcompare or tortoise _after_ the common list, any code that uses the variable cannot assume that the list is sorted, and needs to sort the elements if it wants to give a sorted output, so squashing does not make the Meh-ness go away.
Well, that (mostly) sorted listed still helps to find out a little quicker whether a specific tool that can do both merging and diffing is already in the list. At least that's the case for me.
Quoted 7 lines
By the way, would it make sense to remove these three variables from the completion code, and instead ask "git mergetool --tool-help" when it needs the list of supported tools for the first time? It would be trivial to introduce --tool-list that gives a one tool per line output to both "git difftool" and "git mergetool" and we would remove the risk of separately maintained list drifting away over time.
Sounds like a good idea now that you've added "git mergetool --tool-help". But I'd like to save this for a future exercise to not do too much stuff at the same time.
Signature · 1 line
Sebastian Schuberth
PATCH v2 3/5 Explicitly list all valid diff tools and document --tool-help as an option
In reply to Sebastian Schuberth--- Documentation/git-difftool.txt | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)
Documentation/git-difftool.txt +6 -3
@@ -36,9 +36,12 @@ OPTIONS -t <tool>:: --tool=<tool>:: - Use the diff tool specified by <tool>. Valid values include - emerge, kompare, meld, and vimdiff. Run `git difftool --tool-help` - for the list of valid <tool> settings. + Use the diff tool specified by <tool>. Valid diff tools are: + araxis, bc3, diffuse, ecmerge, emerge, gvimdiff, kcompare, kdiff3, + meld, opendiff, p4merge, tkdiff, vimdiff and xxdiff. + +--tool-help:: + List the supported and available diff tools. + If a diff tool is not specified, 'git difftool' will use the configuration variable `diff.tool`. If the
Signature · 1 line
1.7.11.msysgit.2
Re: [PATCH 3/5] Explicitly list all valid diff tools and document --tool-help as an option
Quoted 10 lines · Sebastian Schuberth
Signed-off-by Sebastian Schuberth <[email protected]>--- Documentation/git-difftool.txt | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)Documentation/git-difftool.txt +0
-0
Documentation/git-difftool.txt OPTIONS · line 41
Quoted 6 lines
Documentation/git-difftool.txt +0
-3@@ -36,9 +36,12 @@ OPTIONS -t <tool>:: --tool=<tool>:: - Use the diff tool specified by <tool>. Valid values include - emerge, kompare, meld, and vimdiff. Run `git difftool --tool-help` - for the list of valid <tool> settings.
I thought we say "include" because we really do not want to list millions of tools here, so mild NAK on this part.
Documentation/git-difftool.txt OPTIONS · line 44
Quoted 6 lines
Documentation/git-difftool.txt +6
-0+ Use the diff tool specified by <tool>. Valid diff tools are: + araxis, bc3, diffuse, ecmerge, emerge, gvimdiff, kcompare, kdiff3, + meld, opendiff, p4merge, tkdiff, vimdiff and xxdiff. + +--tool-help:: + List the supported and available diff tools.
This part is a good addition (but it already is mentioned in the description of --tool above, so it is more or less a "Meh").
Quoted 3 lines
Documentation/git-difftool.txt +0
-0+ If a diff tool is not specified, 'git difftool' will use the configuration variable `diff.tool`. If the
mergetool: support --tool-help option like difftool does
This way we do not have to risk the list of tools go out of sync between the implementation and the documentation.
---
Quoted 5 lines · Junio C Hamano
Review
Quoted 2 lines
+--tool-help::
(quoted diff) +1
-0+ List the supported and available diff tools.This part is a good addition (but it already is mentioned in the description of --tool above, so it is more or less a "Meh").
I noticed that the main while loop has style violations in its case/esac, but I left it as-is. Reindenting it would be a low hanging fruit janitor patch that would be a separate topic.
git-mergetool--lib.sh | 6 +++++- git-mergetool.sh | 42 +++++++++++++++++++++++++++++++++++++++++- 2 files changed, 46 insertions(+), 2 deletions(-)
git-mergetool--lib.sh +5 -1
@@ -111,7 +111,7 @@ run_merge_tool () { return $status } -guess_merge_tool () { +list_merge_tool_candidates () { if merge_mode then tools="tortoisemerge"
@@ -136,6 +136,10 @@ guess_merge_tool () { tools="$tools emerge vimdiff" ;; esac +} + +guess_merge_tool () { + list_merge_tool_candidates echo >&2 "merge tool candidates: $tools" # Loop over each candidate and stop when a valid merge tool is found.
git-mergetool.sh +41 -1
59 lines not on this page · raw message
Re: mergetool: support --tool-help option like difftool does
Quoted 4 lines · Junio C Hamano
This way we do not have to risk the list of tools go out of sync between the implementation and the documentation.
Signed-off-by Junio C Hamano <[email protected]>
Thanks! I've squashed this with
[PATCH v2 5/5] Add a few more code comments and blank lines in guess_merge_tool
and parts of
[PATCH 3/5] Explicitly list all valid diff tools and document --tool-help as an option
and adjusted the docs for git-mergetool accordingly.
Signature · 1 line
Sebastian Schuberth
PATCH 1/1 mergetool: support --tool-help option like difftool does
This way we do not have to risk the list of tools go out of sync between the implementation and the documentation. Adjust the documentation accordingly to not explicitly list the tools but refer to --tool-help.
--- Documentation/git-difftool.txt | 7 ++++--- Documentation/git-mergetool.txt | 8 ++++---- git-mergetool--lib.sh | 11 ++++++++++- git-mergetool.sh | 42 ++++++++++++++++++++++++++++++++++++++++- 4 files changed, 59 insertions(+), 9 deletions(-)
Documentation/git-difftool.txt +4 -3
@@ -36,9 +36,10 @@ OPTIONS -t <tool>:: --tool=<tool>:: - Use the diff tool specified by <tool>. Valid values include - emerge, kompare, meld, and vimdiff. Run `git difftool --tool-help` - for the list of valid <tool> settings. + Use the diff tool specified by <tool>. + +--tool-help:: + List all supported values for <tool>. + If a diff tool is not specified, 'git difftool' will use the configuration variable `diff.tool`. If the
Documentation/git-mergetool.txt +4 -4
@@ -26,10 +26,10 @@ OPTIONS ------- -t <tool>:: --tool=<tool>:: - Use the merge resolution program specified by <tool>. - Valid merge tools are: - araxis, bc3, diffuse, ecmerge, emerge, gvimdiff, kdiff3, - meld, opendiff, p4merge, tkdiff, tortoisemerge, vimdiff and xxdiff. + Use the merge tool specified by <tool>. + +--tool-help:: + List all supported values for <tool>. + If a merge resolution program is not specified, 'git mergetool' will use the configuration variable `merge.tool`. If the
git-mergetool--lib.sh +10 -1
@@ -111,15 +111,18 @@ run_merge_tool () { return $status } -guess_merge_tool () { +list_merge_tool_candidates () { + # Add tools that can either do merging or diffing, but not both. if merge_mode then tools="tortoisemerge" else tools="kompare" fi + if test -n "$DISPLAY" then + # Prefer GTK-based tools under Gnome. if test -n "$GNOME_DESKTOP_SESSION_ID" then tools="meld opendiff kdiff3 tkdiff xxdiff $tools"
@@ -128,6 +131,8 @@ guess_merge_tool () { fi tools="$tools gvimdiff diffuse ecmerge p4merge araxis bc3" fi + + # Prefer vimdiff if vim is the default editor. case "${VISUAL:-$EDITOR}" in *vim*) tools="$tools vimdiff emerge"
@@ -136,6 +141,10 @@ guess_merge_tool () { tools="$tools emerge vimdiff" ;; esac +} + +guess_merge_tool () { + list_merge_tool_candidates echo >&2 "merge tool candidates: $tools" # Loop over each candidate and stop when a valid merge tool is found.
git-mergetool.sh +41 -1
59 lines not on this page · raw message
Signature · 1 line
1.7.11.msysgit.2
Re: [PATCH] mergetool: support --tool-help option like difftool does
Quoted 18 lines · Sebastian Schuberth
This way we do not have to risk the list of tools go out of sync between the implementation and the documentation. Adjust the documentation accordingly to not explicitly list the tools but refer to --tool-help.
Signed-off-by Junio C Hamano <[email protected]>Signed-off-by Sebastian Schuberth <[email protected]>--- Documentation/git-difftool.txt | 7 ++++--- Documentation/git-mergetool.txt | 8 ++++---- git-mergetool--lib.sh | 11 ++++++++++- git-mergetool.sh | 42 ++++++++++++++++++++++++++++++++++++++++- 4 files changed, 59 insertions(+), 9 deletions(-)Documentation/git-difftool.txt +0
-0
Documentation/git-difftool.txt OPTIONS · line 39
Quoted 7 lines
Documentation/git-difftool.txt +1
-3@@ -36,9 +36,10 @@ OPTIONS -t <tool>:: --tool=<tool>:: - Use the diff tool specified by <tool>. Valid values include - emerge, kompare, meld, and vimdiff. Run `git difftool --tool-help` - for the list of valid <tool> settings. + Use the diff tool specified by <tool>.
I do not see how it is an improvement to drop the most common ones. People sometimes read documentation without having an access to shell to run "cmd --tool-help", and a list of handful of well known ones would serve as a good hint to let the reader know the kind of commands the front-end is capable of spawning, which in turn help such a reader to imagine how the command is used to judge if it is something the reader wants to use.
Quoted 11 lines
Documentation/git-difftool.txt +3
-0+ +--tool-help:: + List all supported values for <tool>. + If a diff tool is not specified, 'git difftool' will use the configuration variable `diff.tool`. If theDocumentation/git-mergetool.txt +0
-0
Documentation/git-mergetool.txt OPTIONS · line 29
Quoted 8 lines
Documentation/git-mergetool.txt +1
-4@@ -26,10 +26,10 @@ OPTIONS ------- -t <tool>:: --tool=<tool>:: - Use the merge resolution program specified by <tool>. - Valid merge tools are: - araxis, bc3, diffuse, ecmerge, emerge, gvimdiff, kdiff3, - meld, opendiff, p4merge, tkdiff, tortoisemerge, vimdiff and xxdiff. + Use the merge tool specified by <tool>.
Likewise. Dropping araxis, bc3, etc. to trim the list to 4-5 items that people would know what they are would make sense, though. The purpose of the list is not to tell if the reader's favorite tool is supported or not---it is to hint the flavor of what kind of things the command spawned would be capable of.
Quoted 115 lines
Documentation/git-mergetool.txt +3
-0+ +--tool-help:: + List all supported values for <tool>. + If a merge resolution program is not specified, 'git mergetool' will use the configuration variable `merge.tool`. If thegit-mergetool--lib.sh +10
-1@@ -111,15 +111,18 @@ run_merge_tool () { return $status } -guess_merge_tool () { +list_merge_tool_candidates () { + # Add tools that can either do merging or diffing, but not both. if merge_mode then tools="tortoisemerge" else tools="kompare" fi + if test -n "$DISPLAY" then + # Prefer GTK-based tools under Gnome. if test -n "$GNOME_DESKTOP_SESSION_ID" then tools="meld opendiff kdiff3 tkdiff xxdiff $tools"@@ -128,6 +131,8 @@ guess_merge_tool () { fi tools="$tools gvimdiff diffuse ecmerge p4merge araxis bc3" fi + + # Prefer vimdiff if vim is the default editor. case "${VISUAL:-$EDITOR}" in *vim*) tools="$tools vimdiff emerge"@@ -136,6 +141,10 @@ guess_merge_tool () { tools="$tools emerge vimdiff" ;; esac +} + +guess_merge_tool () { + list_merge_tool_candidates echo >&2 "merge tool candidates: $tools" # Loop over each candidate and stop when a valid merge tool is found.git-mergetool.sh +41
-1@@ -8,7 +8,7 @@ # at the discretion of Junio C Hamano. # -USAGE='[--tool=tool] [-y|--no-prompt|--prompt] [file to merge] ...' +USAGE='[--tool=tool] [--tool-help] [-y|--no-prompt|--prompt] [file to merge] ...' SUBDIRECTORY_OK=Yes OPTIONS_SPEC= TOOL_MODE=merge@@ -284,11 +284,51 @@ merge_file () { return 0 } +show_tool_help () { + TOOL_MODE=merge + list_merge_tool_candidates + unavailable= available= LF=' +' + for i in $tools + do + merge_tool_path=$(translate_merge_tool_path "$i") + if type "$merge_tool_path" >/dev/null 2>&1 + then + available="$available$i$LF" + else + unavailable="$unavailable$i$LF" + fi + done + if test -n "$available" + then + echo "'git mergetool --tool=<tool>' may be set to one of the following:" + echo "$available" | sort | sed -e 's/^/ /' + else + echo "No suitable tool for 'git mergetool --tool=<tool>' found." + fi + if test -n "$unavailable" + then + echo + echo 'The following tools are valid, but not currently available:' + echo "$unavailable" | sort | sed -e 's/^/ /' + fi + if test -n "$unavailable$available" + then + echo + echo "Some of the tools listed above only work in a windowed" + echo "environment. If run in a terminal-only session, they will fail." + fi + exit 0 +} + prompt=$(git config --bool mergetool.prompt || echo true) while test $# != 0 do case "$1" in + --tool-help) + show_tool_help + ;; -t|--tool*) case "$#,$1" in *,*=*)
Re: [PATCH] mergetool: support --tool-help option like difftool does
Quoted 7 lines · Junio C Hamano
Documentation/git-difftool.txt OPTIONS · line 39
Quoted 6 lines
-t <tool>::
--tool=<tool>::Documentation/git-difftool.txt +1
-3- Use the diff tool specified by <tool>. Valid values include - emerge, kompare, meld, and vimdiff. Run `git difftool --tool-help` - for the list of valid <tool> settings. + Use the diff tool specified by <tool>.
Quoted 7 lines
I do not see how it is an improvement to drop the most common ones. People sometimes read documentation without having an access to shell to run "cmd --tool-help", and a list of handful of well known ones would serve as a good hint to let the reader know the kind of commands the front-end is capable of spawning, which in turn help such a reader to imagine how the command is used to judge if it is something the reader wants to use.
I don't agree. What "most common ones" are depends on your platform and is sort of subjective. So it should be either all or non here. Your argument about people not having shell access is a valid one, but still that would mean to list all tools in my opinion. And listing all tools again thwarts our goal to reduce the number of places where new merge / diff tools need to be added. For people adding new merge / diff tools it is just clearer what places need to be modified if there are no places that list an arbitrary subset of tools.
Signature · 1 line
Sebastian Schuberth
Re: [PATCH] mergetool: support --tool-help option like difftool does
Quoted 17 lines · Sebastian Schuberth
Quoted 7 lines · Junio C Hamano
Documentation/git-difftool.txt OPTIONS · line 39
Quoted 6 lines
-t <tool>::
--tool=<tool>::Documentation/git-difftool.txt +1
-3- Use the diff tool specified by <tool>. Valid values include - emerge, kompare, meld, and vimdiff. Run `git difftool --tool-help` - for the list of valid <tool> settings. + Use the diff tool specified by <tool>.Quoted 7 lines
I do not see how it is an improvement to drop the most common ones. People sometimes read documentation without having an access to shell to run "cmd --tool-help", and a list of handful of well known ones would serve as a good hint to let the reader know the kind of commands the front-end is capable of spawning, which in turn help such a reader to imagine how the command is used to judge if it is something the reader wants to use.
Quoted 2 lines
I don't agree. What "most common ones" are depends on your platform and is sort of subjective. So it should be either all or non here.
Let's please leave this section as-is.
This part of the documentation has had a fair amount of churn. Specifically, it would get touched every time a new tool was added.
The point of bf73fc212a159210398b6d46ed5e9101c650e7db was to change it *one last time* into something that is helpful, but not a substitute for the real list output by --tool-help.
If any changes are done then it should be to make git-mergetool.txt match the advice given in git-difftool.txt.
Quoted 6 lines
Your argument about people not having shell access is a valid one, but still that would mean to list all tools in my opinion. And listing all tools again thwarts our goal to reduce the number of places where new merge / diff tools need to be added. For people adding new merge / diff tools it is just clearer what places need to be modified if there are no places that list an arbitrary subset of tools.
Yes, indeed, it is arbitrary. It does have some merit, though--it is also a good compromise between unhelpful (listing nothing) and painful to maintain (listing everything).
Signature · 1 line
David
Re: [PATCH] mergetool: support --tool-help option like difftool does
Quoted 19 lines · David Aguilar
Quoted 17 lines · Sebastian Schuberth
Quoted 14 lines · Junio C Hamano
Review
Quoted 6 lines
-t <tool>::
--tool=<tool>::(quoted diff) +1
-3- Use the diff tool specified by <tool>. Valid values include - emerge, kompare, meld, and vimdiff. Run `git difftool --tool-help` - for the list of valid <tool> settings. + Use the diff tool specified by <tool>.I do not see how it is an improvement to drop the most common ones. People sometimes read documentation without having an access to shell to run "cmd --tool-help", and a list of handful of well known ones would serve as a good hint to let the reader know the kind of commands the front-end is capable of spawning, which in turn help such a reader to imagine how the command is used to judge if it is something the reader wants to use.
Quoted 4 lines
Quoted 2 lines
I don't agree. What "most common ones" are depends on your platform and is sort of subjective. So it should be either all or non here.
Let's please leave this section as-is.
Yes.
There are only five or six classes of environment that matter in the real world for the purpose of giving "well known" examples: Windows, MacOS X, Gnome, KDE and Linux terminal. By picking a representative one from each and listing them, the end result would have at least one that people from various platforms have _heard of_ and can guess what they do. The "most common" is secondary, and "well known" is the keyword. "Can guess what they do" is the point of the phrase "Valid values _include_". Even if you are hard-core Emacs user, it is likely that you've heard of vimdiff---and in that case, including vimdiff would be enough. Likewise for showing kompare to Gnome users.
Unlike POSIXy folks, where IRIX or Solaris users are likely to have heard of Gnome tools even if they do not use the environment on their platforms, Windows users tend to be isolated bunch, so it would not hurt to include at least one well-known Windows-only tool in the list.
Quoted 3 lines
Yes, indeed, it is arbitrary. It does have some merit, though--it is also a good compromise between unhelpful (listing nothing) and painful to maintain (listing everything).
Here is a v2, with documentation updates.
-- >8 --
Subject: [PATCH] mergetool: support --tool-help option like difftool does
This way we do not have to risk the list of tools going out of sync between the implementation and the documentation.
In the same spirit as bf73fc2 (difftool: print list of valid tools with '--tool-help', 2012-03-29), trim the list of merge backends in the documentation. We do not want to have a complete of valid tools there; we only want a list to help people guess what kind of things the tools that can be specified there, and refer them to --tool-help for a complete list.
--- Documentation/git-mergetool.txt | 6 +++--- git-mergetool--lib.sh | 6 +++++- git-mergetool.sh | 42 ++++++++++++++++++++++++++++++++++++++++- 3 files changed, 49 insertions(+), 5 deletions(-)
Documentation/git-mergetool.txt +3 -3
@@ -27,9 +27,9 @@ OPTIONS -t <tool>:: --tool=<tool>:: Use the merge resolution program specified by <tool>. - Valid merge tools are: - araxis, bc3, diffuse, ecmerge, emerge, gvimdiff, kdiff3, - meld, opendiff, p4merge, tkdiff, tortoisemerge, vimdiff and xxdiff. + Valid values include emerge, gvimdiff, kdiff3, + meld, vimdiff, and tortoisemerge. Run `git mergetool --tool-help` + for the list of valid <tool> settings. + If a merge resolution program is not specified, 'git mergetool' will use the configuration variable `merge.tool`. If the
git-mergetool--lib.sh +5 -1
@@ -111,7 +111,7 @@ run_merge_tool () { return $status } -guess_merge_tool () { +list_merge_tool_candidates () { if merge_mode then tools="tortoisemerge"
@@ -136,6 +136,10 @@ guess_merge_tool () { tools="$tools emerge vimdiff" ;; esac +} + +guess_merge_tool () { + list_merge_tool_candidates echo >&2 "merge tool candidates: $tools" # Loop over each candidate and stop when a valid merge tool is found.
git-mergetool.sh +41 -1
59 lines not on this page · raw message
Signature · 1 line
1.7.11.3.339.gbdbf398
Re: [PATCH] mergetool: support --tool-help option like difftool does
Quoted 6 lines · Junio C Hamano
There are only five or six classes of environment that matter in the real world for the purpose of giving "well known" examples: Windows, MacOS X, Gnome, KDE and Linux terminal. By picking a representative one from each and listing them, the end result would have at least one that people from various platforms have
_heardof_and can guess what they do. The "most common" is secondary, and "well known" is
I completely agree with this. So we should take the chance and add a Windows representative to the list of difftools, no?
Quoted 5 lines
Unlike POSIXy folks, where IRIX or Solaris users are likely to have heard of Gnome tools even if they do not use the environment on their platforms, Windows users tend to be isolated bunch, so it would not hurt to include at least one well-known Windows-only tool in the list.
Heh. I believe POSIX folks are no less isolated. (How many Windows-only tools would you recognize by name?) They're just isolated in a bigger world ;-)
Quoted 1 line
Here is a v2, with documentation updates.
This drops the explicit mention of --tool-help as an option in the documentation compared to my patch. Do you want to keep --tool-help being mentioned inline as part of the --tool option documentation only?
Signature · 1 line
Sebastian Schuberth
Re: [PATCH] mergetool: support --tool-help option like difftool does
Quoted 11 lines · Sebastian Schuberth
Quoted 6 lines · Junio C Hamano
There are only five or six classes of environment that matter in the real world for the purpose of giving "well known" examples: Windows, MacOS X, Gnome, KDE and Linux terminal. By picking a representative one from each and listing them, the end result would have at least one that people from various platforms have
_heardof_and can guess what they do. The "most common" is secondary, and "well known" isI completely agree with this. So we should take the chance and add a Windows representative to the list of difftools, no?
I do not care very deeply either way. I am more interested in seeing --tool-list options supported by both so that we can get rid of hardcoded list from the completion script.
Quoted 4 lines
This drops the explicit mention of --tool-help as an option in the documentation compared to my patch. Do you want to keep --tool-help being mentioned inline as part of the --tool option documentation only?
While I do not think having one would hurt that much, I do not think a separate entry would add much value either, so I chose not to clutter the documentation further. The "--tool=<tool>" entry is enough hint to draw eyes of readers who want to find out what kind of backends are supported, and --tool-help is mentioned there quite prominently.
Re*: mergetool: support --tool-help option like difftool does
In reply to Junio C HamanoQuoted 13 lines · Junio C Hamano
This way we do not have to risk the list of tools go out of sync between the implementation and the documentation.
Signed-off-by Junio C Hamano <[email protected]>---
Quoted 5 lines · Junio C Hamano
Review
Quoted 2 lines
+--tool-help::
(quoted diff) +1
-0+ List the supported and available diff tools.This part is a good addition (but it already is mentioned in the description of --tool above, so it is more or less a "Meh").
Quoted 3 lines
I noticed that the main while loop has style violations in its case/esac, but I left it as-is. Reindenting it would be a low hanging fruit janitor patch that would be a separate topic.
After hinting a low-hanging-fruit that would be an easy way for new people to dip their toes, I saw no takers for one month, so I ended up doing it myself.
-- >8 --
Subject: mergetool: style fixes
This script is one of the sizeable ones that tempted people to copy its "neibouring style" in their new code, but was littered with styles incompatible with our style guide.
- use one tab, not four spaces, per indent level;
- long lines can be wrapped after '|', '&&', or '||' for readability.
- structures like "if .. then .. else .. fi", "while .. do .. done" are split into lines in such a way that does not require unnecessary semicolon.
- case, esac and case-arms align at the same column.
---
* The change would be easier to see if the reader runs these command before and after applying this patch:
$ git diff -w git-mergetool.sh
$ git grep -e '^[\t]* ' -e '; *then' -e '; do' git-mergetool.shgit-mergetool.sh | 581 +++++++++++++++++++++++++++++-------------------------- 1 file changed, 308 insertions(+), 273 deletions(-)
git-mergetool.sh +308 -273
@@ -18,270 +18,301 @@ require_work_tree # Returns true if the mode reflects a symlink is_symlink () { - test "$1" = 120000 + test "$1" = 120000 } is_submodule () { - test "$1" = 160000 + test "$1" = 160000 } local_present () { - test -n "$local_mode" + test -n "$local_mode" } remote_present () { - test -n "$remote_mode" + test -n "$remote_mode" } base_present () { - test -n "$base_mode" + test -n "$base_mode" } cleanup_temp_files () { - if test "$1" = --save-backup ; then - rm -rf -- "$MERGED.orig" - test -e "$BACKUP" && mv -- "$BACKUP" "$MERGED.orig" - rm -f -- "$LOCAL" "$REMOTE" "$BASE" - else - rm -f -- "$LOCAL" "$REMOTE" "$BASE" "$BACKUP" - fi + if test "$1" = --save-backup + then + rm -rf -- "$MERGED.orig" + test -e "$BACKUP" && mv -- "$BACKUP" "$MERGED.orig" + rm -f -- "$LOCAL" "$REMOTE" "$BASE" + else + rm -f -- "$LOCAL" "$REMOTE" "$BASE" "$BACKUP" + fi } describe_file () { - mode="$1" - branch="$2" - file="$3" - - printf " {%s}: " "$branch" - if test -z "$mode"; then - echo "deleted" - elif is_symlink "$mode" ; then - echo "a symbolic link -> '$(cat "$file")'" - elif is_submodule "$mode" ; then - echo "submodule commit $file" - else - if base_present; then - echo "modified file" + mode="$1" + branch="$2" + file="$3" + + printf " {%s}: " "$branch" + if test -z "$mode" + then + echo "deleted" + elif is_symlink "$mode" + then + echo "a symbolic link -> '$(cat "$file")'" + elif is_submodule "$mode" + then + echo "submodule commit $file" + elif base_present + then + echo "modified file" else - echo "created file" + echo "created file" fi - fi } - resolve_symlink_merge () { - while true; do - printf "Use (l)ocal or (r)emote, or (a)bort? " - read ans || return 1 - case "$ans" in - [lL]*) - git checkout-index -f --stage=2 -- "$MERGED" - git add -- "$MERGED" - cleanup_temp_files --save-backup - return 0 - ;; - [rR]*) - git checkout-index -f --stage=3 -- "$MERGED" - git add -- "$MERGED" - cleanup_temp_files --save-backup - return 0 - ;; - [aA]*) - return 1 - ;; - esac + while true + do + printf "Use (l)ocal or (r)emote, or (a)bort? " + read ans || return 1 + case "$ans" in + [lL]*) + git checkout-index -f --stage=2 -- "$MERGED" + git add -- "$MERGED" + cleanup_temp_files --save-backup + return 0 + ;; + [rR]*) + git checkout-index -f --stage=3 -- "$MERGED" + git add -- "$MERGED" + cleanup_temp_files --save-backup + return 0 + ;; + [aA]*) + return 1 + ;; + esac done } resolve_deleted_merge () { - while true; do - if base_present; then - printf "Use (m)odified or (d)eleted file, or (a)bort? " - else - printf "Use (c)reated or (d)eleted file, or (a)bort? " - fi - read ans || return 1 - case "$ans" in - [mMcC]*) - git add -- "$MERGED" - cleanup_temp_files --save-backup - return 0 - ;; - [dD]*) - git rm -- "$MERGED" > /dev/null - cleanup_temp_files - return 0 - ;; - [aA]*) - return 1 - ;; - esac + while true + do + if base_present + then + printf "Use (m)odified or (d)eleted file, or (a)bort? " + else + printf "Use (c)reated or (d)eleted file, or (a)bort? " + fi + read ans || return 1 + case "$ans" in + [mMcC]*) + git add -- "$MERGED" + cleanup_temp_files --save-backup + return 0 + ;; + [dD]*) + git rm -- "$MERGED" > /dev/null + cleanup_temp_files + return 0 + ;; + [aA]*) + return 1 + ;; + esac done } resolve_submodule_merge () { - while true; do - printf "Use (l)ocal or (r)emote, or (a)bort? " - read ans || return 1 - case "$ans" in - [lL]*) - if ! local_present; then - if test -n "$(git ls-tree HEAD -- "$MERGED")"; then - # Local isn't present, but it's a subdirectory - git ls-tree --full-name -r HEAD -- "$MERGED" | git update-index --index-info || exit $? - else - test -e "$MERGED" && mv -- "$MERGED" "$BACKUP" - git update-index --force-remove "$MERGED" + while true + do + printf "Use (l)ocal or (r)emote, or (a)bort? " + read ans || return 1 + case "$ans" in + [lL]*) + if ! local_present + then + if test -n "$(git ls-tree HEAD -- "$MERGED")" + then + # Local isn't present, but it's a subdirectory + git ls-tree --full-name -r HEAD -- "$MERGED" | + git update-index --index-info || exit $? + else + test -e "$MERGED" && mv -- "$MERGED" "$BACKUP" + git update-index --force-remove "$MERGED" + cleanup_temp_files --save-backup + fi + elif is_submodule "$local_mode" + then + stage_submodule "$MERGED" "$local_sha1" + else + git checkout-index -f --stage=2 -- "$MERGED" + git add -- "$MERGED" + fi + return 0 + ;; + [rR]*) + if ! remote_present + then + if test -n "$(git ls-tree MERGE_HEAD -- "$MERGED")" + then + # Remote isn't present, but it's a subdirectory + git ls-tree --full-name -r MERGE_HEAD -- "$MERGED" | + git update-index --index-info || exit $? + else + test -e "$MERGED" && mv -- "$MERGED" "$BACKUP" + git update-index --force-remove "$MERGED" + fi + elif is_submodule "$remote_mode" + then + ! is_submodule "$local_mode" && + test -e "$MERGED" && + mv -- "$MERGED" "$BACKUP" + stage_submodule "$MERGED" "$remote_sha1" + else + test -e "$MERGED" && mv -- "$MERGED" "$BACKUP" + git checkout-index -f --stage=3 -- "$MERGED" + git add -- "$MERGED" + fi cleanup_temp_files --save-backup - fi - elif is_submodule "$local_mode"; then - stage_submodule "$MERGED" "$local_sha1" - else - git checkout-index -f --stage=2 -- "$MERGED" - git add -- "$MERGED" - fi - return 0 - ;; - [rR]*) - if ! remote_present; then - if test -n "$(git ls-tree MERGE_HEAD -- "$MERGED")"; then - # Remote isn't present, but it's a subdirectory - git ls-tree --full-name -r MERGE_HEAD -- "$MERGED" | git update-index --index-info || exit $? - else - test -e "$MERGED" && mv -- "$MERGED" "$BACKUP" - git update-index --force-remove "$MERGED" - fi - elif is_submodule "$remote_mode"; then - ! is_submodule "$local_mode" && test -e "$MERGED" && mv -- "$MERGED" "$BACKUP" - stage_submodule "$MERGED" "$remote_sha1" - else - test -e "$MERGED" && mv -- "$MERGED" "$BACKUP" - git checkout-index -f --stage=3 -- "$MERGED" - git add -- "$MERGED" - fi - cleanup_temp_files --save-backup - return 0 - ;; - [aA]*) - return 1 - ;; - esac + return 0 + ;; + [aA]*) + return 1 + ;; + esac done } stage_submodule () { - path="$1" - submodule_sha1="$2" - mkdir -p "$path" || die "fatal: unable to create directory for module at $path" - # Find $path relative to work tree - work_tree_root=$(cd_to_toplevel && pwd) - work_rel_path=$(cd "$path" && GIT_WORK_TREE="${work_tree_root}" git rev-parse --show-prefix) - test -n "$work_rel_path" || die "fatal: unable to get path of module $path relative to work tree" - git update-index --add --replace --cacheinfo 160000 "$submodule_sha1" "${work_rel_path%/}" || die + path="$1" + submodule_sha1="$2" + mkdir -p "$path" || + die "fatal: unable to create directory for module at $path" + # Find $path relative to work tree + work_tree_root=$(cd_to_toplevel && pwd) + work_rel_path=$(cd "$path" && + GIT_WORK_TREE="${work_tree_root}" git rev-parse --show-prefix + ) + test -n "$work_rel_path" || + die "fatal: unable to get path of module $path relative to work tree" + git update-index --add --replace --cacheinfo 160000 "$submodule_sha1" "${work_rel_path%/}" || die } checkout_staged_file () { - tmpfile=$(expr \ - "$(git checkout-index --temp --stage="$1" "$2" 2>/dev/null)" \ - : '\([^ ]*\) ') - - if test $? -eq 0 -a -n "$tmpfile" ; then - mv -- "$(git rev-parse --show-cdup)$tmpfile" "$3" - else - >"$3" - fi + tmpfile=$(expr \ + "$(git checkout-index --temp --stage="$1" "$2" 2>/dev/null)" \ + : '\([^ ]*\) ') + + if test $? -eq 0 -a -n "$tmpfile" + then + mv -- "$(git rev-parse --show-cdup)$tmpfile" "$3" + else + >"$3" + fi } merge_file () { - MERGED="$1" + MERGED="$1" - f=$(git ls-files -u -- "$MERGED") - if test -z "$f" ; then - if test ! -f "$MERGED" ; then - echo "$MERGED: file not found" - else - echo "$MERGED: file does not need merging" + f=$(git ls-files -u -- "$MERGED") + if test -z "$f" + then + if test ! -f "$MERGED" + then + echo "$MERGED: file not found" + else + echo "$MERGED: file does not need merging" + fi + return 1 fi - return 1 - fi - - ext="$$$(expr "$MERGED" : '.*\(\.[^/]*\)$')" - BACKUP="./$MERGED.BACKUP.$ext" - LOCAL="./$MERGED.LOCAL.$ext" - REMOTE="./$MERGED.REMOTE.$ext" - BASE="./$MERGED.BASE.$ext" - - base_mode=$(git ls-files -u -- "$MERGED" | awk '{if ($3==1) print $1;}') - local_mode=$(git ls-files -u -- "$MERGED" | awk '{if ($3==2) print $1;}') - remote_mode=$(git ls-files -u -- "$MERGED" | awk '{if ($3==3) print $1;}') - - if is_submodule "$local_mode" || is_submodule "$remote_mode"; then - echo "Submodule merge conflict for '$MERGED':" - local_sha1=$(git ls-files -u -- "$MERGED" | awk '{if ($3==2) print $2;}') - remote_sha1=$(git ls-files -u -- "$MERGED" | awk '{if ($3==3) print $2;}') - describe_file "$local_mode" "local" "$local_sha1" - describe_file "$remote_mode" "remote" "$remote_sha1" - resolve_submodule_merge - return - fi - - mv -- "$MERGED" "$BACKUP" - cp -- "$BACKUP" "$MERGED" - - checkout_staged_file 1 "$MERGED" "$BASE" - checkout_staged_file 2 "$MERGED" "$LOCAL" - checkout_staged_file 3 "$MERGED" "$REMOTE" - - if test -z "$local_mode" -o -z "$remote_mode"; then - echo "Deleted merge conflict for '$MERGED':" - describe_file "$local_mode" "local" "$LOCAL" - describe_file "$remote_mode" "remote" "$REMOTE" - resolve_deleted_merge - return - fi - if is_symlink "$local_mode" || is_symlink "$remote_mode"; then - echo "Symbolic link merge conflict for '$MERGED':" + ext="$$$(expr "$MERGED" : '.*\(\.[^/]*\)$')" + BACKUP="./$MERGED.BACKUP.$ext" + LOCAL="./$MERGED.LOCAL.$ext" + REMOTE="./$MERGED.REMOTE.$ext" + BASE="./$MERGED.BASE.$ext" + + base_mode=$(git ls-files -u -- "$MERGED" | awk '{if ($3==1) print $1;}') + local_mode=$(git ls-files -u -- "$MERGED" | awk '{if ($3==2) print $1;}') + remote_mode=$(git ls-files -u -- "$MERGED" | awk '{if ($3==3) print $1;}') + + if is_submodule "$local_mode" || is_submodule "$remote_mode" + then + echo "Submodule merge conflict for '$MERGED':" + local_sha1=$(git ls-files -u -- "$MERGED" | awk '{if ($3==2) print $2;}') + remote_sha1=$(git ls-files -u -- "$MERGED" | awk '{if ($3==3) print $2;}') + describe_file "$local_mode" "local" "$local_sha1" + describe_file "$remote_mode" "remote" "$remote_sha1" + resolve_submodule_merge + return + fi + + mv -- "$MERGED" "$BACKUP" + cp -- "$BACKUP" "$MERGED" + + checkout_staged_file 1 "$MERGED" "$BASE" + checkout_staged_file 2 "$MERGED" "$LOCAL" + checkout_staged_file 3 "$MERGED" "$REMOTE" + + if test -z "$local_mode" -o -z "$remote_mode" + then + echo "Deleted merge conflict for '$MERGED':" + describe_file "$local_mode" "local" "$LOCAL" + describe_file "$remote_mode" "remote" "$REMOTE" + resolve_deleted_merge + return + fi + + if is_symlink "$local_mode" || is_symlink "$remote_mode" + then + echo "Symbolic link merge conflict for '$MERGED':" + describe_file "$local_mode" "local" "$LOCAL" + describe_file "$remote_mode" "remote" "$REMOTE" + resolve_symlink_merge + return + fi + + echo "Normal merge conflict for '$MERGED':" describe_file "$local_mode" "local" "$LOCAL" describe_file "$remote_mode" "remote" "$REMOTE" - resolve_symlink_merge - return - fi - - echo "Normal merge conflict for '$MERGED':" - describe_file "$local_mode" "local" "$LOCAL" - describe_file "$remote_mode" "remote" "$REMOTE" - if "$prompt" = true; then - printf "Hit return to start merge resolution tool (%s): " "$merge_tool" - read ans || return 1 - fi - - if base_present; then - present=true - else - present=false - fi - - if ! run_merge_tool "$merge_tool" "$present"; then - echo "merge of $MERGED failed" 1>&2 - mv -- "$BACKUP" "$MERGED" - - if test "$merge_keep_temporaries" = "false"; then - cleanup_temp_files + if "$prompt" = true + then + printf "Hit return to start merge resolution tool (%s): " "$merge_tool" + read ans || return 1 fi - return 1 - fi + if base_present + then + present=true + else + present=false + fi + + if ! run_merge_tool "$merge_tool" "$present" + then + echo "merge of $MERGED failed" 1>&2 + mv -- "$BACKUP" "$MERGED" + + if test "$merge_keep_temporaries" = "false" + then + cleanup_temp_files + fi - if test "$merge_keep_backup" = "true"; then - mv -- "$BACKUP" "$MERGED.orig" - else - rm -- "$BACKUP" - fi + return 1 + fi - git add -- "$MERGED" - cleanup_temp_files - return 0 + if test "$merge_keep_backup" = "true" + then + mv -- "$BACKUP" "$MERGED.orig" + else + rm -- "$BACKUP" + fi + + git add -- "$MERGED" + cleanup_temp_files + return 0 } show_tool_help () {
@@ -325,61 +356,61 @@ prompt=$(git config --bool mergetool.prompt || echo true) while test $# != 0 do - case "$1" in + case "$1" in --tool-help) show_tool_help ;; -t|--tool*) - case "$#,$1" in + case "$#,$1" in *,*=*) - merge_tool=$(expr "z$1" : 'z-[^=]*=\(.*\)') - ;; + merge_tool=$(expr "z$1" : 'z-[^=]*=\(.*\)') + ;; 1,*) - usage ;; + usage ;; *) - merge_tool="$2" - shift ;; - esac - ;; + merge_tool="$2" + shift ;; + esac + ;; -y|--no-prompt) - prompt=false - ;; + prompt=false + ;; --prompt) - prompt=true - ;; + prompt=true + ;; --) - shift - break - ;; + shift + break + ;; -*) - usage - ;; - *) - break - ;; - esac - shift -done - -prompt_after_failed_merge() { - while true; do - printf "Continue merging other unresolved paths (y/n) ? " - read ans || return 1 - case "$ans" in - - [yY]*) - return 0 + usage ;; - - [nN]*) - return 1 + *) + break ;; esac - done + shift +done + +prompt_after_failed_merge () { + while true + do + printf "Continue merging other unresolved paths (y/n) ? " + read ans || return 1 + case "$ans" in + [yY]*) + return 0 + ;; + [nN]*) + return 1 + ;; + esac + done } -if test -z "$merge_tool"; then - merge_tool=$(get_merge_tool "$merge_tool") || exit +if test -z "$merge_tool" +then + merge_tool=$(get_merge_tool "$merge_tool") || exit fi merge_keep_backup="$(git config --bool mergetool.keepBackup || echo true)" merge_keep_temporaries="$(git config --bool mergetool.keepTemporaries || echo false)"
@@ -388,22 +419,24 @@ last_status=0 rollup_status=0 files= -if test $# -eq 0 ; then - cd_to_toplevel +if test $# -eq 0 +then + cd_to_toplevel - if test -e "$GIT_DIR/MERGE_RR" - then - files=$(git rerere remaining) - else - files=$(git ls-files -u | sed -e 's/^[^ ]* //' | sort -u) - fi + if test -e "$GIT_DIR/MERGE_RR" + then + files=$(git rerere remaining) + else + files=$(git ls-files -u | sed -e 's/^[^ ]* //' | sort -u) + fi else - files=$(git ls-files -u -- "$@" | sed -e 's/^[^ ]* //' | sort -u) + files=$(git ls-files -u -- "$@" | sed -e 's/^[^ ]* //' | sort -u) fi -if test -z "$files" ; then - echo "No files need merging" - exit 0 +if test -z "$files" +then + echo "No files need merging" + exit 0 fi printf "Merging:\n"
@@ -413,15 +446,17 @@ IFS=' ' for i in $files do - if test $last_status -ne 0; then - prompt_after_failed_merge || exit 1 - fi - printf "\n" - merge_file "$i" - last_status=$? - if test $last_status -ne 0; then - rollup_status=1 - fi + if test $last_status -ne 0 + then + prompt_after_failed_merge || exit 1 + fi + printf "\n" + merge_file "$i" + last_status=$? + if test $last_status -ne 0 + then + rollup_status=1 + fi done exit $rollup_status
Re: Re*: mergetool: support --tool-help option like difftool does
Quoted 15 lines · Junio C Hamano
Quoted 13 lines · Junio C Hamano
This way we do not have to risk the list of tools go out of sync between the implementation and the documentation.
Signed-off-by Junio C Hamano <[email protected]>---
Quoted 5 lines · Junio C Hamano
Quoted 2 lines
+--tool-help:: + List the supported and available diff tools.This part is a good addition (but it already is mentioned in the description of --tool above, so it is more or less a "Meh").
Quoted 7 lines
Quoted 3 lines
I noticed that the main while loop has style violations in its case/esac, but I left it as-is. Reindenting it would be a low hanging fruit janitor patch that would be a separate topic.
After hinting a low-hanging-fruit that would be an easy way for new people to dip their toes, I saw no takers for one month, so I ended up doing it myself.
My bad, I didn't catch this and should have, especially so because I had started on style fixing mergetool last week but ditched it; I assumed it would be unwanted. I will read more carefully.
Just please don't tell Charles about it ;-)
(I wanted to do this a long ago but at the time we didn't have
the style guide for shell, and the time was perhaps not right...)
I am of course in favor of this patch.
While on the mergetool topic...
Would the ability to resolve the various merge situations using the command-line be a wanted addition?
This would let a submodule or deleted/modified encountering user do something like:
$ git mergetool --theirs -- submodule
...and not have to remember the various git commands that it runs.
This could touch just the parts of mergetool that require stdin, but probably should work for everything.
It seems like exposing the guts of some of these mergetool functions via command-line flags could be helpful, but I'm not sure if that's overloading mergetool with too many features. What do you think?
Signature · 1 line
David
Re: Re*: mergetool: support --tool-help option like difftool does
Quoted 5 lines · David Aguilar
Quoted 3 lines
After hinting a low-hanging-fruit that would be an easy way for new people to dip their toes, I saw no takers for one month, so I ended up doing it myself.
My bad,...
Not yours. These hints I drop from time to time are meant for eager new people who want to dip their toes to the development (we don't do "assignments" but these are the closest that we have); you no longer quite qualify as "new" ;-)
Quoted 1 line
While on the mergetool topic...
Now we are not talking about "dip their toes" low hanging fruit anymore ;-)
Quoted 9 lines
Would the ability to resolve the various merge situations using the command-line be a wanted addition?
This would let a submodule or deleted/modified encountering user do something like:
$ git mergetool --theirs -- submodule...and not have to remember the various git commands that it runs.
Does it have to run various git commands? For a normal path, all it needs to do is "git checkout --theirs $path", no?
What does it mean to resolve a conflicted merge of a gitlink to take "theirs"? We obviously would want to point the resolved gitlink at the submodule commit their side wants in the resulting index but what, if any, should we do to the submodule itself?
Stepping back a bit, if there is no conflict, and as a result of a clean merge (this applies to the case where you check out another branch that has different commit at the submodule path), if gitlink changed to point at a different commit in the submodule, what should happen?
If you start from a clean working tree, with your gitlink pointing at the commit that matches HEAD in the submodule, and if the working tree of the submodule does not have any local modification, it may be ideal to check out the new commit in the submodule (are there cases where "git checkout other_branch" in the superproject does not want to touch the submodule working tree?).
There are cases where it is not possible; checking out the new commit in the submodule working tree may not succeed due to local modifications. But is that kind of complication limited to the merge resolution case? Isn't it shared with normal "switching branches" case as well?
If so, it could be that your "git mergetool --theirs -- submodule" is working around a wrong problem, and the right solution may be to make "git checkout --theirs -- $path" to always do an appropriate thing regardless of what kind of object $path is, no?
Re: Re*: mergetool: support --tool-help option like difftool does
Quoted 38 lines · Junio C Hamano
Quoted 9 lines · David Aguilar
Would the ability to resolve the various merge situations using the command-line be a wanted addition?
This would let a submodule or deleted/modified encountering user do something like:
$ git mergetool --theirs -- submodule...and not have to remember the various git commands that it runs.
Does it have to run various git commands? For a normal path, all it needs to do is "git checkout --theirs $path", no?
What does it mean to resolve a conflicted merge of a gitlink to take "theirs"? We obviously would want to point the resolved gitlink at the submodule commit their side wants in the resulting index but what, if any, should we do to the submodule itself?
Stepping back a bit, if there is no conflict, and as a result of a clean merge (this applies to the case where you check out another branch that has different commit at the submodule path), if gitlink changed to point at a different commit in the submodule, what should happen?
If you start from a clean working tree, with your gitlink pointing at the commit that matches HEAD in the submodule, and if the working tree of the submodule does not have any local modification, it may be ideal to check out the new commit in the submodule (are there cases where "git checkout
other_branch" in the superproject does not want to touch the submodule working tree?).There are cases where it is not possible; checking out the new commit in the submodule working tree may not succeed due to local modifications. But is that kind of complication limited to the merge resolution case? Isn't it shared with normal "switching branches" case as well?
Quoted 4 lines
If so, it could be that your "git mergetool --theirs -- submodule" is working around a wrong problem, and the right solution may be to make "git checkout --theirs -- $path" to always do an appropriate thing regardless of what kind of object $path is, no?
True.
Admittedly, I'm not a heavy submodule user myself so I tried crafting the directory vs submodule conflict to see what the usability is like.
checkout --theirs and --ours could learn a few tricks.
When trying to choose the directory in that situation I had to had to "git rm --cached" the submodule path so that git would recognize that there was no longer a conflict.
That makes sense to me because I was following along by reading the mergetool code, but I don't think most users would know to "git rm" a path which they intend to keep.
Afterwards, the .git file is left behind, which could cause problems elsewhere since we really don't want a .git file in that situation. I'm not even sure what to do about the .gitmodules file either.
Here's the script I was using to setup the merge conflict in case anyone wants to get a feel for the usability around this edge case.
Pass --submodule if you want the remote side to have a submodule. By default, the local side has a submodule and the remote side of the merge brings along a directory.
That said, this really isn't an issue, per say. I first poked at it because I noticed that mergetool still needed stdin for some things.
It's certainly an edge case, and perhaps this just shows that mergetool really is the right porcelain for the job when a user runs into these types of conflicts (the stdin thing really isn't an issue).
#!/bin/sh
if test "$1" = "--submodule"
then
first=with-directory
second=with-submodule
echo "local will be a directory, remote will be a submodule"
else
first=with-submodule
second=with-directory
echo "local will be a submodule, remote will be a directory"
fi
repo=submodule-conflict-$$ &&
echo "creating $repo" &&
mkdir "$repo" &&
cd "$repo" &&
git init &&
git commit --allow-empty -m'initial' &&
git checkout -b with-directory master &&
mkdir the-conflict &&
touch the-conflict/path &&
git add the-conflict/path &&
git commit -m'added path into the-conflict' && git checkout -b with-submodule master &&
git submodule add ./ the-conflict &&
git commit -m'added submodule as the-conflict' && git checkout -b tmp-merge master &&
git merge $first &&
git merge $second
Signature · 1 line
David
Re: Re*: mergetool: support --tool-help option like difftool does
Am 24.08.2012 10:31, schrieb David Aguilar:
Quoted 39 lines · Junio C Hamano
Quoted 38 lines
Quoted 9 lines · David Aguilar
Would the ability to resolve the various merge situations using the command-line be a wanted addition?
This would let a submodule or deleted/modified encountering user do something like:
$ git mergetool --theirs -- submodule...and not have to remember the various git commands that it runs.
Does it have to run various git commands? For a normal path, all it needs to do is "git checkout --theirs $path", no?
What does it mean to resolve a conflicted merge of a gitlink to take "theirs"? We obviously would want to point the resolved gitlink at the submodule commit their side wants in the resulting index but what, if any, should we do to the submodule itself?
Stepping back a bit, if there is no conflict, and as a result of a clean merge (this applies to the case where you check out another branch that has different commit at the submodule path), if gitlink changed to point at a different commit in the submodule, what should happen?
If you start from a clean working tree, with your gitlink pointing at the commit that matches HEAD in the submodule, and if the working tree of the submodule does not have any local modification, it may be ideal to check out the new commit in the submodule (are there cases where "git checkout
other_branch" in the superproject does not want to touch the submodule working tree?).There are cases where it is not possible; checking out the new commit in the submodule working tree may not succeed due to local modifications. But is that kind of complication limited to the merge resolution case? Isn't it shared with normal "switching branches" case as well?
Quoted 6 lines
Quoted 4 lines
If so, it could be that your "git mergetool --theirs -- submodule" is working around a wrong problem, and the right solution may be to make "git checkout --theirs -- $path" to always do an appropriate thing regardless of what kind of object $path is, no?
True.
I agree.
Quoted 5 lines
Admittedly, I'm not a heavy submodule user myself so I tried crafting the directory vs submodule conflict to see what the usability is like.
checkout --theirs and --ours could learn a few tricks.
Me thinks that after I successfully taught checkout to properly recurse into submodule work trees too it should know all those tricks. In my current version it can handle directory/submodule conversions in both directions, this should be sufficient to make "checkout --theirs/--ours" work properly. Note to self: add tests for that.
Quoted 7 lines
When trying to choose the directory in that situation I had to had to "git rm --cached" the submodule path so that git would recognize that there was no longer a conflict.
That makes sense to me because I was following along by reading the mergetool code, but I don't think most users would know to "git rm" a path which they intend to keep.
True. But a submodule recursing checkout would do the right thing here too.
Quoted 3 lines
Afterwards, the .git file is left behind, which could cause problems elsewhere since we really don't want a .git file in that situation.
Hmm, either you remove all the files tracked in the submodule together with the gitfile or you'll possibly have former submodule files lying around there too. Recursive checkout will do all that for you.
Quoted 2 lines
I'm not even sure what to do about the
.gitmodules file either.
Maybe we should issue a warning when the .gitmodules file is not consistent with the [non]existence of a submodule in the work tree?
Quoted 8 lines
That said, this really isn't an issue, per say. I first poked at it because I noticed that mergetool still needed stdin for some things.
It's certainly an edge case, and perhaps this just shows that mergetool really is the right porcelain for the job when a user runs into these types of conflicts (the stdin thing really isn't an issue).
It looks to me as if the submodule/directory conflict can be handled by a recursive checkout without having to add something to mergetool.
PATCH v2 4/5 Make sure to use Araxis' "compare" and not e.g. ImageMagick's
In reply to Sebastian Schuberth--- mergetools/araxis | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)
mergetools/araxis +7 -1
@@ -16,5 +16,11 @@ merge_cmd () { } translate_merge_tool_path() { - echo compare + # Make sure to use Araxis' "compare" and not e.g. ImageMagick's. + if ls "$(dirname "$(which compare)")"/Araxis* >/dev/null 2>&1 + then + echo compare + else + echo "$1" + fi }
Signature · 1 line
1.7.11.msysgit.2
Re: [PATCH v2 4/5] Make sure to use Araxis' "compare" and not e.g. ImageMagick's
Quoted 10 lines · Sebastian Schuberth
Signed-off-by Sebastian Schuberth <[email protected]>--- mergetools/araxis | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)mergetools/araxis +0
-0
mergetools/araxis merge_cmd () { · line 20
Quoted 6 lines
mergetools/araxis +2
-1@@ -16,5 +16,11 @@ merge_cmd () { } translate_merge_tool_path() { - echo compare + # Make sure to use Araxis' "compare" and not e.g. ImageMagick's. + if ls "$(dirname "$(which compare)")"/Araxis* >/dev/null 2>&1
Use of "which" in scripts that are meant to be portable is a no-no. Some platforms would say "compare is /usr/local/bin/compare" instead of saying "/usr/local/bin/compare".
Quoted 6 lines
mergetools/araxis +5
-0+ then + echo compare + else + echo "$1" + fi }
PATCH 1/1 Make sure to use Araxis' "compare" and not e.g. ImageMagick's
--- mergetools/araxis | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)
mergetools/araxis +8 -1
@@ -16,5 +16,12 @@ merge_cmd () { } translate_merge_tool_path() { - echo compare + # Only accept "compare" in a path that contains "araxis" to not + # accidently use e.g. ImageMagick's "compare". + if type compare | grep -i araxis >/dev/null 2>&1 + then + echo compare + else + echo "$1" + fi }
Signature · 1 line
1.7.11.msysgit.2
Re: [PATCH] Make sure to use Araxis' "compare" and not e.g. ImageMagick's
Quoted 13 lines · Sebastian Schuberth
Signed-off-by Sebastian Schuberth <[email protected]>--- mergetools/araxis | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)mergetools/araxis +0
-0@@ -16,5 +16,12 @@ merge_cmd () { } translate_merge_tool_path() {
mergetools/araxis merge_cmd () { · line 27
Quoted 10 lines
mergetools/araxis +8
-1- echo compare + # Only accept "compare" in a path that contains "araxis" to not + # accidently use e.g. ImageMagick's "compare". + if type compare | grep -i araxis >/dev/null 2>&1 + then + echo compare + else + echo "$1" + fi }
I do not use araxis, and I do not know how rigid its installation procedure is to prevent the end user from naming a path that does not have the string in it.
For example, when the user tells it to install in "/home/ss/bin", if it installs its "compare" program in "/home/ss/bin/araxis/compare" without allowing the "/araxis/" part to be stripped away, the above heuristics is sufficiently safe. Otherwise, it is not.
It is unclear from your proposed commit log message what assurance do we have that it is installed under such a path and why the heuristics the patch implements is the sane way forward.
Please convince me.
Re: [PATCH] Make sure to use Araxis' "compare" and not e.g. ImageMagick's
Quoted 4 lines · Junio C Hamano
For example, when the user tells it to install in "
/home/ss/bin", if it installs its "compare" program in "/home/ss/bin/araxis/compare" without allowing the "/araxis/" part to be stripped away, the above heuristics is sufficiently safe. Otherwise, it is not.
To the best of my knowledge, Araxis does not enforce any naming convention for the path it gets installed in. That means the user may indeed install the program in a path that does not contain "araxis". I was aware of this when writing the patch, but should have probably made it more clear in the commit message.
Quoted 3 lines
It is unclear from your proposed commit log message what assurance do we have that it is installed under such a path and why the heuristics the patch implements is the sane way forward.
We have no such assurance. That's why you correctly call it a heuristics after all: it may fail. Personally, I've valued the gain of the patch to not list araxis as an available diff tool by "git difftool --tool-help" when in fact just ImageMagick is in PATH higher than the loss to support araxis installations that are in a path not containung "araxis" but are in PATH.
Please feel free to ignore the patch if you feel the heuristics is not sufficiently safe. I'm currently unable to come up with a safer solution while maintaining portability, i.e. not use "which" or doing rather laborious string parsing on the output of "type".
Signature · 1 line
Sebastian Schuberth
Re: [PATCH] Make sure to use Araxis' "compare" and not e.g. ImageMagick's
Quoted 5 lines · Sebastian Schuberth
Personally, I've valued the gain of
the patch to not list araxis as an available diff tool by "git difftool --tool-help" when in fact just ImageMagick is in PATH higher than the loss to support araxis installations that are in a path not containung "araxis" but are in PATH.
I agree that running ImageMagick's compare by accident is one thing we would want to avoid, but once the problem is diagnosed, it is something the user can easily work around by futzing %PATH%, I think.
On the other hand, if the user has bought and installed Araxis, but we incorrectly identify it as unusable, the user has wasted good money and there is no easy recourse as far as I can see in your patch. That is why I wanted to see a reasonable assurance.
If we limit the problem space by special casing Windows installation (e.g. check "uname -s" or something), would it make the problem easier to solve? Perhaps it is much more likely that the path the program is installed in can be safely identified with a call to "type --path compare" (bash is the only shell shipped in msysgit, isn't it?), and its output is likely to contain "/Program Files/Araxis/" as a substring, or something?
Re: [PATCH] Make sure to use Araxis' "compare" and not e.g. ImageMagick's
Quoted 6 lines · Junio C Hamano
If we limit the problem space by special casing Windows installation (e.g. check "uname -s" or something), would it make the problem easier to solve? Perhaps it is much more likely that the path the program is installed in can be safely identified with a call to "type --path compare" (bash is the only shell shipped in msysgit, isn't it?).
E.g. something along the lines of your original patch. I do not know what other commands are typically installed in the same directory as "compare", so it is likely you need to fix the name of the file to let us positively identify "compare" is from the Araxis suite.
mergetools/araxis | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-)
mergetools/araxis +14 -1
@@ -16,5 +16,18 @@ merge_cmd () { } translate_merge_tool_path() { - echo compare + case "$BASH_VERSION" in + ??*) + # we can safely use "type --path" + if test -f $(dirname "$(type --path compare)")/AraxisMerge + then + echo compare + else + echo "$1" + fi + ;; + *) + echo compare + ;; + esac }
Re: [PATCH] Make sure to use Araxis' "compare" and not e.g. ImageMagick's
Review
Quoted 1 line · Junio C Hamano
(quoted diff) +1
-0+ if test -f $(dirname "$(type --path compare)")/AraxisMerge
We would need additional quotes around the whole path here as the Windows installation path is usually something like "C:\Program Files\Araxis\Araxis Merge" and contains spaces.
Moreover, "test -f" requires the ".exe" extension to be explicitly present for the file to test. But I'd rather not do that because the test would be specific to Windows then and e.g. not work on Mac OS X. That's why I'd still like to use ls like in my first patch:
mergetools/araxis | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-)
mergetools/araxis +14 -1
@@ -16,5 +16,18 @@ merge_cmd () { } translate_merge_tool_path() { - echo compare + case "$BASH_VERSION" in + ??*) + # we can safely use "type --path" + if ls "$(dirname "$(type --path compare)")"/Araxis* >/dev/null 2>&1 + then + echo compare + else + echo "$1" + fi + ;; + *) + echo compare + ;; + esac }
Signature · 1 line
Sebastian Schuberth
Re: [PATCH] Make sure to use Araxis' "compare" and not e.g. ImageMagick's
In reply to Junio C HamanoQuoted 4 lines · Junio C Hamano
On the other hand, if the user has bought and installed Araxis, but we incorrectly identify it as unusable, the user has wasted good money and there is no easy recourse as far as I can see in your patch.
Agreed.
Quoted 7 lines
If we limit the problem space by special casing Windows installation (e.g. check "uname -s" or something), would it make the problem easier to solve? Perhaps it is much more likely that the path the program is installed in can be safely identified with a call to "type --path compare" (bash is the only shell shipped in msysgit, isn't it?), and its output is likely to contain "/Program Files/Araxis/" as a substring, or something?
Yes, I could come up with basically a variant of my first version of the patch that is limited to Windows only and uses "type --path" instead of "which", but then again this is too non-generic for my taste. Moreover, as I'm also using Mac OS X, I'd be interested in a solution that works there, too.
I don't see a good (read generic and concise) solution to the issue. I think we should just drop my patch and not waste all of our time on it any more.
Signature · 1 line
Sebastian Schuberth
Re: [PATCH] Make sure to use Araxis' "compare" and not e.g. ImageMagick's
In reply to Sebastian SchuberthQuoted 2 lines · Sebastian Schuberth
We have no such assurance. That's why you correctly call it a heuristics after all
ImageMagick "compare" takes "--version" and says something like this to its standard output:
$ compare --version
Version: ImageMagick 6.6.0-4 2012-05-02 Q16
http://www.imagemagick.org
Copyright: Copyright (C) 1999-2010 ImageMagick Studio LLCDoes Araxis compare take "--version" and behave in a way that is cheaply controllable? If it opens a GUI window and pops up a dialog that says "Option not understood", then it is not "controllable", but if it quickly dies with "No such option" sent to the standard error output, or sending its version string to the standard output, then we could use something like:
case "$(compare --version 2>/dev/null)" in
"Araxis compare version"*)
echo compare ;;
*)
echo "$1" ;;
esacinstead, and that would be more robust than the path based heuristics.
Re: [PATCH] Make sure to use Araxis' "compare" and not e.g. ImageMagick's
Quoted 3 lines · Junio C Hamano
Does Araxis compare take "--version" and behave in a way that is cheaply controllable? If it opens a GUI window and pops up a dialog that says "Option not understood", then it is not "controllable",
Araxis opens up a GUI dialog with usage info in that case, so it's not controllable, unfortunately.
Signature · 1 line
Sebastian Schuberth
Re: [PATCH] Make sure to use Araxis' "compare" and not e.g. ImageMagick's
In reply to Junio C HamanoQuoted 27 lines · Junio C Hamano
Quoted 2 lines · Sebastian Schuberth
We have no such assurance. That's why you correctly call it a heuristics after all
ImageMagick "compare" takes "--version" and says something like this to its standard output:
$ compare --version Version: ImageMagick 6.6.0-4 2012-05-02 Q16 http://www.imagemagick.org Copyright: Copyright (C) 1999-2010 ImageMagick Studio LLCDoes Araxis compare take "--version" and behave in a way that is cheaply controllable? If it opens a GUI window and pops up a dialog that says "Option not understood", then it is not "controllable", but if it quickly dies with "No such option" sent to the standard error output, or sending its version string to the standard output, then we could use something like:
case "$(compare --version 2>/dev/null)" in "Araxis compare version"*) echo compare ;; *) echo "$1" ;; esac
Quoted 2 lines
instead, and that would be more robust than the path based heuristics.
Araxis compare (the one I have) does not accept --version.
Also, the GraphicsMagick (ImageMagick fork) compare does not have --version, but it does have "compare version":
$ compare version GraphicsMagick 1.3.12 2010-03-08 Q8 http://www.GraphicsMagick.org/ Copyright (C) 2002-2010 GraphicsMagick Group. Additional copyrights and licenses apply to this software. See http://www.GraphicsMagick.org/www/Copyright.html for details. ...
If we care to blacklist *Magick compare, then we may be able to call "compare version" and parse the output looking for "Magic".
I tested ImageMagick compare and it also understands "compare version".
Araxis compare prints nothing when "compare version" is called. Likely because it thinks it has nothing to do. It does the same if I say "compare blahblah", and returns exit status 0. So its output is useless.
It seems like the best we can do is specifically blacklist the *Magick "compare" commands so that they do not show up as false positives. And the only way to identify them is to parse their output, since all commands return status 0 for "compare version".
Another possibility is to parse the output of "compare" (no args) and grep for "merge". The name "Araxis Merge" is never actually printed, but the help text for "-merge" does appear.... yep.. it's a heuristic.
Sebastian, are you testing on Windows? The araxis "compare" I used is OS X. Does "compare version" open a GUI window for you? For me it does not.
What about "compare -h", or just "compare" ?
Signature · 1 line
David
Re: [PATCH] Make sure to use Araxis' "compare" and not e.g. ImageMagick's
Quoted 4 lines · David Aguilar
Sebastian, are you testing on Windows? The araxis "compare" I used is OS X. Does "compare version" open a GUI window for you? For me it does not.
What about "compare -h", or just "compare" ?
Yes, I'm testing on Windows, and unfortunately Araxis behaves differently here:
- running "compare version" or "compare blah" prints nothing on the console, but the GUI opens saying it is unable to open a file "version" (which it would compare to nothing),
- "compare -h" or just "compare" both open up the usage dialog listing all valid command line options (because "-h" is invalid).
Signature · 1 line
Sebastian Schuberth
Re: [PATCH] Make sure to use Araxis' "compare" and not e.g. ImageMagick's
In reply to Sebastian SchuberthQuoted 4 lines · Sebastian Schuberth
Please feel free to ignore the patch if you feel the heuristics is not sufficiently safe. I'm currently unable to come up with a safer solution while maintaining portability, i.e. not use "which" or doing rather laborious string parsing on the output of "type".
How about looking at "compare --version"?
Andreas.
Signature · 3 lines
Andreas Schwab, [email protected] GPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5 "And now for something completely different."
PATCH v2 5/5 Add a few more code comments and blank lines in guess_merge_tool
In reply to Sebastian Schuberth--- git-mergetool--lib.sh | 6 ++++++ 1 file changed, 6 insertions(+)
git-mergetool--lib.sh +6 -0
@@ -112,14 +112,17 @@ run_merge_tool () { } guess_merge_tool () { + # Add tools that can either do merging or diffing, but not both. if merge_mode then tools="tortoisemerge" else tools="kompare" fi + if test -n "$DISPLAY" then + # Prefer GTK-based tools under Gnome. if test -n "$GNOME_DESKTOP_SESSION_ID" then tools="meld opendiff kdiff3 tkdiff xxdiff $tools"
@@ -128,6 +131,8 @@ guess_merge_tool () { fi tools="$tools gvimdiff diffuse ecmerge p4merge araxis bc3" fi + + # Prefer vimdiff if vim is the default editor. case "${VISUAL:-$EDITOR}" in *vim*) tools="$tools vimdiff emerge"
@@ -136,6 +141,7 @@ guess_merge_tool () { tools="$tools emerge vimdiff" ;; esac + echo >&2 "merge tool candidates: $tools" # Loop over each candidate and stop when a valid merge tool is found.
Signature · 1 line
1.7.11.msysgit.2