All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] mergetools: Add tortoisegitmerge helper
@ 2013-01-20 11:27 Sven Strickroth
  2013-01-21  0:43 ` Junio C Hamano
  0 siblings, 1 reply; 31+ messages in thread
From: Sven Strickroth @ 2013-01-20 11:27 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Sebastian Schuberth, davvid, Jeff King

- The TortoiseGit team renamed TortoiseMerge.exe to TortoiseGitMerge.exe
  (starting with 1.8.0) in order to make clear that this one has special
  support for git and prevent confusion with the TortoiseSVN TortoiseMerge
  version.
- The tortoisemerge mergetool does not work with filenames which have
  a space in it. Fixing this required changes in git and also in
  TortoiseGitMerge; see https://github.com/msysgit/msysgit/issues/57.

The new tortoisegitmerge helper was added so that people can still use
TortoiseMerge from TortoiseSVN (and older TortoiseGit versions).

Signed-off-by: Sven Strickroth <email@cs-ware.de>
Reported-by: Sebastian Schuberth <sschuberth@gmail.com>
---
 Documentation/diff-config.txt          |  4 ++--
 Documentation/git-mergetool.txt        |  4 ++--
 Documentation/merge-config.txt         |  6 +++---
 contrib/completion/git-completion.bash |  2 +-
 git-mergetool--lib.sh                  |  2 +-
 mergetools/tortoisegitmerge            | 17 +++++++++++++++++
 6 files changed, 26 insertions(+), 9 deletions(-)
 create mode 100644 mergetools/tortoisegitmerge

diff --git a/Documentation/diff-config.txt b/Documentation/diff-config.txt
index 4314ad0..13cbe5b 100644
--- a/Documentation/diff-config.txt
+++ b/Documentation/diff-config.txt
@@ -151,7 +151,7 @@ diff.<driver>.cachetextconv::
 diff.tool::
 	The diff tool to be used by linkgit:git-difftool[1].  This
 	option overrides `merge.tool`, and has the same valid built-in
-	values as `merge.tool` minus "tortoisemerge" and plus
-	"kompare".  Any other value is treated as a custom diff tool,
+	values as `merge.tool` minus "tortoisemerge"/"tortoisegitmerge" and
+	plus "kompare".  Any other value is treated as a custom diff tool,
 	and there must be a corresponding `difftool.<tool>.cmd`
 	option.
diff --git a/Documentation/git-mergetool.txt b/Documentation/git-mergetool.txt
index 6b563c5..a80cccd 100644
--- a/Documentation/git-mergetool.txt
+++ b/Documentation/git-mergetool.txt
@@ -28,8 +28,8 @@ OPTIONS
 --tool=<tool>::
 	Use the merge resolution program specified by <tool>.
 	Valid values include emerge, gvimdiff, kdiff3,
-	meld, vimdiff, and tortoisemerge. Run `git mergetool --tool-help`
-	for the list of valid <tool> settings.
+	meld, vimdiff, tortoisegitmerge, 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/Documentation/merge-config.txt b/Documentation/merge-config.txt
index 9bb4956..a047646 100644
--- a/Documentation/merge-config.txt
+++ b/Documentation/merge-config.txt
@@ -55,9 +55,9 @@ merge.tool::
 	Controls which merge resolution program is used by
 	linkgit:git-mergetool[1].  Valid built-in values are: "araxis",
 	"bc3", "diffuse", "ecmerge", "emerge", "gvimdiff", "kdiff3", "meld",
-	"opendiff", "p4merge", "tkdiff", "tortoisemerge", "vimdiff"
-	and "xxdiff".  Any other value is treated is custom merge tool
-	and there must be a corresponding mergetool.<tool>.cmd option.
+	"opendiff", "p4merge", "tkdiff", "tortoisegitmerge", "tortoisemerge",
+	"vimdiff" and "xxdiff".  Any other value is treated is custom merge
+	tool and there must be a corresponding mergetool.<tool>.cmd option.
  merge.verbosity::
 	Controls the amount of output shown by the recursive merge
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 90f5f05..5332a33 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1345,7 +1345,7 @@ _git_mergetool ()
 {
 	case "$cur" in
 	--tool=*)
-		__gitcomp "$__git_mergetools_common tortoisemerge" "" "${cur##--tool=}"
+		__gitcomp "$__git_mergetools_common tortoisegitmerge tortoisemerge" "" "${cur##--tool=}"
 		return
 		;;
 	--*)
diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
index f013a03..47183ef 100644
--- a/git-mergetool--lib.sh
+++ b/git-mergetool--lib.sh
@@ -150,7 +150,7 @@ run_merge_cmd () {
 list_merge_tool_candidates () {
 	if merge_mode
 	then
-		tools="tortoisemerge"
+		tools="tortoisegitmerge tortoisemerge"
 	else
 		tools="kompare"
 	fi
diff --git a/mergetools/tortoisegitmerge b/mergetools/tortoisegitmerge
new file mode 100644
index 0000000..5b802a7
--- /dev/null
+++ b/mergetools/tortoisegitmerge
@@ -0,0 +1,17 @@
+can_diff () {
+	return 1
+}
+
+merge_cmd () {
+	if $base_present
+	then
+		touch "$BACKUP"
+		"$merge_tool_path" \
+			-base "$BASE" -mine "$LOCAL" \
+			-theirs "$REMOTE" -merged "$MERGED"
+		check_unchanged
+	else
+		echo "TortoiseGitMerge cannot be used without a base" 1>&2
+		return 1
+	fi
+}
-- 
Best regards,
 Sven Strickroth
 PGP key id F5A9D4C4 @ any key-server

^ permalink raw reply related	[flat|nested] 31+ messages in thread

* Re: [PATCH] mergetools: Add tortoisegitmerge helper
  2013-01-20 11:27 [PATCH] mergetools: Add tortoisegitmerge helper Sven Strickroth
@ 2013-01-21  0:43 ` Junio C Hamano
  2013-01-21  8:24   ` Sven Strickroth
  2013-01-21  8:26   ` Sven Strickroth
  0 siblings, 2 replies; 31+ messages in thread
From: Junio C Hamano @ 2013-01-21  0:43 UTC (permalink / raw)
  To: Sven Strickroth; +Cc: git, Sebastian Schuberth, davvid, Jeff King

Sven Strickroth <sven.strickroth@tu-clausthal.de> writes:

> - The TortoiseGit team renamed TortoiseMerge.exe to TortoiseGitMerge.exe
>   (starting with 1.8.0) in order to make clear that this one has special
>   support for git and prevent confusion with the TortoiseSVN TortoiseMerge
>   version.
> - The tortoisemerge mergetool does not work with filenames which have
>   a space in it. Fixing this required changes in git and also in
>   TortoiseGitMerge; see https://github.com/msysgit/msysgit/issues/57.
>
> The new tortoisegitmerge helper was added so that people can still use
> TortoiseMerge from TortoiseSVN (and older TortoiseGit versions).
>
> Signed-off-by: Sven Strickroth <email@cs-ware.de>
> Reported-by: Sebastian Schuberth <sschuberth@gmail.com>
> ---

Applying: mergetools: Add tortoisegitmerge helper
fatal: corrupt patch at line 56

That comes from here:

> diff --git a/Documentation/merge-config.txt b/Documentation/merge-config.txt
> index 9bb4956..a047646 100644
> --- a/Documentation/merge-config.txt
> +++ b/Documentation/merge-config.txt
> @@ -55,9 +55,9 @@ merge.tool::
>  	Controls which merge resolution program is used by
>  	linkgit:git-mergetool[1].  Valid built-in values are: "araxis",
>  	"bc3", "diffuse", "ecmerge", "emerge", "gvimdiff", "kdiff3", "meld",
> -	"opendiff", "p4merge", "tkdiff", "tortoisemerge", "vimdiff"
> -	and "xxdiff".  Any other value is treated is custom merge tool
> -	and there must be a corresponding mergetool.<tool>.cmd option.
> +	"opendiff", "p4merge", "tkdiff", "tortoisegitmerge", "tortoisemerge",
> +	"vimdiff" and "xxdiff".  Any other value is treated is custom merge
> +	tool and there must be a corresponding mergetool.<tool>.cmd option.
>   merge.verbosity::
>  	Controls the amount of output shown by the recursive merge
> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash

Notice that we have three pre-context lines but only two
post-context lines for this hunk.  There is one context line missing
at the end of this hunk.

I'd usually try to queue a corrupt patch by manually fixing up when
it is a trivial corruption, but a corruption that _loses_ lines is
too dangerous to be handled that way.  There may be additions in
other hunks you wanted to make that were lost for the same reason
why the post-context line was lost here, and my fix-up would end up
committing a wrong patch.

Please investigate how this happened, and re-send after hearing
reviews from others.

Thanks.

^ permalink raw reply	[flat|nested] 31+ messages in thread

