threads / patch / 31074

patch, 4 partsVarious merge / diff tool related minor clean-ups and improvements

Subject: [PATCH 0/4] Various merge / diff tool related minor clean-ups and improvements

## tl;dr

40 messages between Jul 23, 2012 and Aug 26, 2012. Diffs are folded; open one to read it.

replies: 39people: 5as markdown or json

Sebastian Schuberth· Jul 23, 2012, 07:10 UTC · lore
This series introduce various minor clean-ups and improvements to the merge / diff tool scripts and documentation.
Sebastian Schuberth (4):
  Use variables for the lists of tools that support merging / diffing
  Explicitly list all valid diff tools and document --tool-help as an
    option
  Make sure to use Araxis' "compare" and not e.g. ImageMagick's
  Add a few more code comments and blank lines in guess_merge_tool
 Documentation/git-difftool.txt         |  9 ++++++---
 contrib/completion/git-completion.bash | 11 +++++++++--
 git-mergetool--lib.sh                  |  6 ++++++
 mergetools/araxis                      |  8 +++++++-
 4 files changed, 28 insertions(+), 6 deletions(-)
-- 
1.7.11.msysgit.2
Sebastian Schuberth· Jul 23, 2012, 07:14 UTC · re: Sebastian Schuberth · lore

[PATCH v2 0/5] Various merge / diff tool related minor clean-ups and improvements

This series introduces various minor clean-ups and improvements to the merge / diff tool scripts and documentation.
Sorry, the first version was missing a patch.
Sebastian Schuberth (5):
  Sort the list of tools that support both merging and diffing
    alphabetically
  Use variables for the lists of tools that support merging / diffing
  Explicitly list all valid diff tools and document --tool-help as an
    option
  Make sure to use Araxis' "compare" and not e.g. ImageMagick's
  Add a few more code comments and blank lines in guess_merge_tool
 Documentation/git-difftool.txt         |  9 ++++++---
 contrib/completion/git-completion.bash | 15 +++++++++++----
 git-mergetool--lib.sh                  |  6 ++++++
 mergetools/araxis                      |  8 +++++++-
 4 files changed, 30 insertions(+), 8 deletions(-)
-- 
1.7.11.msysgit.2
Sebastian Schuberth· Jul 23, 2012, 07:17 UTC · re: Sebastian Schuberth · lore

[PATCH v2 1/5] Sort the list of tools that support both merging and diffing alphabetically

Signed-off-by: Sebastian Schuberth <sschuberth@gmail.com>
---
 contrib/completion/git-completion.bash | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
Show changes to contrib/completion/git-completion.bash +2 −2
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 5be9dee..f2c4894 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1325,8 +1325,8 @@ _git_diff ()
 	__git_complete_revlist_file
 }
 
-__git_mergetools_common="diffuse ecmerge emerge kdiff3 meld opendiff
-			tkdiff vimdiff gvimdiff xxdiff araxis p4merge bc3
+__git_mergetools_common="araxis bc3 diffuse ecmerge emerge gvimdiff
+			kdiff3 meld opendiff p4merge tkdiff vimdiff xxdiff
 "
 
 _git_difftool ()
-- 
1.7.11.msysgit.2
Sebastian Schuberth· Jul 23, 2012, 07:18 UTC · re: Sebastian Schuberth · lore

[PATCH v2 2/5] Use variables for the lists of tools that support merging / diffing

Also, add a few comments that clarify the meaning of these variables.
Signed-off-by: Sebastian Schuberth <sschuberth@gmail.com>
---
 contrib/completion/git-completion.bash | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)
Show changes to contrib/completion/git-completion.bash +9 −2
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index f2c4894..6b9b79d 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1325,17 +1325,24 @@ _git_diff ()
 	__git_complete_revlist_file
 }
 
+# Tools that support both merging and diffing.
 __git_mergetools_common="araxis bc3 diffuse ecmerge emerge gvimdiff
 			kdiff3 meld opendiff p4merge tkdiff vimdiff xxdiff
 "
 
+# Tools that support diffing.
+__git_difftools="$__git_mergetools_common kcompare"
+
+# Tools that support merging.
+__git_mergetools="$__git_mergetools_common tortoisemerge"
+
 _git_difftool ()
 {
 	__git_has_doubledash && return
 
 	case "$cur" in
 	--tool=*)
-		__gitcomp "$__git_mergetools_common kompare" "" "${cur##--tool=}"
+		__gitcomp "$__git_difftools" "" "${cur##--tool=}"
 		return
 		;;
 	--*)
@@ -1623,7 +1630,7 @@ _git_mergetool ()
 {
 	case "$cur" in
 	--tool=*)
-		__gitcomp "$__git_mergetools_common tortoisemerge" "" "${cur##--tool=}"
+		__gitcomp "$__git_mergetools" "" "${cur##--tool=}"
 		return
 		;;
 	--*)
-- 
1.7.11.msysgit.2
Junio C Hamano· Jul 23, 2012, 16:46 UTC · re: Sebastian Schuberth · lore

Re: [PATCH v2 2/5] Use variables for the lists of tools that support merging / diffing

Sebastian Schuberth <sschuberth@gmail.com> writes:
Show 19 quoted lines
> Also, add a few comments that clarify the meaning of these variables.
>
> Signed-off-by: Sebastian Schuberth <sschuberth@gmail.com>
> ---
>  contrib/completion/git-completion.bash | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> index f2c4894..6b9b79d 100755
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -1325,17 +1325,24 @@ _git_diff ()
>  	__git_complete_revlist_file
>  }
>  
> +# Tools that support both merging and diffing.
>  __git_mergetools_common="araxis bc3 diffuse ecmerge emerge gvimdiff
>  			kdiff3 meld opendiff p4merge tkdiff vimdiff xxdiff
>  "

As the set of merge capable tools is not a superset of diff capable tools (tortoise can only merge but not diff), perhaps rename this to __git_diffmerge_tools or something?

Show 6 quoted lines
> +# Tools that support diffing.
> +__git_difftools="$__git_mergetools_common kcompare"
> +
> +# Tools that support merging.
> +__git_mergetools="$__git_mergetools_common tortoisemerge"
> +

This patch makes sense to me, but at the same time makes [PATCH 1/5] a "Meh", methinks.

Show 20 quoted lines
>  _git_difftool ()
>  {
>  	__git_has_doubledash && return
>  
>  	case "$cur" in
>  	--tool=*)
> -		__gitcomp "$__git_mergetools_common kompare" "" "${cur##--tool=}"
> +		__gitcomp "$__git_difftools" "" "${cur##--tool=}"
>  		return
>  		;;
>  	--*)
> @@ -1623,7 +1630,7 @@ _git_mergetool ()
>  {
>  	case "$cur" in
>  	--tool=*)
> -		__gitcomp "$__git_mergetools_common tortoisemerge" "" "${cur##--tool=}"
> +		__gitcomp "$__git_mergetools" "" "${cur##--tool=}"
>  		return
>  		;;
>  	--*)
Sebastian Schuberth· Jul 23, 2012, 18:30 UTC · re: Junio C Hamano · lore

Re: [PATCH v2 2/5] Use variables for the lists of tools that support merging / diffing

On 23.07.2012 18:46, Junio C Hamano wrote:
Show 8 quoted lines
>> +# Tools that support both merging and diffing.
>>   __git_mergetools_common="araxis bc3 diffuse ecmerge emerge gvimdiff
>>   			kdiff3 meld opendiff p4merge tkdiff vimdiff xxdiff
>>   "
> 
> As the set of merge capable tools is not a superset of diff capable
> tools (tortoise can only merge but not diff), perhaps rename this to
> __git_diffmerge_tools or something?
Makes perfect sense.
> This patch makes sense to me, but at the same time makes [PATCH 1/5]
> a "Meh", methinks.
Yeah, I can see why. So I've renamed __git_mergetools_common to __git_diffmerge_tools and squashed with [PATCH 1/5] to make it less "Meh" as it does not stand on its own.
-- 
Sebastian
Sebastian Schuberth· Jul 23, 2012, 18:32 UTC · re: Sebastian Schuberth · lore

[PATCH 1/4] Use variables for the lists of tools that support merging / diffing

Also, add a few comments that clarify the meaning of these variables and sort the list of tools alphabetically.

Signed-off-by: Sebastian Schuberth <sschuberth@gmail.com>
---
 contrib/completion/git-completion.bash | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)
Show changes to contrib/completion/git-completion.bash +11 −4
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 5be9dee..4a76120 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1325,17 +1325,24 @@ _git_diff ()
 	__git_complete_revlist_file
 }
 
-__git_mergetools_common="diffuse ecmerge emerge kdiff3 meld opendiff
-			tkdiff vimdiff gvimdiff xxdiff araxis p4merge bc3
+# Tools that support both merging and diffing.
+__git_diffmerge_tools="araxis bc3 diffuse ecmerge emerge gvimdiff
+			kdiff3 meld opendiff p4merge tkdiff vimdiff xxdiff
 "
 
+# Tools that support diffing.
+__git_difftools="$__git_diffmerge_tools kcompare"
+
+# Tools that support merging.
+__git_mergetools="$__git_diffmerge_tools tortoisemerge"
+
 _git_difftool ()
 {
 	__git_has_doubledash && return
 
 	case "$cur" in
 	--tool=*)
-		__gitcomp "$__git_mergetools_common kompare" "" "${cur##--tool=}"
+		__gitcomp "$__git_difftools" "" "${cur##--tool=}"
 		return
 		;;
 	--*)
@@ -1623,7 +1630,7 @@ _git_mergetool ()
 {
 	case "$cur" in
 	--tool=*)
-		__gitcomp "$__git_mergetools_common tortoisemerge" "" "${cur##--tool=}"
+		__gitcomp "$__git_mergetools" "" "${cur##--tool=}"
 		return
 		;;
 	--*)
-- 
1.7.11.msysgit.2
Junio C Hamano· Jul 23, 2012, 18:37 UTC · re: Sebastian Schuberth · lore

Re: [PATCH v2 2/5] Use variables for the lists of tools that support merging / diffing

Sebastian Schuberth <sschuberth@gmail.com> writes:
Show 6 quoted lines
>> This patch makes sense to me, but at the same time makes [PATCH 1/5]
>> a "Meh", methinks.
>
> Yeah, I can see why. So I've renamed __git_mergetools_common to
> __git_diffmerge_tools and squashed with [PATCH 1/5] to make it
> less "Meh" as it does not stand on its own.

As you append kcompare or tortoise _after_ the common list, any code that uses the variable cannot assume that the list is sorted, and needs to sort the elements if it wants to give a sorted output, so squashing does not make the Meh-ness go away.

By the way, would it make sense to remove these three variables from the completion code, and instead ask "git mergetool --tool-help" when it needs the list of supported tools for the first time? It would be trivial to introduce --tool-list that gives a one tool per line output to both "git difftool" and "git mergetool" and we would remove the risk of separately maintained list drifting away over time.

Sebastian Schuberth· Jul 23, 2012, 19:03 UTC · re: Junio C Hamano · lore

Re: [PATCH v2 2/5] Use variables for the lists of tools that support merging / diffing

On 23.07.2012 20:37, Junio C Hamano wrote:
Show 11 quoted lines
>>> This patch makes sense to me, but at the same time makes [PATCH 1/5]
>>> a "Meh", methinks.
>>
>> Yeah, I can see why. So I've renamed __git_mergetools_common to
>> __git_diffmerge_tools and squashed with [PATCH 1/5] to make it
>> less "Meh" as it does not stand on its own.
>
> As you append kcompare or tortoise _after_ the common list, any code
> that uses the variable cannot assume that the list is sorted, and
> needs to sort the elements if it wants to give a sorted output, so
> squashing does not make the Meh-ness go away.

Well, that (mostly) sorted listed still helps to find out a little quicker whether a specific tool that can do both merging and diffing is already in the list. At least that's the case for me.

Show 7 quoted lines
> By the way, would it make sense to remove these three variables from
> the completion code, and instead ask "git mergetool --tool-help"
> when it needs the list of supported tools for the first time?  It
> would be trivial to introduce --tool-list that gives a one tool per
> line output to both "git difftool" and "git mergetool" and we would
> remove the risk of separately maintained list drifting away over
> time.

Sounds like a good idea now that you've added "git mergetool --tool-help". But I'd like to save this for a future exercise to not do too much stuff at the same time.

