threads / patch / 39381

v6, 2 partsmergetool--lib: set IFS for difftool and mergetool

Subject: [PATCH v6 1/2] mergetool--lib: set IFS for difftool and mergetool

## tl;dr

11 messages between May 20, 2015 and May 22, 2015. Diffs are folded; open one to read it.

replies: 10people: 5as markdown or json

David Aguilar· May 20, 2015, 09:07 UTC · lore

git-sh-setup sets IFS but it is not used by git-difftool--helper. Set IFS in git-mergetool--lib so that the mergetool scriptlets, difftool, and mergetool do not need to do so.

Signed-off-by: David Aguilar <davvid@gmail.com>
---
Changes since v5: fixed "diftool" typo in commit message
 git-mergetool--lib.sh | 3 +++
 git-mergetool.sh      | 2 --
 2 files changed, 3 insertions(+), 2 deletions(-)
Show changes to 2 files +3 −2

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

diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
index fe61e89..14b039d 100644
--- a/git-mergetool--lib.sh
+++ b/git-mergetool--lib.sh
@@ -2,6 +2,9 @@
 
 : ${MERGE_TOOLS_DIR=$(git --exec-path)/mergetools}
 
+IFS='
+'
+
 mode_ok () {
 	if diff_mode
 	then
diff --git a/git-mergetool.sh b/git-mergetool.sh
index d20581c..9f77e3a 100755
--- a/git-mergetool.sh
+++ b/git-mergetool.sh
@@ -451,8 +451,6 @@ fi
 printf "Merging:\n"
 printf "%s\n" "$files"
 
-IFS='
-'
 rc=0
 for i in $files
 do

-- 
2.4.1.218.gc09a0e5
David Aguilar· May 20, 2015, 09:07 UTC · re: David Aguilar · lore

[PATCH v6 2/2] mergetools: add winmerge as a builtin tool

Add a winmerge scriptlet with the commands described in [1] so that users can use winmerge without needing to perform any additional configuration.

[1] http://thread.gmane.org/gmane.comp.version-control.git/268631
Helped-by: Philip Oakley <philipoakley@iee.org>
Helped-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Helped-by: Sebastian Schuberth <sschuberth@gmail.com>
Helped-by: SZEDER Gábor <szeder@ira.uka.de>
Signed-off-by: David Aguilar <davvid@gmail.com>
---
Unchanged since v5.
Changes since v4: IFS is left as-is since it's done in 1/2
 mergetools/winmerge | 36 ++++++++++++++++++++++++++++++++++++
 1 file changed, 36 insertions(+)
 create mode 100644 mergetools/winmerge
Show changes to mergetools/winmerge +36 −0
diff --git a/mergetools/winmerge b/mergetools/winmerge
new file mode 100644
index 0000000..74a66d4
--- /dev/null
+++ b/mergetools/winmerge
@@ -0,0 +1,36 @@
+diff_cmd () {
+	"$merge_tool_path" -u -e "$LOCAL" "$REMOTE"
+	return 0
+}
+
+merge_cmd () {
+	# mergetool.winmerge.trustExitCode is implicitly false.
+	# touch $BACKUP so that we can check_unchanged.
+	touch "$BACKUP"
+	"$merge_tool_path" -u -e -dl Local -dr Remote \
+		"$LOCAL" "$REMOTE" "$MERGED"
+	check_unchanged
+}
+
+translate_merge_tool_path() {
+	# Use WinMergeU.exe if it exists in $PATH
+	if type -p WinMergeU.exe >/dev/null 2>&1
+	then
+		printf WinMergeU.exe
+		return
+	fi
+
+	# Look for WinMergeU.exe in the typical locations
+	winmerge_exe="WinMerge/WinMergeU.exe"
+	for directory in $(env | grep -Ei '^PROGRAM(FILES(\(X86\))?|W6432)=' |
+		cut -d '=' -f 2- | sort -u)
+	do
+		if test -n "$directory" && test -x "$directory/$winmerge_exe"
+		then
+			printf '%s' "$directory/$winmerge_exe"
+			return
+		fi
+	done
+
+	printf WinMergeU.exe
+}

-- 
2.4.1.218.gc09a0e5
SZEDER Gábor· May 20, 2015, 11:09 UTC · re: David Aguilar · lore

Re: [PATCH v6 2/2] mergetools: add winmerge as a builtin tool

Quoting David Aguilar <davvid@gmail.com>:
Show 10 quoted lines
> +translate_merge_tool_path() {
> +	# Use WinMergeU.exe if it exists in $PATH
> +	if type -p WinMergeU.exe >/dev/null 2>&1
> +	then
> +		printf WinMergeU.exe
> +		return
> +	fi
> +
> +	# Look for WinMergeU.exe in the typical locations
> +	winmerge_exe="WinMerge/WinMergeU.exe"

This variable is not used elsewhere, right? Then you might want to mark it as local to make this clear.

Show 11 quoted lines
> +	for directory in $(env | grep -Ei '^PROGRAM(FILES(\(X86\))?|W6432)=' |
> +		cut -d '=' -f 2- | sort -u)
> +	do
> +		if test -n "$directory" && test -x "$directory/$winmerge_exe"
> +		then
> +			printf '%s' "$directory/$winmerge_exe"
> +			return
> +		fi
> +	done
> +
> +	printf WinMergeU.exe

Please pardon my ignorance and curiosity, but what is the purpose of this last printf? It outputs the same as in the case when winmerge is in $PATH at the beginning of the function. However, if we reach this printf, then winmerge is not in $PATH, so what will be executed?

Gábor
> +}
> --
> 2.4.1.218.gc09a0e5
David Aguilar· May 22, 2015, 19:58 UTC · re: SZEDER Gábor · lore

Re: [PATCH v6 2/2] mergetools: add winmerge as a builtin tool

[just wrapping up the unaswered questions in this thread]
On Wed, May 20, 2015 at 01:09:29PM +0200, SZEDER Gábor wrote:
Show 16 quoted lines
> 
> Quoting David Aguilar <davvid@gmail.com>:
> 
> >+translate_merge_tool_path() {
> >+	# Use WinMergeU.exe if it exists in $PATH
> >+	if type -p WinMergeU.exe >/dev/null 2>&1
> >+	then
> >+		printf WinMergeU.exe
> >+		return
> >+	fi
> >+
> >+	# Look for WinMergeU.exe in the typical locations
> >+	winmerge_exe="WinMerge/WinMergeU.exe"
> 
> This variable is not used elsewhere, right?  Then you might want to
> mark it as local to make this clear.
"local" is a bash-ism, otherwise that'd be a good idea.
Show 17 quoted lines
> >+	for directory in $(env | grep -Ei '^PROGRAM(FILES(\(X86\))?|W6432)=' |
> >+		cut -d '=' -f 2- | sort -u)
> >+	do
> >+		if test -n "$directory" && test -x "$directory/$winmerge_exe"
> >+		then
> >+			printf '%s' "$directory/$winmerge_exe"
> >+			return
> >+		fi
> >+	done
> >+
> >+	printf WinMergeU.exe
> 
> Please pardon my ignorance and curiosity, but what is the purpose of
> this last printf?
> It outputs the same as in the case when winmerge is in $PATH at the
> beginning of the function.  However, if we reach this printf, then
> winmerge is not in $PATH, so what will be executed?

This function maps what we call the tool (winmerge) to the actual executable. That last printf provides the following behavior:

	$ git difftool -t winmerge HEAD~
	
	Viewing (1/1): 'mergetools/winmerge'
	Launch 'winmerge' [Y/n]:
	The diff tool winmerge is not available as 'WinMergeU.exe'
	fatal: external diff died, stopping at mergetools/winmerge

It ensures that the user sees 'WinMergeU.exe' in the error message. That way the user can resolve the problem by e.g. adjusting their $PATH, or realizing that they don't have WinMergeU.exe installed.

-- 
David
Junio C Hamano· May 22, 2015, 20:05 UTC · re: David Aguilar · lore

Re: [PATCH v6 2/2] mergetools: add winmerge as a builtin tool

David Aguilar <davvid@gmail.com> writes:
> [just wrapping up the unaswered questions in this thread]
> ...
> On Wed, May 20, 2015 at 01:09:29PM +0200, SZEDER Gábor wrote:
Thanks for clarifications.  I think all is good now?
David Aguilar· May 22, 2015, 20:16 UTC · re: Junio C Hamano · lore

Re: [PATCH v6 2/2] mergetools: add winmerge as a builtin tool

On Fri, May 22, 2015 at 01:05:28PM -0700, Junio C Hamano wrote:
Show 7 quoted lines
> David Aguilar <davvid@gmail.com> writes:
> 
> > [just wrapping up the unaswered questions in this thread]
> > ...
> > On Wed, May 20, 2015 at 01:09:29PM +0200, SZEDER Gábor wrote:
> 
> Thanks for clarifications.  I think all is good now?

Yes, I think so. I just looked at what you have queued in pu and it looks good.

Thanks all,
-- 
David
Junio C Hamano· May 20, 2015, 20:13 UTC · re: David Aguilar · lore

Re: [PATCH v6 2/2] mergetools: add winmerge as a builtin tool

David Aguilar <davvid@gmail.com> writes:
> +	for directory in $(env | grep -Ei '^PROGRAM(FILES(\(X86\))?|W6432)=' |
> +		cut -d '=' -f 2- | sort -u)

Is the final "sort" really desired? I am wondering if there are fixed precedence/preference order among variants of %PROGRAMFILES% environment variables that the users on the platform are expected to stick to, but the "sort" is sorting by the absolute pathnames of where these things are, which may not reflect that order.

Not that I personally care too deeply, as I would expect that it is likely any one of them found would just work fine ;-)

Show 10 quoted lines
> +	do
> +		if test -n "$directory" && test -x "$directory/$winmerge_exe"
> +		then
> +			printf '%s' "$directory/$winmerge_exe"
> +			return
> +		fi
> +	done
> +
> +	printf WinMergeU.exe
> +}
Sebastian Schuberth· May 20, 2015, 20:20 UTC · re: Junio C Hamano · lore

