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

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

[Sebastian Schuberth]

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

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

[Sebastian Schuberth]

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

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

[Sebastian Schuberth]

Signed-off-by: Sebastian Schuberth <sschuberth@gmail.com>
---
 contrib/completion/git-completion.bash | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

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

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

[Sebastian Schuberth]

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
 "
 
+# 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

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

[Sebastian Schuberth]

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.
+	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

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

[Sebastian Schuberth]

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
+	then
+		echo compare
+	else
+		echo "$1"
+	fi
 }
-- 
1.7.11.msysgit.2

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

[Sebastian Schuberth]

Signed-off-by: Sebastian Schuberth <sschuberth@gmail.com>
---
 git-mergetool--lib.sh | 6 ++++++
 1 file changed, 6 insertions(+)

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

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

[Junio C Hamano]

Sebastian Schuberth <sschuberth@gmail.com> writes:

> 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?

> +# 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.

>  _git_difftool ()
>  {
>  	__git_has_doubledash && return
>  
>  	case "$cur" in
>  	--tool=*)
> -		__gitcomp "$__git_mergetools_common kompare" "" "${cur##--tool=}"
> +		__gitcomp "$__git_difftools" "" "${cur##--tool=}"
>  		return
>  		;;
>  	--*)
> @@ -1623,7 +1630,7 @@ _git_mergetool ()
>  {
>  	case "$cur" in
>  	--tool=*)
> -		__gitcomp "$__git_mergetools_common tortoisemerge" "" "${cur##--tool=}"
> +		__gitcomp "$__git_mergetools" "" "${cur##--tool=}"
>  		return
>  		;;
>  	--*)

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

[Junio C Hamano]

Sebastian Schuberth <sschuberth@gmail.com> writes:

> 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.

> +	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

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

[Junio C Hamano]

Sebastian Schuberth <sschuberth@gmail.com> writes:

> 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".

> +	then
> +		echo compare
> +	else
> +		echo "$1"
> +	fi
>  }

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

[Junio C Hamano]

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

Signed-off-by: Junio C Hamano <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.

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

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
 		*,*=*)

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

[Sebastian Schuberth]

On 23.07.2012 18:46, Junio C Hamano wrote:

>> +# 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

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

[Sebastian Schuberth]

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(-)

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

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

[Junio C Hamano]

Sebastian Schuberth <sschuberth@gmail.com> writes:

>> 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.

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

[Sebastian Schuberth]

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

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

[Sebastian Schuberth]

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

Signed-off-by: Junio C Hamano <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>.
+
+--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

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

[Sebastian Schuberth]

On 23.07.2012 20:37, Junio C Hamano wrote:

>>> 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.

> 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

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

[Sebastian Schuberth]

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
 }
-- 
1.7.11.msysgit.2

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

[Junio C Hamano]

Sebastian Schuberth <sschuberth@gmail.com> writes:

> 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.

> +
> +--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.

> +
> +--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
>  		*,*=*)

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

[Sebastian Schuberth]

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.
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

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

[David Aguilar]

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.

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.

> 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

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

[Junio C Hamano]

Sebastian Schuberth <sschuberth@gmail.com> writes:

> 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.

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

[Sebastian Schuberth]

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

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

[Junio C Hamano]

David Aguilar <davvid@gmail.com> writes:

> 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(-)

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

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

[Junio C Hamano]

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.

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

[Andreas Schwab]

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."

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

[Sebastian Schuberth]

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?

> 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

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

[Sebastian Schuberth]

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

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

[David Aguilar]

On Mon, Jul 23, 2012 at 2:24 PM, Junio C Hamano <gitster@pobox.com> wrote:
> 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

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

[Sebastian Schuberth]

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

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

[Junio C Hamano]

Sebastian Schuberth <sschuberth@gmail.com> writes:

> Personally, I've valued the gain of
> the patch to not list araxis as an available diff tool by "git
> difftool --tool-help" when in fact just ImageMagick is in PATH higher
> than the loss to support araxis installations that are in a path not
> containung "araxis" but are in PATH.

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

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

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

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

[Junio C Hamano]

Sebastian Schuberth <sschuberth@gmail.com> writes:

> 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.

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

[Sebastian Schuberth]

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.

> 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

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

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> 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(-)

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
 }

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

[Sebastian Schuberth]

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(-)

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

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

[Junio C Hamano]

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.

-- >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(-)

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

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

[David Aguilar]

On Wed, Aug 22, 2012 at 10:33 PM, Junio C Hamano <junio@pobox.com> wrote:
> 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

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

[Junio C Hamano]

David Aguilar <davvid@gmail.com> writes:

>> 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 ;-)

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

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

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

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

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

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

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

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

[David Aguilar]

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.

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

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

[Jens Lehmann]

Am 24.08.2012 10:31, schrieb David Aguilar:
> 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.

> 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.

> 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?

> 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.