-- 
Sebastian Schuberth
Sebastian Schuberth· Jul 23, 2012, 07:18 UTC · re: Sebastian Schuberth · lore

[PATCH 3/5] Explicitly list all valid diff tools and document --tool-help as an option

Signed-off-by: Sebastian Schuberth <sschuberth@gmail.com>
---
 Documentation/git-difftool.txt | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)
Show changes to Documentation/git-difftool.txt +6 −3
diff --git a/Documentation/git-difftool.txt b/Documentation/git-difftool.txt
index 31fc2e3..5dd54f1 100644
--- a/Documentation/git-difftool.txt
+++ b/Documentation/git-difftool.txt
@@ -36,9 +36,12 @@ OPTIONS
 
 -t <tool>::
 --tool=<tool>::
-	Use the diff tool specified by <tool>.  Valid values include
-	emerge, kompare, meld, and vimdiff. Run `git difftool --tool-help`
-	for the list of valid <tool> settings.
+	Use the diff tool specified by <tool>.  Valid diff tools are:
+	araxis, bc3, diffuse, ecmerge, emerge, gvimdiff, kcompare, kdiff3,
+	meld, opendiff, p4merge, tkdiff, vimdiff and xxdiff.
+
+--tool-help::
+	List the supported and available diff tools.
 +
 If a diff tool is not specified, 'git difftool'
 will use the configuration variable `diff.tool`.  If the
-- 
1.7.11.msysgit.2
Junio C Hamano· Jul 23, 2012, 16:48 UTC · re: Sebastian Schuberth · lore

Re: [PATCH 3/5] Explicitly list all valid diff tools and document --tool-help as an option

Sebastian Schuberth <sschuberth@gmail.com> writes:
Show 16 quoted lines
> Signed-off-by: Sebastian Schuberth <sschuberth@gmail.com>
> ---
>  Documentation/git-difftool.txt | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/git-difftool.txt b/Documentation/git-difftool.txt
> index 31fc2e3..5dd54f1 100644
> --- a/Documentation/git-difftool.txt
> +++ b/Documentation/git-difftool.txt
> @@ -36,9 +36,12 @@ OPTIONS
>  
>  -t <tool>::
>  --tool=<tool>::
> -	Use the diff tool specified by <tool>.  Valid values include
> -	emerge, kompare, meld, and vimdiff. Run `git difftool --tool-help`
> -	for the list of valid <tool> settings.

I thought we say "include" because we really do not want to list millions of tools here, so mild NAK on this part.

Show 6 quoted lines
> +	Use the diff tool specified by <tool>.  Valid diff tools are:
> +	araxis, bc3, diffuse, ecmerge, emerge, gvimdiff, kcompare, kdiff3,
> +	meld, opendiff, p4merge, tkdiff, vimdiff and xxdiff.
> +
> +--tool-help::
> +	List the supported and available diff tools.

This part is a good addition (but it already is mentioned in the description of --tool above, so it is more or less a "Meh").

>  +
>  If a diff tool is not specified, 'git difftool'
>  will use the configuration variable `diff.tool`.  If the
Junio C Hamano· Jul 23, 2012, 17:21 UTC · re: Junio C Hamano · lore

mergetool: support --tool-help option like difftool does

This way we do not have to risk the list of tools go out of sync between the implementation and the documentation.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
Junio C Hamano <gitster@pobox.com> writes:
Show 5 quoted lines
>> +--tool-help::
>> +	List the supported and available diff tools.
>
> This part is a good addition (but it already is mentioned in the
> description of --tool above, so it is more or less a "Meh").

I noticed that the main while loop has style violations in its case/esac, but I left it as-is. Reindenting it would be a low hanging fruit janitor patch that would be a separate topic.

 git-mergetool--lib.sh |  6 +++++-
 git-mergetool.sh      | 42 +++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 46 insertions(+), 2 deletions(-)
Show changes to 2 files +46 −2

git-mergetool--lib.sh, git-mergetool.sh

diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
index ed630b2..f730253 100644
--- a/git-mergetool--lib.sh
+++ b/git-mergetool--lib.sh
@@ -111,7 +111,7 @@ run_merge_tool () {
 	return $status
 }
 
-guess_merge_tool () {
+list_merge_tool_candidates () {
 	if merge_mode
 	then
 		tools="tortoisemerge"
@@ -136,6 +136,10 @@ guess_merge_tool () {
 		tools="$tools emerge vimdiff"
 		;;
 	esac
+}
+
+guess_merge_tool () {
+	list_merge_tool_candidates
 	echo >&2 "merge tool candidates: $tools"
 
 	# Loop over each candidate and stop when a valid merge tool is found.
diff --git a/git-mergetool.sh b/git-mergetool.sh
index a9f23f7..0db0c44 100755
--- a/git-mergetool.sh
+++ b/git-mergetool.sh
@@ -8,7 +8,7 @@
 # at the discretion of Junio C Hamano.
 #
 
-USAGE='[--tool=tool] [-y|--no-prompt|--prompt] [file to merge] ...'
+USAGE='[--tool=tool] [--tool-help] [-y|--no-prompt|--prompt] [file to merge] ...'
 SUBDIRECTORY_OK=Yes
 OPTIONS_SPEC=
 TOOL_MODE=merge
@@ -284,11 +284,51 @@ merge_file () {
     return 0
 }
 
+show_tool_help () {
+	TOOL_MODE=merge
+	list_merge_tool_candidates
+	unavailable= available= LF='
+'
+	for i in $tools
+	do
+		merge_tool_path=$(translate_merge_tool_path "$i")
+		if type "$merge_tool_path" >/dev/null 2>&1
+		then
+			available="$available$i$LF"
+		else
+			unavailable="$unavailable$i$LF"
+		fi
+	done
+	if test -n "$available"
+	then
+		echo "'git mergetool --tool=<tool>' may be set to one of the following:"
+		echo "$available" | sort | sed -e 's/^/	/'
+	else
+		echo "No suitable tool for 'git mergetool --tool=<tool>' found."
+	fi
+	if test -n "$unavailable"
+	then
+		echo
+		echo 'The following tools are valid, but not currently available:'
+		echo "$unavailable" | sort | sed -e 's/^/	/'
+	fi
+	if test -n "$unavailable$available"
+	then
+		echo
+		echo "Some of the tools listed above only work in a windowed"
+		echo "environment. If run in a terminal-only session, they will fail."
+	fi
+	exit 0
+}
+
 prompt=$(git config --bool mergetool.prompt || echo true)
 
 while test $# != 0
 do
     case "$1" in
+	--tool-help)
+		show_tool_help
+		;;
 	-t|--tool*)
 	    case "$#,$1" in
 		*,*=*)
Sebastian Schuberth· Jul 23, 2012, 18:56 UTC · re: Junio C Hamano · lore

Re: mergetool: support --tool-help option like difftool does

On 23.07.2012 19:21, Junio C Hamano wrote:
> This way we do not have to risk the list of tools go out of sync
> between the implementation and the documentation.
> 
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Thanks! I've squashed this with
[PATCH v2 5/5] Add a few more code comments and blank lines in guess_merge_tool
and parts of
[PATCH 3/5] Explicitly list all valid diff tools and document --tool-help as an option
and adjusted the docs for git-mergetool accordingly.
-- 
Sebastian Schuberth
Sebastian Schuberth· Jul 23, 2012, 18:58 UTC · re: Sebastian Schuberth · lore

[PATCH] mergetool: support --tool-help option like difftool does

This way we do not have to risk the list of tools go out of sync between the implementation and the documentation. Adjust the documentation accordingly to not explicitly list the tools but refer to --tool-help.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Sebastian Schuberth <sschuberth@gmail.com>
---
 Documentation/git-difftool.txt  |  7 ++++---
 Documentation/git-mergetool.txt |  8 ++++----
 git-mergetool--lib.sh           | 11 ++++++++++-
 git-mergetool.sh                | 42 ++++++++++++++++++++++++++++++++++++++++-
 4 files changed, 59 insertions(+), 9 deletions(-)
Show changes to 4 files +59 −9

Documentation/git-difftool.txt, Documentation/git-mergetool.txt, git-mergetool--lib.sh, git-mergetool.sh

diff --git a/Documentation/git-difftool.txt b/Documentation/git-difftool.txt
index 31fc2e3..0bdfe35 100644
--- a/Documentation/git-difftool.txt
+++ b/Documentation/git-difftool.txt
@@ -36,9 +36,10 @@ OPTIONS
 
 -t <tool>::
 --tool=<tool>::
-	Use the diff tool specified by <tool>.  Valid values include
-	emerge, kompare, meld, and vimdiff. Run `git difftool --tool-help`
-	for the list of valid <tool> settings.
+	Use the diff tool specified by <tool>.
+
+--tool-help::
+	List all supported values for <tool>.
 +
 If a diff tool is not specified, 'git difftool'
 will use the configuration variable `diff.tool`.  If the
diff --git a/Documentation/git-mergetool.txt b/Documentation/git-mergetool.txt
index 2a49de7..99e53b1 100644
--- a/Documentation/git-mergetool.txt
+++ b/Documentation/git-mergetool.txt
@@ -26,10 +26,10 @@ OPTIONS
 -------
 -t <tool>::
 --tool=<tool>::
-	Use the merge resolution program specified by <tool>.
-	Valid merge tools are:
-	araxis, bc3, diffuse, ecmerge, emerge, gvimdiff, kdiff3,
-	meld, opendiff, p4merge, tkdiff, tortoisemerge, vimdiff and xxdiff.
+	Use the merge tool specified by <tool>.
+
+--tool-help::
+	List all supported values for <tool>.
 +
 If a merge resolution program is not specified, 'git mergetool'
 will use the configuration variable `merge.tool`.  If the
diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
index ed630b2..f0865d4 100644
--- a/git-mergetool--lib.sh
+++ b/git-mergetool--lib.sh
@@ -111,15 +111,18 @@ run_merge_tool () {
 	return $status
 }
 
-guess_merge_tool () {
+list_merge_tool_candidates () {
+	# Add tools that can either do merging or diffing, but not both.
 	if merge_mode
 	then
 		tools="tortoisemerge"
 	else
 		tools="kompare"
 	fi
+
 	if test -n "$DISPLAY"
 	then
+		# Prefer GTK-based tools under Gnome.
 		if test -n "$GNOME_DESKTOP_SESSION_ID"
 		then
 			tools="meld opendiff kdiff3 tkdiff xxdiff $tools"
@@ -128,6 +131,8 @@ guess_merge_tool () {
 		fi
 		tools="$tools gvimdiff diffuse ecmerge p4merge araxis bc3"
 	fi
+
+	# Prefer vimdiff if vim is the default editor.
 	case "${VISUAL:-$EDITOR}" in
 	*vim*)
 		tools="$tools vimdiff emerge"
@@ -136,6 +141,10 @@ guess_merge_tool () {
 		tools="$tools emerge vimdiff"
 		;;
 	esac
+}
+
+guess_merge_tool () {
+	list_merge_tool_candidates
 	echo >&2 "merge tool candidates: $tools"
 
 	# Loop over each candidate and stop when a valid merge tool is found.
diff --git a/git-mergetool.sh b/git-mergetool.sh
index a9f23f7..0db0c44 100755
--- a/git-mergetool.sh
+++ b/git-mergetool.sh
@@ -8,7 +8,7 @@
 # at the discretion of Junio C Hamano.
 #
 
-USAGE='[--tool=tool] [-y|--no-prompt|--prompt] [file to merge] ...'
+USAGE='[--tool=tool] [--tool-help] [-y|--no-prompt|--prompt] [file to merge] ...'
 SUBDIRECTORY_OK=Yes
 OPTIONS_SPEC=
 TOOL_MODE=merge
@@ -284,11 +284,51 @@ merge_file () {
     return 0
 }
 
+show_tool_help () {
+	TOOL_MODE=merge
+	list_merge_tool_candidates
+	unavailable= available= LF='
+'
+	for i in $tools
+	do
+		merge_tool_path=$(translate_merge_tool_path "$i")
+		if type "$merge_tool_path" >/dev/null 2>&1
+		then
+			available="$available$i$LF"
+		else
+			unavailable="$unavailable$i$LF"
+		fi
+	done
+	if test -n "$available"
+	then
+		echo "'git mergetool --tool=<tool>' may be set to one of the following:"
+		echo "$available" | sort | sed -e 's/^/	/'
+	else
+		echo "No suitable tool for 'git mergetool --tool=<tool>' found."
+	fi
+	if test -n "$unavailable"
+	then
+		echo
+		echo 'The following tools are valid, but not currently available:'
+		echo "$unavailable" | sort | sed -e 's/^/	/'
+	fi
+	if test -n "$unavailable$available"
+	then
+		echo
+		echo "Some of the tools listed above only work in a windowed"
+		echo "environment. If run in a terminal-only session, they will fail."
+	fi
+	exit 0
+}
+
 prompt=$(git config --bool mergetool.prompt || echo true)
 
 while test $# != 0
 do
     case "$1" in
+	--tool-help)
+		show_tool_help
+		;;
 	-t|--tool*)
 	    case "$#,$1" in
 		*,*=*)
