From: Junio C Hamano <gitster@pobox.com>
Date: Fri, 25 Mar 2016 23:00:06 GMT
Subject: Re: [PATCH 1/2] mergetools: create mergetool_find_win32_cmd() helper function for winmerge
Message-ID: <xmqqio0a17ux.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <1458946376-30296-2-git-send-email-jacob.nisnevich@gmail.com>

Jacob Nisnevich <jacob.nisnevich@gmail.com> writes:

> Signed-off-by: Jacob Nisnevich <jacob.nisnevich@gmail.com>
> ---
>  git-mergetool--lib.sh | 25 +++++++++++++++++++++++++
>  mergetools/winmerge   | 21 +--------------------
>  2 files changed, 26 insertions(+), 20 deletions(-)
>
> diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
> index 54ac8e4..c5fa820 100644
> --- a/git-mergetool--lib.sh
> +++ b/git-mergetool--lib.sh
> @@ -372,3 +372,28 @@ get_merge_tool () {
>  	fi
>  	echo "$merge_tool"
>  }
> +
> +mergetool_find_win32_cmd () {
> +	executable=$1
> +	folder=$2
> +
> +	# Use executable.com if it exists in $PATH

s/executable.com/$executable/

> +	if type -p $executable >/dev/null 2>&1

This needs to be quoted,

	if type -p "$executable" >/dev/null 2>&1

> +	then
> +		printf '%s' $executable

Likewise.

> +		return
> +	fi
> +
> +	# Look for executable in the typical locations
> +	for directory in $(env | grep -Ei '^PROGRAM(FILES(\(X86\))?|W6432)=' |
> +		cut -d '=' -f 2- | sort -u)
> +	do
> +		if test -n "$directory" && test -x "$directory/$folder/$executable"
> +		then
> +			printf '%s' "$directory/$folder/$executable"
> +			return
> +		fi
> +	done
> +
> +	printf '%s' $executable

Likewise.

Other than these points, I do not see anything wrong in this patch.
Thanks.

By the way, "directory/folder/stuff" sounds somewhat strange, no?