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

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 24, 2019, 07:27 UTC
Message-ID: <xmqqr29r278v.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <e928db892e35bcb68fcdb52c9bf7158dbbb46616.1556009181.git.liu.denton@gmail.com>

Denton Liu <liu.denton@gmail.com> writes:

Show 18 quoted lines

> +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
	}

Show 19 quoted lines

> 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)"