-- 
1.7.11.msysgit.2
Junio C Hamano· Jul 23, 2012, 19:52 UTC · re: Sebastian Schuberth · lore

Re: [PATCH] mergetool: support --tool-help option like difftool does

Sebastian Schuberth <sschuberth@gmail.com> writes:
Show 25 quoted lines
> This way we do not have to risk the list of tools go out of sync
> between the implementation and the documentation. Adjust the documentation
> accordingly to not explicitly list the tools but refer to --tool-help.
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> Signed-off-by: Sebastian Schuberth <sschuberth@gmail.com>
> ---
>  Documentation/git-difftool.txt  |  7 ++++---
>  Documentation/git-mergetool.txt |  8 ++++----
>  git-mergetool--lib.sh           | 11 ++++++++++-
>  git-mergetool.sh                | 42 ++++++++++++++++++++++++++++++++++++++++-
>  4 files changed, 59 insertions(+), 9 deletions(-)
>
> diff --git a/Documentation/git-difftool.txt b/Documentation/git-difftool.txt
> index 31fc2e3..0bdfe35 100644
> --- a/Documentation/git-difftool.txt
> +++ b/Documentation/git-difftool.txt
> @@ -36,9 +36,10 @@ OPTIONS
>  
>  -t <tool>::
>  --tool=<tool>::
> -	Use the diff tool specified by <tool>.  Valid values include
> -	emerge, kompare, meld, and vimdiff. Run `git difftool --tool-help`
> -	for the list of valid <tool> settings.
> +	Use the diff tool specified by <tool>.

I do not see how it is an improvement to drop the most common ones. People sometimes read documentation without having an access to shell to run "cmd --tool-help", and a list of handful of well known ones would serve as a good hint to let the reader know the kind of commands the front-end is capable of spawning, which in turn help such a reader to imagine how the command is used to judge if it is something the reader wants to use.

Show 19 quoted lines
> +
> +--tool-help::
> +	List all supported values for <tool>.
>  +
>  If a diff tool is not specified, 'git difftool'
>  will use the configuration variable `diff.tool`.  If the
> diff --git a/Documentation/git-mergetool.txt b/Documentation/git-mergetool.txt
> index 2a49de7..99e53b1 100644
> --- a/Documentation/git-mergetool.txt
> +++ b/Documentation/git-mergetool.txt
> @@ -26,10 +26,10 @@ OPTIONS
>  -------
>  -t <tool>::
>  --tool=<tool>::
> -	Use the merge resolution program specified by <tool>.
> -	Valid merge tools are:
> -	araxis, bc3, diffuse, ecmerge, emerge, gvimdiff, kdiff3,
> -	meld, opendiff, p4merge, tkdiff, tortoisemerge, vimdiff and xxdiff.
> +	Use the merge tool specified by <tool>.

Likewise. Dropping araxis, bc3, etc. to trim the list to 4-5 items that people would know what they are would make sense, though. The purpose of the list is not to tell if the reader's favorite tool is supported or not---it is to hint the flavor of what kind of things the command spawned would be capable of.

Show 115 quoted lines
> +
> +--tool-help::
> +	List all supported values for <tool>.
>  +
>  If a merge resolution program is not specified, 'git mergetool'
>  will use the configuration variable `merge.tool`.  If the
> diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
> index ed630b2..f0865d4 100644
> --- a/git-mergetool--lib.sh
> +++ b/git-mergetool--lib.sh
> @@ -111,15 +111,18 @@ run_merge_tool () {
>  	return $status
>  }
>  
> -guess_merge_tool () {
> +list_merge_tool_candidates () {
> +	# Add tools that can either do merging or diffing, but not both.
>  	if merge_mode
>  	then
>  		tools="tortoisemerge"
>  	else
>  		tools="kompare"
>  	fi
> +
>  	if test -n "$DISPLAY"
>  	then
> +		# Prefer GTK-based tools under Gnome.
>  		if test -n "$GNOME_DESKTOP_SESSION_ID"
>  		then
>  			tools="meld opendiff kdiff3 tkdiff xxdiff $tools"
> @@ -128,6 +131,8 @@ guess_merge_tool () {
>  		fi
>  		tools="$tools gvimdiff diffuse ecmerge p4merge araxis bc3"
>  	fi
> +
> +	# Prefer vimdiff if vim is the default editor.
>  	case "${VISUAL:-$EDITOR}" in
>  	*vim*)
>  		tools="$tools vimdiff emerge"
> @@ -136,6 +141,10 @@ guess_merge_tool () {
>  		tools="$tools emerge vimdiff"
>  		;;
>  	esac
> +}
> +
> +guess_merge_tool () {
> +	list_merge_tool_candidates
>  	echo >&2 "merge tool candidates: $tools"
>  
>  	# Loop over each candidate and stop when a valid merge tool is found.
> diff --git a/git-mergetool.sh b/git-mergetool.sh
> index a9f23f7..0db0c44 100755
> --- a/git-mergetool.sh
> +++ b/git-mergetool.sh
> @@ -8,7 +8,7 @@
>  # at the discretion of Junio C Hamano.
>  #
>  
> -USAGE='[--tool=tool] [-y|--no-prompt|--prompt] [file to merge] ...'
> +USAGE='[--tool=tool] [--tool-help] [-y|--no-prompt|--prompt] [file to merge] ...'
>  SUBDIRECTORY_OK=Yes
>  OPTIONS_SPEC=
>  TOOL_MODE=merge
> @@ -284,11 +284,51 @@ merge_file () {
>      return 0
>  }
>  
> +show_tool_help () {
> +	TOOL_MODE=merge
> +	list_merge_tool_candidates
> +	unavailable= available= LF='
> +'
> +	for i in $tools
> +	do
> +		merge_tool_path=$(translate_merge_tool_path "$i")
> +		if type "$merge_tool_path" >/dev/null 2>&1
> +		then
> +			available="$available$i$LF"
> +		else
> +			unavailable="$unavailable$i$LF"
> +		fi
> +	done
> +	if test -n "$available"
> +	then
> +		echo "'git mergetool --tool=<tool>' may be set to one of the following:"
> +		echo "$available" | sort | sed -e 's/^/	/'
> +	else
> +		echo "No suitable tool for 'git mergetool --tool=<tool>' found."
> +	fi
> +	if test -n "$unavailable"
> +	then
> +		echo
> +		echo 'The following tools are valid, but not currently available:'
> +		echo "$unavailable" | sort | sed -e 's/^/	/'
> +	fi
> +	if test -n "$unavailable$available"
> +	then
> +		echo
> +		echo "Some of the tools listed above only work in a windowed"
> +		echo "environment. If run in a terminal-only session, they will fail."
> +	fi
> +	exit 0
> +}
> +
>  prompt=$(git config --bool mergetool.prompt || echo true)
>  
>  while test $# != 0
>  do
>      case "$1" in
> +	--tool-help)
> +		show_tool_help
> +		;;
>  	-t|--tool*)
>  	    case "$#,$1" in
>  		*,*=*)
Sebastian Schuberth· Jul 23, 2012, 20:14 UTC · re: Junio C Hamano · lore

Re: [PATCH] mergetool: support --tool-help option like difftool does

On Mon, Jul 23, 2012 at 9:52 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 14 quoted lines
>>  -t <tool>::
>>  --tool=<tool>::
>> -     Use the diff tool specified by <tool>.  Valid values include
>> -     emerge, kompare, meld, and vimdiff. Run `git difftool --tool-help`
>> -     for the list of valid <tool> settings.
>> +     Use the diff tool specified by <tool>.
>
> I do not see how it is an improvement to drop the most common ones.
> People sometimes read documentation without having an access to
> shell to run "cmd --tool-help", and a list of handful of well known
> ones would serve as a good hint to let the reader know the kind of
> commands the front-end is capable of spawning, which in turn help
> such a reader to imagine how the command is used to judge if it is
> something the reader wants to use.

I don't agree. What "most common ones" are depends on your platform and is sort of subjective. So it should be either all or non here. Your argument about people not having shell access is a valid one, but still that would mean to list all tools in my opinion. And listing all tools again thwarts our goal to reduce the number of places where new merge / diff tools need to be added. For people adding new merge / diff tools it is just clearer what places need to be modified if there are no places that list an arbitrary subset of tools.

-- 
Sebastian Schuberth
David Aguilar· Jul 23, 2012, 20:44 UTC · re: Sebastian Schuberth · lore

Re: [PATCH] mergetool: support --tool-help option like difftool does

On Mon, Jul 23, 2012 at 1:14 PM, Sebastian Schuberth <sschuberth@gmail.com> wrote:

Show 19 quoted lines
> On Mon, Jul 23, 2012 at 9:52 PM, Junio C Hamano <gitster@pobox.com> wrote:
>
>>>  -t <tool>::
>>>  --tool=<tool>::
>>> -     Use the diff tool specified by <tool>.  Valid values include
>>> -     emerge, kompare, meld, and vimdiff. Run `git difftool --tool-help`
>>> -     for the list of valid <tool> settings.
>>> +     Use the diff tool specified by <tool>.
>>
>> I do not see how it is an improvement to drop the most common ones.
>> People sometimes read documentation without having an access to
>> shell to run "cmd --tool-help", and a list of handful of well known
>> ones would serve as a good hint to let the reader know the kind of
>> commands the front-end is capable of spawning, which in turn help
>> such a reader to imagine how the command is used to judge if it is
>> something the reader wants to use.
>
> I don't agree. What "most common ones" are depends on your platform
> and is sort of subjective. So it should be either all or non here.
Let's please leave this section as-is.

This part of the documentation has had a fair amount of churn. Specifically, it would get touched every time a new tool was added.

The point of bf73fc212a159210398b6d46ed5e9101c650e7db was to change it *one last time* into something that is helpful, but not a substitute for the real list output by --tool-help.

If any changes are done then it should be to make git-mergetool.txt match the advice given in git-difftool.txt.

Show 6 quoted lines
> Your argument about people not having shell access is a valid one, but
> still that would mean to list all tools in my opinion. And listing all
> tools again thwarts our goal to reduce the number of places where new
> merge / diff tools need to be added. For people adding new merge /
> diff tools it is just clearer what places need to be modified if there
> are no places that list an arbitrary subset of tools.

Yes, indeed, it is arbitrary. It does have some merit, though--it is also a good compromise between unhelpful (listing nothing) and painful to maintain (listing everything).

-- 
David
Junio C Hamano· Jul 23, 2012, 21:16 UTC · re: David Aguilar · lore

Re: [PATCH] mergetool: support --tool-help option like difftool does

David Aguilar <davvid@gmail.com> writes:
Show 23 quoted lines
> On Mon, Jul 23, 2012 at 1:14 PM, Sebastian Schuberth
> <sschuberth@gmail.com> wrote:
>> On Mon, Jul 23, 2012 at 9:52 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>
>>>>  -t <tool>::
>>>>  --tool=<tool>::
>>>> -     Use the diff tool specified by <tool>.  Valid values include
>>>> -     emerge, kompare, meld, and vimdiff. Run `git difftool --tool-help`
>>>> -     for the list of valid <tool> settings.
>>>> +     Use the diff tool specified by <tool>.
>>>
>>> I do not see how it is an improvement to drop the most common ones.
>>> People sometimes read documentation without having an access to
>>> shell to run "cmd --tool-help", and a list of handful of well known
>>> ones would serve as a good hint to let the reader know the kind of
>>> commands the front-end is capable of spawning, which in turn help
>>> such a reader to imagine how the command is used to judge if it is
>>> something the reader wants to use.
>>
>> I don't agree. What "most common ones" are depends on your platform
>> and is sort of subjective. So it should be either all or non here.
>
> Let's please leave this section as-is.
Yes.  