* [PATCH] mergetools: Add tortoisegitmerge helper
  2013-01-21  0:43 ` Junio C Hamano
@ 2013-01-21  8:24   ` Sven Strickroth
  2013-01-21  8:26   ` Sven Strickroth
  1 sibling, 0 replies; 31+ messages in thread
From: Sven Strickroth @ 2013-01-21  8:24 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Sebastian Schuberth, davvid, Jeff King

- The TortoiseGit team renamed TortoiseMerge.exe to TortoiseGitMerge.exe
  (starting with 1.8.0) in order to make clear that this one has special
  support for git and prevent confusion with the TortoiseSVN TortoiseMerge
  version.
- The tortoisemerge mergetool does not work with filenames which have
  a space in it. Fixing this required changes in git and also in
  TortoiseGitMerge; see https://github.com/msysgit/msysgit/issues/57.

The new tortoisegitmerge helper was added so that people can still use
TortoiseMerge from TortoiseSVN (and older TortoiseGit versions).

Signed-off-by: Sven Strickroth <email@cs-ware.de>
Idea-by: Sebastian Schuberth <sschuberth@gmail.com>
---
 Documentation/diff-config.txt          |  4 ++--
 Documentation/git-mergetool.txt        |  4 ++--
 Documentation/merge-config.txt         |  6 +++---
 contrib/completion/git-completion.bash |  2 +-
 git-mergetool--lib.sh                  |  2 +-
 mergetools/tortoisegitmerge            | 17 +++++++++++++++++
 6 files changed, 26 insertions(+), 9 deletions(-)
 create mode 100644 mergetools/tortoisegitmerge

diff --git a/Documentation/diff-config.txt b/Documentation/diff-config.txt
index 4314ad0..13cbe5b 100644
--- a/Documentation/diff-config.txt
+++ b/Documentation/diff-config.txt
@@ -151,7 +151,7 @@ diff.<driver>.cachetextconv::
 diff.tool::
 	The diff tool to be used by linkgit:git-difftool[1].  This
 	option overrides `merge.tool`, and has the same valid built-in
-	values as `merge.tool` minus "tortoisemerge" and plus
-	"kompare".  Any other value is treated as a custom diff tool,
+	values as `merge.tool` minus "tortoisemerge"/"tortoisegitmerge" and
+	plus "kompare".  Any other value is treated as a custom diff tool,
 	and there must be a corresponding `difftool.<tool>.cmd`
 	option.
diff --git a/Documentation/git-mergetool.txt b/Documentation/git-mergetool.txt
index 6b563c5..a80cccd 100644
--- a/Documentation/git-mergetool.txt
+++ b/Documentation/git-mergetool.txt
@@ -28,8 +28,8 @@ OPTIONS
 --tool=<tool>::
 	Use the merge resolution program specified by <tool>.
 	Valid values include emerge, gvimdiff, kdiff3,
-	meld, vimdiff, and tortoisemerge. Run `git mergetool --tool-help`
-	for the list of valid <tool> settings.
+	meld, vimdiff, tortoisegitmerge, 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/Documentation/merge-config.txt b/Documentation/merge-config.txt
index 9bb4956..a047646 100644
--- a/Documentation/merge-config.txt
+++ b/Documentation/merge-config.txt
@@ -55,9 +55,9 @@ merge.tool::
 	Controls which merge resolution program is used by
 	linkgit:git-mergetool[1].  Valid built-in values are: "araxis",
 	"bc3", "diffuse", "ecmerge", "emerge", "gvimdiff", "kdiff3", "meld",
-	"opendiff", "p4merge", "tkdiff", "tortoisemerge", "vimdiff"
-	and "xxdiff".  Any other value is treated is custom merge tool
-	and there must be a corresponding mergetool.<tool>.cmd option.
+	"opendiff", "p4merge", "tkdiff", "tortoisegitmerge", "tortoisemerge",
+	"vimdiff" and "xxdiff".  Any other value is treated is custom merge
+	tool and there must be a corresponding mergetool.<tool>.cmd option.
 
 merge.verbosity::
 	Controls the amount of output shown by the recursive merge
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 14dd5e7..1557d54 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1345,7 +1345,7 @@ _git_mergetool ()
 {
 	case "$cur" in
 	--tool=*)
-		__gitcomp "$__git_mergetools_common tortoisemerge" "" "${cur##--tool=}"
+		__gitcomp "$__git_mergetools_common tortoisegitmerge tortoisemerge" "" "${cur##--tool=}"
 		return
 		;;
 	--*)
diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
index f013a03..47183ef 100644
--- a/git-mergetool--lib.sh
+++ b/git-mergetool--lib.sh
@@ -150,7 +150,7 @@ run_merge_cmd () {
 list_merge_tool_candidates () {
 	if merge_mode
 	then
-		tools="tortoisemerge"
+		tools="tortoisegitmerge tortoisemerge"
 	else
 		tools="kompare"
 	fi
diff --git a/mergetools/tortoisegitmerge b/mergetools/tortoisegitmerge
new file mode 100644
index 0000000..5b802a7
--- /dev/null
+++ b/mergetools/tortoisegitmerge
@@ -0,0 +1,17 @@
+can_diff () {
+	return 1
+}
+
+merge_cmd () {
+	if $base_present
+	then
+		touch "$BACKUP"
+		"$merge_tool_path" \
+			-base="$BASE" -mine="$LOCAL" \
+			-theirs="$REMOTE" -merged="$MERGED"
+		check_unchanged
+	else
+		echo "TortoiseGitMerge cannot be used without a base" 1>&2
+		return 1
+	fi
+}
-- 
1.8.0.msysgit.0

^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [PATCH] mergetools: Add tortoisegitmerge helper
  2013-01-21  0:43 ` Junio C Hamano
  2013-01-21  8:24   ` Sven Strickroth
@ 2013-01-21  8:26   ` Sven Strickroth
  2013-01-24 11:19     ` Sven Strickroth
  2013-01-24 19:51     ` Junio C Hamano
  1 sibling, 2 replies; 31+ messages in thread
From: Sven Strickroth @ 2013-01-21  8:26 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Sebastian Schuberth, davvid, Jeff King

- The TortoiseGit team renamed TortoiseMerge.exe to TortoiseGitMerge.exe
  (starting with 1.8.0) in order to make clear that this one has special
  support for git and prevent confusion with the TortoiseSVN TortoiseMerge
  version.
- The tortoisemerge mergetool does not work with filenames which have
  a space in it. Fixing this required changes in git and also in
  TortoiseGitMerge; see https://github.com/msysgit/msysgit/issues/57.

The new tortoisegitmerge helper was added so that people can still use
TortoiseMerge from TortoiseSVN (and older TortoiseGit versions).

Signed-off-by: Sven Strickroth <email@cs-ware.de>
Idea-by: Sebastian Schuberth <sschuberth@gmail.com>
---
 Documentation/diff-config.txt          |  4 ++--
 Documentation/git-mergetool.txt        |  4 ++--
 Documentation/merge-config.txt         |  6 +++---
 contrib/completion/git-completion.bash |  2 +-
 git-mergetool--lib.sh                  |  2 +-
 mergetools/tortoisegitmerge            | 17 +++++++++++++++++
 6 files changed, 26 insertions(+), 9 deletions(-)
 create mode 100644 mergetools/tortoisegitmerge

diff --git a/Documentation/diff-config.txt b/Documentation/diff-config.txt
index 4314ad0..13cbe5b 100644
--- a/Documentation/diff-config.txt
+++ b/Documentation/diff-config.txt
@@ -151,7 +151,7 @@ diff.<driver>.cachetextconv::
 diff.tool::
 	The diff tool to be used by linkgit:git-difftool[1].  This
 	option overrides `merge.tool`, and has the same valid built-in
-	values as `merge.tool` minus "tortoisemerge" and plus
-	"kompare".  Any other value is treated as a custom diff tool,
+	values as `merge.tool` minus "tortoisemerge"/"tortoisegitmerge" and
+	plus "kompare".  Any other value is treated as a custom diff tool,
 	and there must be a corresponding `difftool.<tool>.cmd`
 	option.
diff --git a/Documentation/git-mergetool.txt b/Documentation/git-mergetool.txt
index 6b563c5..a80cccd 100644
--- a/Documentation/git-mergetool.txt
+++ b/Documentation/git-mergetool.txt
@@ -28,8 +28,8 @@ OPTIONS
 --tool=<tool>::
 	Use the merge resolution program specified by <tool>.
 	Valid values include emerge, gvimdiff, kdiff3,
-	meld, vimdiff, and tortoisemerge. Run `git mergetool --tool-help`
-	for the list of valid <tool> settings.
+	meld, vimdiff, tortoisegitmerge, 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/Documentation/merge-config.txt b/Documentation/merge-config.txt
index 9bb4956..a047646 100644
--- a/Documentation/merge-config.txt
+++ b/Documentation/merge-config.txt
@@ -55,9 +55,9 @@ merge.tool::
 	Controls which merge resolution program is used by
 	linkgit:git-mergetool[1].  Valid built-in values are: "araxis",
 	"bc3", "diffuse", "ecmerge", "emerge", "gvimdiff", "kdiff3", "meld",
-	"opendiff", "p4merge", "tkdiff", "tortoisemerge", "vimdiff"
-	and "xxdiff".  Any other value is treated is custom merge tool
-	and there must be a corresponding mergetool.<tool>.cmd option.
+	"opendiff", "p4merge", "tkdiff", "tortoisegitmerge", "tortoisemerge",
+	"vimdiff" and "xxdiff".  Any other value is treated is custom merge
+	tool and there must be a corresponding mergetool.<tool>.cmd option.
 
 merge.verbosity::
 	Controls the amount of output shown by the recursive merge
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 14dd5e7..1557d54 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1345,7 +1345,7 @@ _git_mergetool ()
 {
 	case "$cur" in
 	--tool=*)
-		__gitcomp "$__git_mergetools_common tortoisemerge" "" "${cur##--tool=}"
+		__gitcomp "$__git_mergetools_common tortoisegitmerge tortoisemerge" "" "${cur##--tool=}"
 		return
 		;;
 	--*)
diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
index f013a03..47183ef 100644
--- a/git-mergetool--lib.sh
+++ b/git-mergetool--lib.sh
@@ -150,7 +150,7 @@ run_merge_cmd () {
 list_merge_tool_candidates () {
 	if merge_mode
 	then
-		tools="tortoisemerge"
+		tools="tortoisegitmerge tortoisemerge"
 	else
 		tools="kompare"
 	fi
diff --git a/mergetools/tortoisegitmerge b/mergetools/tortoisegitmerge
new file mode 100644
index 0000000..5b802a7
--- /dev/null
+++ b/mergetools/tortoisegitmerge
@@ -0,0 +1,17 @@
+can_diff () {
+	return 1
+}
+
+merge_cmd () {
+	if $base_present
+	then
+		touch "$BACKUP"
+		"$merge_tool_path" \
+			-base "$BASE" -mine "$LOCAL" \
+			-theirs "$REMOTE" -merged "$MERGED"
+		check_unchanged
+	else
+		echo "TortoiseGitMerge cannot be used without a base" 1>&2
+		return 1
+	fi
+}
-- 
1.8.0.msysgit.0

^ permalink raw reply related	[flat|nested] 31+ messages in thread

* Re: [PATCH] mergetools: Add tortoisegitmerge helper
  2013-01-21  8:26   ` Sven Strickroth
@ 2013-01-24 11:19     ` Sven Strickroth
  2013-01-24 19:51     ` Junio C Hamano
  1 sibling, 0 replies; 31+ messages in thread
From: Sven Strickroth @ 2013-01-24 11:19 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Sebastian Schuberth, davvid, Jeff King

Am 21.01.2013 09:26 schrieb Sven Strickroth:
> - The TortoiseGit team renamed TortoiseMerge.exe to TortoiseGitMerge.exe
>   (starting with 1.8.0) in order to make clear that this one has special
>   support for git and prevent confusion with the TortoiseSVN TortoiseMerge
>   version.
> - The tortoisemerge mergetool does not work with filenames which have
>   a space in it. Fixing this required changes in git and also in
>   TortoiseGitMerge; see https://github.com/msysgit/msysgit/issues/57.
> 
> The new tortoisegitmerge helper was added so that people can still use
> TortoiseMerge from TortoiseSVN (and older TortoiseGit versions).

Any comments for this patch?

-- 
Best regards,
 Sven Strickroth
 PGP key id F5A9D4C4 @ any key-server

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH] mergetools: Add tortoisegitmerge helper
  2013-01-21  8:26   ` Sven Strickroth
  2013-01-24 11:19     ` Sven Strickroth
@ 2013-01-24 19:51     ` Junio C Hamano
  2013-01-24 22:07       ` Sven Strickroth
  1 sibling, 1 reply; 31+ messages in thread
From: Junio C Hamano @ 2013-01-24 19:51 UTC (permalink / raw)
  To: Sven Strickroth; +Cc: git, Sebastian Schuberth, davvid, Jeff King

Sven Strickroth <sven.strickroth@tu-clausthal.de> writes:

> - The TortoiseGit team renamed TortoiseMerge.exe to TortoiseGitMerge.exe
>   (starting with 1.8.0) in order to make clear that this one has special
>   support for git and prevent confusion with the TortoiseSVN TortoiseMerge
>   version.

Wouldn't it make more sense in such a situation if your users can
keep using the old "tortoisemerge" configured in their configuration
and when the renamed one is found the mergetool automatically used
it, rather than the way your patch is done?  It seems that you are
forcing all the users to reconfigure or retrain their fingers.  Is
that the best we can do?  Is it too cumbersome to autodetect the
presense of tortoisegitmerge and redirect a request for tortoisemerge
to it, perhaps using translate_merge_tool_path (cf. mergetools/bc3)?

Assuming that people that have both variants will always want
mergetool to use tortoisegitmerge, that is.  If there are some
features missing from or extra bugs in tortoisegitmerge that makes
some people favor tortoisemerge, then giving two choices like your
patch does may make more sense.  I only know the difference between
the two from your four-line description above, but it does not look
like it is the case.

> diff --git a/Documentation/diff-config.txt b/Documentation/diff-config.txt
> index 4314ad0..13cbe5b 100644
> --- a/Documentation/diff-config.txt
> +++ b/Documentation/diff-config.txt
> @@ -151,7 +151,7 @@ diff.<driver>.cachetextconv::
>  diff.tool::
>  	The diff tool to be used by linkgit:git-difftool[1].  This
>  	option overrides `merge.tool`, and has the same valid built-in
> -	values as `merge.tool` minus "tortoisemerge" and plus
> -	"kompare".  Any other value is treated as a custom diff tool,
> +	values as `merge.tool` minus "tortoisemerge"/"tortoisegitmerge" and
> +	plus "kompare".  Any other value is treated as a custom diff tool,
>  	and there must be a corresponding `difftool.<tool>.cmd`
>  	option.

So in short, two tortoises and kompare are only valid as mergetool
but cannot be used as difftool?  No, I am reading it wrong.
merge.tool can be used for both, kompare can be used as difftool,
and two tortoises can only be used as mergetool.

This paragraph needs to be rewritten to unconfuse readers.  The
original is barely intelligible, and it becomes unreadable as the
set of tools subtracted by "minus" and added by "plus" grows.

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH] mergetools: Add tortoisegitmerge helper
  2013-01-24 19:51     ` Junio C Hamano
@ 2013-01-24 22:07       ` Sven Strickroth
  2013-01-24 22:15         ` Junio C Hamano
  2013-01-25  9:06         ` [PATCH] mergetools: Enhance tortoisemerge to work with Sven Strickroth
  0 siblings, 2 replies; 31+ messages in thread
From: Sven Strickroth @ 2013-01-24 22:07 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Sebastian Schuberth, davvid, Jeff King

Am 24.01.2013 20:51 schrieb Junio C Hamano:
> Sven Strickroth <sven.strickroth@tu-clausthal.de> writes:
> 
>> - The TortoiseGit team renamed TortoiseMerge.exe to TortoiseGitMerge.exe
>>   (starting with 1.8.0) in order to make clear that this one has special
>>   support for git and prevent confusion with the TortoiseSVN TortoiseMerge
>>   version.
> 
> Wouldn't it make more sense in such a situation if your users can
> keep using the old "tortoisemerge" configured in their configuration
> and when the renamed one is found the mergetool automatically used
> it, rather than the way your patch is done?

That was also my first idea, however, TortoiseMerge uses parameters as
follows: '-base:"$BASE"'. TortoiseGitMerge uses values separated by
space from keys: '-base "$BASE"'. So both are incompatible (the first
approach has problems with spaces in filenames, the TortoiseGitMerge
approach fixes this).

>> diff --git a/Documentation/diff-config.txt b/Documentation/diff-config.txt
>> index 4314ad0..13cbe5b 100644
>> --- a/Documentation/diff-config.txt
>> +++ b/Documentation/diff-config.txt
>> @@ -151,7 +151,7 @@ diff.<driver>.cachetextconv::
>>  diff.tool::
>>  	The diff tool to be used by linkgit:git-difftool[1].  This
>>  	option overrides `merge.tool`, and has the same valid built-in
>> -	values as `merge.tool` minus "tortoisemerge" and plus
>> -	"kompare".  Any other value is treated as a custom diff tool,
>> +	values as `merge.tool` minus "tortoisemerge"/"tortoisegitmerge" and
>> +	plus "kompare".  Any other value is treated as a custom diff tool,
>>  	and there must be a corresponding `difftool.<tool>.cmd`
>>  	option.
> 
> So in short, two tortoises and kompare are only valid as mergetool
> but cannot be used as difftool?  No, I am reading it wrong.
> merge.tool can be used for both, kompare can be used as difftool,
> and two tortoises can only be used as mergetool.
> 
> This paragraph needs to be rewritten to unconfuse readers.  The
> original is barely intelligible, and it becomes unreadable as the
> set of tools subtracted by "minus" and added by "plus" grows.

But I think this should not be part of this patch.

-- 
Best regards,
 Sven Strickroth
 PGP key id F5A9D4C4 @ any key-server

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH] mergetools: Add tortoisegitmerge helper
  2013-01-24 22:07       ` Sven Strickroth
@ 2013-01-24 22:15         ` Junio C Hamano
  2013-01-25  6:11           ` David Aguilar
  2013-01-25  9:06         ` [PATCH] mergetools: Enhance tortoisemerge to work with Sven Strickroth
  1 sibling, 1 reply; 31+ messages in thread
From: Junio C Hamano @ 2013-01-24 22:15 UTC (permalink / raw)
  To: Sven Strickroth; +Cc: git, Sebastian Schuberth, davvid, Jeff King

Sven Strickroth <sven.strickroth@tu-clausthal.de> writes:

> Am 24.01.2013 20:51 schrieb Junio C Hamano:
>> Sven Strickroth <sven.strickroth@tu-clausthal.de> writes:
>> 
>>> - The TortoiseGit team renamed TortoiseMerge.exe to TortoiseGitMerge.exe
>>>   (starting with 1.8.0) in order to make clear that this one has special
>>>   support for git and prevent confusion with the TortoiseSVN TortoiseMerge
>>>   version.
>> 
>> Wouldn't it make more sense in such a situation if your users can
>> keep using the old "tortoisemerge" configured in their configuration
>> and when the renamed one is found the mergetool automatically used
>> it, rather than the way your patch is done?
>
> That was also my first idea, however, TortoiseMerge uses parameters as
> follows: '-base:"$BASE"'. TortoiseGitMerge uses values separated by
> space from keys: '-base "$BASE"'. So both are incompatible (the first
> approach has problems with spaces in filenames, the TortoiseGitMerge
> approach fixes this).

OK.  Please unconfuse future readers of "git log" by saying why such
a unification does not work in the proposed log message.

>>> diff --git a/Documentation/diff-config.txt b/Documentation/diff-config.txt
>>> index 4314ad0..13cbe5b 100644
>>> --- a/Documentation/diff-config.txt
>>> +++ b/Documentation/diff-config.txt
>>> @@ -151,7 +151,7 @@ diff.<driver>.cachetextconv::
>>>  diff.tool::
>>>  	The diff tool to be used by linkgit:git-difftool[1].  This
>>>  	option overrides `merge.tool`, and has the same valid built-in
>>> -	values as `merge.tool` minus "tortoisemerge" and plus
>>> -	"kompare".  Any other value is treated as a custom diff tool,
>>> +	values as `merge.tool` minus "tortoisemerge"/"tortoisegitmerge" and
>>> +	plus "kompare".  Any other value is treated as a custom diff tool,
>>>  	and there must be a corresponding `difftool.<tool>.cmd`
>>>  	option.
>> 
>> So in short, two tortoises and kompare are only valid as mergetool
>> but cannot be used as difftool?  No, I am reading it wrong.
>> merge.tool can be used for both, kompare can be used as difftool,
>> and two tortoises can only be used as mergetool.
>> 
>> This paragraph needs to be rewritten to unconfuse readers.  The
>> original is barely intelligible, and it becomes unreadable as the
>> set of tools subtracted by "minus" and added by "plus" grows.
>
> But I think this should not be part of this patch.

I agree that it can be done (and it is better to be done) as a
preparatory step.  The current text is barely readable, but with
this patch there will be two "minus", and the result becomes
unreadable at that point.

It also could be done as a follow-up documentation readability fix.

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH] mergetools: Add tortoisegitmerge helper
  2013-01-24 22:15         ` Junio C Hamano
@ 2013-01-25  6:11           ` David Aguilar
  2013-01-25  7:21             ` Junio C Hamano
  0 siblings, 1 reply; 31+ messages in thread
From: David Aguilar @ 2013-01-25  6:11 UTC (permalink / raw)
  To: Sven Strickroth; +Cc: Junio C Hamano, git, Sebastian Schuberth, Jeff King

On Thu, Jan 24, 2013 at 2:15 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Sven Strickroth <sven.strickroth@tu-clausthal.de> writes:
>
>> Am 24.01.2013 20:51 schrieb Junio C Hamano:
>>> Sven Strickroth <sven.strickroth@tu-clausthal.de> writes:
>>>
>>>> - The TortoiseGit team renamed TortoiseMerge.exe to TortoiseGitMerge.exe
>>>>   (starting with 1.8.0) in order to make clear that this one has special
>>>>   support for git and prevent confusion with the TortoiseSVN TortoiseMerge
>>>>   version.
>>>
>>> Wouldn't it make more sense in such a situation if your users can
>>> keep using the old "tortoisemerge" configured in their configuration
>>> and when the renamed one is found the mergetool automatically used
>>> it, rather than the way your patch is done?
>>
>> That was also my first idea, however, TortoiseMerge uses parameters as
>> follows: '-base:"$BASE"'. TortoiseGitMerge uses values separated by
>> space from keys: '-base "$BASE"'. So both are incompatible (the first
>> approach has problems with spaces in filenames, the TortoiseGitMerge
>> approach fixes this).
>
> OK.  Please unconfuse future readers of "git log" by saying why such
> a unification does not work in the proposed log message.

Even though the old tortoisemerge and the new tortoisegitmerge
have completely different syntax, could we still use the existence
of one when deciding which syntax to use?

pseudo-code at the top of the scriptlet:

if test -z "$tortoisegitmerge"
then
    if type tortoisegitmerge 2>&1 >/dev/null
    then
        tortoisegitmerge=true
    else
        tortoisegitmerge=false
    fi
fi

...and then later merge_cmd and diff_cmd
can delegate to {diff,merge}_cmd_legacy() and
{diff,merge}_cmd_gitmerge() functions to do the work.

It's just a thought.  translate_merge_tool_path()
is too low-level to do it, but it seems like we could
get away with it by having some extra smarts in the
scriptlet.

>>>> diff --git a/Documentation/diff-config.txt b/Documentation/diff-config.txt
>>>> index 4314ad0..13cbe5b 100644
>>>> --- a/Documentation/diff-config.txt
>>>> +++ b/Documentation/diff-config.txt
>>>> @@ -151,7 +151,7 @@ diff.<driver>.cachetextconv::
>>>>  diff.tool::
>>>>     The diff tool to be used by linkgit:git-difftool[1].  This
>>>>     option overrides `merge.tool`, and has the same valid built-in
>>>> -   values as `merge.tool` minus "tortoisemerge" and plus
>>>> -   "kompare".  Any other value is treated as a custom diff tool,
>>>> +   values as `merge.tool` minus "tortoisemerge"/"tortoisegitmerge" and
>>>> +   plus "kompare".  Any other value is treated as a custom diff tool,
>>>>     and there must be a corresponding `difftool.<tool>.cmd`
>>>>     option.
>>>
>>> So in short, two tortoises and kompare are only valid as mergetool
>>> but cannot be used as difftool?  No, I am reading it wrong.
>>> merge.tool can be used for both, kompare can be used as difftool,
>>> and two tortoises can only be used as mergetool.
>>>
>>> This paragraph needs to be rewritten to unconfuse readers.  The
>>> original is barely intelligible, and it becomes unreadable as the
>>> set of tools subtracted by "minus" and added by "plus" grows.
>>
>> But I think this should not be part of this patch.
>
> I agree that it can be done (and it is better to be done) as a
> preparatory step.  The current text is barely readable, but with
> this patch there will be two "minus", and the result becomes
> unreadable at that point.
>
> It also could be done as a follow-up documentation readability fix.

Another thought would be to minimize this section as much
as possible and point users to "git difftool --tool-help".

We can then improve --tool-help (there are already preliminary
patches in-flight to do so) so that we do not have to maintain
this documentation in the future.

Likewise, if we are able to teach the scriptlet to choose
the best one (and not require a new scriptlet) then this
section could be left as-is for this patch.
-- 
David

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH] mergetools: Add tortoisegitmerge helper
  2013-01-25  6:11           ` David Aguilar
@ 2013-01-25  7:21             ` Junio C Hamano
  2013-01-25  7:54               ` David Aguilar
  0 siblings, 1 reply; 31+ messages in thread
From: Junio C Hamano @ 2013-01-25  7:21 UTC (permalink / raw)
  To: David Aguilar; +Cc: Sven Strickroth, git, Sebastian Schuberth, Jeff King

David Aguilar <davvid@gmail.com> writes:

> Even though the old tortoisemerge and the new tortoisegitmerge
> have completely different syntax, could we still use the existence
> of one when deciding which syntax to use?
> ...
> ...and then later merge_cmd and diff_cmd
> can delegate to {diff,merge}_cmd_legacy() and
> {diff,merge}_cmd_gitmerge() functions to do the work.
>
> It's just a thought.  translate_merge_tool_path()
> is too low-level to do it, but it seems like we could
> get away with it by having some extra smarts in the
> scriptlet.

Sounds like a far better approach to me.  I'd like to at least see
an attempt be made to make that work first.

>>>> This paragraph needs to be rewritten to unconfuse readers.  The
>>>> original is barely intelligible, and it becomes unreadable as the
>>>> set of tools subtracted by "minus" and added by "plus" grows.
>>>
>>> But I think this should not be part of this patch.
>>
>> I agree that it can be done (and it is better to be done) as a
>> preparatory step.  The current text is barely readable, but with
>> this patch there will be two "minus", and the result becomes
>> unreadable at that point.
>>
>> It also could be done as a follow-up documentation readability fix.
>
> Another thought would be to minimize this section as much
> as possible and point users to "git difftool --tool-help".

We had a similar discussion here:

  http://thread.gmane.org/gmane.comp.version-control.git/201913/focus=201976

and Documentation/git-{diff,merge}tool.txt have stayed quiet since
then.

But Documentation/merge-config.txt tries to list everything that _could_
be enabled, and I do not necessarily think having one single
location that lists everything is such a bad idea.

Is there a way for me to programatically tell what merge.tool and
diff.tool could be enabled for a particular source checkout of Git
regardless of what platform am I on (that is, even though I won't
touch Windows, I want to see 'tortoise' appear in the output of such
a procedure)?  We could generate a small text file from the Makefile
in Documentation and include it when building the manual pages if
such a procedure is available.

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH] mergetools: Add tortoisegitmerge helper
  2013-01-25  7:21             ` Junio C Hamano
@ 2013-01-25  7:54               ` David Aguilar
  2013-01-25  9:48                 ` John Keeping
  0 siblings, 1 reply; 31+ messages in thread
From: David Aguilar @ 2013-01-25  7:54 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Sven Strickroth, git, Sebastian Schuberth, Jeff King

On Thu, Jan 24, 2013 at 11:21 PM, Junio C Hamano <gitster@pobox.com> wrote:
> David Aguilar <davvid@gmail.com> writes:
>>>>> This paragraph needs to be rewritten to unconfuse readers.  The
>>>>> original is barely intelligible, and it becomes unreadable as the
>>>>> set of tools subtracted by "minus" and added by "plus" grows.
>>>>
>>>> But I think this should not be part of this patch.
>>>
>>> I agree that it can be done (and it is better to be done) as a
>>> preparatory step.  The current text is barely readable, but with
>>> this patch there will be two "minus", and the result becomes
>>> unreadable at that point.
>>>
>>> It also could be done as a follow-up documentation readability fix.
>>
>> Another thought would be to minimize this section as much
>> as possible and point users to "git difftool --tool-help".
>
> We had a similar discussion here:
>
>   http://thread.gmane.org/gmane.comp.version-control.git/201913/focus=201976
>
> and Documentation/git-{diff,merge}tool.txt have stayed quiet since
> then.
>
> But Documentation/merge-config.txt tries to list everything that _could_
> be enabled, and I do not necessarily think having one single
> location that lists everything is such a bad idea.
>
> Is there a way for me to programatically tell what merge.tool and
> diff.tool could be enabled for a particular source checkout of Git
> regardless of what platform am I on (that is, even though I won't
> touch Windows, I want to see 'tortoise' appear in the output of such
> a procedure)?  We could generate a small text file from the Makefile
> in Documentation and include it when building the manual pages if
> such a procedure is available.

That's a good idea.
Here's one way... (typed into gmail, so probably broken)

LF='
'
mergetools=
difftools=
scriptlets="$(git --exec-path)"/mergetools

for script in "$scriptlets"/*
do
    tool="$(basename "$script")"
    if test "$tool" = "defaults"
    then
        continue
    fi
    . "$scriptlets"/defaults
    can_diff && difftools="$difftools$tool$LF"
    can_merge && mergetools="$mergetools$tool$LF"
done

I can follow up with a Documentation patch along these lines.
I'm would imagine it would be hooked up similarly to how the
command lists are constructed.

This should allow the tortoisemerge improvements to happen independently.
-- 
David

^ permalink raw reply	[flat|nested] 31+ messages in thread

* [PATCH] mergetools: Enhance tortoisemerge to work with
  2013-01-24 22:07       ` Sven Strickroth
  2013-01-24 22:15         ` Junio C Hamano
@ 2013-01-25  9:06         ` Sven Strickroth
  2013-01-25 10:09           ` David Aguilar
  1 sibling, 1 reply; 31+ messages in thread
From: Sven Strickroth @ 2013-01-25  9:06 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Sebastian Schuberth, davvid, Jeff King

TortoiseGitMerge and filenames with spaces

- The TortoiseGit team renamed TortoiseMerge.exe to TortoiseGitMerge.exe
  (starting with 1.8.0) in order to make clear that this one has special
  support for git, (uses spaces as cli parameter key-value separators)
  and prevent confusion with the TortoiseSVN TortoiseMerge version.
- The tortoisemerge mergetool does not work with filenames which have
  a space in it. Fixing this required changes in git and also in
  TortoiseGitMerge; see https://github.com/msysgit/msysgit/issues/57.

Signed-off-by: Sven Strickroth <email@cs-ware.de>
Reported-by: Sebastian Schuberth <sschuberth@gmail.com>
---
 mergetools/tortoisemerge | 24 ++++++++++++++++++++----
 1 file changed, 20 insertions(+), 4 deletions(-)

diff --git a/mergetools/tortoisemerge b/mergetools/tortoisemerge
index ed7db49..9890737 100644
--- a/mergetools/tortoisemerge
+++ b/mergetools/tortoisemerge
@@ -6,12 +6,28 @@ merge_cmd () {
 	if $base_present
 	then
 		touch "$BACKUP"
-		"$merge_tool_path" \
-			-base:"$BASE" -mine:"$LOCAL" \
-			-theirs:"$REMOTE" -merged:"$MERGED"
+		if test "$merge_tool_path" == "tortoisegitmerge"
+		then
+			"$merge_tool_path" \
+				-base "$BASE" -mine "$LOCAL" \
+				-theirs "$REMOTE" -merged "$MERGED"
+		else 
+			"$merge_tool_path" \
+				-base:"$BASE" -mine:"$LOCAL" \
+				-theirs:"$REMOTE" -merged:"$MERGED"
+		fi
 		check_unchanged
 	else
-		echo "TortoiseMerge cannot be used without a base" 1>&2
+		echo "$merge_tool_path cannot be used without a base" 1>&2
 		return 1
 	fi
 }
+
+translate_merge_tool_path() {
+	if type tortoisegitmerge >/dev/null 2>/dev/null
+	then
+		echo tortoisegitmerge
+	else
+		echo tortoisemerge
+	fi
+}
-- 
Best regards,
 Sven Strickroth
 PGP key id F5A9D4C4 @ any key-server

^ permalink raw reply related	[flat|nested] 31+ messages in thread

* Re: [PATCH] mergetools: Add tortoisegitmerge helper
  2013-01-25  7:54               ` David Aguilar
@ 2013-01-25  9:48                 ` John Keeping
  0 siblings, 0 replies; 31+ messages in thread
From: John Keeping @ 2013-01-25  9:48 UTC (permalink / raw)
  To: David Aguilar
  Cc: Junio C Hamano, Sven Strickroth, git, Sebastian Schuberth, Jeff King

On Thu, Jan 24, 2013 at 11:54:25PM -0800, David Aguilar wrote:
> On Thu, Jan 24, 2013 at 11:21 PM, Junio C Hamano <gitster@pobox.com> wrote:
> > Is there a way for me to programatically tell what merge.tool and
> > diff.tool could be enabled for a particular source checkout of Git
> > regardless of what platform am I on (that is, even though I won't
> > touch Windows, I want to see 'tortoise' appear in the output of such
> > a procedure)?  We could generate a small text file from the Makefile
> > in Documentation and include it when building the manual pages if
> > such a procedure is available.
> 
> That's a good idea.
> Here's one way... (typed into gmail, so probably broken)
>
> LF='
> '
> mergetools=
> difftools=
> scriptlets="$(git --exec-path)"/mergetools
> 
> for script in "$scriptlets"/*
> do
>     tool="$(basename "$script")"
>     if test "$tool" = "defaults"
>     then
>         continue
>     fi
>     . "$scriptlets"/defaults
>     can_diff && difftools="$difftools$tool$LF"
>     can_merge && mergetools="$mergetools$tool$LF"
> done

I don't think this will work since the names of the valid tools are not
necessarily the same as the names of the scriptlets - this is the exact
issue that prompted my patches to git-difftool yesterday.

The best option I can see given what's currently available is something
like this:

-- >8 --

sed -n -e '/^list_merge_tool_candidates/,/^}/ {
        /tools=/ {
                s/.*tools=//
                s/"//g
                s/\$tools//
                s/ /\n/g
                p
        }
}' git-mergetool--lib.sh |sort |uniq |while read -r tool
do
	test -z "$tool" && continue
	( . git-mergetool--lib && setup_tool $tool
        	# Use can_diff and can_merge here.
	)
done

-- 8< --


John

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH] mergetools: Enhance tortoisemerge to work with
  2013-01-25  9:06         ` [PATCH] mergetools: Enhance tortoisemerge to work with Sven Strickroth
@ 2013-01-25 10:09           ` David Aguilar
  2013-01-25 13:07             ` Sven Strickroth
  0 siblings, 1 reply; 31+ messages in thread
From: David Aguilar @ 2013-01-25 10:09 UTC (permalink / raw)
  To: Sven Strickroth; +Cc: git, Junio C Hamano, Sebastian Schuberth, Jeff King

On Fri, Jan 25, 2013 at 1:06 AM, Sven Strickroth
<sven.strickroth@tu-clausthal.de> wrote:
> TortoiseGitMerge and filenames with spaces
>
> - The TortoiseGit team renamed TortoiseMerge.exe to TortoiseGitMerge.exe
>   (starting with 1.8.0) in order to make clear that this one has special
>   support for git, (uses spaces as cli parameter key-value separators)
>   and prevent confusion with the TortoiseSVN TortoiseMerge version.
> - The tortoisemerge mergetool does not work with filenames which have
>   a space in it. Fixing this required changes in git and also in
>   TortoiseGitMerge; see https://github.com/msysgit/msysgit/issues/57.
>
> Signed-off-by: Sven Strickroth <email@cs-ware.de>
> Reported-by: Sebastian Schuberth <sschuberth@gmail.com>
> ---
>  mergetools/tortoisemerge | 24 ++++++++++++++++++++----
>  1 file changed, 20 insertions(+), 4 deletions(-)
>
> diff --git a/mergetools/tortoisemerge b/mergetools/tortoisemerge
> index ed7db49..9890737 100644
> --- a/mergetools/tortoisemerge
> +++ b/mergetools/tortoisemerge
> @@ -6,12 +6,28 @@ merge_cmd () {
>         if $base_present
>         then
>                 touch "$BACKUP"
> -               "$merge_tool_path" \
> -                       -base:"$BASE" -mine:"$LOCAL" \
> -                       -theirs:"$REMOTE" -merged:"$MERGED"
> +               if test "$merge_tool_path" == "tortoisegitmerge"

I like the approach this is taking.  Thank you.
I have one small note:

I think this should use "=" instead of "==" here.

It might also make sense to wrap a basename call around it
so that users can set their own mergetool.tortoisemerge.path

basename="$(basename "$merge_tool_path" .exe)"
if test "$basename" = "tortoisegitmerge"
...


> +               then
> +                       "$merge_tool_path" \
> +                               -base "$BASE" -mine "$LOCAL" \
> +                               -theirs "$REMOTE" -merged "$MERGED"
> +               else
> +                       "$merge_tool_path" \
> +                               -base:"$BASE" -mine:"$LOCAL" \
> +                               -theirs:"$REMOTE" -merged:"$MERGED"
> +               fi
>                 check_unchanged
>         else
> -               echo "TortoiseMerge cannot be used without a base" 1>&2
> +               echo "$merge_tool_path cannot be used without a base" 1>&2
>                 return 1
>         fi
>  }
> +
> +translate_merge_tool_path() {
> +       if type tortoisegitmerge >/dev/null 2>/dev/null
> +       then
> +               echo tortoisegitmerge
> +       else
> +               echo tortoisemerge
> +       fi
> +}
> --
> Best regards,
>  Sven Strickroth
>  PGP key id F5A9D4C4 @ any key-server
-- 
David

^ permalink raw reply	[flat|nested] 31+ messages in thread

* [PATCH] mergetools: Enhance tortoisemerge to work with
  2013-01-25 10:09           ` David Aguilar
@ 2013-01-25 13:07             ` Sven Strickroth
  2013-01-25 18:28               ` Junio C Hamano
  0 siblings, 1 reply; 31+ messages in thread
From: Sven Strickroth @ 2013-01-25 13:07 UTC (permalink / raw)
  To: git; +Cc: David Aguilar, Junio C Hamano, Sebastian Schuberth, Jeff King

 TortoiseGitMerge and filenames with spaces

- The TortoiseGit team renamed TortoiseMerge.exe to TortoiseGitMerge.exe
  (starting with 1.8.0) in order to make clear that this one has special
  support for git, (uses spaces as cli parameter key-value separators)
  and prevent confusion with the TortoiseSVN TortoiseMerge version.
- The tortoisemerge mergetool does not work with filenames which have
  a space in it. Fixing this required changes in git and also in
  TortoiseGitMerge; see https://github.com/msysgit/msysgit/issues/57.

Signed-off-by: Sven Strickroth <email@cs-ware.de>
Reported-by: Sebastian Schuberth <sschuberth@gmail.com>
---
 mergetools/tortoisemerge | 51 ++++++++++++++++++++++++++++++++----------------
 1 file changed, 34 insertions(+), 17 deletions(-)

diff --git a/mergetools/tortoisemerge b/mergetools/tortoisemerge
index ed7db49..8ee99a5 100644
--- a/mergetools/tortoisemerge
+++ b/mergetools/tortoisemerge
@@ -1,17 +1,34 @@
-can_diff () {
-	return 1
-}
-
-merge_cmd () {
-	if $base_present
-	then
-		touch "$BACKUP"
-		"$merge_tool_path" \
-			-base:"$BASE" -mine:"$LOCAL" \
-			-theirs:"$REMOTE" -merged:"$MERGED"
-		check_unchanged
-	else
-		echo "TortoiseMerge cannot be used without a base" 1>&2
-		return 1
-	fi
-}
+can_diff () {
+	return 1
+}
+
+merge_cmd () {
+	if $base_present
+	then
+		touch "$BACKUP"
+		basename="$(basename "$merge_tool_path" .exe)"
+		if test "$basename" = "tortoisegitmerge"
+		then
+			"$merge_tool_path" \
+				-base "$BASE" -mine "$LOCAL" \
+				-theirs "$REMOTE" -merged "$MERGED"
+		else 
+			"$merge_tool_path" \
+				-base:"$BASE" -mine:"$LOCAL" \
+				-theirs:"$REMOTE" -merged:"$MERGED"
+		fi
+		check_unchanged
+	else
+		echo "$merge_tool_path cannot be used without a base" 1>&2
+		return 1
+	fi
+}
+
+translate_merge_tool_path() {
+	if type tortoisegitmerge >/dev/null 2>/dev/null
+	then
+		echo tortoisegitmerge
+	else
+		echo tortoisemerge
+	fi
+}
-- 
Best regards,
 Sven Strickroth
 PGP key id F5A9D4C4 @ any key-server

^ permalink raw reply related	[flat|nested] 31+ messages in thread

* Re: [PATCH] mergetools: Enhance tortoisemerge to work with
  2013-01-25 13:07             ` Sven Strickroth
@ 2013-01-25 18:28               ` Junio C Hamano
  2013-01-26  0:58                 ` Sven Strickroth
                                   ` (3 more replies)
  0 siblings, 4 replies; 31+ messages in thread
From: Junio C Hamano @ 2013-01-25 18:28 UTC (permalink / raw)
  To: Sven Strickroth; +Cc: git, David Aguilar, Sebastian Schuberth, Jeff King

Sven Strickroth <sven.strickroth@tu-clausthal.de> writes:

>  TortoiseGitMerge and filenames with spaces

??? ECANNOTPARSE.

... ah, wait.  Is this a broken-off tail of your subject line?

It may be a sign that you are doing too many unrelated things in a
single patch when your subject does not fit on a single line.

Perhaps this is better done as a two-patch series?

 * mergetools: fix tortoisemerge support for pathnames with SP
 * mergetools: support tortoisegitmerge

>  mergetools/tortoisemerge | 51 ++++++++++++++++++++++++++++++++----------------
>  1 file changed, 34 insertions(+), 17 deletions(-)
>
> diff --git a/mergetools/tortoisemerge b/mergetools/tortoisemerge
> index ed7db49..8ee99a5 100644
> --- a/mergetools/tortoisemerge
> +++ b/mergetools/tortoisemerge
> @@ -1,17 +1,34 @@
> -can_diff () {
> -	return 1
> -}
> -
> -merge_cmd () {
> -	if $base_present
> -	then
> -		touch "$BACKUP"
> -		"$merge_tool_path" \
> -			-base:"$BASE" -mine:"$LOCAL" \
> -			-theirs:"$REMOTE" -merged:"$MERGED"
> -		check_unchanged
> -	else
> -		echo "TortoiseMerge cannot be used without a base" 1>&2
> -		return 1
> -	fi
> -}
> +can_diff () {
> +	return 1
> +}
> +
> +merge_cmd () {
> +	if $base_present
> +	then
> +		touch "$BACKUP"
> +		basename="$(basename "$merge_tool_path" .exe)"
> +		if test "$basename" = "tortoisegitmerge"
> +		then
> +			"$merge_tool_path" \
> +				-base "$BASE" -mine "$LOCAL" \
> +				-theirs "$REMOTE" -merged "$MERGED"
> +		else 
> +			"$merge_tool_path" \
> +				-base:"$BASE" -mine:"$LOCAL" \
> +				-theirs:"$REMOTE" -merged:"$MERGED"

Hmph.

How was the support for "names with spaces" added in this new code?
I do not spot what is different between this "else" clause and the
original body of the merge_cmd (which only supported tortoisemerge).

They seem to be doing exactly the same thing.

> +		fi
> +		check_unchanged
> +	else
> +		echo "$merge_tool_path cannot be used without a base" 1>&2
> +		return 1
> +	fi
> +}
> +
> +translate_merge_tool_path() {
> +	if type tortoisegitmerge >/dev/null 2>/dev/null
> +	then
> +		echo tortoisegitmerge
> +	else
> +		echo tortoisemerge
> +	fi
> +}

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH] mergetools: Enhance tortoisemerge to work with
  2013-01-25 18:28               ` Junio C Hamano
@ 2013-01-26  0:58                 ` Sven Strickroth
  2013-01-26  1:14                 ` Sven Strickroth
                                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 31+ messages in thread
From: Sven Strickroth @ 2013-01-26  0:58 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, David Aguilar, Sebastian Schuberth, Jeff King

Am 25.01.2013 19:28 schrieb Junio C Hamano:> Sven Strickroth
<sven.strickroth@tu-clausthal.de> writes:
>
>>  TortoiseGitMerge and filenames with spaces
>
> ??? ECANNOTPARSE.
>
> ... ah, wait.  Is this a broken-off tail of your subject line?

Yes.

>> +		touch "$BACKUP"
>> +		basename="$(basename "$merge_tool_path" .exe)"
>> +		if test "$basename" = "tortoisegitmerge"
>> +		then
>> +			"$merge_tool_path" \
>> +				-base "$BASE" -mine "$LOCAL" \
>> +				-theirs "$REMOTE" -merged "$MERGED"
>> +		else
>> +			"$merge_tool_path" \
>> +				-base:"$BASE" -mine:"$LOCAL" \
>> +				-theirs:"$REMOTE" -merged:"$MERGED"
>
> Hmph.
>
> How was the support for "names with spaces" added in this new code?
> I do not spot what is different between this "else" clause and the
> original body of the merge_cmd (which only supported tortoisemerge).
>
> They seem to be doing exactly the same thing.

Erhm, no. As already stated and also mentioned in the commit log:
TortoiseMerge has cli parameter key-values separated by colons,
TortoiseGitMerge has key-values separated by spaces.

-- 
Best regards,
 Sven Strickroth
 PGP key id F5A9D4C4 @ any key-server

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH] mergetools: Enhance tortoisemerge to work with
  2013-01-25 18:28               ` Junio C Hamano
  2013-01-26  0:58                 ` Sven Strickroth
@ 2013-01-26  1:14                 ` Sven Strickroth
  2013-01-26  1:15                 ` [PATCH 1/2] mergetools: Added support for TortoiseGitMerge Sven Strickroth
  2013-01-26  1:17                 ` [PATCH 2/2] mergetools: Make tortoisemerge work with Sven Strickroth
  3 siblings, 0 replies; 31+ messages in thread
From: Sven Strickroth @ 2013-01-26  1:14 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, David Aguilar, Sebastian Schuberth, Jeff King

The TortoiseGit team renamed TortoiseMerge.exe to TortoiseGitMerge.exe
(starting with 1.8.0) in order to make clear that this one has special
support for git and prevent confusion with the TortoiseSVN TortoiseMerge
version.

Signed-off-by: Sven Strickroth <email@cs-ware.de>
---
 mergetools/tortoisemerge | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/mergetools/tortoisemerge b/mergetools/tortoisemerge
index ed7db49..8476afa 100644
--- a/mergetools/tortoisemerge
+++ b/mergetools/tortoisemerge
@@ -11,7 +11,16 @@ merge_cmd () {
 			-theirs:"$REMOTE" -merged:"$MERGED"
 		check_unchanged
 	else
-		echo "TortoiseMerge cannot be used without a base" 1>&2
+		echo "$merge_tool_path cannot be used without a base" 1>&2
 		return 1
 	fi
 }
+
+translate_merge_tool_path() {
+	if type tortoisegitmerge >/dev/null 2>/dev/null
+	then
+		echo tortoisegitmerge
+	else
+		echo tortoisemerge
+	fi
+}
-- 
Best regards,
 Sven Strickroth
 PGP key id F5A9D4C4 @ any key-server

^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [PATCH 1/2] mergetools: Added support for TortoiseGitMerge
  2013-01-25 18:28               ` Junio C Hamano
  2013-01-26  0:58                 ` Sven Strickroth
  2013-01-26  1:14                 ` Sven Strickroth
@ 2013-01-26  1:15                 ` Sven Strickroth
  2013-01-26  1:17                 ` [PATCH 2/2] mergetools: Make tortoisemerge work with Sven Strickroth
  3 siblings, 0 replies; 31+ messages in thread
From: Sven Strickroth @ 2013-01-26  1:15 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, David Aguilar, Sebastian Schuberth, Jeff King

The TortoiseGit team renamed TortoiseMerge.exe to TortoiseGitMerge.exe
(starting with 1.8.0) in order to make clear that this one has special
support for git and prevent confusion with the TortoiseSVN TortoiseMerge
version.

Signed-off-by: Sven Strickroth <email@cs-ware.de>
---
 mergetools/tortoisemerge | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/mergetools/tortoisemerge b/mergetools/tortoisemerge
index ed7db49..8476afa 100644
--- a/mergetools/tortoisemerge
+++ b/mergetools/tortoisemerge
@@ -11,7 +11,16 @@ merge_cmd () {
 			-theirs:"$REMOTE" -merged:"$MERGED"
 		check_unchanged
 	else
-		echo "TortoiseMerge cannot be used without a base" 1>&2
+		echo "$merge_tool_path cannot be used without a base" 1>&2
 		return 1
 	fi
 }
+
+translate_merge_tool_path() {
+	if type tortoisegitmerge >/dev/null 2>/dev/null
+	then
+		echo tortoisegitmerge
+	else
+		echo tortoisemerge
+	fi
+}
-- 
Best regards,
 Sven Strickroth
 PGP key id F5A9D4C4 @ any key-server

^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [PATCH 2/2] mergetools: Make tortoisemerge work with
  2013-01-25 18:28               ` Junio C Hamano
                                   ` (2 preceding siblings ...)
  2013-01-26  1:15                 ` [PATCH 1/2] mergetools: Added support for TortoiseGitMerge Sven Strickroth
@ 2013-01-26  1:17                 ` Sven Strickroth
  2013-01-26  7:10                   ` David Aguilar
  3 siblings, 1 reply; 31+ messages in thread
From: Sven Strickroth @ 2013-01-26  1:17 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, David Aguilar, Sebastian Schuberth, Jeff King

tortoisegitmerge and filesnames with space

The tortoisemerge mergetool does not work with filenames which have
a space in it. Fixing this required changes in git and also in
TortoiseGitMerge; see https://github.com/msysgit/msysgit/issues/57.

TortoiseGitMerge now separates cli parameter key-values by space instead
of colons as TortoiseSVN TortoiseMerge does and supports filesnames
with spaces in it this way now.

Signed-off-by: Sven Strickroth <email@cs-ware.de>
Reported-by: Sebastian Schuberth <sschuberth@gmail.com>
---
 mergetools/tortoisemerge | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/mergetools/tortoisemerge b/mergetools/tortoisemerge
index 8476afa..2f98829 100644
--- a/mergetools/tortoisemerge
+++ b/mergetools/tortoisemerge
@@ -6,9 +6,17 @@ merge_cmd () {
 	if $base_present
 	then
 		touch "$BACKUP"
-		"$merge_tool_path" \
-			-base:"$BASE" -mine:"$LOCAL" \
-			-theirs:"$REMOTE" -merged:"$MERGED"
+		basename="$(basename "$merge_tool_path" .exe)"
+		if test "$basename" = "tortoisegitmerge"
+		then
+			"$merge_tool_path" \
+				-base "$BASE" -mine "$LOCAL" \
+				-theirs "$REMOTE" -merged "$MERGED"
+		else 
+			"$merge_tool_path" \
+				-base:"$BASE" -mine:"$LOCAL" \
+				-theirs:"$REMOTE" -merged:"$MERGED"
+		fi
 		check_unchanged
 	else
 		echo "$merge_tool_path cannot be used without a base" 1>&2
-- 
Best regards,
 Sven Strickroth
 PGP key id F5A9D4C4 @ any key-server

^ permalink raw reply related	[flat|nested] 31+ messages in thread

* Re: [PATCH 2/2] mergetools: Make tortoisemerge work with
  2013-01-26  1:17                 ` [PATCH 2/2] mergetools: Make tortoisemerge work with Sven Strickroth
@ 2013-01-26  7:10                   ` David Aguilar
  2013-01-27  9:14                     ` Sven Strickroth
  0 siblings, 1 reply; 31+ messages in thread
From: David Aguilar @ 2013-01-26  7:10 UTC (permalink / raw)
  To: Sven Strickroth; +Cc: Junio C Hamano, git, Sebastian Schuberth, Jeff King

On Fri, Jan 25, 2013 at 5:17 PM, Sven Strickroth
<sven.strickroth@tu-clausthal.de> wrote:
> TortoiseGitMerge now separates cli parameter key-values by space instead
> of colons as TortoiseSVN TortoiseMerge does and supports filesnames
> with spaces in it this way now.

These patches look correct (I do not have the tool to test)
but I think we should fixup this commit message.

How about something like...

mergetools: Teach tortoisemerge about TortoiseGitMerge

TortoiseGitMerge improved its syntax to allow for file paths
with spaces.  Detect when it is installed and prefer it over
TortoiseMerge.
-- 
David

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH 2/2] mergetools: Make tortoisemerge work with
  2013-01-26  7:10                   ` David Aguilar
@ 2013-01-27  9:14                     ` Sven Strickroth
  2013-01-27 17:48                       ` Junio C Hamano
  0 siblings, 1 reply; 31+ messages in thread
From: Sven Strickroth @ 2013-01-27  9:14 UTC (permalink / raw)
  To: git; +Cc: David Aguilar, Junio C Hamano, Sebastian Schuberth, Jeff King

Am 26.01.2013 08:10 schrieb David Aguilar:
> These patches look correct (I do not have the tool to test)
> but I think we should fixup this commit message.
> 
> How about something like...
> 
> mergetools: Teach tortoisemerge about TortoiseGitMerge
> 
> TortoiseGitMerge improved its syntax to allow for file paths
> with spaces.  Detect when it is installed and prefer it over
> TortoiseMerge.

This message implies, that I have to combine two patches again?!

-- 
Best regards,
 Sven Strickroth
 PGP key id F5A9D4C4 @ any key-server

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH 2/2] mergetools: Make tortoisemerge work with
  2013-01-27  9:14                     ` Sven Strickroth
@ 2013-01-27 17:48                       ` Junio C Hamano
  2013-02-01 19:33                         ` [PATCH] mergetools: Enable tortoisemerge to handle filenames with Sven Strickroth
  0 siblings, 1 reply; 31+ messages in thread
From: Junio C Hamano @ 2013-01-27 17:48 UTC (permalink / raw)
  To: Sven Strickroth; +Cc: git, David Aguilar, Sebastian Schuberth, Jeff King

Sven Strickroth <sven.strickroth@tu-clausthal.de> writes:

> Am 26.01.2013 08:10 schrieb David Aguilar:
>> These patches look correct (I do not have the tool to test)
>> but I think we should fixup this commit message.
>> 
>> How about something like...
>> 
>> mergetools: Teach tortoisemerge about TortoiseGitMerge
>> 
>> TortoiseGitMerge improved its syntax to allow for file paths
>> with spaces.  Detect when it is installed and prefer it over
>> TortoiseMerge.
>
> This message implies, that I have to combine two patches again?!

We can see that [1/2] teaches mergetool to use tortoisegitmerge when
the user tells it to use tortoisemerge and the former is available.

The change in [2/2] is to use -base "$BASE" instead of -base:"$BASE"
when the real tool is tortoisegitmerge (when it is tortoisemerge,
nothing changes).

By reading these two patches, I would imagine that tortoisegitmerge
can accept both forms, i.e. -base:"$BASE" and -base "$BASE", but the
patch [2/2] considers that the latter form is preferrable in some
way.  As you talked about "paths with SPs in them", I would imagine
that is the difference?  That is -base:"$BASE" form will not work if
the varilable $BASE has a SP in it (even though it is encolosed in
dq, which does not make much sense from my POSIXy point of view, but
perhaps command line argument processing in the Windows land may
have different rules) but if you write -base "$BASE" then "$BASE"
will be taken as a single thing even it has a SP in it?  Also I
would guess that the reason why patch [2/2] does this only for
tortoisegitmerge is either because tortoisemerge will break paths
with SPs even if it is given -base "$BASE" form, or because it only
accepts -base:"$BASE" form? I cannot read it from your description,
but let's assume that is the reason.

If that is the case, then the log message for the second patch would
be easier to understand if it says so in a more explicit way,
perhaps like this:

	TortoiseGitMerge, unlike TortoiseMerge, can be told to
	handle paths with SPs in them by using -option "$FILE" (not
	-option:"$FILE", which does not work for such paths) syntax.

	Use it to allow such paths to be handled correctly.

But I cannot read exactly why the patch [2/2] considers -base "$BASE"
is preferrable over -base:"$BASE" from your original description, so
this may well be way off the mark.

In short, I think proposed log message for [2/2] was not clear what
is being fixed and how.

^ permalink raw reply	[flat|nested] 31+ messages in thread

* [PATCH] mergetools: Enable tortoisemerge to handle filenames with
  2013-01-27 17:48                       ` Junio C Hamano
@ 2013-02-01 19:33                         ` Sven Strickroth
  2013-02-01 20:07                           ` Sebastian Schuberth
  0 siblings, 1 reply; 31+ messages in thread
From: Sven Strickroth @ 2013-02-01 19:33 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, David Aguilar, Sebastian Schuberth, Jeff King

spaces with TortoiseGitMerge

TortoiseGitMerge, unlike TortoiseMerge, can be told to handle paths
with spaces in them by using -option "$FILE" (not -option:"$FILE",
which does not work for such paths) syntax. Both do not have a fully
posix compatible cli parameter parser, however, TortoiseGitMerge was
modified in order to handle filenames with spaces correctly. The
"-key value" form was choosen because this way no escaping for
quotes within quotes is necessary; see
https://github.com/msysgit/msysgit/issues/57

Signed-off-by: Sven Strickroth <email@cs-ware.de>
Reported-by: Sebastian Schuberth <sschuberth@gmail.com>
---
 mergetools/tortoisemerge | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/mergetools/tortoisemerge b/mergetools/tortoisemerge
index 8476afa..3b89f1c 100644
--- a/mergetools/tortoisemerge
+++ b/mergetools/tortoisemerge
@@ -6,9 +6,17 @@ merge_cmd () {
 	if $base_present
 	then
 		touch "$BACKUP"
-		"$merge_tool_path" \
-			-base:"$BASE" -mine:"$LOCAL" \
-			-theirs:"$REMOTE" -merged:"$MERGED"
+		basename="$(basename "$merge_tool_path" .exe)"
+		if test "$basename" = "tortoisegitmerge"
+		then
+			"$merge_tool_path" \
+				-base "$BASE" -mine "$LOCAL" \
+				-theirs "$REMOTE" -merged "$MERGED"
+		else
+			"$merge_tool_path" \
+				-base:"$BASE" -mine:"$LOCAL" \
+				-theirs:"$REMOTE" -merged:"$MERGED"
+		fi
 		check_unchanged
 	else
 		echo "$merge_tool_path cannot be used without a base" 1>&2
-- 
Best regards,
 Sven Strickroth
 PGP key id F5A9D4C4 @ any key-server

^ permalink raw reply related	[flat|nested] 31+ messages in thread

* Re: [PATCH] mergetools: Enable tortoisemerge to handle filenames with
  2013-02-01 19:33                         ` [PATCH] mergetools: Enable tortoisemerge to handle filenames with Sven Strickroth
@ 2013-02-01 20:07                           ` Sebastian Schuberth
  2013-02-01 20:10                             ` Sven Strickroth
                                               ` (2 more replies)
  0 siblings, 3 replies; 31+ messages in thread
From: Sebastian Schuberth @ 2013-02-01 20:07 UTC (permalink / raw)
  To: Sven Strickroth; +Cc: git, Junio C Hamano, David Aguilar, Jeff King

On Fri, Feb 1, 2013 at 8:33 PM, Sven Strickroth
<sven.strickroth@tu-clausthal.de> wrote:

> TortoiseGitMerge, unlike TortoiseMerge, can be told to handle paths
> with spaces in them by using -option "$FILE" (not -option:"$FILE",
> which does not work for such paths) syntax. Both do not have a fully
> posix compatible cli parameter parser, however, TortoiseGitMerge was
> modified in order to handle filenames with spaces correctly. The
> "-key value" form was choosen because this way no escaping for
> quotes within quotes is necessary; see
> https://github.com/msysgit/msysgit/issues/57

The commit message still does not mention MSYS path mangling at all,
which probably is why the reasoning of this patch was not yet fully
understood. I'd recommend something like the following:

mergetools: Teach tortoisemerge about TortoiseGitMerge

TortoiseGitMerge is an improved version of TortoiseMerge specifically
for use with Git on Windows. Due to MSYS path mangling [1], the ":"
after the "base" etc. arguments to TortoiseMerge caused to whole
argument instead of just the file name to be quoted in case of file
names with spaces. So TortoiseMerge was passed

    "-base:new file.txt"

instead of

    -base:"new file.txt"

(including the quotes). To work around this, TortoiseGitMerge does not
require the ":" after the arguments anymore which fixes handling file
names with spaces.

[1] http://www.mingw.org/wiki/Posix_path_conversion

-- 
Sebastian Schuberth

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH] mergetools: Enable tortoisemerge to handle filenames with
  2013-02-01 20:07                           ` Sebastian Schuberth
@ 2013-02-01 20:10                             ` Sven Strickroth
  2013-02-01 20:15                             ` Junio C Hamano
  2013-02-01 20:16                             ` [PATCH] mergetools: Enable tortoisemerge to handle filenames with spaces with TortoiseGitMerge Sven Strickroth
  2 siblings, 0 replies; 31+ messages in thread
From: Sven Strickroth @ 2013-02-01 20:10 UTC (permalink / raw)
  To: Sebastian Schuberth; +Cc: git, Junio C Hamano, David Aguilar, Jeff King

Am 01.02.2013 21:07 schrieb Sebastian Schuberth:
> mergetools: Teach tortoisemerge about TortoiseGitMerge

This subject doesn't make any sense if we don't combine the two patches.

-- 
Best regards,
 Sven Strickroth
 PGP key id F5A9D4C4 @ any key-server

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH] mergetools: Enable tortoisemerge to handle filenames with
  2013-02-01 20:07                           ` Sebastian Schuberth
  2013-02-01 20:10                             ` Sven Strickroth
@ 2013-02-01 20:15                             ` Junio C Hamano
  2013-02-01 20:17                               ` Sven Strickroth
  2013-02-01 20:16                             ` [PATCH] mergetools: Enable tortoisemerge to handle filenames with spaces with TortoiseGitMerge Sven Strickroth
  2 siblings, 1 reply; 31+ messages in thread
From: Junio C Hamano @ 2013-02-01 20:15 UTC (permalink / raw)
  To: Sebastian Schuberth; +Cc: Sven Strickroth, git, David Aguilar, Jeff King

Sebastian Schuberth <sschuberth@gmail.com> writes:

> The commit message still does not mention MSYS path mangling at all,
> which probably is why the reasoning of this patch was not yet fully
> understood.

Ahh, you are very right.  I didn't realize that was what this funny
"with colon, with SP" business was about.

> I'd recommend something like the following:
>
> mergetools: Teach tortoisemerge about TortoiseGitMerge
>
> TortoiseGitMerge is an improved version of TortoiseMerge specifically
> for use with Git on Windows. Due to MSYS path mangling [1], the ":"
> after the "base" etc. arguments to TortoiseMerge caused to whole
> argument instead of just the file name to be quoted in case of file
> names with spaces. So TortoiseMerge was passed
>
>     "-base:new file.txt"
>
> instead of
>
>     -base:"new file.txt"
>
> (including the quotes). To work around this, TortoiseGitMerge does not
> require the ":" after the arguments anymore which fixes handling file
> names with spaces.
>
> [1] http://www.mingw.org/wiki/Posix_path_conversion

Sven?

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH] mergetools: Enable tortoisemerge to handle filenames with spaces with TortoiseGitMerge
  2013-02-01 20:07                           ` Sebastian Schuberth
  2013-02-01 20:10                             ` Sven Strickroth
  2013-02-01 20:15                             ` Junio C Hamano
@ 2013-02-01 20:16                             ` Sven Strickroth
  2013-02-02  1:59                               ` David Aguilar
  2 siblings, 1 reply; 31+ messages in thread
From: Sven Strickroth @ 2013-02-01 20:16 UTC (permalink / raw)
  To: Sebastian Schuberth; +Cc: git, Junio C Hamano, David Aguilar, Jeff King

TortoiseGitMerge, unlike TortoiseMerge, can be told to handle paths
with spaces in them by using -option "$FILE" (not -option:"$FILE",
which does not work for such paths) syntax.

This change was necessary because of MSYS path mangling [1], the ":"
after the "base" etc. arguments to TortoiseMerge caused to whole
argument instead of just the file name to be quoted in case of file
names with spaces. So TortoiseMerge was passed

    "-base:new file.txt"

instead of

    -base:"new file.txt"

(including the quotes). To work around this, TortoiseGitMerge does not
require the ":" after the arguments anymore which fixes handling file
names with spaces [2] (as written above).

[1] http://www.mingw.org/wiki/Posix_path_conversion
[2] https://github.com/msysgit/msysgit/issues/57

Signed-off-by: Sven Strickroth <email@cs-ware.de>
Reported-by: Sebastian Schuberth <sschuberth@gmail.com>
---
 mergetools/tortoisemerge | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/mergetools/tortoisemerge b/mergetools/tortoisemerge
index 8476afa..3b89f1c 100644
--- a/mergetools/tortoisemerge
+++ b/mergetools/tortoisemerge
@@ -6,9 +6,17 @@ merge_cmd () {
 	if $base_present
 	then
 		touch "$BACKUP"
-		"$merge_tool_path" \
-			-base:"$BASE" -mine:"$LOCAL" \
-			-theirs:"$REMOTE" -merged:"$MERGED"
+		basename="$(basename "$merge_tool_path" .exe)"
+		if test "$basename" = "tortoisegitmerge"
+		then
+			"$merge_tool_path" \
+				-base "$BASE" -mine "$LOCAL" \
+				-theirs "$REMOTE" -merged "$MERGED"
+		else
+			"$merge_tool_path" \
+				-base:"$BASE" -mine:"$LOCAL" \
+				-theirs:"$REMOTE" -merged:"$MERGED"
+		fi
 		check_unchanged
 	else
 		echo "$merge_tool_path cannot be used without a base" 1>&2
-- 
1.8.1.msysgit.1

^ permalink raw reply related	[flat|nested] 31+ messages in thread

* Re: [PATCH] mergetools: Enable tortoisemerge to handle filenames with
  2013-02-01 20:15                             ` Junio C Hamano
@ 2013-02-01 20:17                               ` Sven Strickroth
  0 siblings, 0 replies; 31+ messages in thread
From: Sven Strickroth @ 2013-02-01 20:17 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Sebastian Schuberth, git, David Aguilar, Jeff King

Am 01.02.2013 21:15 schrieb Junio C Hamano:
>> TortoiseGitMerge is an improved version of TortoiseMerge specifically
>> for use with Git on Windows. Due to MSYS path mangling [1], the ":"
>> after the "base" etc. arguments to TortoiseMerge caused to whole
>> argument instead of just the file name to be quoted in case of file
>> names with spaces. So TortoiseMerge was passed
>>
>>     "-base:new file.txt"
>>
>> instead of
>>
>>     -base:"new file.txt"
>>
>> (including the quotes). To work around this, TortoiseGitMerge does not
>> require the ":" after the arguments anymore which fixes handling file
>> names with spaces.
>>
>> [1] http://www.mingw.org/wiki/Posix_path_conversion
> 
> Sven?

I just mailed a new patch. Thanks to Sebastian for pointing this out!

@Junio: Feel free to optimize the commit message.

-- 
Best regards,
 Sven Strickroth
 PGP key id F5A9D4C4 @ any key-server

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH] mergetools: Enable tortoisemerge to handle filenames with spaces with TortoiseGitMerge
  2013-02-01 20:16                             ` [PATCH] mergetools: Enable tortoisemerge to handle filenames with spaces with TortoiseGitMerge Sven Strickroth
@ 2013-02-02  1:59                               ` David Aguilar
  2013-02-02  2:08                                 ` Junio C Hamano
  0 siblings, 1 reply; 31+ messages in thread
From: David Aguilar @ 2013-02-02  1:59 UTC (permalink / raw)
  To: Sven Strickroth; +Cc: Sebastian Schuberth, git, Junio C Hamano, Jeff King

On Fri, Feb 1, 2013 at 12:16 PM, Sven Strickroth
<sven.strickroth@tu-clausthal.de> wrote:
> TortoiseMerge caused to whole
> argument instead of just the file name to be quoted

s/caused to whole/caused the whole/

I think this commit message is very nice.  Is it too late to replace
the current patch with this one?
-- 
David

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH] mergetools: Enable tortoisemerge to handle filenames with spaces with TortoiseGitMerge
  2013-02-02  1:59                               ` David Aguilar
@ 2013-02-02  2:08                                 ` Junio C Hamano
  0 siblings, 0 replies; 31+ messages in thread
From: Junio C Hamano @ 2013-02-02  2:08 UTC (permalink / raw)
  To: David Aguilar; +Cc: Sven Strickroth, Sebastian Schuberth, git, Jeff King

David Aguilar <davvid@gmail.com> writes:

> On Fri, Feb 1, 2013 at 12:16 PM, Sven Strickroth
> <sven.strickroth@tu-clausthal.de> wrote:
>> TortoiseMerge caused to whole
>> argument instead of just the file name to be quoted
>
> s/caused to whole/caused the whole/
>
> I think this commit message is very nice.  Is it too late to replace
> the current patch with this one?

Haven't merged it to 'next'; I will replace with this, with a bit of
retitling to make it shorter.


commit 81ed7b9581f7eafb334824264abb492d85a5ffb8
Author: Sven Strickroth <sven.strickroth@tu-clausthal.de>
Date:   Fri Feb 1 21:16:30 2013 +0100

    mergetools: teach tortoisemerge to handle filenames with SP correctly
    
    TortoiseGitMerge, unlike TortoiseMerge, can be told to handle paths
    with spaces in them by using -option "$FILE" (not -option:"$FILE",
    which does not work for such paths) syntax.
    
    This change was necessary because of MSYS path mangling [1], the ":"
    after the "base" etc. arguments to TortoiseMerge caused the whole
    argument instead of just the file name to be quoted in case of file
    names with spaces. So TortoiseMerge was passed
    
        "-base:new file.txt"
    
    instead of
    
        -base:"new file.txt"
    
    (including the quotes). To work around this, TortoiseGitMerge does not
    require the ":" after the arguments anymore which fixes handling file
    names with spaces [2] (as written above).
    
    [1] http://www.mingw.org/wiki/Posix_path_conversion
    [2] https://github.com/msysgit/msysgit/issues/57
    
    Signed-off-by: Sven Strickroth <email@cs-ware.de>
    Reported-by: Sebastian Schuberth <sschuberth@gmail.com>
    Signed-off-by: Junio C Hamano <gitster@pobox.com>

^ permalink raw reply	[flat|nested] 31+ messages in thread

end of thread, other threads:[~2013-02-02  2:08 UTC | newest]

Thread overview: 31+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-01-20 11:27 [PATCH] mergetools: Add tortoisegitmerge helper Sven Strickroth
2013-01-21  0:43 ` Junio C Hamano
2013-01-21  8:24   ` Sven Strickroth
2013-01-21  8:26   ` Sven Strickroth
2013-01-24 11:19     ` Sven Strickroth
2013-01-24 19:51     ` Junio C Hamano
2013-01-24 22:07       ` Sven Strickroth
2013-01-24 22:15         ` Junio C Hamano
2013-01-25  6:11           ` David Aguilar
2013-01-25  7:21             ` Junio C Hamano
2013-01-25  7:54               ` David Aguilar
2013-01-25  9:48                 ` John Keeping
2013-01-25  9:06         ` [PATCH] mergetools: Enhance tortoisemerge to work with Sven Strickroth
2013-01-25 10:09           ` David Aguilar
2013-01-25 13:07             ` Sven Strickroth
2013-01-25 18:28               ` Junio C Hamano
2013-01-26  0:58                 ` Sven Strickroth
2013-01-26  1:14                 ` Sven Strickroth
2013-01-26  1:15                 ` [PATCH 1/2] mergetools: Added support for TortoiseGitMerge Sven Strickroth
2013-01-26  1:17                 ` [PATCH 2/2] mergetools: Make tortoisemerge work with Sven Strickroth
2013-01-26  7:10                   ` David Aguilar
2013-01-27  9:14                     ` Sven Strickroth
2013-01-27 17:48                       ` Junio C Hamano
2013-02-01 19:33                         ` [PATCH] mergetools: Enable tortoisemerge to handle filenames with Sven Strickroth
2013-02-01 20:07                           ` Sebastian Schuberth
2013-02-01 20:10                             ` Sven Strickroth
2013-02-01 20:15                             ` Junio C Hamano
2013-02-01 20:17                               ` Sven Strickroth
2013-02-01 20:16                             ` [PATCH] mergetools: Enable tortoisemerge to handle filenames with spaces with TortoiseGitMerge Sven Strickroth
2013-02-02  1:59                               ` David Aguilar
2013-02-02  2:08                                 ` Junio C Hamano

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.