Re: [PATCH v2] mergetools: add winmerge as a builtin tool

[Sebastian Schuberth <sschuberth@gmail.com>]

On 13.05.2015 04:00, David Aguilar wrote:

> +	if test -n "$ProgramW6432" && test -x "$ProgramW6432/$winmerge_exe"
> +	then
> +		printf '%s' "$ProgramW6432/$winmerge_exe"

I don't think it makes sense to check "$ProgramW6432". The content of 
that variable depends on the bitness of the process requesting the 
environment. Just checking "$PROGRAMFILES" and "$PROGRAMFILES(X86)" 
should be sufficient and more clear.

Also, note that you should use all upper case names when referring to 
Windows environment variables. While it's true that on plain Windows 
environment variable names are case-insensitive, MSYS1/2 converts all 
variable names to upper case and *is* case sensitive. I.e. while "echo 
$PROGRAMFILES" works as expected from a Git Bash on Windows, "echo 
$ProgramFiles" results in an empty string.

 > +	elif test -x "/c/Program Files (x86)/$winmerge_exe"
 > +	then
 > +		printf '%s' "/c/Program Files (x86)/$winmerge_exe"
 > +	elif test -x "/c/Program Files/$winmerge_exe"
 > +	then
 > +		printf '%s' "/c/Program Files/$winmerge_exe"

I also think these fallbacks can simply go away. It is *very* unlikely 
that "$PROGRAMFILES" / "$PROGRAMFILES(X86)" point to something else but 
WinMerge still is installed there.

-- 
Sebastian Schuberth