There are only five or six classes of environment that matter in the real world for the purpose of giving "well known" examples: Windows, MacOS X, Gnome, KDE and Linux terminal. By picking a representative one from each and listing them, the end result would have at least one that people from various platforms have _heard of_ and can guess what they do. The "most common" is secondary, and "well known" is the keyword. "Can guess what they do" is the point of the phrase "Valid values _include_". Even if you are hard-core Emacs user, it is likely that you've heard of vimdiff---and in that case, including vimdiff would be enough. Likewise for showing kompare to Gnome users.

Unlike POSIXy folks, where IRIX or Solaris users are likely to have heard of Gnome tools even if they do not use the environment on their platforms, Windows users tend to be isolated bunch, so it would not hurt to include at least one well-known Windows-only tool in the list.

> Yes, indeed, it is arbitrary.  It does have some merit, though--it is
> also a good compromise between unhelpful (listing nothing) and painful
> to maintain (listing everything).
Here is a v2, with documentation updates.
-- >8 --
Subject: [PATCH] mergetool: support --tool-help option like difftool does

This way we do not have to risk the list of tools going out of sync between the implementation and the documentation.

In the same spirit as bf73fc2 (difftool: print list of valid tools with '--tool-help', 2012-03-29), trim the list of merge backends in the documentation. We do not want to have a complete of valid tools there; we only want a list to help people guess what kind of things the tools that can be specified there, and refer them to --tool-help for a complete list.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/git-mergetool.txt |  6 +++---
 git-mergetool--lib.sh           |  6 +++++-
 git-mergetool.sh                | 42 ++++++++++++++++++++++++++++++++++++++++-
 3 files changed, 49 insertions(+), 5 deletions(-)
Show changes to 3 files +49 −5

Documentation/git-mergetool.txt, git-mergetool--lib.sh, git-mergetool.sh

diff --git a/Documentation/git-mergetool.txt b/Documentation/git-mergetool.txt
index 2a49de7..d1e08d2 100644
--- a/Documentation/git-mergetool.txt
+++ b/Documentation/git-mergetool.txt
@@ -27,9 +27,9 @@ OPTIONS
 -t <tool>::
 --tool=<tool>::
 	Use the merge resolution program specified by <tool>.
-	Valid merge tools are:
-	araxis, bc3, diffuse, ecmerge, emerge, gvimdiff, kdiff3,
-	meld, opendiff, p4merge, tkdiff, tortoisemerge, vimdiff and xxdiff.
+	Valid values include emerge, gvimdiff, kdiff3,
+	meld, vimdiff, and tortoisemerge. Run `git mergetool --tool-help`
+	for the list of valid <tool> settings.
 +
 If a merge resolution program is not specified, 'git mergetool'
 will use the configuration variable `merge.tool`.  If the
diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
index ed630b2..f730253 100644
--- a/git-mergetool--lib.sh
+++ b/git-mergetool--lib.sh
@@ -111,7 +111,7 @@ run_merge_tool () {
 	return $status
 }
 
-guess_merge_tool () {
+list_merge_tool_candidates () {
 	if merge_mode
 	then
 		tools="tortoisemerge"
@@ -136,6 +136,10 @@ guess_merge_tool () {
 		tools="$tools emerge vimdiff"
 		;;
 	esac
+}
+
+guess_merge_tool () {
+	list_merge_tool_candidates
 	echo >&2 "merge tool candidates: $tools"
 
 	# Loop over each candidate and stop when a valid merge tool is found.
diff --git a/git-mergetool.sh b/git-mergetool.sh
index a9f23f7..0db0c44 100755
--- a/git-mergetool.sh
+++ b/git-mergetool.sh
@@ -8,7 +8,7 @@
 # at the discretion of Junio C Hamano.
 #
 
-USAGE='[--tool=tool] [-y|--no-prompt|--prompt] [file to merge] ...'
+USAGE='[--tool=tool] [--tool-help] [-y|--no-prompt|--prompt] [file to merge] ...'
 SUBDIRECTORY_OK=Yes
 OPTIONS_SPEC=
 TOOL_MODE=merge
@@ -284,11 +284,51 @@ merge_file () {
     return 0
 }
 
+show_tool_help () {
+	TOOL_MODE=merge
+	list_merge_tool_candidates
+	unavailable= available= LF='
+'
+	for i in $tools
+	do
+		merge_tool_path=$(translate_merge_tool_path "$i")
+		if type "$merge_tool_path" >/dev/null 2>&1
+		then
+			available="$available$i$LF"
+		else
+			unavailable="$unavailable$i$LF"
+		fi
+	done
+	if test -n "$available"
+	then
+		echo "'git mergetool --tool=<tool>' may be set to one of the following:"
+		echo "$available" | sort | sed -e 's/^/	/'
+	else
+		echo "No suitable tool for 'git mergetool --tool=<tool>' found."
+	fi
+	if test -n "$unavailable"
+	then
+		echo
+		echo 'The following tools are valid, but not currently available:'
+		echo "$unavailable" | sort | sed -e 's/^/	/'
+	fi
+	if test -n "$unavailable$available"
+	then
+		echo
+		echo "Some of the tools listed above only work in a windowed"
+		echo "environment. If run in a terminal-only session, they will fail."
+	fi
+	exit 0
+}
+
 prompt=$(git config --bool mergetool.prompt || echo true)
 
 while test $# != 0
 do
     case "$1" in
+	--tool-help)
+		show_tool_help
+		;;
 	-t|--tool*)
 	    case "$#,$1" in
 		*,*=*)
-- 
1.7.11.3.339.gbdbf398
Sebastian Schuberth· Jul 23, 2012, 21:27 UTC · re: Junio C Hamano · lore

Re: [PATCH] mergetool: support --tool-help option like difftool does

On Mon, Jul 23, 2012 at 11:16 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 6 quoted lines
> There are only five or six classes of environment that matter in the
> real world for the purpose of giving "well known" examples: Windows,
> MacOS X, Gnome, KDE and Linux terminal.  By picking a representative
> one from each and listing them, the end result would have at least
> one that people from various platforms have _heard of_ and can guess
> what they do.  The "most common" is secondary, and "well known" is

I completely agree with this. So we should take the chance and add a Windows representative to the list of difftools, no?

Show 5 quoted lines
> Unlike POSIXy folks, where IRIX or Solaris users are likely to have
> heard of Gnome tools even if they do not use the environment on
> their platforms, Windows users tend to be isolated bunch, so it
> would not hurt to include at least one well-known Windows-only tool
> in the list.

Heh. I believe POSIX folks are no less isolated. (How many Windows-only tools would you recognize by name?) They're just isolated in a bigger world ;-)

> Here is a v2, with documentation updates.

This drops the explicit mention of --tool-help as an option in the documentation compared to my patch. Do you want to keep --tool-help being mentioned inline as part of the --tool option documentation only?

-- 
Sebastian Schuberth
Junio C Hamano· Jul 23, 2012, 22:31 UTC · re: Sebastian Schuberth · lore

Re: [PATCH] mergetool: support --tool-help option like difftool does

Sebastian Schuberth <sschuberth@gmail.com> writes:
Show 11 quoted lines
> On Mon, Jul 23, 2012 at 11:16 PM, Junio C Hamano <gitster@pobox.com> wrote:
>
>> There are only five or six classes of environment that matter in the
>> real world for the purpose of giving "well known" examples: Windows,
>> MacOS X, Gnome, KDE and Linux terminal.  By picking a representative
>> one from each and listing them, the end result would have at least
>> one that people from various platforms have _heard of_ and can guess
>> what they do.  The "most common" is secondary, and "well known" is
>
> I completely agree with this. So we should take the chance and add a
> Windows representative to the list of difftools, no?

I do not care very deeply either way. I am more interested in seeing --tool-list options supported by both so that we can get rid of hardcoded list from the completion script.

> This drops the explicit mention of --tool-help as an option in the
> documentation compared to my patch. Do you want to keep --tool-help
> being mentioned inline as part of the --tool option documentation
> only?

While I do not think having one would hurt that much, I do not think a separate entry would add much value either, so I chose not to clutter the documentation further. The "--tool=<tool>" entry is enough hint to draw eyes of readers who want to find out what kind of backends are supported, and --tool-help is mentioned there quite prominently.

Junio C Hamano· Aug 23, 2012, 05:33 UTC · re: Junio C Hamano · lore

Re*: mergetool: support --tool-help option like difftool does

Junio C Hamano <gitster@pobox.com> writes:
Show 16 quoted lines
> This way we do not have to risk the list of tools go out of sync
> between the implementation and the documentation.
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
> Junio C Hamano <gitster@pobox.com> writes:
>
>>> +--tool-help::
>>> +	List the supported and available diff tools.
>>
>> This part is a good addition (but it already is mentioned in the
>> description of --tool above, so it is more or less a "Meh").
>
> I noticed that the main while loop has style violations in its
> case/esac, but I left it as-is.  Reindenting it would be a low
> hanging fruit janitor patch that would be a separate topic.

After hinting a low-hanging-fruit that would be an easy way for new people to dip their toes, I saw no takers for one month, so I ended up doing it myself.

-- >8 --
Subject: mergetool: style fixes

This script is one of the sizeable ones that tempted people to copy its "neibouring style" in their new code, but was littered with styles incompatible with our style guide.

 - use one tab, not four spaces, per indent level;
 - long lines can be wrapped after '|', '&&', or '||' for
   readability.
 - structures like "if .. then .. else .. fi", "while .. do .. done"
   are split into lines in such a way that does not require
   unnecessary semicolon.
 - case, esac and case-arms align at the same column.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 * The change would be easier to see if the reader runs these
   command before and after applying this patch:
    $ git diff -w git-mergetool.sh
    $ git grep -e '^[\t]* ' -e '; *then' -e '; do' git-mergetool.sh
 git-mergetool.sh | 581 +++++++++++++++++++++++++++++--------------------------
 1 file changed, 308 insertions(+), 273 deletions(-)
Show changes to diff +308 −273
diff --git i/git-mergetool.sh w/git-mergetool.sh
index 0db0c44..c50e18a 100755
--- i/git-mergetool.sh
+++ w/git-mergetool.sh
@@ -18,270 +18,301 @@ require_work_tree
 
 # Returns true if the mode reflects a symlink
 is_symlink () {
-    test "$1" = 120000
+	test "$1" = 120000
 }
 
 is_submodule () {
-    test "$1" = 160000
+	test "$1" = 160000
 }
 
 local_present () {
-    test -n "$local_mode"
+	test -n "$local_mode"
 }
 
 remote_present () {
-    test -n "$remote_mode"
+	test -n "$remote_mode"
 }
 
 base_present () {
-    test -n "$base_mode"
+	test -n "$base_mode"
 }
 
 cleanup_temp_files () {
-    if test "$1" = --save-backup ; then
-	rm -rf -- "$MERGED.orig"
-	test -e "$BACKUP" && mv -- "$BACKUP" "$MERGED.orig"
-	rm -f -- "$LOCAL" "$REMOTE" "$BASE"
-    else
-	rm -f -- "$LOCAL" "$REMOTE" "$BASE" "$BACKUP"
-    fi
+	if test "$1" = --save-backup
+	then
+		rm -rf -- "$MERGED.orig"
+		test -e "$BACKUP" && mv -- "$BACKUP" "$MERGED.orig"
+		rm -f -- "$LOCAL" "$REMOTE" "$BASE"
+	else
+		rm -f -- "$LOCAL" "$REMOTE" "$BASE" "$BACKUP"
+	fi
 }
 
 describe_file () {
-    mode="$1"
-    branch="$2"
-    file="$3"
-
-    printf "  {%s}: " "$branch"
-    if test -z "$mode"; then
-	echo "deleted"
-    elif is_symlink "$mode" ; then
-	echo "a symbolic link -> '$(cat "$file")'"
-    elif is_submodule "$mode" ; then
-	echo "submodule commit $file"
-    else
-	if base_present; then
-	    echo "modified file"
+	mode="$1"
+	branch="$2"
+	file="$3"
+
+	printf "  {%s}: " "$branch"
+	if test -z "$mode"
+	then
+		echo "deleted"
+	elif is_symlink "$mode"
+	then
+		echo "a symbolic link -> '$(cat "$file")'"
+	elif is_submodule "$mode"
+	then
+		echo "submodule commit $file"
+	elif base_present
+	then
+		echo "modified file"
 	else
-	    echo "created file"
+		echo "created file"
 	fi
-    fi
 }
 
