Re: [PATCH v2 2/5] mergetool: use get_merge_tool_guessed function

[Junio C Hamano <gitster@xxxxxxxxx>]

Denton Liu <liu.denton@xxxxxxxxx> writes:

> +get_merge_tool_guessed () {
> +	is_guessed=false
>  	# Check if a merge tool has been configured
> -	merge_tool=$(get_configured_merge_tool)
> +	merge_tool=$(get_configured_merge_tool $GIT_MERGETOOL_GUI)
>  	# Try to guess an appropriate merge tool if no tool has been set.
>  	if test -z "$merge_tool"
>  	then
>  		merge_tool=$(guess_merge_tool) || exit
> +		is_guessed=true
>  	fi
> -	echo "$merge_tool"
> +	echo "$is_guessed:$merge_tool"
> +}
> +
> +get_merge_tool () {
> +	get_merge_tool_guessed | sed -e 's/^[a-z]*://'
>  }

Yuck.  Returning a:b is fine if the main use is to match that string
using shell builtins like "test" and "case", but piping to "sed"
feels a bit too much overhead.  Especially given that the other
reader in git-emrgetool.sh is not protected for $merge_tool that has
a colon in it.  Do not try to be too cute and end up with a hack
that is both inefficient and brittle at the same time.

Possible alternatives:

 - Because variables in bourne family of shells are global, the
   caller can easily peek at $is_guessed; or

 - One bit "did we guess, or did we get from the user?" boolean
   choice can sufficiently be conveyed by ending the fuction like so
   instead:

		...
	fi
		echo "$merge_tool"
		test "$is_guessed" = true
	}

> diff --git a/git-mergetool.sh b/git-mergetool.sh
> index 01b9ad59b2..63e4da1b2f 100755
> --- a/git-mergetool.sh
> +++ b/git-mergetool.sh
> @@ -449,14 +449,9 @@ main () {
>  
>  	if test -z "$merge_tool"
>  	then
> -		# Check if a merge tool has been configured
> -		merge_tool=$(get_configured_merge_tool $gui_tool)
> -		# Try to guess an appropriate merge tool if no tool has been set.
> -		if test -z "$merge_tool"
> -		then
> -			merge_tool=$(guess_merge_tool) || exit
> -			guessed_merge_tool=true
> -		fi
> +		IFS=':' read guessed_merge_tool merge_tool <<-EOF
> +		$(GIT_MERGETOOL_GUI=$gui_tool get_merge_tool_guessed)
> +		EOF

With the "let the return code speak" alternative, this would become
something like

	if merge_tool=$(GIT_MERGETOOL_GUI=$gui_tool; get_merge_tool_guessed)
	then
		guessed_merge_tool=true
	else
		guessed_merge_tool=false
	fi
	
I do not know what you are trying with GIT_MERGETOOL_GUI=$gui_tool
before the shell function, though.  It does not work as one-shot
assignment to an environment variable.  I _think_ it is to feed the
all-caps variable to get_configured_merge_tool that is invoked by
the get_merge_tool_guessed function, so it does not have to be
exported as an environment in the first place, so in the above
illustration, I simply wrote an assignment statement, followed by a
separate statement that is a parameterless call of a shell function,
separated by a semicolon.

>  	fi
>  	merge_keep_backup="$(git config --bool mergetool.keepBackup || echo true)"
>  	merge_keep_temporaries="$(git config --bool mergetool.keepTemporaries || echo false)"