Re: [PATCH v6 2/2] mergetools: add winmerge as a builtin tool

On Wed, May 20, 2015 at 10:13 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 10 quoted lines
> David Aguilar <davvid@gmail.com> writes:
>
>> +     for directory in $(env | grep -Ei '^PROGRAM(FILES(\(X86\))?|W6432)=' |
>> +             cut -d '=' -f 2- | sort -u)
>
> Is the final "sort" really desired?  I am wondering if there are
> fixed precedence/preference order among variants of %PROGRAMFILES%
> environment variables that the users on the platform are expected
> to stick to, but the "sort" is sorting by the absolute pathnames of
> where these things are, which may not reflect that order.

I did add the sort (and -u) by intention, to ensure that "C:\Program Files" (which is what %PROGRAMFILES% expands to by default) comes before "C:\Program Files (x86)" (which is what %PROGRAMFILES(X86)% expands to by default), so that programs of the OS-native bitness are preferred.

-- 
Sebastian Schuberth
Junio C Hamano· May 20, 2015, 21:00 UTC · re: Sebastian Schuberth · lore

Re: [PATCH v6 2/2] mergetools: add winmerge as a builtin tool

Sebastian Schuberth <sschuberth@gmail.com> writes:
Show 18 quoted lines
> On Wed, May 20, 2015 at 10:13 PM, Junio C Hamano <gitster@pobox.com> wrote:
>
>> David Aguilar <davvid@gmail.com> writes:
>>
>>> +     for directory in $(env | grep -Ei '^PROGRAM(FILES(\(X86\))?|W6432)=' |
>>> +             cut -d '=' -f 2- | sort -u)
>>
>> Is the final "sort" really desired?  I am wondering if there are
>> fixed precedence/preference order among variants of %PROGRAMFILES%
>> environment variables that the users on the platform are expected
>> to stick to, but the "sort" is sorting by the absolute pathnames of
>> where these things are, which may not reflect that order.
>
> I did add the sort (and -u) by intention, to ensure that "C:\Program
> Files" (which is what %PROGRAMFILES% expands to by default) comes
> before "C:\Program Files (x86)" (which is what %PROGRAMFILES(X86)%
> expands to by default), so that programs of the OS-native bitness are
> preferred.