-
 resolve_symlink_merge () {
-    while true; do
-	printf "Use (l)ocal or (r)emote, or (a)bort? "
-	read ans || return 1
-	case "$ans" in
-	    [lL]*)
-		git checkout-index -f --stage=2 -- "$MERGED"
-		git add -- "$MERGED"
-		cleanup_temp_files --save-backup
-		return 0
-		;;
-	    [rR]*)
-		git checkout-index -f --stage=3 -- "$MERGED"
-		git add -- "$MERGED"
-		cleanup_temp_files --save-backup
-		return 0
-		;;
-	    [aA]*)
-		return 1
-		;;
-	    esac
+	while true
+	do
+		printf "Use (l)ocal or (r)emote, or (a)bort? "
+		read ans || return 1
+		case "$ans" in
+		[lL]*)
+			git checkout-index -f --stage=2 -- "$MERGED"
+			git add -- "$MERGED"
+			cleanup_temp_files --save-backup
+			return 0
+			;;
+		[rR]*)
+			git checkout-index -f --stage=3 -- "$MERGED"
+			git add -- "$MERGED"
+			cleanup_temp_files --save-backup
+			return 0
+			;;
+		[aA]*)
+			return 1
+			;;
+		esac
 	done
 }
 
 resolve_deleted_merge () {
-    while true; do
-	if base_present; then
-	    printf "Use (m)odified or (d)eleted file, or (a)bort? "
-	else
-	    printf "Use (c)reated or (d)eleted file, or (a)bort? "
-	fi
-	read ans || return 1
-	case "$ans" in
-	    [mMcC]*)
-		git add -- "$MERGED"
-		cleanup_temp_files --save-backup
-		return 0
-		;;
-	    [dD]*)
-		git rm -- "$MERGED" > /dev/null
-		cleanup_temp_files
-		return 0
-		;;
-	    [aA]*)
-		return 1
-		;;
-	    esac
+	while true
+	do
+		if base_present
+		then
+			printf "Use (m)odified or (d)eleted file, or (a)bort? "
+		else
+			printf "Use (c)reated or (d)eleted file, or (a)bort? "
+		fi
+		read ans || return 1
+		case "$ans" in
+		[mMcC]*)
+			git add -- "$MERGED"
+			cleanup_temp_files --save-backup
+			return 0
+			;;
+		[dD]*)
+			git rm -- "$MERGED" > /dev/null
+			cleanup_temp_files
+			return 0
+			;;
+		[aA]*)
+			return 1
+			;;
+		esac
 	done
 }
 
 resolve_submodule_merge () {
-    while true; do
-	printf "Use (l)ocal or (r)emote, or (a)bort? "
-	read ans || return 1
-	case "$ans" in
-	    [lL]*)
-		if ! local_present; then
-		    if test -n "$(git ls-tree HEAD -- "$MERGED")"; then
-			# Local isn't present, but it's a subdirectory
-			git ls-tree --full-name -r HEAD -- "$MERGED" | git update-index --index-info || exit $?
-		    else
-			test -e "$MERGED" && mv -- "$MERGED" "$BACKUP"
-			git update-index --force-remove "$MERGED"
+	while true
+	do
+		printf "Use (l)ocal or (r)emote, or (a)bort? "
+		read ans || return 1
+		case "$ans" in
+		[lL]*)
+			if ! local_present
+			then
+				if test -n "$(git ls-tree HEAD -- "$MERGED")"
+				then
+					# Local isn't present, but it's a subdirectory
+					git ls-tree --full-name -r HEAD -- "$MERGED" |
+					git update-index --index-info || exit $?
+				else
+					test -e "$MERGED" && mv -- "$MERGED" "$BACKUP"
+					git update-index --force-remove "$MERGED"
+					cleanup_temp_files --save-backup
+				fi
+			elif is_submodule "$local_mode"
+			then
+				stage_submodule "$MERGED" "$local_sha1"
+			else
+				git checkout-index -f --stage=2 -- "$MERGED"
+				git add -- "$MERGED"
+			fi
+			return 0
+			;;
+		[rR]*)
+			if ! remote_present
+			then
+				if test -n "$(git ls-tree MERGE_HEAD -- "$MERGED")"
+				then
+					# Remote isn't present, but it's a subdirectory
+					git ls-tree --full-name -r MERGE_HEAD -- "$MERGED" |
+					git update-index --index-info || exit $?
+				else
+					test -e "$MERGED" && mv -- "$MERGED" "$BACKUP"
+					git update-index --force-remove "$MERGED"
+				fi
+			elif is_submodule "$remote_mode"
+			then
+				! is_submodule "$local_mode" &&
+				test -e "$MERGED" &&
+				mv -- "$MERGED" "$BACKUP"
+				stage_submodule "$MERGED" "$remote_sha1"
+			else
+				test -e "$MERGED" && mv -- "$MERGED" "$BACKUP"
+				git checkout-index -f --stage=3 -- "$MERGED"
+				git add -- "$MERGED"
+			fi
 			cleanup_temp_files --save-backup
-		    fi
-		elif is_submodule "$local_mode"; then
-		    stage_submodule "$MERGED" "$local_sha1"
-		else
-		    git checkout-index -f --stage=2 -- "$MERGED"
-		    git add -- "$MERGED"
-		fi
-		return 0
-		;;
-	    [rR]*)
-		if ! remote_present; then
-		    if test -n "$(git ls-tree MERGE_HEAD -- "$MERGED")"; then
-			# Remote isn't present, but it's a subdirectory
-			git ls-tree --full-name -r MERGE_HEAD -- "$MERGED" | git update-index --index-info || exit $?
-		    else
-			test -e "$MERGED" && mv -- "$MERGED" "$BACKUP"
-			git update-index --force-remove "$MERGED"
-		    fi
-		elif is_submodule "$remote_mode"; then
-		    ! is_submodule "$local_mode" && test -e "$MERGED" && mv -- "$MERGED" "$BACKUP"
-		    stage_submodule "$MERGED" "$remote_sha1"
-		else
-		    test -e "$MERGED" && mv -- "$MERGED" "$BACKUP"
-		    git checkout-index -f --stage=3 -- "$MERGED"
-		    git add -- "$MERGED"
-		fi
-		cleanup_temp_files --save-backup
-		return 0
-		;;
-	    [aA]*)
-		return 1
-		;;
-	    esac
+			return 0
+			;;
+		[aA]*)
+			return 1
+			;;
+		esac
 	done
 }
 
 stage_submodule () {
-    path="$1"
-    submodule_sha1="$2"
-    mkdir -p "$path" || die "fatal: unable to create directory for module at $path"
-    # Find $path relative to work tree
-    work_tree_root=$(cd_to_toplevel && pwd)
-    work_rel_path=$(cd "$path" && GIT_WORK_TREE="${work_tree_root}" git rev-parse --show-prefix)
-    test -n "$work_rel_path" || die "fatal: unable to get path of module $path relative to work tree"
-    git update-index --add --replace --cacheinfo 160000 "$submodule_sha1" "${work_rel_path%/}" || die
+	path="$1"
+	submodule_sha1="$2"
+	mkdir -p "$path" ||
+	die "fatal: unable to create directory for module at $path"
+	# Find $path relative to work tree
+	work_tree_root=$(cd_to_toplevel && pwd)
+	work_rel_path=$(cd "$path" &&
+		GIT_WORK_TREE="${work_tree_root}" git rev-parse --show-prefix
+	)
+	test -n "$work_rel_path" ||
+	die "fatal: unable to get path of module $path relative to work tree"
+	git update-index --add --replace --cacheinfo 160000 "$submodule_sha1" "${work_rel_path%/}" || die
 }
 
 checkout_staged_file () {
-    tmpfile=$(expr \
-	    "$(git checkout-index --temp --stage="$1" "$2" 2>/dev/null)" \
-	    : '\([^	]*\)	')
-
-    if test $? -eq 0 -a -n "$tmpfile" ; then
-	mv -- "$(git rev-parse --show-cdup)$tmpfile" "$3"
-    else
-	>"$3"
-    fi
+	tmpfile=$(expr \
+		"$(git checkout-index --temp --stage="$1" "$2" 2>/dev/null)" \
+		: '\([^	]*\)	')
+
+	if test $? -eq 0 -a -n "$tmpfile"
+	then
+		mv -- "$(git rev-parse --show-cdup)$tmpfile" "$3"
+	else
+		>"$3"
+	fi
 }
 
 merge_file () {
-    MERGED="$1"
+	MERGED="$1"
 
-    f=$(git ls-files -u -- "$MERGED")
-    if test -z "$f" ; then
-	if test ! -f "$MERGED" ; then
-	    echo "$MERGED: file not found"
-	else
-	    echo "$MERGED: file does not need merging"
+	f=$(git ls-files -u -- "$MERGED")
+	if test -z "$f"
+	then
+		if test ! -f "$MERGED"
+		then
+			echo "$MERGED: file not found"
+		else
+			echo "$MERGED: file does not need merging"
+		fi
+		return 1
 	fi
-	return 1
-    fi
-
-    ext="$$$(expr "$MERGED" : '.*\(\.[^/]*\)$')"
-    BACKUP="./$MERGED.BACKUP.$ext"
-    LOCAL="./$MERGED.LOCAL.$ext"
-    REMOTE="./$MERGED.REMOTE.$ext"
-    BASE="./$MERGED.BASE.$ext"
-
-    base_mode=$(git ls-files -u -- "$MERGED" | awk '{if ($3==1) print $1;}')
-    local_mode=$(git ls-files -u -- "$MERGED" | awk '{if ($3==2) print $1;}')
-    remote_mode=$(git ls-files -u -- "$MERGED" | awk '{if ($3==3) print $1;}')
-
-    if is_submodule "$local_mode" || is_submodule "$remote_mode"; then
-	echo "Submodule merge conflict for '$MERGED':"
-	local_sha1=$(git ls-files -u -- "$MERGED" | awk '{if ($3==2) print $2;}')
-	remote_sha1=$(git ls-files -u -- "$MERGED" | awk '{if ($3==3) print $2;}')
-	describe_file "$local_mode" "local" "$local_sha1"
-	describe_file "$remote_mode" "remote" "$remote_sha1"
-	resolve_submodule_merge
-	return
-    fi
-
-    mv -- "$MERGED" "$BACKUP"
-    cp -- "$BACKUP" "$MERGED"
-
-    checkout_staged_file 1 "$MERGED" "$BASE"
-    checkout_staged_file 2 "$MERGED" "$LOCAL"
-    checkout_staged_file 3 "$MERGED" "$REMOTE"
-
-    if test -z "$local_mode" -o -z "$remote_mode"; then
-	echo "Deleted merge conflict for '$MERGED':"
-	describe_file "$local_mode" "local" "$LOCAL"
-	describe_file "$remote_mode" "remote" "$REMOTE"
-	resolve_deleted_merge
-	return
-    fi
 
-    if is_symlink "$local_mode" || is_symlink "$remote_mode"; then
-	echo "Symbolic link merge conflict for '$MERGED':"
+	ext="$$$(expr "$MERGED" : '.*\(\.[^/]*\)$')"
+	BACKUP="./$MERGED.BACKUP.$ext"
+	LOCAL="./$MERGED.LOCAL.$ext"
+	REMOTE="./$MERGED.REMOTE.$ext"
+	BASE="./$MERGED.BASE.$ext"
+
+	base_mode=$(git ls-files -u -- "$MERGED" | awk '{if ($3==1) print $1;}')
+	local_mode=$(git ls-files -u -- "$MERGED" | awk '{if ($3==2) print $1;}')
+	remote_mode=$(git ls-files -u -- "$MERGED" | awk '{if ($3==3) print $1;}')
+
+	if is_submodule "$local_mode" || is_submodule "$remote_mode"
+	then
+		echo "Submodule merge conflict for '$MERGED':"
+		local_sha1=$(git ls-files -u -- "$MERGED" | awk '{if ($3==2) print $2;}')
+		remote_sha1=$(git ls-files -u -- "$MERGED" | awk '{if ($3==3) print $2;}')
+		describe_file "$local_mode" "local" "$local_sha1"
+		describe_file "$remote_mode" "remote" "$remote_sha1"
+		resolve_submodule_merge
+		return
+	fi
+
+	mv -- "$MERGED" "$BACKUP"
+	cp -- "$BACKUP" "$MERGED"
+
+	checkout_staged_file 1 "$MERGED" "$BASE"
+	checkout_staged_file 2 "$MERGED" "$LOCAL"
+	checkout_staged_file 3 "$MERGED" "$REMOTE"
+
+	if test -z "$local_mode" -o -z "$remote_mode"
+	then
+		echo "Deleted merge conflict for '$MERGED':"
+		describe_file "$local_mode" "local" "$LOCAL"
+		describe_file "$remote_mode" "remote" "$REMOTE"
+		resolve_deleted_merge
+		return
+	fi
+
+	if is_symlink "$local_mode" || is_symlink "$remote_mode"
+	then
+		echo "Symbolic link merge conflict for '$MERGED':"
+		describe_file "$local_mode" "local" "$LOCAL"
+		describe_file "$remote_mode" "remote" "$REMOTE"
+		resolve_symlink_merge
+		return
+	fi
+
+	echo "Normal merge conflict for '$MERGED':"
 	describe_file "$local_mode" "local" "$LOCAL"
 	describe_file "$remote_mode" "remote" "$REMOTE"
-	resolve_symlink_merge
-	return
-    fi
-
-    echo "Normal merge conflict for '$MERGED':"
-    describe_file "$local_mode" "local" "$LOCAL"
-    describe_file "$remote_mode" "remote" "$REMOTE"
-    if "$prompt" = true; then
-	printf "Hit return to start merge resolution tool (%s): " "$merge_tool"
-	read ans || return 1
-    fi
-
-    if base_present; then
-	    present=true
-    else
-	    present=false
-    fi
-
-    if ! run_merge_tool "$merge_tool" "$present"; then
-	echo "merge of $MERGED failed" 1>&2
-	mv -- "$BACKUP" "$MERGED"
-
-	if test "$merge_keep_temporaries" = "false"; then
-	    cleanup_temp_files
+	if "$prompt" = true
+	then
+		printf "Hit return to start merge resolution tool (%s): " "$merge_tool"
+		read ans || return 1
 	fi
 
-	return 1
-    fi
+	if base_present
+	then
+		present=true
+	else
+		present=false
+	fi
+
+	if ! run_merge_tool "$merge_tool" "$present"
+	then
+		echo "merge of $MERGED failed" 1>&2
+		mv -- "$BACKUP" "$MERGED"
+
+		if test "$merge_keep_temporaries" = "false"
+		then
+			cleanup_temp_files
+		fi
 
-    if test "$merge_keep_backup" = "true"; then
-	mv -- "$BACKUP" "$MERGED.orig"
-    else
-	rm -- "$BACKUP"
-    fi
+		return 1
+	fi
 
-    git add -- "$MERGED"
-    cleanup_temp_files
-    return 0
+	if test "$merge_keep_backup" = "true"
+	then
+		mv -- "$BACKUP" "$MERGED.orig"
+	else
+		rm -- "$BACKUP"
+	fi
+
+	git add -- "$MERGED"
+	cleanup_temp_files
+	return 0
 }
 
 show_tool_help () {
@@ -325,61 +356,61 @@ prompt=$(git config --bool mergetool.prompt || echo true)
 
 while test $# != 0
 do
-    case "$1" in
+	case "$1" in
 	--tool-help)
 		show_tool_help
 		;;
 	-t|--tool*)
