{"thread":{"id":"31074","subject":"[PATCH 0/4] Various merge / diff tool related minor clean-ups and improvements","startedAt":"2012-07-23T07:10:06Z","lastAt":"2012-08-26T18:38:22Z","messageCount":40,"participants":["Sebastian Schuberth","Junio C Hamano","David Aguilar","Andreas Schwab","Jens Lehmann"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"195505","messageId":"500CF8CE.90906@gmail.com","threadId":"31074","inReplyTo":null,"subject":"[PATCH 0/4] Various merge / diff tool related minor clean-ups and improvements","fromName":"Sebastian Schuberth","fromEmail":"sschuberth@gmail.com","sentAt":"2012-07-23T07:10:06Z","receivedAt":"2012-07-23T07:10:06Z","isPatch":true,"sender":{"key":"sschuberth@gmail.com","avatar":"https://avatars.githubusercontent.com/u/349154?v=4"},"body":"This series introduce various minor clean-ups and improvements to the merge / diff tool scripts and documentation.\n\nSebastian Schuberth (4):\n Use variables for the lists of tools that support merging / diffing\n Explicitly list all valid diff tools and document --tool-help as an\n option\n Make sure to use Araxis' \"compare\" and not e.g. ImageMagick's\n Add a few more code comments and blank lines in guess_merge_tool\n\n Documentation/git-difftool.txt | 9 ++++++---\n contrib/completion/git-completion.bash | 11 +++++++++--\n git-mergetool--lib.sh | 6 ++++++\n mergetools/araxis | 8 +++++++-\n 4 files changed, 28 insertions(+), 6 deletions(-)\n\n-- \n1.7.11.msysgit.2\n"},{"id":"195506","messageId":"500CF9D2.30102@gmail.com","threadId":"31074","inReplyTo":"500CF8CE.90906@gmail.com","subject":"[PATCH v2 0/5] Various merge / diff tool related minor clean-ups and improvements","fromName":"Sebastian Schuberth","fromEmail":"sschuberth@gmail.com","sentAt":"2012-07-23T07:14:26Z","receivedAt":"2012-07-23T07:14:26Z","isPatch":true,"sender":{"key":"sschuberth@gmail.com","avatar":"https://avatars.githubusercontent.com/u/349154?v=4"},"body":"This series introduces various minor clean-ups and improvements to the merge / diff tool scripts and documentation.\n\nSorry, the first version was missing a patch.\n\nSebastian Schuberth (5):\n Sort the list of tools that support both merging and diffing\n alphabetically\n Use variables for the lists of tools that support merging / diffing\n Explicitly list all valid diff tools and document --tool-help as an\n option\n Make sure to use Araxis' \"compare\" and not e.g. ImageMagick's\n Add a few more code comments and blank lines in guess_merge_tool\n\n Documentation/git-difftool.txt | 9 ++++++---\n contrib/completion/git-completion.bash | 15 +++++++++++----\n git-mergetool--lib.sh | 6 ++++++\n mergetools/araxis | 8 +++++++-\n 4 files changed, 30 insertions(+), 8 deletions(-)\n\n-- \n1.7.11.msysgit.2\n"},{"id":"195509","messageId":"500CFA76.3060600@gmail.com","threadId":"31074","inReplyTo":"500CF9D2.30102@gmail.com","subject":"[PATCH v2 1/5] Sort the list of tools that support both merging and diffing alphabetically","fromName":"Sebastian Schuberth","fromEmail":"sschuberth@gmail.com","sentAt":"2012-07-23T07:17:10Z","receivedAt":"2012-07-23T07:17:10Z","isPatch":true,"sender":{"key":"sschuberth@gmail.com","avatar":"https://avatars.githubusercontent.com/u/349154?v=4"},"body":"Signed-off-by: Sebastian Schuberth \n---\n contrib/completion/git-completion.bash | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\nindex 5be9dee..f2c4894 100755\n--- a/contrib/completion/git-completion.bash\n+++ b/contrib/completion/git-completion.bash\n@@ -1325,8 +1325,8 @@ _git_diff ()\n \t__git_complete_revlist_file\n }\n \n-__git_mergetools_common=\"diffuse ecmerge emerge kdiff3 meld opendiff\n-\t\t\ttkdiff vimdiff gvimdiff xxdiff araxis p4merge bc3\n+__git_mergetools_common=\"araxis bc3 diffuse ecmerge emerge gvimdiff\n+\t\t\tkdiff3 meld opendiff p4merge tkdiff vimdiff xxdiff\n \"\n \n _git_difftool ()\n-- \n1.7.11.msysgit.2\n"},{"id":"195507","messageId":"500CFAB4.4050905@gmail.com","threadId":"31074","inReplyTo":"500CF9D2.30102@gmail.com","subject":"[PATCH v2 2/5] Use variables for the lists of tools that support merging / diffing","fromName":"Sebastian Schuberth","fromEmail":"sschuberth@gmail.com","sentAt":"2012-07-23T07:18:12Z","receivedAt":"2012-07-23T07:18:12Z","isPatch":true,"sender":{"key":"sschuberth@gmail.com","avatar":"https://avatars.githubusercontent.com/u/349154?v=4"},"body":"Also, add a few comments that clarify the meaning of these variables.\n\nSigned-off-by: Sebastian Schuberth \n---\n contrib/completion/git-completion.bash | 11 +++++++++--\n 1 file changed, 9 insertions(+), 2 deletions(-)\n\ndiff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\nindex f2c4894..6b9b79d 100755\n--- a/contrib/completion/git-completion.bash\n+++ b/contrib/completion/git-completion.bash\n@@ -1325,17 +1325,24 @@ _git_diff ()\n \t__git_complete_revlist_file\n }\n \n+# Tools that support both merging and diffing.\n __git_mergetools_common=\"araxis bc3 diffuse ecmerge emerge gvimdiff\n \t\t\tkdiff3 meld opendiff p4merge tkdiff vimdiff xxdiff\n \"\n \n+# Tools that support diffing.\n+__git_difftools=\"$__git_mergetools_common kcompare\"\n+\n+# Tools that support merging.\n+__git_mergetools=\"$__git_mergetools_common tortoisemerge\"\n+\n _git_difftool ()\n {\n \t__git_has_doubledash && return\n \n \tcase \"$cur\" in\n \t--tool=*)\n-\t\t__gitcomp \"$__git_mergetools_common kompare\" \"\" \"${cur##--tool=}\"\n+\t\t__gitcomp \"$__git_difftools\" \"\" \"${cur##--tool=}\"\n \t\treturn\n \t\t;;\n \t--*)\n@@ -1623,7 +1630,7 @@ _git_mergetool ()\n {\n \tcase \"$cur\" in\n \t--tool=*)\n-\t\t__gitcomp \"$__git_mergetools_common tortoisemerge\" \"\" \"${cur##--tool=}\"\n+\t\t__gitcomp \"$__git_mergetools\" \"\" \"${cur##--tool=}\"\n \t\treturn\n \t\t;;\n \t--*)\n-- \n1.7.11.msysgit.2\n"},{"id":"195514","messageId":"500CFAE1.3070304@gmail.com","threadId":"31074","inReplyTo":"500CF9D2.30102@gmail.com","subject":"[PATCH 3/5] Explicitly list all valid diff tools and document --tool-help as an option","fromName":"Sebastian Schuberth","fromEmail":"sschuberth@gmail.com","sentAt":"2012-07-23T07:18:57Z","receivedAt":"2012-07-23T07:18:57Z","isPatch":true,"sender":{"key":"sschuberth@gmail.com","avatar":"https://avatars.githubusercontent.com/u/349154?v=4"},"body":"Signed-off-by: Sebastian Schuberth \n---\n Documentation/git-difftool.txt | 9 ++++++---\n 1 file changed, 6 insertions(+), 3 deletions(-)\n\ndiff --git a/Documentation/git-difftool.txt b/Documentation/git-difftool.txt\nindex 31fc2e3..5dd54f1 100644\n--- a/Documentation/git-difftool.txt\n+++ b/Documentation/git-difftool.txt\n@@ -36,9 +36,12 @@ OPTIONS\n \n -t ::\n --tool=::\n-\tUse the diff tool specified by . Valid values include\n-\temerge, kompare, meld, and vimdiff. Run `git difftool --tool-help`\n-\tfor the list of valid settings.\n+\tUse the diff tool specified by . Valid diff tools are:\n+\taraxis, bc3, diffuse, ecmerge, emerge, gvimdiff, kcompare, kdiff3,\n+\tmeld, opendiff, p4merge, tkdiff, vimdiff and xxdiff.\n+\n+--tool-help::\n+\tList the supported and available diff tools.\n +\n If a diff tool is not specified, 'git difftool'\n will use the configuration variable `diff.tool`. If the\n-- \n1.7.11.msysgit.2\n"},{"id":"195510","messageId":"500CFB19.6010905@gmail.com","threadId":"31074","inReplyTo":"500CF9D2.30102@gmail.com","subject":"[PATCH v2 4/5] Make sure to use Araxis' \"compare\" and not e.g. ImageMagick's","fromName":"Sebastian Schuberth","fromEmail":"sschuberth@gmail.com","sentAt":"2012-07-23T07:19:53Z","receivedAt":"2012-07-23T07:19:53Z","isPatch":true,"sender":{"key":"sschuberth@gmail.com","avatar":"https://avatars.githubusercontent.com/u/349154?v=4"},"body":"Signed-off-by: Sebastian Schuberth \n---\n mergetools/araxis | 8 +++++++-\n 1 file changed, 7 insertions(+), 1 deletion(-)\n\ndiff --git a/mergetools/araxis b/mergetools/araxis\nindex 64f97c5..aeba1b9 100644\n--- a/mergetools/araxis\n+++ b/mergetools/araxis\n@@ -16,5 +16,11 @@ merge_cmd () {\n }\n \n translate_merge_tool_path() {\n-\techo compare\n+\t# Make sure to use Araxis' \"compare\" and not e.g. ImageMagick's.\n+\tif ls \"$(dirname \"$(which compare)\")\"/Araxis* >/dev/null 2>&1\n+\tthen\n+\t\techo compare\n+\telse\n+\t\techo \"$1\"\n+\tfi\n }\n-- \n1.7.11.msysgit.2\n"},{"id":"195512","messageId":"500CFB54.2030109@gmail.com","threadId":"31074","inReplyTo":"500CF9D2.30102@gmail.com","subject":"[PATCH v2 5/5] Add a few more code comments and blank lines in guess_merge_tool","fromName":"Sebastian Schuberth","fromEmail":"sschuberth@gmail.com","sentAt":"2012-07-23T07:20:52Z","receivedAt":"2012-07-23T07:20:52Z","isPatch":true,"sender":{"key":"sschuberth@gmail.com","avatar":"https://avatars.githubusercontent.com/u/349154?v=4"},"body":"Signed-off-by: Sebastian Schuberth \n---\n git-mergetool--lib.sh | 6 ++++++\n 1 file changed, 6 insertions(+)\n\ndiff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh\nindex ed630b2..ac9a8f0 100644\n--- a/git-mergetool--lib.sh\n+++ b/git-mergetool--lib.sh\n@@ -112,14 +112,17 @@ run_merge_tool () {\n }\n \n guess_merge_tool () {\n+\t# Add tools that can either do merging or diffing, but not both.\n \tif merge_mode\n \tthen\n \t\ttools=\"tortoisemerge\"\n \telse\n \t\ttools=\"kompare\"\n \tfi\n+\n \tif test -n \"$DISPLAY\"\n \tthen\n+\t\t# Prefer GTK-based tools under Gnome.\n \t\tif test -n \"$GNOME_DESKTOP_SESSION_ID\"\n \t\tthen\n \t\t\ttools=\"meld opendiff kdiff3 tkdiff xxdiff $tools\"\n@@ -128,6 +131,8 @@ guess_merge_tool () {\n \t\tfi\n \t\ttools=\"$tools gvimdiff diffuse ecmerge p4merge araxis bc3\"\n \tfi\n+\n+\t# Prefer vimdiff if vim is the default editor.\n \tcase \"${VISUAL:-$EDITOR}\" in\n \t*vim*)\n \t\ttools=\"$tools vimdiff emerge\"\n@@ -136,6 +141,7 @@ guess_merge_tool () {\n \t\ttools=\"$tools emerge vimdiff\"\n \t\t;;\n \tesac\n+\n \techo >&2 \"merge tool candidates: $tools\"\n \n \t# Loop over each candidate and stop when a valid merge tool is found.\n-- \n1.7.11.msysgit.2\n"},{"id":"195530","messageId":"7vvchemnmn.fsf@alter.siamese.dyndns.org","threadId":"31074","inReplyTo":"500CFAB4.4050905@gmail.com","subject":"Re: [PATCH v2 2/5] Use variables for the lists of tools that support merging / diffing","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-07-23T16:46:24Z","receivedAt":"2012-07-23T16:46:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sebastian Schuberth writes:\n\n> Also, add a few comments that clarify the meaning of these variables.\n>\n> Signed-off-by: Sebastian Schuberth \n> ---\n> contrib/completion/git-completion.bash | 11 +++++++++--\n> 1 file changed, 9 insertions(+), 2 deletions(-)\n>\n> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\n> index f2c4894..6b9b79d 100755\n> --- a/contrib/completion/git-completion.bash\n> +++ b/contrib/completion/git-completion.bash\n> @@ -1325,17 +1325,24 @@ _git_diff ()\n> \t__git_complete_revlist_file\n> }\n> \n> +# Tools that support both merging and diffing.\n> __git_mergetools_common=\"araxis bc3 diffuse ecmerge emerge gvimdiff\n> \t\t\tkdiff3 meld opendiff p4merge tkdiff vimdiff xxdiff\n> \"\n\nAs the set of merge capable tools is not a superset of diff capable\ntools (tortoise can only merge but not diff), perhaps rename this to\n__git_diffmerge_tools or something?\n\n> +# Tools that support diffing.\n> +__git_difftools=\"$__git_mergetools_common kcompare\"\n> +\n> +# Tools that support merging.\n> +__git_mergetools=\"$__git_mergetools_common tortoisemerge\"\n> +\n\nThis patch makes sense to me, but at the same time makes [PATCH 1/5]\na \"Meh\", methinks.\n\n> _git_difftool ()\n> {\n> \t__git_has_doubledash && return\n> \n> \tcase \"$cur\" in\n> \t--tool=*)\n> -\t\t__gitcomp \"$__git_mergetools_common kompare\" \"\" \"${cur##--tool=}\"\n> +\t\t__gitcomp \"$__git_difftools\" \"\" \"${cur##--tool=}\"\n> \t\treturn\n> \t\t;;\n> \t--*)\n> @@ -1623,7 +1630,7 @@ _git_mergetool ()\n> {\n> \tcase \"$cur\" in\n> \t--tool=*)\n> -\t\t__gitcomp \"$__git_mergetools_common tortoisemerge\" \"\" \"${cur##--tool=}\"\n> +\t\t__gitcomp \"$__git_mergetools\" \"\" \"${cur##--tool=}\"\n> \t\treturn\n> \t\t;;\n> \t--*)\n"},{"id":"195531","messageId":"7vr4s2mnir.fsf@alter.siamese.dyndns.org","threadId":"31074","inReplyTo":"500CFAE1.3070304@gmail.com","subject":"Re: [PATCH 3/5] Explicitly list all valid diff tools and document --tool-help as an option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-07-23T16:48:44Z","receivedAt":"2012-07-23T16:48:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sebastian Schuberth writes:\n\n> Signed-off-by: Sebastian Schuberth \n> ---\n> Documentation/git-difftool.txt | 9 ++++++---\n> 1 file changed, 6 insertions(+), 3 deletions(-)\n>\n> diff --git a/Documentation/git-difftool.txt b/Documentation/git-difftool.txt\n> index 31fc2e3..5dd54f1 100644\n> --- a/Documentation/git-difftool.txt\n> +++ b/Documentation/git-difftool.txt\n> @@ -36,9 +36,12 @@ OPTIONS\n> \n> -t ::\n> --tool=::\n> -\tUse the diff tool specified by . Valid values include\n> -\temerge, kompare, meld, and vimdiff. Run `git difftool --tool-help`\n> -\tfor the list of valid settings.\n\nI thought we say \"include\" because we really do not want to list\nmillions of tools here, so mild NAK on this part.\n\n> +\tUse the diff tool specified by . Valid diff tools are:\n> +\taraxis, bc3, diffuse, ecmerge, emerge, gvimdiff, kcompare, kdiff3,\n> +\tmeld, opendiff, p4merge, tkdiff, vimdiff and xxdiff.\n> +\n> +--tool-help::\n> +\tList the supported and available diff tools.\n\nThis part is a good addition (but it already is mentioned in the\ndescription of --tool above, so it is more or less a \"Meh\").\n\n> +\n> If a diff tool is not specified, 'git difftool'\n> will use the configuration variable `diff.tool`. If the\n"},{"id":"195532","messageId":"7vmx2qmnfw.fsf@alter.siamese.dyndns.org","threadId":"31074","inReplyTo":"500CFB19.6010905@gmail.com","subject":"Re: [PATCH v2 4/5] Make sure to use Araxis' \"compare\" and not e.g. ImageMagick's","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-07-23T16:50:27Z","receivedAt":"2012-07-23T16:50:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sebastian Schuberth writes:\n\n> Signed-off-by: Sebastian Schuberth \n> ---\n> mergetools/araxis | 8 +++++++-\n> 1 file changed, 7 insertions(+), 1 deletion(-)\n>\n> diff --git a/mergetools/araxis b/mergetools/araxis\n> index 64f97c5..aeba1b9 100644\n> --- a/mergetools/araxis\n> +++ b/mergetools/araxis\n> @@ -16,5 +16,11 @@ merge_cmd () {\n> }\n> \n> translate_merge_tool_path() {\n> -\techo compare\n> +\t# Make sure to use Araxis' \"compare\" and not e.g. ImageMagick's.\n> +\tif ls \"$(dirname \"$(which compare)\")\"/Araxis* >/dev/null 2>&1\n\nUse of \"which\" in scripts that are meant to be portable is a no-no.\nSome platforms would say \"compare is /usr/local/bin/compare\" instead\nof saying \"/usr/local/bin/compare\".\n\n> +\tthen\n> +\t\techo compare\n> +\telse\n> +\t\techo \"$1\"\n> +\tfi\n> }\n"},{"id":"195533","messageId":"7vipdemm0a.fsf_-_@alter.siamese.dyndns.org","threadId":"31074","inReplyTo":"7vr4s2mnir.fsf@alter.siamese.dyndns.org","subject":"mergetool: support --tool-help option like difftool does","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-07-23T17:21:25Z","receivedAt":"2012-07-23T17:21:25Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"This way we do not have to risk the list of tools go out of sync\nbetween the implementation and the documentation.\n\nSigned-off-by: Junio C Hamano \n---\nJunio C Hamano writes:\n\n>> +--tool-help::\n>> +\tList the supported and available diff tools.\n>\n> This part is a good addition (but it already is mentioned in the\n> description of --tool above, so it is more or less a \"Meh\").\n\nI noticed that the main while loop has style violations in its\ncase/esac, but I left it as-is. Reindenting it would be a low\nhanging fruit janitor patch that would be a separate topic.\n\n git-mergetool--lib.sh | 6 +++++-\n git-mergetool.sh | 42 +++++++++++++++++++++++++++++++++++++++++-\n 2 files changed, 46 insertions(+), 2 deletions(-)\n\ndiff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh\nindex ed630b2..f730253 100644\n--- a/git-mergetool--lib.sh\n+++ b/git-mergetool--lib.sh\n@@ -111,7 +111,7 @@ run_merge_tool () {\n \treturn $status\n }\n \n-guess_merge_tool () {\n+list_merge_tool_candidates () {\n \tif merge_mode\n \tthen\n \t\ttools=\"tortoisemerge\"\n@@ -136,6 +136,10 @@ guess_merge_tool () {\n \t\ttools=\"$tools emerge vimdiff\"\n \t\t;;\n \tesac\n+}\n+\n+guess_merge_tool () {\n+\tlist_merge_tool_candidates\n \techo >&2 \"merge tool candidates: $tools\"\n \n \t# Loop over each candidate and stop when a valid merge tool is found.\ndiff --git a/git-mergetool.sh b/git-mergetool.sh\nindex a9f23f7..0db0c44 100755\n--- a/git-mergetool.sh\n+++ b/git-mergetool.sh\n@@ -8,7 +8,7 @@\n # at the discretion of Junio C Hamano.\n #\n \n-USAGE='[--tool=tool] [-y|--no-prompt|--prompt] [file to merge] ...'\n+USAGE='[--tool=tool] [--tool-help] [-y|--no-prompt|--prompt] [file to merge] ...'\n SUBDIRECTORY_OK=Yes\n OPTIONS_SPEC=\n TOOL_MODE=merge\n@@ -284,11 +284,51 @@ merge_file () {\n return 0\n }\n \n+show_tool_help () {\n+\tTOOL_MODE=merge\n+\tlist_merge_tool_candidates\n+\tunavailable= available= LF='\n+'\n+\tfor i in $tools\n+\tdo\n+\t\tmerge_tool_path=$(translate_merge_tool_path \"$i\")\n+\t\tif type \"$merge_tool_path\" >/dev/null 2>&1\n+\t\tthen\n+\t\t\tavailable=\"$available$i$LF\"\n+\t\telse\n+\t\t\tunavailable=\"$unavailable$i$LF\"\n+\t\tfi\n+\tdone\n+\tif test -n \"$available\"\n+\tthen\n+\t\techo \"'git mergetool --tool=' may be set to one of the following:\"\n+\t\techo \"$available\" | sort | sed -e 's/^/\t/'\n+\telse\n+\t\techo \"No suitable tool for 'git mergetool --tool=' found.\"\n+\tfi\n+\tif test -n \"$unavailable\"\n+\tthen\n+\t\techo\n+\t\techo 'The following tools are valid, but not currently available:'\n+\t\techo \"$unavailable\" | sort | sed -e 's/^/\t/'\n+\tfi\n+\tif test -n \"$unavailable$available\"\n+\tthen\n+\t\techo\n+\t\techo \"Some of the tools listed above only work in a windowed\"\n+\t\techo \"environment. If run in a terminal-only session, they will fail.\"\n+\tfi\n+\texit 0\n+}\n+\n prompt=$(git config --bool mergetool.prompt || echo true)\n \n while test $# != 0\n do\n case \"$1\" in\n+\t--tool-help)\n+\t\tshow_tool_help\n+\t\t;;\n \t-t|--tool*)\n \t case \"$#,$1\" in\n \t\t*,*=*)\n"},{"id":"195542","messageId":"500D984D.4070909@gmail.com","threadId":"31074","inReplyTo":"7vvchemnmn.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2 2/5] Use variables for the lists of tools that support merging / diffing","fromName":"Sebastian Schuberth","fromEmail":"sschuberth@gmail.com","sentAt":"2012-07-23T18:30:37Z","receivedAt":"2012-07-23T18:30:37Z","isPatch":true,"sender":{"key":"sschuberth@gmail.com","avatar":"https://avatars.githubusercontent.com/u/349154?v=4"},"body":"On 23.07.2012 18:46, Junio C Hamano wrote:\n\n>> +# Tools that support both merging and diffing.\n>> __git_mergetools_common=\"araxis bc3 diffuse ecmerge emerge gvimdiff\n>> \t\t\tkdiff3 meld opendiff p4merge tkdiff vimdiff xxdiff\n>> \"\n> \n> As the set of merge capable tools is not a superset of diff capable\n> tools (tortoise can only merge but not diff), perhaps rename this to\n> __git_diffmerge_tools or something?\n\nMakes perfect sense.\n\n> This patch makes sense to me, but at the same time makes [PATCH 1/5]\n> a \"Meh\", methinks.\n\nYeah, 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.\n\n-- \nSebastian\n"},{"id":"195543","messageId":"500D98AA.2030902@gmail.com","threadId":"31074","inReplyTo":"500D984D.4070909@gmail.com","subject":"[PATCH 1/4] Use variables for the lists of tools that support merging / diffing","fromName":"Sebastian Schuberth","fromEmail":"sschuberth@gmail.com","sentAt":"2012-07-23T18:32:10Z","receivedAt":"2012-07-23T18:32:10Z","isPatch":true,"sender":{"key":"sschuberth@gmail.com","avatar":"https://avatars.githubusercontent.com/u/349154?v=4"},"body":"Also, add a few comments that clarify the meaning of these variables and\nsort the list of tools alphabetically.\n\nSigned-off-by: Sebastian Schuberth \n---\n contrib/completion/git-completion.bash | 15 +++++++++++----\n 1 file changed, 11 insertions(+), 4 deletions(-)\n\ndiff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\nindex 5be9dee..4a76120 100755\n--- a/contrib/completion/git-completion.bash\n+++ b/contrib/completion/git-completion.bash\n@@ -1325,17 +1325,24 @@ _git_diff ()\n \t__git_complete_revlist_file\n }\n \n-__git_mergetools_common=\"diffuse ecmerge emerge kdiff3 meld opendiff\n-\t\t\ttkdiff vimdiff gvimdiff xxdiff araxis p4merge bc3\n+# Tools that support both merging and diffing.\n+__git_diffmerge_tools=\"araxis bc3 diffuse ecmerge emerge gvimdiff\n+\t\t\tkdiff3 meld opendiff p4merge tkdiff vimdiff xxdiff\n \"\n \n+# Tools that support diffing.\n+__git_difftools=\"$__git_diffmerge_tools kcompare\"\n+\n+# Tools that support merging.\n+__git_mergetools=\"$__git_diffmerge_tools tortoisemerge\"\n+\n _git_difftool ()\n {\n \t__git_has_doubledash && return\n \n \tcase \"$cur\" in\n \t--tool=*)\n-\t\t__gitcomp \"$__git_mergetools_common kompare\" \"\" \"${cur##--tool=}\"\n+\t\t__gitcomp \"$__git_difftools\" \"\" \"${cur##--tool=}\"\n \t\treturn\n \t\t;;\n \t--*)\n@@ -1623,7 +1630,7 @@ _git_mergetool ()\n {\n \tcase \"$cur\" in\n \t--tool=*)\n-\t\t__gitcomp \"$__git_mergetools_common tortoisemerge\" \"\" \"${cur##--tool=}\"\n+\t\t__gitcomp \"$__git_mergetools\" \"\" \"${cur##--tool=}\"\n \t\treturn\n \t\t;;\n \t--*)\n-- \n1.7.11.msysgit.2\n"},{"id":"195546","messageId":"7vzk6ql3wk.fsf@alter.siamese.dyndns.org","threadId":"31074","inReplyTo":"500D984D.4070909@gmail.com","subject":"Re: [PATCH v2 2/5] Use variables for the lists of tools that support merging / diffing","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-07-23T18:37:47Z","receivedAt":"2012-07-23T18:37:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sebastian Schuberth writes:\n\n>> This patch makes sense to me, but at the same time makes [PATCH 1/5]\n>> a \"Meh\", methinks.\n>\n> Yeah, I can see why. So I've renamed __git_mergetools_common to\n> __git_diffmerge_tools and squashed with [PATCH 1/5] to make it\n> less \"Meh\" as it does not stand on its own.\n\nAs you append kcompare or tortoise _after_ the common list, any code\nthat uses the variable cannot assume that the list is sorted, and\nneeds to sort the elements if it wants to give a sorted output, so\nsquashing does not make the Meh-ness go away.\n\nBy the way, would it make sense to remove these three variables from\nthe completion code, and instead ask \"git mergetool --tool-help\"\nwhen it needs the list of supported tools for the first time? It\nwould be trivial to introduce --tool-list that gives a one tool per\nline output to both \"git difftool\" and \"git mergetool\" and we would\nremove the risk of separately maintained list drifting away over\ntime.\n"},{"id":"195552","messageId":"500D9E5A.2000204@gmail.com","threadId":"31074","inReplyTo":"7vipdemm0a.fsf_-_@alter.siamese.dyndns.org","subject":"Re: mergetool: support --tool-help option like difftool does","fromName":"Sebastian Schuberth","fromEmail":"sschuberth@gmail.com","sentAt":"2012-07-23T18:56:26Z","receivedAt":"2012-07-23T18:56:26Z","isPatch":false,"sender":{"key":"sschuberth@gmail.com","avatar":"https://avatars.githubusercontent.com/u/349154?v=4"},"body":"On 23.07.2012 19:21, Junio C Hamano wrote:\n\n> This way we do not have to risk the list of tools go out of sync\n> between the implementation and the documentation.\n> \n> Signed-off-by: Junio C Hamano \n\nThanks! I've squashed this with\n\n[PATCH v2 5/5] Add a few more code comments and blank lines in guess_merge_tool\n\nand parts of\n\n[PATCH 3/5] Explicitly list all valid diff tools and document --tool-help as an option\n\nand adjusted the docs for git-mergetool accordingly.\n\n-- \nSebastian Schuberth\n"},{"id":"195553","messageId":"500D9EDB.90603@gmail.com","threadId":"31074","inReplyTo":"500D9E5A.2000204@gmail.com","subject":"[PATCH] mergetool: support --tool-help option like difftool does","fromName":"Sebastian Schuberth","fromEmail":"sschuberth@gmail.com","sentAt":"2012-07-23T18:58:35Z","receivedAt":"2012-07-23T18:58:35Z","isPatch":true,"sender":{"key":"sschuberth@gmail.com","avatar":"https://avatars.githubusercontent.com/u/349154?v=4"},"body":"This way we do not have to risk the list of tools go out of sync\nbetween the implementation and the documentation. Adjust the documentation\naccordingly to not explicitly list the tools but refer to --tool-help.\n\nSigned-off-by: Junio C Hamano \nSigned-off-by: Sebastian Schuberth \n---\n Documentation/git-difftool.txt | 7 ++++---\n Documentation/git-mergetool.txt | 8 ++++----\n git-mergetool--lib.sh | 11 ++++++++++-\n git-mergetool.sh | 42 ++++++++++++++++++++++++++++++++++++++++-\n 4 files changed, 59 insertions(+), 9 deletions(-)\n\ndiff --git a/Documentation/git-difftool.txt b/Documentation/git-difftool.txt\nindex 31fc2e3..0bdfe35 100644\n--- a/Documentation/git-difftool.txt\n+++ b/Documentation/git-difftool.txt\n@@ -36,9 +36,10 @@ OPTIONS\n \n -t ::\n --tool=::\n-\tUse the diff tool specified by . Valid values include\n-\temerge, kompare, meld, and vimdiff. Run `git difftool --tool-help`\n-\tfor the list of valid settings.\n+\tUse the diff tool specified by .\n+\n+--tool-help::\n+\tList all supported values for .\n +\n If a diff tool is not specified, 'git difftool'\n will use the configuration variable `diff.tool`. If the\ndiff --git a/Documentation/git-mergetool.txt b/Documentation/git-mergetool.txt\nindex 2a49de7..99e53b1 100644\n--- a/Documentation/git-mergetool.txt\n+++ b/Documentation/git-mergetool.txt\n@@ -26,10 +26,10 @@ OPTIONS\n -------\n -t ::\n --tool=::\n-\tUse the merge resolution program specified by .\n-\tValid merge tools are:\n-\taraxis, bc3, diffuse, ecmerge, emerge, gvimdiff, kdiff3,\n-\tmeld, opendiff, p4merge, tkdiff, tortoisemerge, vimdiff and xxdiff.\n+\tUse the merge tool specified by .\n+\n+--tool-help::\n+\tList all supported values for .\n +\n If a merge resolution program is not specified, 'git mergetool'\n will use the configuration variable `merge.tool`. If the\ndiff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh\nindex ed630b2..f0865d4 100644\n--- a/git-mergetool--lib.sh\n+++ b/git-mergetool--lib.sh\n@@ -111,15 +111,18 @@ run_merge_tool () {\n \treturn $status\n }\n \n-guess_merge_tool () {\n+list_merge_tool_candidates () {\n+\t# Add tools that can either do merging or diffing, but not both.\n \tif merge_mode\n \tthen\n \t\ttools=\"tortoisemerge\"\n \telse\n \t\ttools=\"kompare\"\n \tfi\n+\n \tif test -n \"$DISPLAY\"\n \tthen\n+\t\t# Prefer GTK-based tools under Gnome.\n \t\tif test -n \"$GNOME_DESKTOP_SESSION_ID\"\n \t\tthen\n \t\t\ttools=\"meld opendiff kdiff3 tkdiff xxdiff $tools\"\n@@ -128,6 +131,8 @@ guess_merge_tool () {\n \t\tfi\n \t\ttools=\"$tools gvimdiff diffuse ecmerge p4merge araxis bc3\"\n \tfi\n+\n+\t# Prefer vimdiff if vim is the default editor.\n \tcase \"${VISUAL:-$EDITOR}\" in\n \t*vim*)\n \t\ttools=\"$tools vimdiff emerge\"\n@@ -136,6 +141,10 @@ guess_merge_tool () {\n \t\ttools=\"$tools emerge vimdiff\"\n \t\t;;\n \tesac\n+}\n+\n+guess_merge_tool () {\n+\tlist_merge_tool_candidates\n \techo >&2 \"merge tool candidates: $tools\"\n \n \t# Loop over each candidate and stop when a valid merge tool is found.\ndiff --git a/git-mergetool.sh b/git-mergetool.sh\nindex a9f23f7..0db0c44 100755\n--- a/git-mergetool.sh\n+++ b/git-mergetool.sh\n@@ -8,7 +8,7 @@\n # at the discretion of Junio C Hamano.\n #\n \n-USAGE='[--tool=tool] [-y|--no-prompt|--prompt] [file to merge] ...'\n+USAGE='[--tool=tool] [--tool-help] [-y|--no-prompt|--prompt] [file to merge] ...'\n SUBDIRECTORY_OK=Yes\n OPTIONS_SPEC=\n TOOL_MODE=merge\n@@ -284,11 +284,51 @@ merge_file () {\n return 0\n }\n \n+show_tool_help () {\n+\tTOOL_MODE=merge\n+\tlist_merge_tool_candidates\n+\tunavailable= available= LF='\n+'\n+\tfor i in $tools\n+\tdo\n+\t\tmerge_tool_path=$(translate_merge_tool_path \"$i\")\n+\t\tif type \"$merge_tool_path\" >/dev/null 2>&1\n+\t\tthen\n+\t\t\tavailable=\"$available$i$LF\"\n+\t\telse\n+\t\t\tunavailable=\"$unavailable$i$LF\"\n+\t\tfi\n+\tdone\n+\tif test -n \"$available\"\n+\tthen\n+\t\techo \"'git mergetool --tool=' may be set to one of the following:\"\n+\t\techo \"$available\" | sort | sed -e 's/^/\t/'\n+\telse\n+\t\techo \"No suitable tool for 'git mergetool --tool=' found.\"\n+\tfi\n+\tif test -n \"$unavailable\"\n+\tthen\n+\t\techo\n+\t\techo 'The following tools are valid, but not currently available:'\n+\t\techo \"$unavailable\" | sort | sed -e 's/^/\t/'\n+\tfi\n+\tif test -n \"$unavailable$available\"\n+\tthen\n+\t\techo\n+\t\techo \"Some of the tools listed above only work in a windowed\"\n+\t\techo \"environment. If run in a terminal-only session, they will fail.\"\n+\tfi\n+\texit 0\n+}\n+\n prompt=$(git config --bool mergetool.prompt || echo true)\n \n while test $# != 0\n do\n case \"$1\" in\n+\t--tool-help)\n+\t\tshow_tool_help\n+\t\t;;\n \t-t|--tool*)\n \t case \"$#,$1\" in\n \t\t*,*=*)\n-- \n1.7.11.msysgit.2\n"},{"id":"195554","messageId":"500DA01F.20502@gmail.com","threadId":"31074","inReplyTo":"7vzk6ql3wk.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2 2/5] Use variables for the lists of tools that support merging / diffing","fromName":"Sebastian Schuberth","fromEmail":"sschuberth@gmail.com","sentAt":"2012-07-23T19:03:59Z","receivedAt":"2012-07-23T19:03:59Z","isPatch":true,"sender":{"key":"sschuberth@gmail.com","avatar":"https://avatars.githubusercontent.com/u/349154?v=4"},"body":"On 23.07.2012 20:37, Junio C Hamano wrote:\n\n>>> This patch makes sense to me, but at the same time makes [PATCH 1/5]\n>>> a \"Meh\", methinks.\n>>\n>> Yeah, I can see why. So I've renamed __git_mergetools_common to\n>> __git_diffmerge_tools and squashed with [PATCH 1/5] to make it\n>> less \"Meh\" as it does not stand on its own.\n>\n> As you append kcompare or tortoise _after_ the common list, any code\n> that uses the variable cannot assume that the list is sorted, and\n> needs to sort the elements if it wants to give a sorted output, so\n> squashing does not make the Meh-ness go away.\n\nWell, that (mostly) sorted listed still helps to find out a little \nquicker whether a specific tool that can do both merging and diffing is \nalready in the list. At least that's the case for me.\n\n> By the way, would it make sense to remove these three variables from\n> the completion code, and instead ask \"git mergetool --tool-help\"\n> when it needs the list of supported tools for the first time? It\n> would be trivial to introduce --tool-list that gives a one tool per\n> line output to both \"git difftool\" and \"git mergetool\" and we would\n> remove the risk of separately maintained list drifting away over\n> time.\n\nSounds like a good idea now that you've added \"git mergetool \n--tool-help\". But I'd like to save this for a future exercise to not do \ntoo much stuff at the same time.\n\n-- \nSebastian Schuberth\n"},{"id":"195558","messageId":"500DA7F3.3000403@gmail.com","threadId":"31074","inReplyTo":"7vmx2qmnfw.fsf@alter.siamese.dyndns.org","subject":"[PATCH] Make sure to use Araxis' \"compare\" and not e.g. ImageMagick's","fromName":"Sebastian Schuberth","fromEmail":"sschuberth@gmail.com","sentAt":"2012-07-23T19:37:23Z","receivedAt":"2012-07-23T19:37:23Z","isPatch":true,"sender":{"key":"sschuberth@gmail.com","avatar":"https://avatars.githubusercontent.com/u/349154?v=4"},"body":"Signed-off-by: Sebastian Schuberth \n---\n mergetools/araxis | 9 ++++++++-\n 1 file changed, 8 insertions(+), 1 deletion(-)\n\ndiff --git a/mergetools/araxis b/mergetools/araxis\nindex 64f97c5..f8899f8 100644\n--- a/mergetools/araxis\n+++ b/mergetools/araxis\n@@ -16,5 +16,12 @@ merge_cmd () {\n }\n \n translate_merge_tool_path() {\n-\techo compare\n+\t# Only accept \"compare\" in a path that contains \"araxis\" to not\n+\t# accidently use e.g. ImageMagick's \"compare\".\n+\tif type compare | grep -i araxis >/dev/null 2>&1\n+\tthen\n+\t\techo compare\n+\telse\n+\t\techo \"$1\"\n+\tfi\n }\n-- \n1.7.11.msysgit.2\n"},{"id":"195560","messageId":"7vhasyl0gc.fsf@alter.siamese.dyndns.org","threadId":"31074","inReplyTo":"500D9EDB.90603@gmail.com","subject":"Re: [PATCH] mergetool: support --tool-help option like difftool does","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-07-23T19:52:19Z","receivedAt":"2012-07-23T19:52:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sebastian Schuberth writes:\n\n> This way we do not have to risk the list of tools go out of sync\n> between the implementation and the documentation. Adjust the documentation\n> accordingly to not explicitly list the tools but refer to --tool-help.\n>\n> Signed-off-by: Junio C Hamano \n> Signed-off-by: Sebastian Schuberth \n> ---\n> Documentation/git-difftool.txt | 7 ++++---\n> Documentation/git-mergetool.txt | 8 ++++----\n> git-mergetool--lib.sh | 11 ++++++++++-\n> git-mergetool.sh | 42 ++++++++++++++++++++++++++++++++++++++++-\n> 4 files changed, 59 insertions(+), 9 deletions(-)\n>\n> diff --git a/Documentation/git-difftool.txt b/Documentation/git-difftool.txt\n> index 31fc2e3..0bdfe35 100644\n> --- a/Documentation/git-difftool.txt\n> +++ b/Documentation/git-difftool.txt\n> @@ -36,9 +36,10 @@ OPTIONS\n> \n> -t ::\n> --tool=::\n> -\tUse the diff tool specified by . Valid values include\n> -\temerge, kompare, meld, and vimdiff. Run `git difftool --tool-help`\n> -\tfor the list of valid settings.\n> +\tUse the diff tool specified by .\n\nI do not see how it is an improvement to drop the most common ones.\nPeople sometimes read documentation without having an access to\nshell to run \"cmd --tool-help\", and a list of handful of well known\nones would serve as a good hint to let the reader know the kind of\ncommands the front-end is capable of spawning, which in turn help\nsuch a reader to imagine how the command is used to judge if it is\nsomething the reader wants to use.\n\n> +\n> +--tool-help::\n> +\tList all supported values for .\n> +\n> If a diff tool is not specified, 'git difftool'\n> will use the configuration variable `diff.tool`. If the\n> diff --git a/Documentation/git-mergetool.txt b/Documentation/git-mergetool.txt\n> index 2a49de7..99e53b1 100644\n> --- a/Documentation/git-mergetool.txt\n> +++ b/Documentation/git-mergetool.txt\n> @@ -26,10 +26,10 @@ OPTIONS\n> -------\n> -t ::\n> --tool=::\n> -\tUse the merge resolution program specified by .\n> -\tValid merge tools are:\n> -\taraxis, bc3, diffuse, ecmerge, emerge, gvimdiff, kdiff3,\n> -\tmeld, opendiff, p4merge, tkdiff, tortoisemerge, vimdiff and xxdiff.\n> +\tUse the merge tool specified by .\n\nLikewise. Dropping araxis, bc3, etc. to trim the list to 4-5 items\nthat people would know what they are would make sense, though. The\npurpose of the list is not to tell if the reader's favorite tool is\nsupported or not---it is to hint the flavor of what kind of things\nthe command spawned would be capable of.\n\n> +\n> +--tool-help::\n> +\tList all supported values for .\n> +\n> If a merge resolution program is not specified, 'git mergetool'\n> will use the configuration variable `merge.tool`. If the\n> diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh\n> index ed630b2..f0865d4 100644\n> --- a/git-mergetool--lib.sh\n> +++ b/git-mergetool--lib.sh\n> @@ -111,15 +111,18 @@ run_merge_tool () {\n> \treturn $status\n> }\n> \n> -guess_merge_tool () {\n> +list_merge_tool_candidates () {\n> +\t# Add tools that can either do merging or diffing, but not both.\n> \tif merge_mode\n> \tthen\n> \t\ttools=\"tortoisemerge\"\n> \telse\n> \t\ttools=\"kompare\"\n> \tfi\n> +\n> \tif test -n \"$DISPLAY\"\n> \tthen\n> +\t\t# Prefer GTK-based tools under Gnome.\n> \t\tif test -n \"$GNOME_DESKTOP_SESSION_ID\"\n> \t\tthen\n> \t\t\ttools=\"meld opendiff kdiff3 tkdiff xxdiff $tools\"\n> @@ -128,6 +131,8 @@ guess_merge_tool () {\n> \t\tfi\n> \t\ttools=\"$tools gvimdiff diffuse ecmerge p4merge araxis bc3\"\n> \tfi\n> +\n> +\t# Prefer vimdiff if vim is the default editor.\n> \tcase \"${VISUAL:-$EDITOR}\" in\n> \t*vim*)\n> \t\ttools=\"$tools vimdiff emerge\"\n> @@ -136,6 +141,10 @@ guess_merge_tool () {\n> \t\ttools=\"$tools emerge vimdiff\"\n> \t\t;;\n> \tesac\n> +}\n> +\n> +guess_merge_tool () {\n> +\tlist_merge_tool_candidates\n> \techo >&2 \"merge tool candidates: $tools\"\n> \n> \t# Loop over each candidate and stop when a valid merge tool is found.\n> diff --git a/git-mergetool.sh b/git-mergetool.sh\n> index a9f23f7..0db0c44 100755\n> --- a/git-mergetool.sh\n> +++ b/git-mergetool.sh\n> @@ -8,7 +8,7 @@\n> # at the discretion of Junio C Hamano.\n> #\n> \n> -USAGE='[--tool=tool] [-y|--no-prompt|--prompt] [file to merge] ...'\n> +USAGE='[--tool=tool] [--tool-help] [-y|--no-prompt|--prompt] [file to merge] ...'\n> SUBDIRECTORY_OK=Yes\n> OPTIONS_SPEC=\n> TOOL_MODE=merge\n> @@ -284,11 +284,51 @@ merge_file () {\n> return 0\n> }\n> \n> +show_tool_help () {\n> +\tTOOL_MODE=merge\n> +\tlist_merge_tool_candidates\n> +\tunavailable= available= LF='\n> +'\n> +\tfor i in $tools\n> +\tdo\n> +\t\tmerge_tool_path=$(translate_merge_tool_path \"$i\")\n> +\t\tif type \"$merge_tool_path\" >/dev/null 2>&1\n> +\t\tthen\n> +\t\t\tavailable=\"$available$i$LF\"\n> +\t\telse\n> +\t\t\tunavailable=\"$unavailable$i$LF\"\n> +\t\tfi\n> +\tdone\n> +\tif test -n \"$available\"\n> +\tthen\n> +\t\techo \"'git mergetool --tool=' may be set to one of the following:\"\n> +\t\techo \"$available\" | sort | sed -e 's/^/\t/'\n> +\telse\n> +\t\techo \"No suitable tool for 'git mergetool --tool=' found.\"\n> +\tfi\n> +\tif test -n \"$unavailable\"\n> +\tthen\n> +\t\techo\n> +\t\techo 'The following tools are valid, but not currently available:'\n> +\t\techo \"$unavailable\" | sort | sed -e 's/^/\t/'\n> +\tfi\n> +\tif test -n \"$unavailable$available\"\n> +\tthen\n> +\t\techo\n> +\t\techo \"Some of the tools listed above only work in a windowed\"\n> +\t\techo \"environment. If run in a terminal-only session, they will fail.\"\n> +\tfi\n> +\texit 0\n> +}\n> +\n> prompt=$(git config --bool mergetool.prompt || echo true)\n> \n> while test $# != 0\n> do\n> case \"$1\" in\n> +\t--tool-help)\n> +\t\tshow_tool_help\n> +\t\t;;\n> \t-t|--tool*)\n> \t case \"$#,$1\" in\n> \t\t*,*=*)\n"},{"id":"195564","messageId":"CAHGBnuMCsxmVN8fCSgazqKUwtJzJJxtrVxjKwCEEhZJ_U0cW9w@mail.gmail.com","threadId":"31074","inReplyTo":"7vhasyl0gc.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] mergetool: support --tool-help option like difftool does","fromName":"Sebastian Schuberth","fromEmail":"sschuberth@gmail.com","sentAt":"2012-07-23T20:14:33Z","receivedAt":"2012-07-23T20:14:33Z","isPatch":true,"sender":{"key":"sschuberth@gmail.com","avatar":"https://avatars.githubusercontent.com/u/349154?v=4"},"body":"On Mon, Jul 23, 2012 at 9:52 PM, Junio C Hamano wrote:\n\n>> -t ::\n>> --tool=::\n>> - Use the diff tool specified by . Valid values include\n>> - emerge, kompare, meld, and vimdiff. Run `git difftool --tool-help`\n>> - for the list of valid settings.\n>> + Use the diff tool specified by .\n>\n> I do not see how it is an improvement to drop the most common ones.\n> People sometimes read documentation without having an access to\n> shell to run \"cmd --tool-help\", and a list of handful of well known\n> ones would serve as a good hint to let the reader know the kind of\n> commands the front-end is capable of spawning, which in turn help\n> such a reader to imagine how the command is used to judge if it is\n> something the reader wants to use.\n\nI don't agree. What \"most common ones\" are depends on your platform\nand is sort of subjective. So it should be either all or non here.\nYour argument about people not having shell access is a valid one, but\nstill that would mean to list all tools in my opinion. And listing all\ntools again thwarts our goal to reduce the number of places where new\nmerge / diff tools need to be added. For people adding new merge /\ndiff tools it is just clearer what places need to be modified if there\nare no places that list an arbitrary subset of tools.\n\n-- \nSebastian Schuberth\n"},{"id":"195567","messageId":"CAJDDKr4TyDp5vxqqHGkBBSiMJXJHW528+nqrag9Rgz+vksqjhg@mail.gmail.com","threadId":"31074","inReplyTo":"CAHGBnuMCsxmVN8fCSgazqKUwtJzJJxtrVxjKwCEEhZJ_U0cW9w@mail.gmail.com","subject":"Re: [PATCH] mergetool: support --tool-help option like difftool does","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2012-07-23T20:44:32Z","receivedAt":"2012-07-23T20:44:32Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"On Mon, Jul 23, 2012 at 1:14 PM, Sebastian Schuberth\n wrote:\n> On Mon, Jul 23, 2012 at 9:52 PM, Junio C Hamano wrote:\n>\n>>> -t ::\n>>> --tool=::\n>>> - Use the diff tool specified by . Valid values include\n>>> - emerge, kompare, meld, and vimdiff. Run `git difftool --tool-help`\n>>> - for the list of valid settings.\n>>> + Use the diff tool specified by .\n>>\n>> I do not see how it is an improvement to drop the most common ones.\n>> People sometimes read documentation without having an access to\n>> shell to run \"cmd --tool-help\", and a list of handful of well known\n>> ones would serve as a good hint to let the reader know the kind of\n>> commands the front-end is capable of spawning, which in turn help\n>> such a reader to imagine how the command is used to judge if it is\n>> something the reader wants to use.\n>\n> I don't agree. What \"most common ones\" are depends on your platform\n> and is sort of subjective. So it should be either all or non here.\n\nLet's please leave this section as-is.\n\nThis part of the documentation has had a fair amount of churn.\nSpecifically, it would get touched every time a new tool was added.\n\nThe point of bf73fc212a159210398b6d46ed5e9101c650e7db was to change it\n*one last time* into something that is helpful, but not a substitute\nfor the real list output by --tool-help.\n\nIf any changes are done then it should be to make git-mergetool.txt\nmatch the advice given in git-difftool.txt.\n\n> Your argument about people not having shell access is a valid one, but\n> still that would mean to list all tools in my opinion. And listing all\n> tools again thwarts our goal to reduce the number of places where new\n> merge / diff tools need to be added. For people adding new merge /\n> diff tools it is just clearer what places need to be modified if there\n> are no places that list an arbitrary subset of tools.\n\nYes, indeed, it is arbitrary. It does have some merit, though--it is\nalso a good compromise between unhelpful (listing nothing) and painful\nto maintain (listing everything).\n-- \nDavid\n"},{"id":"195568","messageId":"7v4noykxvm.fsf@alter.siamese.dyndns.org","threadId":"31074","inReplyTo":"500DA7F3.3000403@gmail.com","subject":"Re: [PATCH] Make sure to use Araxis' \"compare\" and not e.g. ImageMagick's","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-07-23T20:47:57Z","receivedAt":"2012-07-23T20:47:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sebastian Schuberth writes:\n\n> Signed-off-by: Sebastian Schuberth \n> ---\n> mergetools/araxis | 9 ++++++++-\n> 1 file changed, 8 insertions(+), 1 deletion(-)\n>\n> diff --git a/mergetools/araxis b/mergetools/araxis\n> index 64f97c5..f8899f8 100644\n> --- a/mergetools/araxis\n> +++ b/mergetools/araxis\n> @@ -16,5 +16,12 @@ merge_cmd () {\n> }\n> \n> translate_merge_tool_path() {\n> -\techo compare\n> +\t# Only accept \"compare\" in a path that contains \"araxis\" to not\n> +\t# accidently use e.g. ImageMagick's \"compare\".\n> +\tif type compare | grep -i araxis >/dev/null 2>&1\n> +\tthen\n> +\t\techo compare\n> +\telse\n> +\t\techo \"$1\"\n> +\tfi\n> }\n\nI do not use araxis, and I do not know how rigid its installation\nprocedure is to prevent the end user from naming a path that does\nnot have the string in it.\n\nFor example, when the user tells it to install in \"/home/ss/bin\", if\nit installs its \"compare\" program in \"/home/ss/bin/araxis/compare\"\nwithout allowing the \"/araxis/\" part to be stripped away, the above\nheuristics is sufficiently safe. Otherwise, it is not.\n\nIt is unclear from your proposed commit log message what assurance\ndo we have that it is installed under such a path and why the\nheuristics the patch implements is the sane way forward.\n\nPlease convince me.\n"},{"id":"195574","messageId":"CAHGBnuOz94YR9wx_goL5YaWzPt5Z9c3gBB9CtyfcE40F5amrXw@mail.gmail.com","threadId":"31074","inReplyTo":"7v4noykxvm.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] Make sure to use Araxis' \"compare\" and not e.g. ImageMagick's","fromName":"Sebastian Schuberth","fromEmail":"sschuberth@gmail.com","sentAt":"2012-07-23T21:09:29Z","receivedAt":"2012-07-23T21:09:29Z","isPatch":true,"sender":{"key":"sschuberth@gmail.com","avatar":"https://avatars.githubusercontent.com/u/349154?v=4"},"body":"On Mon, Jul 23, 2012 at 10:47 PM, Junio C Hamano wrote:\n\n> For example, when the user tells it to install in \"/home/ss/bin\", if\n> it installs its \"compare\" program in \"/home/ss/bin/araxis/compare\"\n> without allowing the \"/araxis/\" part to be stripped away, the above\n> heuristics is sufficiently safe. Otherwise, it is not.\n\nTo the best of my knowledge, Araxis does not enforce any naming\nconvention for the path it gets installed in. That means the user may\nindeed install the program in a path that does not contain \"araxis\". I\nwas aware of this when writing the patch, but should have probably\nmade it more clear in the commit message.\n\n> It is unclear from your proposed commit log message what assurance\n> do we have that it is installed under such a path and why the\n> heuristics the patch implements is the sane way forward.\n\nWe have no such assurance. That's why you correctly call it a\nheuristics after all: it may fail. Personally, I've valued the gain of\nthe patch to not list araxis as an available diff tool by \"git\ndifftool --tool-help\" when in fact just ImageMagick is in PATH higher\nthan the loss to support araxis installations that are in a path not\ncontainung \"araxis\" but are in PATH.\n\nPlease feel free to ignore the patch if you feel the heuristics is not\nsufficiently safe. I'm currently unable to come up with a safer\nsolution while maintaining portability, i.e. not use \"which\" or doing\nrather laborious string parsing on the output of \"type\".\n\n-- \nSebastian Schuberth\n"},{"id":"195576","messageId":"7vmx2qji03.fsf@alter.siamese.dyndns.org","threadId":"31074","inReplyTo":"CAJDDKr4TyDp5vxqqHGkBBSiMJXJHW528+nqrag9Rgz+vksqjhg@mail.gmail.com","subject":"Re: [PATCH] mergetool: support --tool-help option like difftool does","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-07-23T21:16:12Z","receivedAt":"2012-07-23T21:16:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"David Aguilar writes:\n\n> On Mon, Jul 23, 2012 at 1:14 PM, Sebastian Schuberth\n> wrote:\n>> On Mon, Jul 23, 2012 at 9:52 PM, Junio C Hamano wrote:\n>>\n>>>> -t ::\n>>>> --tool=::\n>>>> - Use the diff tool specified by . Valid values include\n>>>> - emerge, kompare, meld, and vimdiff. Run `git difftool --tool-help`\n>>>> - for the list of valid settings.\n>>>> + Use the diff tool specified by .\n>>>\n>>> I do not see how it is an improvement to drop the most common ones.\n>>> People sometimes read documentation without having an access to\n>>> shell to run \"cmd --tool-help\", and a list of handful of well known\n>>> ones would serve as a good hint to let the reader know the kind of\n>>> commands the front-end is capable of spawning, which in turn help\n>>> such a reader to imagine how the command is used to judge if it is\n>>> something the reader wants to use.\n>>\n>> I don't agree. What \"most common ones\" are depends on your platform\n>> and is sort of subjective. So it should be either all or non here.\n>\n> Let's please leave this section as-is.\n\nYes. \n\nThere are only five or six classes of environment that matter in the\nreal world for the purpose of giving \"well known\" examples: Windows,\nMacOS X, Gnome, KDE and Linux terminal. By picking a representative\none from each and listing them, the end result would have at least\none that people from various platforms have _heard of_ and can guess\nwhat they do. The \"most common\" is secondary, and \"well known\" is\nthe keyword. \"Can guess what they do\" is the point of the phrase\n\"Valid values _include_\". Even if you are hard-core Emacs user, it\nis likely that you've heard of vimdiff---and in that case, including\nvimdiff would be enough. Likewise for showing kompare to Gnome users.\n\nUnlike POSIXy folks, where IRIX or Solaris users are likely to have\nheard of Gnome tools even if they do not use the environment on\ntheir platforms, Windows users tend to be isolated bunch, so it\nwould not hurt to include at least one well-known Windows-only tool\nin the list.\n\n> Yes, indeed, it is arbitrary. It does have some merit, though--it is\n> also a good compromise between unhelpful (listing nothing) and painful\n> to maintain (listing everything).\n\nHere is a v2, with documentation updates.\n\n-- >8 --\nSubject: [PATCH] mergetool: support --tool-help option like difftool does\n\nThis way we do not have to risk the list of tools going out of sync\nbetween the implementation and the documentation.\n\nIn the same spirit as bf73fc2 (difftool: print list of valid tools\nwith '--tool-help', 2012-03-29), trim the list of merge backends in\nthe documentation. We do not want to have a complete of valid tools\nthere; we only want a list to help people guess what kind of things\nthe tools that can be specified there, and refer them to --tool-help\nfor a complete list.\n\nSigned-off-by: Junio C Hamano \n---\n Documentation/git-mergetool.txt | 6 +++---\n git-mergetool--lib.sh | 6 +++++-\n git-mergetool.sh | 42 ++++++++++++++++++++++++++++++++++++++++-\n 3 files changed, 49 insertions(+), 5 deletions(-)\n\ndiff --git a/Documentation/git-mergetool.txt b/Documentation/git-mergetool.txt\nindex 2a49de7..d1e08d2 100644\n--- a/Documentation/git-mergetool.txt\n+++ b/Documentation/git-mergetool.txt\n@@ -27,9 +27,9 @@ OPTIONS\n -t ::\n --tool=::\n \tUse the merge resolution program specified by .\n-\tValid merge tools are:\n-\taraxis, bc3, diffuse, ecmerge, emerge, gvimdiff, kdiff3,\n-\tmeld, opendiff, p4merge, tkdiff, tortoisemerge, vimdiff and xxdiff.\n+\tValid values include emerge, gvimdiff, kdiff3,\n+\tmeld, vimdiff, and tortoisemerge. Run `git mergetool --tool-help`\n+\tfor the list of valid settings.\n +\n If a merge resolution program is not specified, 'git mergetool'\n will use the configuration variable `merge.tool`. If the\ndiff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh\nindex ed630b2..f730253 100644\n--- a/git-mergetool--lib.sh\n+++ b/git-mergetool--lib.sh\n@@ -111,7 +111,7 @@ run_merge_tool () {\n \treturn $status\n }\n \n-guess_merge_tool () {\n+list_merge_tool_candidates () {\n \tif merge_mode\n \tthen\n \t\ttools=\"tortoisemerge\"\n@@ -136,6 +136,10 @@ guess_merge_tool () {\n \t\ttools=\"$tools emerge vimdiff\"\n \t\t;;\n \tesac\n+}\n+\n+guess_merge_tool () {\n+\tlist_merge_tool_candidates\n \techo >&2 \"merge tool candidates: $tools\"\n \n \t# Loop over each candidate and stop when a valid merge tool is found.\ndiff --git a/git-mergetool.sh b/git-mergetool.sh\nindex a9f23f7..0db0c44 100755\n--- a/git-mergetool.sh\n+++ b/git-mergetool.sh\n@@ -8,7 +8,7 @@\n # at the discretion of Junio C Hamano.\n #\n \n-USAGE='[--tool=tool] [-y|--no-prompt|--prompt] [file to merge] ...'\n+USAGE='[--tool=tool] [--tool-help] [-y|--no-prompt|--prompt] [file to merge] ...'\n SUBDIRECTORY_OK=Yes\n OPTIONS_SPEC=\n TOOL_MODE=merge\n@@ -284,11 +284,51 @@ merge_file () {\n return 0\n }\n \n+show_tool_help () {\n+\tTOOL_MODE=merge\n+\tlist_merge_tool_candidates\n+\tunavailable= available= LF='\n+'\n+\tfor i in $tools\n+\tdo\n+\t\tmerge_tool_path=$(translate_merge_tool_path \"$i\")\n+\t\tif type \"$merge_tool_path\" >/dev/null 2>&1\n+\t\tthen\n+\t\t\tavailable=\"$available$i$LF\"\n+\t\telse\n+\t\t\tunavailable=\"$unavailable$i$LF\"\n+\t\tfi\n+\tdone\n+\tif test -n \"$available\"\n+\tthen\n+\t\techo \"'git mergetool --tool=' may be set to one of the following:\"\n+\t\techo \"$available\" | sort | sed -e 's/^/\t/'\n+\telse\n+\t\techo \"No suitable tool for 'git mergetool --tool=' found.\"\n+\tfi\n+\tif test -n \"$unavailable\"\n+\tthen\n+\t\techo\n+\t\techo 'The following tools are valid, but not currently available:'\n+\t\techo \"$unavailable\" | sort | sed -e 's/^/\t/'\n+\tfi\n+\tif test -n \"$unavailable$available\"\n+\tthen\n+\t\techo\n+\t\techo \"Some of the tools listed above only work in a windowed\"\n+\t\techo \"environment. If run in a terminal-only session, they will fail.\"\n+\tfi\n+\texit 0\n+}\n+\n prompt=$(git config --bool mergetool.prompt || echo true)\n \n while test $# != 0\n do\n case \"$1\" in\n+\t--tool-help)\n+\t\tshow_tool_help\n+\t\t;;\n \t-t|--tool*)\n \t case \"$#,$1\" in\n \t\t*,*=*)\n-- \n1.7.11.3.339.gbdbf398\n"},{"id":"195577","messageId":"7vipdejhmh.fsf@alter.siamese.dyndns.org","threadId":"31074","inReplyTo":"CAHGBnuOz94YR9wx_goL5YaWzPt5Z9c3gBB9CtyfcE40F5amrXw@mail.gmail.com","subject":"Re: [PATCH] Make sure to use Araxis' \"compare\" and not e.g. ImageMagick's","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-07-23T21:24:22Z","receivedAt":"2012-07-23T21:24:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sebastian Schuberth writes:\n\n> We have no such assurance. That's why you correctly call it a\n> heuristics after all\n\nImageMagick \"compare\" takes \"--version\" and says something like\nthis to its standard output:\n\n $ compare --version\n Version: ImageMagick 6.6.0-4 2012-05-02 Q16\n http://www.imagemagick.org\n Copyright: Copyright (C) 1999-2010 ImageMagick Studio LLC\n\nDoes Araxis compare take \"--version\" and behave in a way that is\ncheaply controllable? If it opens a GUI window and pops up a dialog\nthat says \"Option not understood\", then it is not \"controllable\",\nbut if it quickly dies with \"No such option\" sent to the standard\nerror output, or sending its version string to the standard output,\nthen we could use something like:\n\n\tcase \"$(compare --version 2>/dev/null)\" in\n \"Araxis compare version\"*)\n \techo compare ;;\n\t*)\n \techo \"$1\" ;;\n\tesac\n\ninstead, and that would be more robust than the path based\nheuristics.\n"},{"id":"195578","messageId":"m2zk6qxj7l.fsf@igel.home","threadId":"31074","inReplyTo":"CAHGBnuOz94YR9wx_goL5YaWzPt5Z9c3gBB9CtyfcE40F5amrXw@mail.gmail.com","subject":"Re: [PATCH] Make sure to use Araxis' \"compare\" and not e.g. ImageMagick's","fromName":"Andreas Schwab","fromEmail":"schwab@linux-m68k.org","sentAt":"2012-07-23T21:26:22Z","receivedAt":"2012-07-23T21:26:22Z","isPatch":true,"sender":{"key":"schwab@linux-m68k.org","avatar":"https://avatars.githubusercontent.com/u/2175493?v=4"},"body":"Sebastian Schuberth writes:\n\n> Please feel free to ignore the patch if you feel the heuristics is not\n> sufficiently safe. I'm currently unable to come up with a safer\n> solution while maintaining portability, i.e. not use \"which\" or doing\n> rather laborious string parsing on the output of \"type\".\n\nHow about looking at \"compare --version\"?\n\nAndreas.\n\n-- \nAndreas Schwab, schwab@linux-m68k.org\nGPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5\n\"And now for something completely different.\"\n"},{"id":"195579","messageId":"CAHGBnuNvzoSkrujoxLt-M5VS2Pm=e6CL_P39pyRAv-NEi1y6QA@mail.gmail.com","threadId":"31074","inReplyTo":"7vmx2qji03.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] mergetool: support --tool-help option like difftool does","fromName":"Sebastian Schuberth","fromEmail":"sschuberth@gmail.com","sentAt":"2012-07-23T21:27:04Z","receivedAt":"2012-07-23T21:27:04Z","isPatch":true,"sender":{"key":"sschuberth@gmail.com","avatar":"https://avatars.githubusercontent.com/u/349154?v=4"},"body":"On Mon, Jul 23, 2012 at 11:16 PM, Junio C Hamano wrote:\n\n> There are only five or six classes of environment that matter in the\n> real world for the purpose of giving \"well known\" examples: Windows,\n> MacOS X, Gnome, KDE and Linux terminal. By picking a representative\n> one from each and listing them, the end result would have at least\n> one that people from various platforms have _heard of_ and can guess\n> what they do. The \"most common\" is secondary, and \"well known\" is\n\nI completely agree with this. So we should take the chance and add a\nWindows representative to the list of difftools, no?\n\n> Unlike POSIXy folks, where IRIX or Solaris users are likely to have\n> heard of Gnome tools even if they do not use the environment on\n> their platforms, Windows users tend to be isolated bunch, so it\n> would not hurt to include at least one well-known Windows-only tool\n> in the list.\n\nHeh. I believe POSIX folks are no less isolated. (How many\nWindows-only tools would you recognize by name?) They're just isolated\nin a bigger world ;-)\n\n> Here is a v2, with documentation updates.\n\nThis drops the explicit mention of --tool-help as an option in the\ndocumentation compared to my patch. Do you want to keep --tool-help\nbeing mentioned inline as part of the --tool option documentation\nonly?\n\n-- \nSebastian Schuberth\n"},{"id":"195580","messageId":"CAHGBnuMEHdNrOoXycAn5JxF4cCvDSA1aTgLG3mZw2PoT9GMN-A@mail.gmail.com","threadId":"31074","inReplyTo":"7vipdejhmh.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] Make sure to use Araxis' \"compare\" and not e.g. ImageMagick's","fromName":"Sebastian Schuberth","fromEmail":"sschuberth@gmail.com","sentAt":"2012-07-23T21:31:11Z","receivedAt":"2012-07-23T21:31:11Z","isPatch":true,"sender":{"key":"sschuberth@gmail.com","avatar":"https://avatars.githubusercontent.com/u/349154?v=4"},"body":"On Mon, Jul 23, 2012 at 11:24 PM, Junio C Hamano wrote:\n\n> Does Araxis compare take \"--version\" and behave in a way that is\n> cheaply controllable? If it opens a GUI window and pops up a dialog\n> that says \"Option not understood\", then it is not \"controllable\",\n\nAraxis opens up a GUI dialog with usage info in that case, so it's not\ncontrollable, unfortunately.\n\n-- \nSebastian Schuberth\n"},{"id":"195581","messageId":"CAJDDKr71SYf3=iUahGLVA2kmKYqT85Thaiooo2YCJqprS5nVag@mail.gmail.com","threadId":"31074","inReplyTo":"7vipdejhmh.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] Make sure to use Araxis' \"compare\" and not e.g. ImageMagick's","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2012-07-23T21:34:17Z","receivedAt":"2012-07-23T21:34:17Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"On Mon, Jul 23, 2012 at 2:24 PM, Junio C Hamano wrote:\n> Sebastian Schuberth writes:\n>\n>> We have no such assurance. That's why you correctly call it a\n>> heuristics after all\n>\n> ImageMagick \"compare\" takes \"--version\" and says something like\n> this to its standard output:\n>\n> $ compare --version\n> Version: ImageMagick 6.6.0-4 2012-05-02 Q16\n> http://www.imagemagick.org\n> Copyright: Copyright (C) 1999-2010 ImageMagick Studio LLC\n>\n> Does Araxis compare take \"--version\" and behave in a way that is\n> cheaply controllable? If it opens a GUI window and pops up a dialog\n> that says \"Option not understood\", then it is not \"controllable\",\n> but if it quickly dies with \"No such option\" sent to the standard\n> error output, or sending its version string to the standard output,\n> then we could use something like:\n>\n> case \"$(compare --version 2>/dev/null)\" in\n> \"Araxis compare version\"*)\n> echo compare ;;\n> *)\n> echo \"$1\" ;;\n> esac\n>\n> instead, and that would be more robust than the path based\n> heuristics.\n\nAraxis compare (the one I have) does not accept --version.\n\nAlso, the GraphicsMagick (ImageMagick fork) compare does not have\n--version, but it does have \"compare version\":\n\n$ compare version\nGraphicsMagick 1.3.12 2010-03-08 Q8 http://www.GraphicsMagick.org/\nCopyright (C) 2002-2010 GraphicsMagick Group.\nAdditional copyrights and licenses apply to this software.\nSee http://www.GraphicsMagick.org/www/Copyright.html for details.\n...\n\n\nIf we care to blacklist *Magick compare, then we may be able to call\n\"compare version\" and parse the output looking for \"Magic\".\n\nI tested ImageMagick compare and it also understands \"compare version\".\n\n\nAraxis compare prints nothing when \"compare version\" is called.\nLikely because it thinks it has nothing to do. It does the same if I\nsay \"compare blahblah\", and returns exit status 0. So its output is\nuseless.\n\nIt seems like the best we can do is specifically blacklist the *Magick\n\"compare\" commands so that they do not show up as false positives.\nAnd the only way to identify them is to parse their output, since all\ncommands return status 0 for \"compare version\".\n\n\nAnother possibility is to parse the output of \"compare\" (no args) and\ngrep for \"merge\". The name \"Araxis Merge\" is never actually printed,\nbut the help text for \"-merge\" does appear.... yep.. it's a heuristic.\n\nSebastian, are you testing on Windows? The araxis \"compare\" I used is\nOS X. Does \"compare version\" open a GUI window for you? For me it\ndoes not.\nWhat about \"compare -h\", or just \"compare\" ?\n-- \nDavid\n"},{"id":"195584","messageId":"CAHGBnuNP6VmDUXr_01zWZgcTDOUSX2_v7UDf6sOnEYU95RCW4g@mail.gmail.com","threadId":"31074","inReplyTo":"CAJDDKr71SYf3=iUahGLVA2kmKYqT85Thaiooo2YCJqprS5nVag@mail.gmail.com","subject":"Re: [PATCH] Make sure to use Araxis' \"compare\" and not e.g. ImageMagick's","fromName":"Sebastian Schuberth","fromEmail":"sschuberth@gmail.com","sentAt":"2012-07-23T21:44:25Z","receivedAt":"2012-07-23T21:44:25Z","isPatch":true,"sender":{"key":"sschuberth@gmail.com","avatar":"https://avatars.githubusercontent.com/u/349154?v=4"},"body":"On Mon, Jul 23, 2012 at 11:34 PM, David Aguilar wrote:\n\n> Sebastian, are you testing on Windows? The araxis \"compare\" I used is\n> OS X. Does \"compare version\" open a GUI window for you? For me it\n> does not.\n> What about \"compare -h\", or just \"compare\" ?\n\nYes, I'm testing on Windows, and unfortunately Araxis behaves differently here:\n\n- running \"compare version\" or \"compare blah\" prints nothing on the\nconsole, but the GUI opens saying it is unable to open a file\n\"version\" (which it would compare to nothing),\n\n- \"compare -h\" or just \"compare\" both open up the usage dialog listing\nall valid command line options (because \"-h\" is invalid).\n\n-- \nSebastian Schuberth\n"},{"id":"195586","messageId":"7v629ejexo.fsf@alter.siamese.dyndns.org","threadId":"31074","inReplyTo":"CAHGBnuOz94YR9wx_goL5YaWzPt5Z9c3gBB9CtyfcE40F5amrXw@mail.gmail.com","subject":"Re: [PATCH] Make sure to use Araxis' \"compare\" and not e.g. ImageMagick's","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-07-23T22:22:27Z","receivedAt":"2012-07-23T22:22:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sebastian Schuberth writes:\n\n> Personally, I've valued the gain of\n> the patch to not list araxis as an available diff tool by \"git\n> difftool --tool-help\" when in fact just ImageMagick is in PATH higher\n> than the loss to support araxis installations that are in a path not\n> containung \"araxis\" but are in PATH.\n\nI agree that running ImageMagick's compare by accident is one thing\nwe would want to avoid, but once the problem is diagnosed, it is\nsomething the user can easily work around by futzing %PATH%, I\nthink.\n\nOn the other hand, if the user has bought and installed Araxis, but\nwe incorrectly identify it as unusable, the user has wasted good\nmoney and there is no easy recourse as far as I can see in your\npatch. That is why I wanted to see a reasonable assurance.\n\nIf we limit the problem space by special casing Windows installation\n(e.g. check \"uname -s\" or something), would it make the problem\neasier to solve? Perhaps it is much more likely that the path the\nprogram is installed in can be safely identified with a call to\n\"type --path compare\" (bash is the only shell shipped in msysgit,\nisn't it?), and its output is likely to contain \"/Program Files/Araxis/\"\nas a substring, or something?\n"},{"id":"195588","messageId":"7vy5mahzyv.fsf@alter.siamese.dyndns.org","threadId":"31074","inReplyTo":"CAHGBnuNvzoSkrujoxLt-M5VS2Pm=e6CL_P39pyRAv-NEi1y6QA@mail.gmail.com","subject":"Re: [PATCH] mergetool: support --tool-help option like difftool does","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-07-23T22:31:04Z","receivedAt":"2012-07-23T22:31:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sebastian Schuberth writes:\n\n> On Mon, Jul 23, 2012 at 11:16 PM, Junio C Hamano wrote:\n>\n>> There are only five or six classes of environment that matter in the\n>> real world for the purpose of giving \"well known\" examples: Windows,\n>> MacOS X, Gnome, KDE and Linux terminal. By picking a representative\n>> one from each and listing them, the end result would have at least\n>> one that people from various platforms have _heard of_ and can guess\n>> what they do. The \"most common\" is secondary, and \"well known\" is\n>\n> I completely agree with this. So we should take the chance and add a\n> Windows representative to the list of difftools, no?\n\nI do not care very deeply either way. I am more interested in\nseeing --tool-list options supported by both so that we can get rid\nof hardcoded list from the completion script.\n\n> This drops the explicit mention of --tool-help as an option in the\n> documentation compared to my patch. Do you want to keep --tool-help\n> being mentioned inline as part of the --tool option documentation\n> only?\n\nWhile I do not think having one would hurt that much, I do not think\na separate entry would add much value either, so I chose not to\nclutter the documentation further. The \"--tool=\" entry is\nenough hint to draw eyes of readers who want to find out what kind\nof backends are supported, and --tool-help is mentioned there quite\nprominently.\n"},{"id":"195589","messageId":"CAHGBnuOXNb5NKB-aiSmciU7OQaGm_T-cn4JUSj5yS3bVVFiBUw@mail.gmail.com","threadId":"31074","inReplyTo":"7v629ejexo.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] Make sure to use Araxis' \"compare\" and not e.g. ImageMagick's","fromName":"Sebastian Schuberth","fromEmail":"sschuberth@gmail.com","sentAt":"2012-07-23T22:39:09Z","receivedAt":"2012-07-23T22:39:09Z","isPatch":true,"sender":{"key":"sschuberth@gmail.com","avatar":"https://avatars.githubusercontent.com/u/349154?v=4"},"body":"On Tue, Jul 24, 2012 at 12:22 AM, Junio C Hamano wrote:\n\n> On the other hand, if the user has bought and installed Araxis, but\n> we incorrectly identify it as unusable, the user has wasted good\n> money and there is no easy recourse as far as I can see in your\n> patch.\n\nAgreed.\n\n> If we limit the problem space by special casing Windows installation\n> (e.g. check \"uname -s\" or something), would it make the problem\n> easier to solve? Perhaps it is much more likely that the path the\n> program is installed in can be safely identified with a call to\n> \"type --path compare\" (bash is the only shell shipped in msysgit,\n> isn't it?), and its output is likely to contain \"/Program Files/Araxis/\"\n> as a substring, or something?\n\nYes, I could come up with basically a variant of my first version of\nthe patch that is limited to Windows only and uses \"type --path\"\ninstead of \"which\", but then again this is too non-generic for my\ntaste. Moreover, as I'm also using Mac OS X, I'd be interested in a\nsolution that works there, too.\n\nI don't see a good (read generic and concise) solution to the issue. I\nthink we should just drop my patch and not waste all of our time on it\nany more.\n\n-- \nSebastian Schuberth\n"},{"id":"195590","messageId":"7vtxwyhzi8.fsf@alter.siamese.dyndns.org","threadId":"31074","inReplyTo":"7v629ejexo.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] Make sure to use Araxis' \"compare\" and not e.g. ImageMagick's","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-07-23T22:41:03Z","receivedAt":"2012-07-23T22:41:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> If we limit the problem space by special casing Windows installation\n> (e.g. check \"uname -s\" or something), would it make the problem\n> easier to solve? Perhaps it is much more likely that the path the\n> program is installed in can be safely identified with a call to\n> \"type --path compare\" (bash is the only shell shipped in msysgit,\n> isn't it?).\n\nE.g. something along the lines of your original patch. I do not\nknow what other commands are typically installed in the same\ndirectory as \"compare\", so it is likely you need to fix the name of\nthe file to let us positively identify \"compare\" is from the Araxis\nsuite.\n\n mergetools/araxis | 15 ++++++++++++++-\n 1 file changed, 14 insertions(+), 1 deletion(-)\n\ndiff --git a/mergetools/araxis b/mergetools/araxis\nindex 64f97c5..1180a32 100644\n--- a/mergetools/araxis\n+++ b/mergetools/araxis\n@@ -16,5 +16,18 @@ merge_cmd () {\n }\n \n translate_merge_tool_path() {\n-\techo compare\n+\tcase \"$BASH_VERSION\" in\n+\t??*)\n+\t\t# we can safely use \"type --path\"\n+\t\tif test -f $(dirname \"$(type --path compare)\")/AraxisMerge\n+\t\tthen\n+\t\t\techo compare\n+\t\telse\n+\t\t\techo \"$1\"\n+\t\tfi\n+\t\t;;\n+\t*)\n+\t\techo compare\n+\t\t;;\n+\tesac\n }\n"},{"id":"195591","messageId":"500DD998.2020600@gmail.com","threadId":"31074","inReplyTo":"7vtxwyhzi8.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] Make sure to use Araxis' \"compare\" and not e.g. ImageMagick's","fromName":"Sebastian Schuberth","fromEmail":"sschuberth@gmail.com","sentAt":"2012-07-23T23:09:12Z","receivedAt":"2012-07-23T23:09:12Z","isPatch":true,"sender":{"key":"sschuberth@gmail.com","avatar":"https://avatars.githubusercontent.com/u/349154?v=4"},"body":"On 24.07.2012 00:41, Junio C Hamano wrote:\n\n> +\t\tif test -f $(dirname \"$(type --path compare)\")/AraxisMerge\n\nWe 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.\n\nMoreover, \"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:\n\n mergetools/araxis | 15 ++++++++++++++-\n 1 file changed, 14 insertions(+), 1 deletion(-)\n\ndiff --git a/mergetools/araxis b/mergetools/araxis\nindex 64f97c5..c406ead 100644\n--- a/mergetools/araxis\n+++ b/mergetools/araxis\n@@ -16,5 +16,18 @@ merge_cmd () {\n }\n \n translate_merge_tool_path() {\n-\techo compare\n+\tcase \"$BASH_VERSION\" in\n+\t??*)\n+\t\t# we can safely use \"type --path\"\n+\t\tif ls \"$(dirname \"$(type --path compare)\")\"/Araxis* >/dev/null 2>&1\n+\t\tthen\n+\t\t\techo compare\n+\t\telse\n+\t\t\techo \"$1\"\n+\t\tfi\n+\t\t;;\n+\t*)\n+\t\techo compare\n+\t\t;;\n+\tesac\n }\n\n-- \nSebastian Schuberth\n"},{"id":"197677","messageId":"7vzk5m41gk.fsf_-_@alter.siamese.dyndns.org","threadId":"31074","inReplyTo":"7vipdemm0a.fsf_-_@alter.siamese.dyndns.org","subject":"Re*: mergetool: support --tool-help option like difftool does","fromName":"Junio C Hamano","fromEmail":"junio@pobox.com","sentAt":"2012-08-23T05:33:15Z","receivedAt":"2012-08-23T05:33:15Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> This way we do not have to risk the list of tools go out of sync\n> between the implementation and the documentation.\n>\n> Signed-off-by: Junio C Hamano \n> ---\n> Junio C Hamano writes:\n>\n>>> +--tool-help::\n>>> +\tList the supported and available diff tools.\n>>\n>> This part is a good addition (but it already is mentioned in the\n>> description of --tool above, so it is more or less a \"Meh\").\n>\n> I noticed that the main while loop has style violations in its\n> case/esac, but I left it as-is. Reindenting it would be a low\n> hanging fruit janitor patch that would be a separate topic.\n\nAfter hinting a low-hanging-fruit that would be an easy way for new\npeople to dip their toes, I saw no takers for one month, so I ended\nup doing it myself.\n\n-- >8 --\nSubject: mergetool: style fixes\n\nThis script is one of the sizeable ones that tempted people to copy\nits \"neibouring style\" in their new code, but was littered with\nstyles incompatible with our style guide.\n\n - use one tab, not four spaces, per indent level;\n\n - long lines can be wrapped after '|', '&&', or '||' for\n readability.\n\n - structures like \"if .. then .. else .. fi\", \"while .. do .. done\"\n are split into lines in such a way that does not require\n unnecessary semicolon.\n\n - case, esac and case-arms align at the same column.\n\nSigned-off-by: Junio C Hamano \n---\n\n * The change would be easier to see if the reader runs these\n command before and after applying this patch:\n\n $ git diff -w git-mergetool.sh\n $ git grep -e '^[\\t]* ' -e '; *then' -e '; do' git-mergetool.sh\n\n git-mergetool.sh | 581 +++++++++++++++++++++++++++++--------------------------\n 1 file changed, 308 insertions(+), 273 deletions(-)\n\ndiff --git i/git-mergetool.sh w/git-mergetool.sh\nindex 0db0c44..c50e18a 100755\n--- i/git-mergetool.sh\n+++ w/git-mergetool.sh\n@@ -18,270 +18,301 @@ require_work_tree\n \n # Returns true if the mode reflects a symlink\n is_symlink () {\n- test \"$1\" = 120000\n+\ttest \"$1\" = 120000\n }\n \n is_submodule () {\n- test \"$1\" = 160000\n+\ttest \"$1\" = 160000\n }\n \n local_present () {\n- test -n \"$local_mode\"\n+\ttest -n \"$local_mode\"\n }\n \n remote_present () {\n- test -n \"$remote_mode\"\n+\ttest -n \"$remote_mode\"\n }\n \n base_present () {\n- test -n \"$base_mode\"\n+\ttest -n \"$base_mode\"\n }\n \n cleanup_temp_files () {\n- if test \"$1\" = --save-backup ; then\n-\trm -rf -- \"$MERGED.orig\"\n-\ttest -e \"$BACKUP\" && mv -- \"$BACKUP\" \"$MERGED.orig\"\n-\trm -f -- \"$LOCAL\" \"$REMOTE\" \"$BASE\"\n- else\n-\trm -f -- \"$LOCAL\" \"$REMOTE\" \"$BASE\" \"$BACKUP\"\n- fi\n+\tif test \"$1\" = --save-backup\n+\tthen\n+\t\trm -rf -- \"$MERGED.orig\"\n+\t\ttest -e \"$BACKUP\" && mv -- \"$BACKUP\" \"$MERGED.orig\"\n+\t\trm -f -- \"$LOCAL\" \"$REMOTE\" \"$BASE\"\n+\telse\n+\t\trm -f -- \"$LOCAL\" \"$REMOTE\" \"$BASE\" \"$BACKUP\"\n+\tfi\n }\n \n describe_file () {\n- mode=\"$1\"\n- branch=\"$2\"\n- file=\"$3\"\n-\n- printf \" {%s}: \" \"$branch\"\n- if test -z \"$mode\"; then\n-\techo \"deleted\"\n- elif is_symlink \"$mode\" ; then\n-\techo \"a symbolic link -> '$(cat \"$file\")'\"\n- elif is_submodule \"$mode\" ; then\n-\techo \"submodule commit $file\"\n- else\n-\tif base_present; then\n-\t echo \"modified file\"\n+\tmode=\"$1\"\n+\tbranch=\"$2\"\n+\tfile=\"$3\"\n+\n+\tprintf \" {%s}: \" \"$branch\"\n+\tif test -z \"$mode\"\n+\tthen\n+\t\techo \"deleted\"\n+\telif is_symlink \"$mode\"\n+\tthen\n+\t\techo \"a symbolic link -> '$(cat \"$file\")'\"\n+\telif is_submodule \"$mode\"\n+\tthen\n+\t\techo \"submodule commit $file\"\n+\telif base_present\n+\tthen\n+\t\techo \"modified file\"\n \telse\n-\t echo \"created file\"\n+\t\techo \"created file\"\n \tfi\n- fi\n }\n \n-\n resolve_symlink_merge () {\n- while true; do\n-\tprintf \"Use (l)ocal or (r)emote, or (a)bort? \"\n-\tread ans || return 1\n-\tcase \"$ans\" in\n-\t [lL]*)\n-\t\tgit checkout-index -f --stage=2 -- \"$MERGED\"\n-\t\tgit add -- \"$MERGED\"\n-\t\tcleanup_temp_files --save-backup\n-\t\treturn 0\n-\t\t;;\n-\t [rR]*)\n-\t\tgit checkout-index -f --stage=3 -- \"$MERGED\"\n-\t\tgit add -- \"$MERGED\"\n-\t\tcleanup_temp_files --save-backup\n-\t\treturn 0\n-\t\t;;\n-\t [aA]*)\n-\t\treturn 1\n-\t\t;;\n-\t esac\n+\twhile true\n+\tdo\n+\t\tprintf \"Use (l)ocal or (r)emote, or (a)bort? \"\n+\t\tread ans || return 1\n+\t\tcase \"$ans\" in\n+\t\t[lL]*)\n+\t\t\tgit checkout-index -f --stage=2 -- \"$MERGED\"\n+\t\t\tgit add -- \"$MERGED\"\n+\t\t\tcleanup_temp_files --save-backup\n+\t\t\treturn 0\n+\t\t\t;;\n+\t\t[rR]*)\n+\t\t\tgit checkout-index -f --stage=3 -- \"$MERGED\"\n+\t\t\tgit add -- \"$MERGED\"\n+\t\t\tcleanup_temp_files --save-backup\n+\t\t\treturn 0\n+\t\t\t;;\n+\t\t[aA]*)\n+\t\t\treturn 1\n+\t\t\t;;\n+\t\tesac\n \tdone\n }\n \n resolve_deleted_merge () {\n- while true; do\n-\tif base_present; then\n-\t printf \"Use (m)odified or (d)eleted file, or (a)bort? \"\n-\telse\n-\t printf \"Use (c)reated or (d)eleted file, or (a)bort? \"\n-\tfi\n-\tread ans || return 1\n-\tcase \"$ans\" in\n-\t [mMcC]*)\n-\t\tgit add -- \"$MERGED\"\n-\t\tcleanup_temp_files --save-backup\n-\t\treturn 0\n-\t\t;;\n-\t [dD]*)\n-\t\tgit rm -- \"$MERGED\" > /dev/null\n-\t\tcleanup_temp_files\n-\t\treturn 0\n-\t\t;;\n-\t [aA]*)\n-\t\treturn 1\n-\t\t;;\n-\t esac\n+\twhile true\n+\tdo\n+\t\tif base_present\n+\t\tthen\n+\t\t\tprintf \"Use (m)odified or (d)eleted file, or (a)bort? \"\n+\t\telse\n+\t\t\tprintf \"Use (c)reated or (d)eleted file, or (a)bort? \"\n+\t\tfi\n+\t\tread ans || return 1\n+\t\tcase \"$ans\" in\n+\t\t[mMcC]*)\n+\t\t\tgit add -- \"$MERGED\"\n+\t\t\tcleanup_temp_files --save-backup\n+\t\t\treturn 0\n+\t\t\t;;\n+\t\t[dD]*)\n+\t\t\tgit rm -- \"$MERGED\" > /dev/null\n+\t\t\tcleanup_temp_files\n+\t\t\treturn 0\n+\t\t\t;;\n+\t\t[aA]*)\n+\t\t\treturn 1\n+\t\t\t;;\n+\t\tesac\n \tdone\n }\n \n resolve_submodule_merge () {\n- while true; do\n-\tprintf \"Use (l)ocal or (r)emote, or (a)bort? \"\n-\tread ans || return 1\n-\tcase \"$ans\" in\n-\t [lL]*)\n-\t\tif ! local_present; then\n-\t\t if test -n \"$(git ls-tree HEAD -- \"$MERGED\")\"; then\n-\t\t\t# Local isn't present, but it's a subdirectory\n-\t\t\tgit ls-tree --full-name -r HEAD -- \"$MERGED\" | git update-index --index-info || exit $?\n-\t\t else\n-\t\t\ttest -e \"$MERGED\" && mv -- \"$MERGED\" \"$BACKUP\"\n-\t\t\tgit update-index --force-remove \"$MERGED\"\n+\twhile true\n+\tdo\n+\t\tprintf \"Use (l)ocal or (r)emote, or (a)bort? \"\n+\t\tread ans || return 1\n+\t\tcase \"$ans\" in\n+\t\t[lL]*)\n+\t\t\tif ! local_present\n+\t\t\tthen\n+\t\t\t\tif test -n \"$(git ls-tree HEAD -- \"$MERGED\")\"\n+\t\t\t\tthen\n+\t\t\t\t\t# Local isn't present, but it's a subdirectory\n+\t\t\t\t\tgit ls-tree --full-name -r HEAD -- \"$MERGED\" |\n+\t\t\t\t\tgit update-index --index-info || exit $?\n+\t\t\t\telse\n+\t\t\t\t\ttest -e \"$MERGED\" && mv -- \"$MERGED\" \"$BACKUP\"\n+\t\t\t\t\tgit update-index --force-remove \"$MERGED\"\n+\t\t\t\t\tcleanup_temp_files --save-backup\n+\t\t\t\tfi\n+\t\t\telif is_submodule \"$local_mode\"\n+\t\t\tthen\n+\t\t\t\tstage_submodule \"$MERGED\" \"$local_sha1\"\n+\t\t\telse\n+\t\t\t\tgit checkout-index -f --stage=2 -- \"$MERGED\"\n+\t\t\t\tgit add -- \"$MERGED\"\n+\t\t\tfi\n+\t\t\treturn 0\n+\t\t\t;;\n+\t\t[rR]*)\n+\t\t\tif ! remote_present\n+\t\t\tthen\n+\t\t\t\tif test -n \"$(git ls-tree MERGE_HEAD -- \"$MERGED\")\"\n+\t\t\t\tthen\n+\t\t\t\t\t# Remote isn't present, but it's a subdirectory\n+\t\t\t\t\tgit ls-tree --full-name -r MERGE_HEAD -- \"$MERGED\" |\n+\t\t\t\t\tgit update-index --index-info || exit $?\n+\t\t\t\telse\n+\t\t\t\t\ttest -e \"$MERGED\" && mv -- \"$MERGED\" \"$BACKUP\"\n+\t\t\t\t\tgit update-index --force-remove \"$MERGED\"\n+\t\t\t\tfi\n+\t\t\telif is_submodule \"$remote_mode\"\n+\t\t\tthen\n+\t\t\t\t! is_submodule \"$local_mode\" &&\n+\t\t\t\ttest -e \"$MERGED\" &&\n+\t\t\t\tmv -- \"$MERGED\" \"$BACKUP\"\n+\t\t\t\tstage_submodule \"$MERGED\" \"$remote_sha1\"\n+\t\t\telse\n+\t\t\t\ttest -e \"$MERGED\" && mv -- \"$MERGED\" \"$BACKUP\"\n+\t\t\t\tgit checkout-index -f --stage=3 -- \"$MERGED\"\n+\t\t\t\tgit add -- \"$MERGED\"\n+\t\t\tfi\n \t\t\tcleanup_temp_files --save-backup\n-\t\t fi\n-\t\telif is_submodule \"$local_mode\"; then\n-\t\t stage_submodule \"$MERGED\" \"$local_sha1\"\n-\t\telse\n-\t\t git checkout-index -f --stage=2 -- \"$MERGED\"\n-\t\t git add -- \"$MERGED\"\n-\t\tfi\n-\t\treturn 0\n-\t\t;;\n-\t [rR]*)\n-\t\tif ! remote_present; then\n-\t\t if test -n \"$(git ls-tree MERGE_HEAD -- \"$MERGED\")\"; then\n-\t\t\t# Remote isn't present, but it's a subdirectory\n-\t\t\tgit ls-tree --full-name -r MERGE_HEAD -- \"$MERGED\" | git update-index --index-info || exit $?\n-\t\t else\n-\t\t\ttest -e \"$MERGED\" && mv -- \"$MERGED\" \"$BACKUP\"\n-\t\t\tgit update-index --force-remove \"$MERGED\"\n-\t\t fi\n-\t\telif is_submodule \"$remote_mode\"; then\n-\t\t ! is_submodule \"$local_mode\" && test -e \"$MERGED\" && mv -- \"$MERGED\" \"$BACKUP\"\n-\t\t stage_submodule \"$MERGED\" \"$remote_sha1\"\n-\t\telse\n-\t\t test -e \"$MERGED\" && mv -- \"$MERGED\" \"$BACKUP\"\n-\t\t git checkout-index -f --stage=3 -- \"$MERGED\"\n-\t\t git add -- \"$MERGED\"\n-\t\tfi\n-\t\tcleanup_temp_files --save-backup\n-\t\treturn 0\n-\t\t;;\n-\t [aA]*)\n-\t\treturn 1\n-\t\t;;\n-\t esac\n+\t\t\treturn 0\n+\t\t\t;;\n+\t\t[aA]*)\n+\t\t\treturn 1\n+\t\t\t;;\n+\t\tesac\n \tdone\n }\n \n stage_submodule () {\n- path=\"$1\"\n- submodule_sha1=\"$2\"\n- mkdir -p \"$path\" || die \"fatal: unable to create directory for module at $path\"\n- # Find $path relative to work tree\n- work_tree_root=$(cd_to_toplevel && pwd)\n- work_rel_path=$(cd \"$path\" && GIT_WORK_TREE=\"${work_tree_root}\" git rev-parse --show-prefix)\n- test -n \"$work_rel_path\" || die \"fatal: unable to get path of module $path relative to work tree\"\n- git update-index --add --replace --cacheinfo 160000 \"$submodule_sha1\" \"${work_rel_path%/}\" || die\n+\tpath=\"$1\"\n+\tsubmodule_sha1=\"$2\"\n+\tmkdir -p \"$path\" ||\n+\tdie \"fatal: unable to create directory for module at $path\"\n+\t# Find $path relative to work tree\n+\twork_tree_root=$(cd_to_toplevel && pwd)\n+\twork_rel_path=$(cd \"$path\" &&\n+\t\tGIT_WORK_TREE=\"${work_tree_root}\" git rev-parse --show-prefix\n+\t)\n+\ttest -n \"$work_rel_path\" ||\n+\tdie \"fatal: unable to get path of module $path relative to work tree\"\n+\tgit update-index --add --replace --cacheinfo 160000 \"$submodule_sha1\" \"${work_rel_path%/}\" || die\n }\n \n checkout_staged_file () {\n- tmpfile=$(expr \\\n-\t \"$(git checkout-index --temp --stage=\"$1\" \"$2\" 2>/dev/null)\" \\\n-\t : '\\([^\t]*\\)\t')\n-\n- if test $? -eq 0 -a -n \"$tmpfile\" ; then\n-\tmv -- \"$(git rev-parse --show-cdup)$tmpfile\" \"$3\"\n- else\n-\t>\"$3\"\n- fi\n+\ttmpfile=$(expr \\\n+\t\t\"$(git checkout-index --temp --stage=\"$1\" \"$2\" 2>/dev/null)\" \\\n+\t\t: '\\([^\t]*\\)\t')\n+\n+\tif test $? -eq 0 -a -n \"$tmpfile\"\n+\tthen\n+\t\tmv -- \"$(git rev-parse --show-cdup)$tmpfile\" \"$3\"\n+\telse\n+\t\t>\"$3\"\n+\tfi\n }\n \n merge_file () {\n- MERGED=\"$1\"\n+\tMERGED=\"$1\"\n \n- f=$(git ls-files -u -- \"$MERGED\")\n- if test -z \"$f\" ; then\n-\tif test ! -f \"$MERGED\" ; then\n-\t echo \"$MERGED: file not found\"\n-\telse\n-\t echo \"$MERGED: file does not need merging\"\n+\tf=$(git ls-files -u -- \"$MERGED\")\n+\tif test -z \"$f\"\n+\tthen\n+\t\tif test ! -f \"$MERGED\"\n+\t\tthen\n+\t\t\techo \"$MERGED: file not found\"\n+\t\telse\n+\t\t\techo \"$MERGED: file does not need merging\"\n+\t\tfi\n+\t\treturn 1\n \tfi\n-\treturn 1\n- fi\n-\n- ext=\"$$$(expr \"$MERGED\" : '.*\\(\\.[^/]*\\)$')\"\n- BACKUP=\"./$MERGED.BACKUP.$ext\"\n- LOCAL=\"./$MERGED.LOCAL.$ext\"\n- REMOTE=\"./$MERGED.REMOTE.$ext\"\n- BASE=\"./$MERGED.BASE.$ext\"\n-\n- base_mode=$(git ls-files -u -- \"$MERGED\" | awk '{if ($3==1) print $1;}')\n- local_mode=$(git ls-files -u -- \"$MERGED\" | awk '{if ($3==2) print $1;}')\n- remote_mode=$(git ls-files -u -- \"$MERGED\" | awk '{if ($3==3) print $1;}')\n-\n- if is_submodule \"$local_mode\" || is_submodule \"$remote_mode\"; then\n-\techo \"Submodule merge conflict for '$MERGED':\"\n-\tlocal_sha1=$(git ls-files -u -- \"$MERGED\" | awk '{if ($3==2) print $2;}')\n-\tremote_sha1=$(git ls-files -u -- \"$MERGED\" | awk '{if ($3==3) print $2;}')\n-\tdescribe_file \"$local_mode\" \"local\" \"$local_sha1\"\n-\tdescribe_file \"$remote_mode\" \"remote\" \"$remote_sha1\"\n-\tresolve_submodule_merge\n-\treturn\n- fi\n-\n- mv -- \"$MERGED\" \"$BACKUP\"\n- cp -- \"$BACKUP\" \"$MERGED\"\n-\n- checkout_staged_file 1 \"$MERGED\" \"$BASE\"\n- checkout_staged_file 2 \"$MERGED\" \"$LOCAL\"\n- checkout_staged_file 3 \"$MERGED\" \"$REMOTE\"\n-\n- if test -z \"$local_mode\" -o -z \"$remote_mode\"; then\n-\techo \"Deleted merge conflict for '$MERGED':\"\n-\tdescribe_file \"$local_mode\" \"local\" \"$LOCAL\"\n-\tdescribe_file \"$remote_mode\" \"remote\" \"$REMOTE\"\n-\tresolve_deleted_merge\n-\treturn\n- fi\n \n- if is_symlink \"$local_mode\" || is_symlink \"$remote_mode\"; then\n-\techo \"Symbolic link merge conflict for '$MERGED':\"\n+\text=\"$$$(expr \"$MERGED\" : '.*\\(\\.[^/]*\\)$')\"\n+\tBACKUP=\"./$MERGED.BACKUP.$ext\"\n+\tLOCAL=\"./$MERGED.LOCAL.$ext\"\n+\tREMOTE=\"./$MERGED.REMOTE.$ext\"\n+\tBASE=\"./$MERGED.BASE.$ext\"\n+\n+\tbase_mode=$(git ls-files -u -- \"$MERGED\" | awk '{if ($3==1) print $1;}')\n+\tlocal_mode=$(git ls-files -u -- \"$MERGED\" | awk '{if ($3==2) print $1;}')\n+\tremote_mode=$(git ls-files -u -- \"$MERGED\" | awk '{if ($3==3) print $1;}')\n+\n+\tif is_submodule \"$local_mode\" || is_submodule \"$remote_mode\"\n+\tthen\n+\t\techo \"Submodule merge conflict for '$MERGED':\"\n+\t\tlocal_sha1=$(git ls-files -u -- \"$MERGED\" | awk '{if ($3==2) print $2;}')\n+\t\tremote_sha1=$(git ls-files -u -- \"$MERGED\" | awk '{if ($3==3) print $2;}')\n+\t\tdescribe_file \"$local_mode\" \"local\" \"$local_sha1\"\n+\t\tdescribe_file \"$remote_mode\" \"remote\" \"$remote_sha1\"\n+\t\tresolve_submodule_merge\n+\t\treturn\n+\tfi\n+\n+\tmv -- \"$MERGED\" \"$BACKUP\"\n+\tcp -- \"$BACKUP\" \"$MERGED\"\n+\n+\tcheckout_staged_file 1 \"$MERGED\" \"$BASE\"\n+\tcheckout_staged_file 2 \"$MERGED\" \"$LOCAL\"\n+\tcheckout_staged_file 3 \"$MERGED\" \"$REMOTE\"\n+\n+\tif test -z \"$local_mode\" -o -z \"$remote_mode\"\n+\tthen\n+\t\techo \"Deleted merge conflict for '$MERGED':\"\n+\t\tdescribe_file \"$local_mode\" \"local\" \"$LOCAL\"\n+\t\tdescribe_file \"$remote_mode\" \"remote\" \"$REMOTE\"\n+\t\tresolve_deleted_merge\n+\t\treturn\n+\tfi\n+\n+\tif is_symlink \"$local_mode\" || is_symlink \"$remote_mode\"\n+\tthen\n+\t\techo \"Symbolic link merge conflict for '$MERGED':\"\n+\t\tdescribe_file \"$local_mode\" \"local\" \"$LOCAL\"\n+\t\tdescribe_file \"$remote_mode\" \"remote\" \"$REMOTE\"\n+\t\tresolve_symlink_merge\n+\t\treturn\n+\tfi\n+\n+\techo \"Normal merge conflict for '$MERGED':\"\n \tdescribe_file \"$local_mode\" \"local\" \"$LOCAL\"\n \tdescribe_file \"$remote_mode\" \"remote\" \"$REMOTE\"\n-\tresolve_symlink_merge\n-\treturn\n- fi\n-\n- echo \"Normal merge conflict for '$MERGED':\"\n- describe_file \"$local_mode\" \"local\" \"$LOCAL\"\n- describe_file \"$remote_mode\" \"remote\" \"$REMOTE\"\n- if \"$prompt\" = true; then\n-\tprintf \"Hit return to start merge resolution tool (%s): \" \"$merge_tool\"\n-\tread ans || return 1\n- fi\n-\n- if base_present; then\n-\t present=true\n- else\n-\t present=false\n- fi\n-\n- if ! run_merge_tool \"$merge_tool\" \"$present\"; then\n-\techo \"merge of $MERGED failed\" 1>&2\n-\tmv -- \"$BACKUP\" \"$MERGED\"\n-\n-\tif test \"$merge_keep_temporaries\" = \"false\"; then\n-\t cleanup_temp_files\n+\tif \"$prompt\" = true\n+\tthen\n+\t\tprintf \"Hit return to start merge resolution tool (%s): \" \"$merge_tool\"\n+\t\tread ans || return 1\n \tfi\n \n-\treturn 1\n- fi\n+\tif base_present\n+\tthen\n+\t\tpresent=true\n+\telse\n+\t\tpresent=false\n+\tfi\n+\n+\tif ! run_merge_tool \"$merge_tool\" \"$present\"\n+\tthen\n+\t\techo \"merge of $MERGED failed\" 1>&2\n+\t\tmv -- \"$BACKUP\" \"$MERGED\"\n+\n+\t\tif test \"$merge_keep_temporaries\" = \"false\"\n+\t\tthen\n+\t\t\tcleanup_temp_files\n+\t\tfi\n \n- if test \"$merge_keep_backup\" = \"true\"; then\n-\tmv -- \"$BACKUP\" \"$MERGED.orig\"\n- else\n-\trm -- \"$BACKUP\"\n- fi\n+\t\treturn 1\n+\tfi\n \n- git add -- \"$MERGED\"\n- cleanup_temp_files\n- return 0\n+\tif test \"$merge_keep_backup\" = \"true\"\n+\tthen\n+\t\tmv -- \"$BACKUP\" \"$MERGED.orig\"\n+\telse\n+\t\trm -- \"$BACKUP\"\n+\tfi\n+\n+\tgit add -- \"$MERGED\"\n+\tcleanup_temp_files\n+\treturn 0\n }\n \n show_tool_help () {\n@@ -325,61 +356,61 @@ prompt=$(git config --bool mergetool.prompt || echo true)\n \n while test $# != 0\n do\n- case \"$1\" in\n+\tcase \"$1\" in\n \t--tool-help)\n \t\tshow_tool_help\n \t\t;;\n \t-t|--tool*)\n-\t case \"$#,$1\" in\n+\t\tcase \"$#,$1\" in\n \t\t*,*=*)\n-\t\t merge_tool=$(expr \"z$1\" : 'z-[^=]*=\\(.*\\)')\n-\t\t ;;\n+\t\t\tmerge_tool=$(expr \"z$1\" : 'z-[^=]*=\\(.*\\)')\n+\t\t\t;;\n \t\t1,*)\n-\t\t usage ;;\n+\t\t\tusage ;;\n \t\t*)\n-\t\t merge_tool=\"$2\"\n-\t\t shift ;;\n-\t esac\n-\t ;;\n+\t\t\tmerge_tool=\"$2\"\n+\t\t\tshift ;;\n+\t\tesac\n+\t\t;;\n \t-y|--no-prompt)\n-\t prompt=false\n-\t ;;\n+\t\tprompt=false\n+\t\t;;\n \t--prompt)\n-\t prompt=true\n-\t ;;\n+\t\tprompt=true\n+\t\t;;\n \t--)\n-\t shift\n-\t break\n-\t ;;\n+\t\tshift\n+\t\tbreak\n+\t\t;;\n \t-*)\n-\t usage\n-\t ;;\n-\t*)\n-\t break\n-\t ;;\n- esac\n- shift\n-done\n-\n-prompt_after_failed_merge() {\n- while true; do\n-\tprintf \"Continue merging other unresolved paths (y/n) ? \"\n-\tread ans || return 1\n-\tcase \"$ans\" in\n-\n-\t [yY]*)\n-\t\treturn 0\n+\t\tusage\n \t\t;;\n-\n-\t [nN]*)\n-\t\treturn 1\n+\t*)\n+\t\tbreak\n \t\t;;\n \tesac\n- done\n+\tshift\n+done\n+\n+prompt_after_failed_merge () {\n+\twhile true\n+\tdo\n+\t\tprintf \"Continue merging other unresolved paths (y/n) ? \"\n+\t\tread ans || return 1\n+\t\tcase \"$ans\" in\n+\t\t[yY]*)\n+\t\t\treturn 0\n+\t\t\t;;\n+\t\t[nN]*)\n+\t\t\treturn 1\n+\t\t\t;;\n+\t\tesac\n+\tdone\n }\n \n-if test -z \"$merge_tool\"; then\n- merge_tool=$(get_merge_tool \"$merge_tool\") || exit\n+if test -z \"$merge_tool\"\n+then\n+\tmerge_tool=$(get_merge_tool \"$merge_tool\") || exit\n fi\n merge_keep_backup=\"$(git config --bool mergetool.keepBackup || echo true)\"\n merge_keep_temporaries=\"$(git config --bool mergetool.keepTemporaries || echo false)\"\n@@ -388,22 +419,24 @@ last_status=0\n rollup_status=0\n files=\n \n-if test $# -eq 0 ; then\n- cd_to_toplevel\n+if test $# -eq 0\n+then\n+\tcd_to_toplevel\n \n- if test -e \"$GIT_DIR/MERGE_RR\"\n- then\n-\tfiles=$(git rerere remaining)\n- else\n-\tfiles=$(git ls-files -u | sed -e 's/^[^\t]*\t//' | sort -u)\n- fi\n+\tif test -e \"$GIT_DIR/MERGE_RR\"\n+\tthen\n+\t\tfiles=$(git rerere remaining)\n+\telse\n+\t\tfiles=$(git ls-files -u | sed -e 's/^[^\t]*\t//' | sort -u)\n+\tfi\n else\n- files=$(git ls-files -u -- \"$@\" | sed -e 's/^[^\t]*\t//' | sort -u)\n+\tfiles=$(git ls-files -u -- \"$@\" | sed -e 's/^[^\t]*\t//' | sort -u)\n fi\n \n-if test -z \"$files\" ; then\n- echo \"No files need merging\"\n- exit 0\n+if test -z \"$files\"\n+then\n+\techo \"No files need merging\"\n+\texit 0\n fi\n \n printf \"Merging:\\n\"\n@@ -413,15 +446,17 @@ IFS='\n '\n for i in $files\n do\n- if test $last_status -ne 0; then\n-\tprompt_after_failed_merge || exit 1\n- fi\n- printf \"\\n\"\n- merge_file \"$i\"\n- last_status=$?\n- if test $last_status -ne 0; then\n-\trollup_status=1\n- fi\n+\tif test $last_status -ne 0\n+\tthen\n+\t\tprompt_after_failed_merge || exit 1\n+\tfi\n+\tprintf \"\\n\"\n+\tmerge_file \"$i\"\n+\tlast_status=$?\n+\tif test $last_status -ne 0\n+\tthen\n+\t\trollup_status=1\n+\tfi\n done\n \n exit $rollup_status\n"},{"id":"197680","messageId":"CAJDDKr42YtfcpARQx6Vx8-Uy7KF3yEoGOQ0VzPKsWQA_hG=MgA@mail.gmail.com","threadId":"31074","inReplyTo":"7vzk5m41gk.fsf_-_@alter.siamese.dyndns.org","subject":"Re: Re*: mergetool: support --tool-help option like difftool does","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2012-08-23T07:39:06Z","receivedAt":"2012-08-23T07:39:06Z","isPatch":false,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"On Wed, Aug 22, 2012 at 10:33 PM, Junio C Hamano wrote:\n> Junio C Hamano writes:\n>\n>> This way we do not have to risk the list of tools go out of sync\n>> between the implementation and the documentation.\n>>\n>> Signed-off-by: Junio C Hamano \n>> ---\n>> Junio C Hamano writes:\n>>\n>>>> +--tool-help::\n>>>> + List the supported and available diff tools.\n>>>\n>>> This part is a good addition (but it already is mentioned in the\n>>> description of --tool above, so it is more or less a \"Meh\").\n>>\n>> I noticed that the main while loop has style violations in its\n>> case/esac, but I left it as-is. Reindenting it would be a low\n>> hanging fruit janitor patch that would be a separate topic.\n>\n> After hinting a low-hanging-fruit that would be an easy way for new\n> people to dip their toes, I saw no takers for one month, so I ended\n> up doing it myself.\n\nMy bad, I didn't catch this and should have, especially so because\nI had started on style fixing mergetool last week but ditched it;\nI assumed it would be unwanted. I will read more carefully.\n\nJust please don't tell Charles about it ;-)\n\n(I wanted to do this a long ago but at the time we didn't have\n the style guide for shell, and the time was perhaps not right...)\n\nI am of course in favor of this patch.\n\nWhile on the mergetool topic...\n\nWould the ability to resolve the various merge situations using\nthe command-line be a wanted addition?\n\nThis would let a submodule or deleted/modified encountering\nuser do something like:\n\n$ git mergetool --theirs -- submodule\n\n...and not have to remember the various git commands that it runs.\n\nThis could touch just the parts of mergetool that require stdin,\nbut probably should work for everything.\n\nIt seems like exposing the guts of some of these mergetool functions\nvia command-line flags could be helpful, but I'm not sure if that's\noverloading mergetool with too many features. What do you think?\n-- \nDavid\n"},{"id":"197723","messageId":"7vvcg94if1.fsf@alter.siamese.dyndns.org","threadId":"31074","inReplyTo":"CAJDDKr42YtfcpARQx6Vx8-Uy7KF3yEoGOQ0VzPKsWQA_hG=MgA@mail.gmail.com","subject":"Re: Re*: mergetool: support --tool-help option like difftool does","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-08-23T17:39:14Z","receivedAt":"2012-08-23T17:39:14Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"David Aguilar writes:\n\n>> After hinting a low-hanging-fruit that would be an easy way for new\n>> people to dip their toes, I saw no takers for one month, so I ended\n>> up doing it myself.\n>\n> My bad,...\n\nNot yours. These hints I drop from time to time are meant for eager\nnew people who want to dip their toes to the development (we don't\ndo \"assignments\" but these are the closest that we have); you no\nlonger quite qualify as \"new\" ;-)\n\n> While on the mergetool topic...\n\nNow we are not talking about \"dip their toes\" low hanging fruit\nanymore ;-)\n\n> Would the ability to resolve the various merge situations using\n> the command-line be a wanted addition?\n>\n> This would let a submodule or deleted/modified encountering\n> user do something like:\n>\n> $ git mergetool --theirs -- submodule\n>\n> ...and not have to remember the various git commands that it runs.\n\nDoes it have to run various git commands? For a normal path, all it\nneeds to do is \"git checkout --theirs $path\", no?\n\nWhat does it mean to resolve a conflicted merge of a gitlink to take\n\"theirs\"? We obviously would want to point the resolved gitlink at\nthe submodule commit their side wants in the resulting index but what,\nif any, should we do to the submodule itself?\n\nStepping back a bit, if there is no conflict, and as a result of a\nclean merge (this applies to the case where you check out another\nbranch that has different commit at the submodule path), if gitlink\nchanged to point at a different commit in the submodule, what should\nhappen?\n\nIf you start from a clean working tree, with your gitlink pointing\nat the commit that matches HEAD in the submodule, and if the working\ntree of the submodule does not have any local modification, it may\nbe ideal to check out the new commit in the submodule (are there\ncases where \"git checkout other_branch\" in the superproject does not\nwant to touch the submodule working tree?).\n\nThere are cases where it is not possible; checking out the new\ncommit in the submodule working tree may not succeed due to local\nmodifications. But is that kind of complication limited to the\nmerge resolution case? Isn't it shared with normal \"switching\nbranches\" case as well?\n\nIf so, it could be that your \"git mergetool --theirs -- submodule\"\nis working around a wrong problem, and the right solution may be to\nmake \"git checkout --theirs -- $path\" to always do an appropriate\nthing regardless of what kind of object $path is, no?\n"},{"id":"197753","messageId":"CAJDDKr6zk3ztEXeX8=vn3apZ3k1DxdCAu2ZDyqtNnb=Cb5ZUvQ@mail.gmail.com","threadId":"31074","inReplyTo":"7vvcg94if1.fsf@alter.siamese.dyndns.org","subject":"Re: Re*: mergetool: support --tool-help option like difftool does","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2012-08-24T08:31:04Z","receivedAt":"2012-08-24T08:31:04Z","isPatch":false,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"On Thu, Aug 23, 2012 at 10:39 AM, Junio C Hamano wrote:\n> David Aguilar writes:\n>> Would the ability to resolve the various merge situations using\n>> the command-line be a wanted addition?\n>>\n>> This would let a submodule or deleted/modified encountering\n>> user do something like:\n>>\n>> $ git mergetool --theirs -- submodule\n>>\n>> ...and not have to remember the various git commands that it runs.\n>\n> Does it have to run various git commands? For a normal path, all it\n> needs to do is \"git checkout --theirs $path\", no?\n>\n> What does it mean to resolve a conflicted merge of a gitlink to take\n> \"theirs\"? We obviously would want to point the resolved gitlink at\n> the submodule commit their side wants in the resulting index but what,\n> if any, should we do to the submodule itself?\n>\n> Stepping back a bit, if there is no conflict, and as a result of a\n> clean merge (this applies to the case where you check out another\n> branch that has different commit at the submodule path), if gitlink\n> changed to point at a different commit in the submodule, what should\n> happen?\n>\n> If you start from a clean working tree, with your gitlink pointing\n> at the commit that matches HEAD in the submodule, and if the working\n> tree of the submodule does not have any local modification, it may\n> be ideal to check out the new commit in the submodule (are there\n> cases where \"git checkout other_branch\" in the superproject does not\n> want to touch the submodule working tree?).\n>\n> There are cases where it is not possible; checking out the new\n> commit in the submodule working tree may not succeed due to local\n> modifications. But is that kind of complication limited to the\n> merge resolution case? Isn't it shared with normal \"switching\n> branches\" case as well?\n>\n> If so, it could be that your \"git mergetool --theirs -- submodule\"\n> is working around a wrong problem, and the right solution may be to\n> make \"git checkout --theirs -- $path\" to always do an appropriate\n> thing regardless of what kind of object $path is, no?\n\nTrue.\n\nAdmittedly, I'm not a heavy submodule user myself so I\ntried crafting the directory vs submodule conflict to see\nwhat the usability is like.\n\ncheckout --theirs and --ours could learn a few tricks.\n\nWhen trying to choose the directory in that situation\nI had to had to \"git rm --cached\" the submodule path\nso that git would recognize that there was no longer a conflict.\n\nThat makes sense to me because I was following along by\nreading the mergetool code, but I don't think most users\nwould know to \"git rm\" a path which they intend to keep.\n\nAfterwards, the .git file is left behind, which could cause\nproblems elsewhere since we really don't want a .git file\nin that situation. I'm not even sure what to do about the\n.gitmodules file either.\n\nHere's the script I was using to setup the merge conflict\nin case anyone wants to get a feel for the usability around\nthis edge case.\n\nPass --submodule if you want the remote side to have a\nsubmodule. By default, the local side has a submodule and\nthe remote side of the merge brings along a directory.\n\nThat said, this really isn't an issue, per say.\nI first poked at it because I noticed that mergetool\nstill needed stdin for some things.\n\nIt's certainly an edge case, and perhaps this just shows\nthat mergetool really is the right porcelain for the job\nwhen a user runs into these types of conflicts\n(the stdin thing really isn't an issue).\n\n\n#!/bin/sh\nif test \"$1\" = \"--submodule\"\nthen\n\tfirst=with-directory\n\tsecond=with-submodule\n\techo \"local will be a directory, remote will be a submodule\"\nelse\n\tfirst=with-submodule\n\tsecond=with-directory\n\techo \"local will be a submodule, remote will be a directory\"\nfi\n\nrepo=submodule-conflict-$$ &&\necho \"creating $repo\" &&\nmkdir \"$repo\" &&\ncd \"$repo\" &&\ngit init &&\ngit commit --allow-empty -m'initial' &&\ngit checkout -b with-directory master &&\nmkdir the-conflict &&\ntouch the-conflict/path &&\ngit add the-conflict/path &&\ngit commit -m'added path into the-conflict' &&\ngit checkout -b with-submodule master &&\ngit submodule add ./ the-conflict &&\ngit commit -m'added submodule as the-conflict' &&\ngit checkout -b tmp-merge master &&\ngit merge $first &&\ngit merge $second\n-- \nDavid\n"},{"id":"197870","messageId":"503A6D1E.5070909@web.de","threadId":"31074","inReplyTo":"CAJDDKr6zk3ztEXeX8=vn3apZ3k1DxdCAu2ZDyqtNnb=Cb5ZUvQ@mail.gmail.com","subject":"Re: Re*: mergetool: support --tool-help option like difftool does","fromName":"Jens Lehmann","fromEmail":"jens.lehmann@web.de","sentAt":"2012-08-26T18:38:22Z","receivedAt":"2012-08-26T18:38:22Z","isPatch":false,"sender":{"key":"jens.lehmann@web.de","avatar":"https://avatars.githubusercontent.com/u/135220?v=4"},"body":"Am 24.08.2012 10:31, schrieb David Aguilar:\n> On Thu, Aug 23, 2012 at 10:39 AM, Junio C Hamano wrote:\n>> David Aguilar writes:\n>>> Would the ability to resolve the various merge situations using\n>>> the command-line be a wanted addition?\n>>>\n>>> This would let a submodule or deleted/modified encountering\n>>> user do something like:\n>>>\n>>> $ git mergetool --theirs -- submodule\n>>>\n>>> ...and not have to remember the various git commands that it runs.\n>>\n>> Does it have to run various git commands? For a normal path, all it\n>> needs to do is \"git checkout --theirs $path\", no?\n>>\n>> What does it mean to resolve a conflicted merge of a gitlink to take\n>> \"theirs\"? We obviously would want to point the resolved gitlink at\n>> the submodule commit their side wants in the resulting index but what,\n>> if any, should we do to the submodule itself?\n>>\n>> Stepping back a bit, if there is no conflict, and as a result of a\n>> clean merge (this applies to the case where you check out another\n>> branch that has different commit at the submodule path), if gitlink\n>> changed to point at a different commit in the submodule, what should\n>> happen?\n>>\n>> If you start from a clean working tree, with your gitlink pointing\n>> at the commit that matches HEAD in the submodule, and if the working\n>> tree of the submodule does not have any local modification, it may\n>> be ideal to check out the new commit in the submodule (are there\n>> cases where \"git checkout other_branch\" in the superproject does not\n>> want to touch the submodule working tree?).\n>>\n>> There are cases where it is not possible; checking out the new\n>> commit in the submodule working tree may not succeed due to local\n>> modifications. But is that kind of complication limited to the\n>> merge resolution case? Isn't it shared with normal \"switching\n>> branches\" case as well?\n>>\n>> If so, it could be that your \"git mergetool --theirs -- submodule\"\n>> is working around a wrong problem, and the right solution may be to\n>> make \"git checkout --theirs -- $path\" to always do an appropriate\n>> thing regardless of what kind of object $path is, no?\n> \n> True.\n\nI agree.\n\n> Admittedly, I'm not a heavy submodule user myself so I\n> tried crafting the directory vs submodule conflict to see\n> what the usability is like.\n> \n> checkout --theirs and --ours could learn a few tricks.\n\nMe thinks that after I successfully taught checkout to properly\nrecurse into submodule work trees too it should know all those\ntricks. In my current version it can handle directory/submodule\nconversions in both directions, this should be sufficient to\nmake \"checkout --theirs/--ours\" work properly. Note to self: add\ntests for that.\n\n> When trying to choose the directory in that situation\n> I had to had to \"git rm --cached\" the submodule path\n> so that git would recognize that there was no longer a conflict.\n> \n> That makes sense to me because I was following along by\n> reading the mergetool code, but I don't think most users\n> would know to \"git rm\" a path which they intend to keep.\n\nTrue. But a submodule recursing checkout would do the right thing\nhere too.\n\n> Afterwards, the .git file is left behind, which could cause\n> problems elsewhere since we really don't want a .git file\n> in that situation.\n\nHmm, either you remove all the files tracked in the submodule\ntogether with the gitfile or you'll possibly have former submodule\nfiles lying around there too. Recursive checkout will do all that\nfor you.\n\n> I'm not even sure what to do about the\n> .gitmodules file either.\n\nMaybe we should issue a warning when the .gitmodules file is not\nconsistent with the [non]existence of a submodule in the work tree?\n\n> That said, this really isn't an issue, per say.\n> I first poked at it because I noticed that mergetool\n> still needed stdin for some things.\n> \n> It's certainly an edge case, and perhaps this just shows\n> that mergetool really is the right porcelain for the job\n> when a user runs into these types of conflicts\n> (the stdin thing really isn't an issue).\n\nIt looks to me as if the submodule/directory conflict can be handled\nby a recursive checkout without having to add something to mergetool.\n"}]}