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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]