-	    case "$#,$1" in
+		case "$#,$1" in
 		*,*=*)
-		    merge_tool=$(expr "z$1" : 'z-[^=]*=\(.*\)')
-		    ;;
+			merge_tool=$(expr "z$1" : 'z-[^=]*=\(.*\)')
+			;;
 		1,*)
-		    usage ;;
+			usage ;;
 		*)
-		    merge_tool="$2"
-		    shift ;;
-	    esac
-	    ;;
+			merge_tool="$2"
+			shift ;;
+		esac
+		;;
 	-y|--no-prompt)
-	    prompt=false
-	    ;;
+		prompt=false
+		;;
 	--prompt)
-	    prompt=true
-	    ;;
+		prompt=true
+		;;
 	--)
-	    shift
-	    break
-	    ;;
+		shift
+		break
+		;;
 	-*)
-	    usage
-	    ;;
-	*)
-	    break
-	    ;;
-    esac
-    shift
-done
-
-prompt_after_failed_merge() {
-    while true; do
-	printf "Continue merging other unresolved paths (y/n) ? "
-	read ans || return 1
-	case "$ans" in
-
-	    [yY]*)
-		return 0
+		usage
 		;;
-
-	    [nN]*)
-		return 1
+	*)
+		break
 		;;
 	esac
-    done
+	shift
+done
+
+prompt_after_failed_merge () {
+	while true
+	do
+		printf "Continue merging other unresolved paths (y/n) ? "
+		read ans || return 1
+		case "$ans" in
+		[yY]*)
+			return 0
+			;;
+		[nN]*)
+			return 1
+			;;
+		esac
+	done
 }
 
-if test -z "$merge_tool"; then
-    merge_tool=$(get_merge_tool "$merge_tool") || exit
+if test -z "$merge_tool"
+then
+	merge_tool=$(get_merge_tool "$merge_tool") || exit
 fi
 merge_keep_backup="$(git config --bool mergetool.keepBackup || echo true)"
 merge_keep_temporaries="$(git config --bool mergetool.keepTemporaries || echo false)"
@@ -388,22 +419,24 @@ last_status=0
 rollup_status=0
 files=
 
-if test $# -eq 0 ; then
-    cd_to_toplevel
+if test $# -eq 0
+then
+	cd_to_toplevel
 
-    if test -e "$GIT_DIR/MERGE_RR"
-    then
-	files=$(git rerere remaining)
-    else
-	files=$(git ls-files -u | sed -e 's/^[^	]*	//' | sort -u)
-    fi
+	if test -e "$GIT_DIR/MERGE_RR"
+	then
+		files=$(git rerere remaining)
+	else
+		files=$(git ls-files -u | sed -e 's/^[^	]*	//' | sort -u)
+	fi
 else
-    files=$(git ls-files -u -- "$@" | sed -e 's/^[^	]*	//' | sort -u)
+	files=$(git ls-files -u -- "$@" | sed -e 's/^[^	]*	//' | sort -u)
 fi
 
-if test -z "$files" ; then
-    echo "No files need merging"
-    exit 0
+if test -z "$files"
+then
+	echo "No files need merging"
+	exit 0
 fi
 
 printf "Merging:\n"
@@ -413,15 +446,17 @@ IFS='
 '
 for i in $files
 do
-    if test $last_status -ne 0; then
-	prompt_after_failed_merge || exit 1
-    fi
-    printf "\n"
-    merge_file "$i"
-    last_status=$?
-    if test $last_status -ne 0; then
-	rollup_status=1
-    fi
+	if test $last_status -ne 0
+	then
+		prompt_after_failed_merge || exit 1
+	fi
+	printf "\n"
+	merge_file "$i"
+	last_status=$?
+	if test $last_status -ne 0
+	then
+		rollup_status=1
+	fi
 done
 
 exit $rollup_status
David Aguilar· Aug 23, 2012, 07:39 UTC · re: Junio C Hamano · lore

Re: Re*: mergetool: support --tool-help option like difftool does

On Wed, Aug 22, 2012 at 10:33 PM, Junio C Hamano <junio@pobox.com> wrote:
Show 22 quoted lines
> Junio C Hamano <gitster@pobox.com> writes:
>
>> This way we do not have to risk the list of tools go out of sync
>> between the implementation and the documentation.
>>
>> Signed-off-by: Junio C Hamano <gitster@pobox.com>
>> ---
>> Junio C Hamano <gitster@pobox.com> writes:
>>
>>>> +--tool-help::
>>>> +   List the supported and available diff tools.
>>>
>>> This part is a good addition (but it already is mentioned in the
>>> description of --tool above, so it is more or less a "Meh").
>>
>> I noticed that the main while loop has style violations in its
>> case/esac, but I left it as-is.  Reindenting it would be a low
>> hanging fruit janitor patch that would be a separate topic.
>
> After hinting a low-hanging-fruit that would be an easy way for new
> people to dip their toes, I saw no takers for one month, so I ended
> up doing it myself.

My bad, I didn't catch this and should have, especially so because I had started on style fixing mergetool last week but ditched it; I assumed it would be unwanted. I will read more carefully.

Just please don't tell Charles about it ;-)
(I wanted to do this a long ago but at the time we didn't have
 the style guide for shell, and the time was perhaps not right...)
I am of course in favor of this patch.
While on the mergetool topic...

Would the ability to resolve the various merge situations using the command-line be a wanted addition?

This would let a submodule or deleted/modified encountering user do something like:

$ git mergetool --theirs -- submodule
...and not have to remember the various git commands that it runs.

This could touch just the parts of mergetool that require stdin, but probably should work for everything.

It seems like exposing the guts of some of these mergetool functions via command-line flags could be helpful, but I'm not sure if that's overloading mergetool with too many features. What do you think?

-- 
David
Junio C Hamano· Aug 23, 2012, 17:39 UTC · re: David Aguilar · lore

Re: Re*: mergetool: support --tool-help option like difftool does

David Aguilar <davvid@gmail.com> writes:
Show 5 quoted lines
>> After hinting a low-hanging-fruit that would be an easy way for new
>> people to dip their toes, I saw no takers for one month, so I ended
>> up doing it myself.
>
> My bad,...

Not yours. These hints I drop from time to time are meant for eager new people who want to dip their toes to the development (we don't do "assignments" but these are the closest that we have); you no longer quite qualify as "new" ;-)

> While on the mergetool topic...

Now we are not talking about "dip their toes" low hanging fruit anymore ;-)

Show 9 quoted lines
> Would the ability to resolve the various merge situations using
> the command-line be a wanted addition?
>
> This would let a submodule or deleted/modified encountering
> user do something like:
>
> $ git mergetool --theirs -- submodule
>
> ...and not have to remember the various git commands that it runs.

Does it have to run various git commands? For a normal path, all it needs to do is "git checkout --theirs $path", no?

What does it mean to resolve a conflicted merge of a gitlink to take "theirs"? We obviously would want to point the resolved gitlink at the submodule commit their side wants in the resulting index but what, if any, should we do to the submodule itself?

Stepping back a bit, if there is no conflict, and as a result of a clean merge (this applies to the case where you check out another branch that has different commit at the submodule path), if gitlink changed to point at a different commit in the submodule, what should happen?

If you start from a clean working tree, with your gitlink pointing at the commit that matches HEAD in the submodule, and if the working tree of the submodule does not have any local modification, it may be ideal to check out the new commit in the submodule (are there cases where "git checkout other_branch" in the superproject does not want to touch the submodule working tree?).

There are cases where it is not possible; checking out the new commit in the submodule working tree may not succeed due to local modifications. But is that kind of complication limited to the merge resolution case? Isn't it shared with normal "switching branches" case as well?

If so, it could be that your "git mergetool --theirs -- submodule" is working around a wrong problem, and the right solution may be to make "git checkout --theirs -- $path" to always do an appropriate thing regardless of what kind of object $path is, no?

David Aguilar· Aug 24, 2012, 08:31 UTC · re: Junio C Hamano · lore

Re: Re*: mergetool: support --tool-help option like difftool does

On Thu, Aug 23, 2012 at 10:39 AM, Junio C Hamano <gitster@pobox.com> wrote:
Show 42 quoted lines
> David Aguilar <davvid@gmail.com> writes:
>> Would the ability to resolve the various merge situations using
>> the command-line be a wanted addition?
>>
>> This would let a submodule or deleted/modified encountering
>> user do something like:
>>
>> $ git mergetool --theirs -- submodule
>>
>> ...and not have to remember the various git commands that it runs.
>
> Does it have to run various git commands?  For a normal path, all it
> needs to do is "git checkout --theirs $path", no?
>
> What does it mean to resolve a conflicted merge of a gitlink to take
> "theirs"?  We obviously would want to point the resolved gitlink at
> the submodule commit their side wants in the resulting index but what,
> if any, should we do to the submodule itself?
>
> Stepping back a bit, if there is no conflict, and as a result of a
> clean merge (this applies to the case where you check out another
> branch that has different commit at the submodule path), if gitlink
> changed to point at a different commit in the submodule, what should
> happen?
>
> If you start from a clean working tree, with your gitlink pointing
> at the commit that matches HEAD in the submodule, and if the working
> tree of the submodule does not have any local modification, it may
> be ideal to check out the new commit in the submodule (are there
> cases where "git checkout other_branch" in the superproject does not
> want to touch the submodule working tree?).
>
> There are cases where it is not possible; checking out the new
> commit in the submodule working tree may not succeed due to local
> modifications.  But is that kind of complication limited to the
> merge resolution case?  Isn't it shared with normal "switching
> branches" case as well?
>
> If so, it could be that your "git mergetool --theirs -- submodule"
> is working around a wrong problem, and the right solution may be to
> make "git checkout --theirs -- $path" to always do an appropriate
> thing regardless of what kind of object $path is, no?
True.