Yuck. So even though %PROGRAMFILES% and %PROGRAMFILES(X86)% look as if they are variables that can point at arbitrary places, they in reality don't? Otherwise %PROGRAMFILES% may point at D:\Program while %PROGRAMFILES(X86)% may piont at C:\X86 and the latter would sort before the former, which would defeat that "logic".

But of course if I view this not as a "logic" but as a "heuristics" that happens to do the right thing in common environments, it is perfectly OK ;-).

I've queued the patches as-is.
Thanks.
Sebastian Schuberth· May 20, 2015, 21:20 UTC · re: Junio C Hamano · lore

Re: [PATCH v6 2/2] mergetools: add winmerge as a builtin tool

On Wed, May 20, 2015 at 11:00 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Yuck.  So even though %PROGRAMFILES% and %PROGRAMFILES(X86)% look as
> if they are variables that can point at arbitrary places, they in
> reality don't?  Otherwise %PROGRAMFILES% may point at D:\Program

Correct. In the vast majority of WIndows installations these variables contain the default values.

> But of course if I view this not as a "logic" but as a "heuristics"
> that happens to do the right thing in common environments, it is
> perfectly OK ;-).
Exactly a heuristic is what it's supposed to be :-)
-- 
Sebastian Schuberth
Johannes Schindelin· May 21, 2015, 10:06 UTC · re: Junio C Hamano · lore

