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

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 3, 2020, 04:50 UTC
Message-ID: <xmqqo8oxb3rv.fsf@gitster.c.googlers.com>
In-Reply-To: <pull.781.v7.git.git.1593746805771.gitgitgadget@gmail.com>

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

>      ++	When the `--auto-merge` is given, meld will merges all non-conflicting

Grammo: "will merges"???

Show 15 quoted lines

>      ++	part automatically, highlight the conflicting part and waiting for user
>      ++	decision.  Setting `mergetool.meld.useAutoMerge` to `true` tells Git to
>      ++	unconditionally use the `--auto-merge` option with `meld`.  Setting this
>      ++	value to `auto` makes git detect whether `--auto-merge` is supported and
>      ++	will only use `--auto-merge` when available.  A value of `false` avoids
>      ++	using `--auto-merge` altogether, and is the default value.
>       +
>        mergetool.keepBackup::
>        	After performing a merge, the original file with conflict markers
>      @@ mergetools/meld: diff_cmd () {
>       -	meld_path="${meld_path:-meld}"
>       +# Get meld help message
>       +init_meld_help_msg () {
>      -+	if test -z "${meld_help_msg:+set}"
>      ++	if test -z "${meld_help_msg}"

Now we do not use the :+ magic, there is no reason to use extra {brace} around the variable name. Just sticking to the normal "$meld_help_msg" would help the readers to hint that there is no strange thing going on.

>      ++		case "${meld_use_auto_merge_option,,*}" in

DON'T.

The ${parameter,,pattern} expansion is one of the non-portable bash-isms we do not use in our codebase. Use of it won't fix the correctness problem coming from not using --bool anyway, because ...

>      ++		true|yes|on|1)

... any numeric value that is not 0 is taken as true. Writing 1 here does not help.