Admittedly, I'm not a heavy submodule user myself so I tried crafting the directory vs submodule conflict to see what the usability is like.

checkout --theirs and --ours could learn a few tricks.

When trying to choose the directory in that situation I had to had to "git rm --cached" the submodule path so that git would recognize that there was no longer a conflict.

That makes sense to me because I was following along by reading the mergetool code, but I don't think most users would know to "git rm" a path which they intend to keep.

Afterwards, the .git file is left behind, which could cause problems elsewhere since we really don't want a .git file in that situation. I'm not even sure what to do about the .gitmodules file either.

Here's the script I was using to setup the merge conflict in case anyone wants to get a feel for the usability around this edge case.

Pass --submodule if you want the remote side to have a submodule. By default, the local side has a submodule and the remote side of the merge brings along a directory.

That said, this really isn't an issue, per say. I first poked at it because I noticed that mergetool still needed stdin for some things.

It's certainly an edge case, and perhaps this just shows that mergetool really is the right porcelain for the job when a user runs into these types of conflicts (the stdin thing really isn't an issue).

#!/bin/sh
if test "$1" = "--submodule"
then
	first=with-directory
	second=with-submodule
	echo "local will be a directory, remote will be a submodule"
else
	first=with-submodule
	second=with-directory
	echo "local will be a submodule, remote will be a directory"
fi

repo=submodule-conflict-$$ && echo "creating $repo" && mkdir "$repo" && cd "$repo" && git init && git commit --allow-empty -m'initial' && git checkout -b with-directory master && mkdir the-conflict && touch the-conflict/path && git add the-conflict/path && git commit -m'added path into the-conflict' && git checkout -b with-submodule master && git submodule add ./ the-conflict && git commit -m'added submodule as the-conflict' && git checkout -b tmp-merge master && git merge $first && git merge $second

-- 
David
Jens Lehmann· Aug 26, 2012, 18:38 UTC · re: David Aguilar · lore

Re: Re*: mergetool: support --tool-help option like difftool does

Am 24.08.2012 10:31, schrieb David Aguilar:
Show 45 quoted lines
> On Thu, Aug 23, 2012 at 10:39 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> David Aguilar <davvid@gmail.com> writes:
>>> Would the ability to resolve the various merge situations using
>>> the command-line be a wanted addition?
>>>
>>> This would let a submodule or deleted/modified encountering
>>> user do something like:
>>>
>>> $ git mergetool --theirs -- submodule
>>>
>>> ...and not have to remember the various git commands that it runs.
>>
>> Does it have to run various git commands?  For a normal path, all it
>> needs to do is "git checkout --theirs $path", no?
>>
>> What does it mean to resolve a conflicted merge of a gitlink to take
>> "theirs"?  We obviously would want to point the resolved gitlink at
>> the submodule commit their side wants in the resulting index but what,
>> if any, should we do to the submodule itself?
>>
>> Stepping back a bit, if there is no conflict, and as a result of a
>> clean merge (this applies to the case where you check out another
>> branch that has different commit at the submodule path), if gitlink
>> changed to point at a different commit in the submodule, what should
>> happen?
>>
>> If you start from a clean working tree, with your gitlink pointing
>> at the commit that matches HEAD in the submodule, and if the working
>> tree of the submodule does not have any local modification, it may
>> be ideal to check out the new commit in the submodule (are there
>> cases where "git checkout other_branch" in the superproject does not
>> want to touch the submodule working tree?).
>>
>> There are cases where it is not possible; checking out the new
>> commit in the submodule working tree may not succeed due to local
>> modifications.  But is that kind of complication limited to the
>> merge resolution case?  Isn't it shared with normal "switching
>> branches" case as well?
>>
>> If so, it could be that your "git mergetool --theirs -- submodule"
>> is working around a wrong problem, and the right solution may be to
>> make "git checkout --theirs -- $path" to always do an appropriate
>> thing regardless of what kind of object $path is, no?
> 
> True.
I agree.
Show 5 quoted lines
> Admittedly, I'm not a heavy submodule user myself so I
> tried crafting the directory vs submodule conflict to see
> what the usability is like.
> 
> checkout --theirs and --ours could learn a few tricks.

Me thinks that after I successfully taught checkout to properly recurse into submodule work trees too it should know all those tricks. In my current version it can handle directory/submodule conversions in both directions, this should be sufficient to make "checkout --theirs/--ours" work properly. Note to self: add tests for that.

Show 7 quoted lines
> When trying to choose the directory in that situation
> I had to  had to "git rm --cached" the submodule path
> so that git would recognize that there was no longer a conflict.
> 
> That makes sense to me because I was following along by
> reading the mergetool code, but I don't think most users
> would know to "git rm" a path which they intend to keep.

True. But a submodule recursing checkout would do the right thing here too.

> Afterwards, the .git file is left behind, which could cause
> problems elsewhere since we really don't want a .git file
> in that situation.

Hmm, either you remove all the files tracked in the submodule together with the gitfile or you'll possibly have former submodule files lying around there too. Recursive checkout will do all that for you.

>  I'm not even sure what to do about the
> .gitmodules file either.

Maybe we should issue a warning when the .gitmodules file is not consistent with the [non]existence of a submodule in the work tree?

Show 8 quoted lines
> That said, this really isn't an issue, per say.
> I first poked at it because I noticed that mergetool
> still needed stdin for some things.
> 
> It's certainly an edge case, and perhaps this just shows
> that mergetool really is the right porcelain for the job
> when a user runs into these types of conflicts
> (the stdin thing really isn't an issue).

It looks to me as if the submodule/directory conflict can be handled by a recursive checkout without having to add something to mergetool.

Sebastian Schuberth· Jul 23, 2012, 07:19 UTC · re: Sebastian Schuberth · lore

[PATCH v2 4/5] Make sure to use Araxis' "compare" and not e.g. ImageMagick's

Signed-off-by: Sebastian Schuberth <sschuberth@gmail.com>
---
 mergetools/araxis | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)
Show changes to mergetools/araxis +7 −1
diff --git a/mergetools/araxis b/mergetools/araxis
index 64f97c5..aeba1b9 100644
--- a/mergetools/araxis
+++ b/mergetools/araxis
@@ -16,5 +16,11 @@ merge_cmd () {
 }
 
 translate_merge_tool_path() {
-	echo compare
+	# Make sure to use Araxis' "compare" and not e.g. ImageMagick's.
+	if ls "$(dirname "$(which compare)")"/Araxis* >/dev/null 2>&1
+	then
+		echo compare
+	else
+		echo "$1"
+	fi
 }
-- 
1.7.11.msysgit.2
Junio C Hamano· Jul 23, 2012, 16:50 UTC · re: Sebastian Schuberth · lore

Re: [PATCH v2 4/5] Make sure to use Araxis' "compare" and not e.g. ImageMagick's

Sebastian Schuberth <sschuberth@gmail.com> writes:
Show 16 quoted lines
> Signed-off-by: Sebastian Schuberth <sschuberth@gmail.com>
> ---
>  mergetools/araxis | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/mergetools/araxis b/mergetools/araxis
> index 64f97c5..aeba1b9 100644
> --- a/mergetools/araxis
> +++ b/mergetools/araxis
> @@ -16,5 +16,11 @@ merge_cmd () {
>  }
>  
>  translate_merge_tool_path() {
> -	echo compare
> +	# Make sure to use Araxis' "compare" and not e.g. ImageMagick's.
> +	if ls "$(dirname "$(which compare)")"/Araxis* >/dev/null 2>&1

Use of "which" in scripts that are meant to be portable is a no-no. Some platforms would say "compare is /usr/local/bin/compare" instead of saying "/usr/local/bin/compare".

Show 6 quoted lines
> +	then
> +		echo compare
> +	else
> +		echo "$1"
> +	fi
>  }
Sebastian Schuberth· Jul 23, 2012, 19:37 UTC · re: Junio C Hamano · lore

[PATCH] Make sure to use Araxis' "compare" and not e.g. ImageMagick's

Signed-off-by: Sebastian Schuberth <sschuberth@gmail.com>
---
 mergetools/araxis | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)
Show changes to mergetools/araxis +8 −1
diff --git a/mergetools/araxis b/mergetools/araxis
index 64f97c5..f8899f8 100644
--- a/mergetools/araxis
+++ b/mergetools/araxis
@@ -16,5 +16,12 @@ merge_cmd () {
 }
 
 translate_merge_tool_path() {
-	echo compare
+	# Only accept "compare" in a path that contains "araxis" to not
+	# accidently use e.g. ImageMagick's "compare".
+	if type compare | grep -i araxis >/dev/null 2>&1
+	then
+		echo compare
+	else
+		echo "$1"
+	fi
 }
-- 
1.7.11.msysgit.2
Junio C Hamano· Jul 23, 2012, 20:47 UTC · re: Sebastian Schuberth · lore

Re: [PATCH] Make sure to use Araxis' "compare" and not e.g. ImageMagick's

Sebastian Schuberth <sschuberth@gmail.com> writes:
Show 23 quoted lines
> Signed-off-by: Sebastian Schuberth <sschuberth@gmail.com>
> ---
>  mergetools/araxis | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/mergetools/araxis b/mergetools/araxis
> index 64f97c5..f8899f8 100644
> --- a/mergetools/araxis
> +++ b/mergetools/araxis
> @@ -16,5 +16,12 @@ merge_cmd () {
>  }
>  
>  translate_merge_tool_path() {
> -	echo compare
> +	# Only accept "compare" in a path that contains "araxis" to not
> +	# accidently use e.g. ImageMagick's "compare".
> +	if type compare | grep -i araxis >/dev/null 2>&1
> +	then
> +		echo compare
> +	else
> +		echo "$1"
> +	fi
>  }

I do not use araxis, and I do not know how rigid its installation procedure is to prevent the end user from naming a path that does not have the string in it.

For example, when the user tells it to install in "/home/ss/bin", if it installs its "compare" program in "/home/ss/bin/araxis/compare" without allowing the "/araxis/" part to be stripped away, the above heuristics is sufficiently safe. Otherwise, it is not.

It is unclear from your proposed commit log message what assurance do we have that it is installed under such a path and why the heuristics the patch implements is the sane way forward.

Please convince me.
Sebastian Schuberth· Jul 23, 2012, 21:09 UTC · re: Junio C Hamano · lore

Re: [PATCH] Make sure to use Araxis' "compare" and not e.g. ImageMagick's

On Mon, Jul 23, 2012 at 10:47 PM, Junio C Hamano <gitster@pobox.com> wrote:
> For example, when the user tells it to install in "/home/ss/bin", if
> it installs its "compare" program in "/home/ss/bin/araxis/compare"
> without allowing the "/araxis/" part to be stripped away, the above
> heuristics is sufficiently safe.  Otherwise, it is not.

To the best of my knowledge, Araxis does not enforce any naming convention for the path it gets installed in. That means the user may indeed install the program in a path that does not contain "araxis". I was aware of this when writing the patch, but should have probably made it more clear in the commit message.

> It is unclear from your proposed commit log message what assurance
> do we have that it is installed under such a path and why the
> heuristics the patch implements is the sane way forward.

We have no such assurance. That's why you correctly call it a heuristics after all: it may fail. Personally, I've valued the gain of the patch to not list araxis as an available diff tool by "git difftool --tool-help" when in fact just ImageMagick is in PATH higher than the loss to support araxis installations that are in a path not containung "araxis" but are in PATH.

Please feel free to ignore the patch if you feel the heuristics is not sufficiently safe. I'm currently unable to come up with a safer solution while maintaining portability, i.e. not use "which" or doing rather laborious string parsing on the output of "type".

-- 
Sebastian Schuberth
Junio C Hamano· Jul 23, 2012, 21:24 UTC · re: Sebastian Schuberth · lore

Re: [PATCH] Make sure to use Araxis' "compare" and not e.g. ImageMagick's

Sebastian Schuberth <sschuberth@gmail.com> writes:
> We have no such assurance. That's why you correctly call it a
> heuristics after all

