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

From: Junio C Hamano <gitster@pobox.com>
To: "sunlin via GitGitGadget" <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org, sunlin <sunlin7@yahoo.com>,
	Lin Sun <lin.sun@zoom.us>
Subject: Re: [PATCH v7] Support auto-merge for meld to follow the vim-diff behavior
Date: Thu, 02 Jul 2020 21:50:44 -0700	[thread overview]
Message-ID: <xmqqo8oxb3rv.fsf@gitster.c.googlers.com> (raw)
In-Reply-To: <pull.781.v7.git.git.1593746805771.gitgitgadget@gmail.com> (sunlin via GitGitGadget's message of "Fri, 03 Jul 2020 03:26:45 +0000")

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

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

Grammo: "will merges"???

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