Re: [PATCH v6 2/2] mergetools: add winmerge as a builtin tool

Hi Junio,
On 2015-05-20 23:00, Junio C Hamano wrote:
Show 26 quoted lines
> Sebastian Schuberth <sschuberth@gmail.com> writes:
> 
>> On Wed, May 20, 2015 at 10:13 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>
>>> David Aguilar <davvid@gmail.com> writes:
>>>
>>>> +     for directory in $(env | grep -Ei '^PROGRAM(FILES(\(X86\))?|W6432)=' |
>>>> +             cut -d '=' -f 2- | sort -u)
>>>
>>> Is the final "sort" really desired?  I am wondering if there are
>>> fixed precedence/preference order among variants of %PROGRAMFILES%
>>> environment variables that the users on the platform are expected
>>> to stick to, but the "sort" is sorting by the absolute pathnames of
>>> where these things are, which may not reflect that order.
>>
>> I did add the sort (and -u) by intention, to ensure that "C:\Program
>> Files" (which is what %PROGRAMFILES% expands to by default) comes
>> before "C:\Program Files (x86)" (which is what %PROGRAMFILES(X86)%
>> expands to by default), so that programs of the OS-native bitness are
>> preferred.
> 
> Yuck.  So even though %PROGRAMFILES% and %PROGRAMFILES(X86)% look as
> if they are variables that can point at arbitrary places, they in
> reality don't?  Otherwise %PROGRAMFILES% may point at D:\Program
> while %PROGRAMFILES(X86)% may piont at C:\X86 and the latter would
> sort before the former, which would defeat that "logic".
Well, you are correct, theoretically an administrator could set the registry values (which are the source of the environment variables in question) of the `ProgramFilesDir` key in both
HKEY_LOCAL_MACHINE\Software\Microsoft\Windows\CurrentVersion
and
HKEY_LOCAL_MACHINE\Software\WOW64\Microsoft\Windows\CurrentVersion
to wildly different locations as you outlined. However, it is not supported by Microsoft to change those locations via the registry:
https://support.microsoft.com/en-us/kb/933700

Ciao, Dscho

← back to recent threads