Re: [PATCH v13] Support auto-merge for meld to follow the vim-diff behavior

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 9, 2020, 02:56 UTC
Message-ID: <xmqqh7uhz98q.fsf@gitster.c.googlers.com>
In-Reply-To: <pull.781.v13.git.git.1594254906647.gitgitgadget@gmail.com>

"sunlin via GitGitGadget" <gitgitgadget@gmail.com> writes:

> +			case "$meld_help_msg" in
> +			*"--output="*|*'[OPTION...]'*)

This may be personal preference, but I think the way you spelled this line with SP on both sides of '|' was much easier to read.

Show 8 quoted lines

> +	# Check whether we should use 'meld --auto-merge ...'
> +	if test -z "$meld_use_auto_merge_option"
>  	then
> +		meld_use_auto_merge_option=$(git config mergetool.meld.useAutoMerge)
> +		case "$meld_use_auto_merge_option" in
> +		true|false)
> +			: use well formatted boolean value
> +			;;

OK.

Show 13 quoted lines

> +		auto)
> +			# testing the "--auto-merge" option only if config is "auto"
> +			init_meld_help_msg
> +
> +			case "$meld_help_msg" in
> +			*"--auto-merge"*|*'[OPTION...]'*)
> +				meld_use_auto_merge_option=true
> +				;;
> +			*)
> +				meld_use_auto_merge_option=false
> +				;;
> +			esac
> +			;;

OK.

> +		*)

This is the case where we saw "", "True", "treu", etc. from the string "git config --get".

> +			if meld_use_auto_merge_option=$(\
> +				 git config --bool mergetool.meld.useAutoMerge)

You do not need that backslash, do you?

Perhaps

			if meld_use_auto_merge_option=$(
				 git config --bool mergetool.meld.useAutoMerge
			)	
			then

if you really want to spread them into multiple lines.

> +			then
> +				: use normalized boolean value

Sure, we got a valid boolean.

> +			else
> +				meld_use_auto_merge_option=false

Why? Shoudln't we loudly complain to let the user know of a misspelt value in the configuration?

Show 7 quoted lines

> +			fi
> +			;;
> +		esac
>  	fi
>  }
>
> base-commit: 07d8ea56f2ecb64b75b92264770c0a664231ce17