ImageMagick "compare" takes "--version" and says something like this to its standard output:

    $ compare --version
    Version: ImageMagick 6.6.0-4 2012-05-02 Q16
    http://www.imagemagick.org
    Copyright: Copyright (C) 1999-2010 ImageMagick Studio LLC

Does Araxis compare take "--version" and behave in a way that is cheaply controllable? If it opens a GUI window and pops up a dialog that says "Option not understood", then it is not "controllable", but if it quickly dies with "No such option" sent to the standard error output, or sending its version string to the standard output, then we could use something like:

	case "$(compare --version 2>/dev/null)" in
        "Araxis compare version"*)
        	echo compare ;;
	*)
        	echo "$1" ;;
	esac

instead, and that would be more robust than the path based heuristics.

Sebastian Schuberth· Jul 23, 2012, 21:31 UTC · re: Junio C Hamano · lore

Re: [PATCH] Make sure to use Araxis' "compare" and not e.g. ImageMagick's

On Mon, Jul 23, 2012 at 11:24 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Does Araxis compare take "--version" and behave in a way that is
> cheaply controllable?  If it opens a GUI window and pops up a dialog
> that says "Option not understood", then it is not "controllable",

Araxis opens up a GUI dialog with usage info in that case, so it's not controllable, unfortunately.

-- 
Sebastian Schuberth
David Aguilar· Jul 23, 2012, 21:34 UTC · re: Junio C Hamano · lore

Re: [PATCH] Make sure to use Araxis' "compare" and not e.g. ImageMagick's

On Mon, Jul 23, 2012 at 2:24 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 29 quoted lines
> Sebastian Schuberth <sschuberth@gmail.com> writes:
>
>> We have no such assurance. That's why you correctly call it a
>> heuristics after all
>
> ImageMagick "compare" takes "--version" and says something like
> this to its standard output:
>
>     $ compare --version
>     Version: ImageMagick 6.6.0-4 2012-05-02 Q16
>     http://www.imagemagick.org
>     Copyright: Copyright (C) 1999-2010 ImageMagick Studio LLC
>
> Does Araxis compare take "--version" and behave in a way that is
> cheaply controllable?  If it opens a GUI window and pops up a dialog
> that says "Option not understood", then it is not "controllable",
> but if it quickly dies with "No such option" sent to the standard
> error output, or sending its version string to the standard output,
> then we could use something like:
>
>         case "$(compare --version 2>/dev/null)" in
>         "Araxis compare version"*)
>                 echo compare ;;
>         *)
>                 echo "$1" ;;
>         esac
>
> instead, and that would be more robust than the path based
> heuristics.
Araxis compare (the one I have) does not accept --version.

Also, the GraphicsMagick (ImageMagick fork) compare does not have --version, but it does have "compare version":

$ compare version GraphicsMagick 1.3.12 2010-03-08 Q8 http://www.GraphicsMagick.org/ Copyright (C) 2002-2010 GraphicsMagick Group. Additional copyrights and licenses apply to this software. See http://www.GraphicsMagick.org/www/Copyright.html for details. ...

If we care to blacklist *Magick compare, then we may be able to call "compare version" and parse the output looking for "Magic".

I tested ImageMagick compare and it also understands "compare version".

Araxis compare prints nothing when "compare version" is called. Likely because it thinks it has nothing to do. It does the same if I say "compare blahblah", and returns exit status 0. So its output is useless.

It seems like the best we can do is specifically blacklist the *Magick "compare" commands so that they do not show up as false positives. And the only way to identify them is to parse their output, since all commands return status 0 for "compare version".

Another possibility is to parse the output of "compare" (no args) and grep for "merge". The name "Araxis Merge" is never actually printed, but the help text for "-merge" does appear.... yep.. it's a heuristic.

Sebastian, are you testing on Windows? The araxis "compare" I used is OS X. Does "compare version" open a GUI window for you? For me it does not. What about "compare -h", or just "compare" ?

-- 
David
Sebastian Schuberth· Jul 23, 2012, 21:44 UTC · re: David Aguilar · lore

Re: [PATCH] Make sure to use Araxis' "compare" and not e.g. ImageMagick's

On Mon, Jul 23, 2012 at 11:34 PM, David Aguilar <davvid@gmail.com> wrote:
> Sebastian, are you testing on Windows?  The araxis "compare" I used is
> OS X.  Does "compare version" open a GUI window for you?  For me it
> does not.
> What about "compare -h", or just "compare" ?
Yes, I'm testing on Windows, and unfortunately Araxis behaves differently here:
- running "compare version" or "compare blah" prints nothing on the
console, but the GUI opens saying it is unable to open a file
"version" (which it would compare to nothing),
- "compare -h" or just "compare" both open up the usage dialog listing
all valid command line options (because "-h" is invalid).
-- 
Sebastian Schuberth
Andreas Schwab· Jul 23, 2012, 21:26 UTC · re: Sebastian Schuberth · lore

Re: [PATCH] Make sure to use Araxis' "compare" and not e.g. ImageMagick's

Sebastian Schuberth <sschuberth@gmail.com> writes:
> Please feel free to ignore the patch if you feel the heuristics is not
> sufficiently safe. I'm currently unable to come up with a safer
> solution while maintaining portability, i.e. not use "which" or doing
> rather laborious string parsing on the output of "type".
How about looking at "compare --version"?
Andreas.
-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."
Junio C Hamano· Jul 23, 2012, 22:22 UTC · re: Sebastian Schuberth · lore

Re: [PATCH] Make sure to use Araxis' "compare" and not e.g. ImageMagick's

Sebastian Schuberth <sschuberth@gmail.com> writes:
Show 5 quoted lines
> Personally, I've valued the gain of
> the patch to not list araxis as an available diff tool by "git
> difftool --tool-help" when in fact just ImageMagick is in PATH higher
> than the loss to support araxis installations that are in a path not
> containung "araxis" but are in PATH.

I agree that running ImageMagick's compare by accident is one thing we would want to avoid, but once the problem is diagnosed, it is something the user can easily work around by futzing %PATH%, I think.

On the other hand, if the user has bought and installed Araxis, but we incorrectly identify it as unusable, the user has wasted good money and there is no easy recourse as far as I can see in your patch. That is why I wanted to see a reasonable assurance.

If we limit the problem space by special casing Windows installation (e.g. check "uname -s" or something), would it make the problem easier to solve? Perhaps it is much more likely that the path the program is installed in can be safely identified with a call to "type --path compare" (bash is the only shell shipped in msysgit, isn't it?), and its output is likely to contain "/Program Files/Araxis/" as a substring, or something?

Sebastian Schuberth· Jul 23, 2012, 22:39 UTC · re: Junio C Hamano · lore

Re: [PATCH] Make sure to use Araxis' "compare" and not e.g. ImageMagick's

On Tue, Jul 24, 2012 at 12:22 AM, Junio C Hamano <gitster@pobox.com> wrote:
> On the other hand, if the user has bought and installed Araxis, but
> we incorrectly identify it as unusable, the user has wasted good
> money and there is no easy recourse as far as I can see in your
> patch.
Agreed.
Show 7 quoted lines
> If we limit the problem space by special casing Windows installation
> (e.g. check "uname -s" or something), would it make the problem
> easier to solve?  Perhaps it is much more likely that the path the
> program is installed in can be safely identified with a call to
> "type --path compare" (bash is the only shell shipped in msysgit,
> isn't it?), and its output is likely to contain "/Program Files/Araxis/"
> as a substring, or something?

Yes, I could come up with basically a variant of my first version of the patch that is limited to Windows only and uses "type --path" instead of "which", but then again this is too non-generic for my taste. Moreover, as I'm also using Mac OS X, I'd be interested in a solution that works there, too.

I don't see a good (read generic and concise) solution to the issue. I think we should just drop my patch and not waste all of our time on it any more.

-- 
Sebastian Schuberth
Junio C Hamano· Jul 23, 2012, 22:41 UTC · re: Junio C Hamano · lore

Re: [PATCH] Make sure to use Araxis' "compare" and not e.g. ImageMagick's

Junio C Hamano <gitster@pobox.com> writes:
Show 6 quoted lines
> If we limit the problem space by special casing Windows installation
> (e.g. check "uname -s" or something), would it make the problem
> easier to solve?  Perhaps it is much more likely that the path the
> program is installed in can be safely identified with a call to
> "type --path compare" (bash is the only shell shipped in msysgit,
> isn't it?).

E.g. something along the lines of your original patch. I do not know what other commands are typically installed in the same directory as "compare", so it is likely you need to fix the name of the file to let us positively identify "compare" is from the Araxis suite.

 mergetools/araxis | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)
Show changes to mergetools/araxis +14 −1
diff --git a/mergetools/araxis b/mergetools/araxis
index 64f97c5..1180a32 100644
--- a/mergetools/araxis
+++ b/mergetools/araxis
@@ -16,5 +16,18 @@ merge_cmd () {
 }
 
 translate_merge_tool_path() {
-	echo compare
+	case "$BASH_VERSION" in
+	??*)
+		# we can safely use "type --path"
+		if test -f $(dirname "$(type --path compare)")/AraxisMerge
+		then
+			echo compare
+		else
+			echo "$1"
+		fi
+		;;
+	*)
+		echo compare
+		;;
+	esac
 }
Sebastian Schuberth· Jul 23, 2012, 23:09 UTC · re: Junio C Hamano · lore

Re: [PATCH] Make sure to use Araxis' "compare" and not e.g. ImageMagick's

On 24.07.2012 00:41, Junio C Hamano wrote:
> +		if test -f $(dirname "$(type --path compare)")/AraxisMerge
We would need additional quotes around the whole path here as the Windows installation path is usually something like "C:\Program Files\Araxis\Araxis Merge" and contains spaces.
Moreover, "test -f" requires the ".exe" extension to be explicitly present for the file to test. But I'd rather not do that because the test would be specific to Windows then and e.g. not work on Mac OS X. That's why I'd still like to use ls like in my first patch:
 mergetools/araxis | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)
Show changes to mergetools/araxis +14 −1
diff --git a/mergetools/araxis b/mergetools/araxis
index 64f97c5..c406ead 100644
--- a/mergetools/araxis
+++ b/mergetools/araxis
@@ -16,5 +16,18 @@ merge_cmd () {
 }
 
 translate_merge_tool_path() {
-	echo compare
+	case "$BASH_VERSION" in
+	??*)
+		# we can safely use "type --path"
+		if ls "$(dirname "$(type --path compare)")"/Araxis* >/dev/null 2>&1
+		then
+			echo compare
+		else
+			echo "$1"
+		fi
+		;;
+	*)
+		echo compare
+		;;
+	esac
 }
-- 
Sebastian Schuberth
Sebastian Schuberth· Jul 23, 2012, 07:20 UTC · re: Sebastian Schuberth · lore

[PATCH v2 5/5] Add a few more code comments and blank lines in guess_merge_tool

Signed-off-by: Sebastian Schuberth <sschuberth@gmail.com>
---
 git-mergetool--lib.sh | 6 ++++++
 1 file changed, 6 insertions(+)
Show changes to git-mergetool--lib.sh +6 −0
diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
index ed630b2..ac9a8f0 100644
--- a/git-mergetool--lib.sh
+++ b/git-mergetool--lib.sh
@@ -112,14 +112,17 @@ run_merge_tool () {
 }
 
 guess_merge_tool () {
+	# Add tools that can either do merging or diffing, but not both.
 	if merge_mode
 	then
 		tools="tortoisemerge"
 	else
 		tools="kompare"
 	fi
+
 	if test -n "$DISPLAY"
 	then
+		# Prefer GTK-based tools under Gnome.
 		if test -n "$GNOME_DESKTOP_SESSION_ID"
 		then
 			tools="meld opendiff kdiff3 tkdiff xxdiff $tools"
@@ -128,6 +131,8 @@ guess_merge_tool () {
 		fi
 		tools="$tools gvimdiff diffuse ecmerge p4merge araxis bc3"
 	fi
+
+	# Prefer vimdiff if vim is the default editor.
 	case "${VISUAL:-$EDITOR}" in
 	*vim*)
 		tools="$tools vimdiff emerge"
@@ -136,6 +141,7 @@ guess_merge_tool () {
 		tools="$tools emerge vimdiff"
 		;;
 	esac
+
 	echo >&2 "merge tool candidates: $tools"
 
 	# Loop over each candidate and stop when a valid merge tool is found.
-- 
1.7.11.msysgit.2

← back to recent threads