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

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

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

Show 13 quoted lines

> From: Lin Sun <lin.sun@zoom.us>
>
> Make the mergetool used with "meld" backend behave similarly to how
> "vimdiff" behavior by telling it to auto-merge parts without conflicts
> and highlight the parts with conflicts when configuring
> `mergetool.meld.useAutoMerge` with `true`, or `auto` for automatically
> detecting the option.
>
> Helped-by: Đoàn Trần Công Danh <congdanhqx@gmail.com>
> Helped-by: Junio C Hamano <gitster@pobox.com>
> Helped-by: David Aguilar <davvid@gmail.com>
> Signed-off-by: Lin Sun <lin.sun@zoom.us>
> ---

Thanks.

> +mergetool.meld.useAutoMerge::
> +	Older versions of `meld` do not support the `--auto-merge` option.

I do not mind the above text at all, but I do not think it is the most important point about this configuration option. Are the readers of this manual page, at least the subset of them who are interested in using the meld backend, expected to know what 'meld' would do when given (and not given) the `--auto-merge` option?

At least, what meld does with and without `--auto-merge` briefly is more important, especially if your belief is that it is plausible that most users would prefer to use it. Something like (I am not a `meld` user, so everything I wrote after the first comma ',' in these two paragraphs may be total rubbish---I am just writing these as an illustration to show the level of details necessary to help readers decide if the option is for them)...

    When meld is given `--auto-merge`, a part of the conflicted file
    that was modified only by one side is automatically resolved to
    take that change, and a part that was touched by both sides are
    highlighted for manual conflict resolution.

    Without `--auto-merge` option given, the automatic resolution
    does not happen---all changes in the conflicted file are
    highlighted for manual conflict resolution.

Only after that, telling them "(even though you may want to use the option all the time) Older versions of `meld` do not support it" becomes relevant---for users with ancient versions of `meld`, the option, even if it is desirable, may not be available to them so it is worth warning them that they should not set it to 'true' blindly without checking with their version of `meld` first.

Show 24 quoted lines

> diff --git a/mergetools/meld b/mergetools/meld
> index 7a08470f88..5bc03f564a 100644
> --- a/mergetools/meld
> +++ b/mergetools/meld
> @@ -3,34 +3,82 @@ diff_cmd () {
>  }
>  
>  merge_cmd () {
> +	check_meld_for_features
> +
> +	option_auto_merge=
> +	if test "$meld_use_auto_merge_option" = true
>  	then
> +		option_auto_merge="--auto-merge"
>  	fi
>  
>  	if test "$meld_has_output_option" = true
>  	then
> +		"$merge_tool_path" $option_auto_merge --output="$MERGED" \
>  			"$LOCAL" "$BASE" "$REMOTE"
>  	else
> +		"$merge_tool_path" $option_auto_merge "$LOCAL" "$MERGED" "$REMOTE"
>  	fi
>  }

That's straight-forward to follow.  Good.

> +# Get meld help message
> +init_meld_help_msg () {
> +	if test -z "${meld_help_msg:+set}"

I suspect that you copied the pattern

    if test -z "${var:+set}"

from the original, but it looks rather strange. "${var:+set}" means "if $var is set to a non-empty string, give me 'set'; otherwise give me $var". And checking if that result is an empty string with "-z" would mean you can safely write

    if test -z "$var"

no? If you are "set -u" proofing, you might write "${var-}" instead of "$var" there, but that does not change the story all that much.

Show 5 quoted lines

> +	then
> +		meld_path="$(git config mergetool.meld.path || echo meld)"
> +		meld_help_msg=$($meld_path --help 2>&1)
> +	fi
> +}

In any case, the if/then/fi makes sure we'll ask `meld` only once, which is good.

> +# Check the features and set flags
> +check_meld_for_features () {
> +	# Check whether we should use 'meld --output <file>'
> +	if test -z "${meld_has_output_option:+set}"

Likewise about "test -z ${var:+set}".

So, if we do not know yet, then...

>  	then
> +		meld_has_output_option=$(git config --bool mergetool.meld.hasOutput)

We ask "config --bool" and are prepared to see two primary cases, i.e.

> +		case "$meld_has_output_option" in
> +		true|false)
> +			: use configured value

In this case, the user may have configured, so the variable already has the right value. Good.

> +			;;
> +		*)
> +			: treat meld_has_output_option as "auto"
> +			init_meld_help_msg

Otherwise, we ask `meld`.

> +			case "$meld_help_msg" in
> +			*"--output="* | *'[OPTION...]'*)
> +				# All version that has [OPTION...] supports --output

Very good comment.

Show 9 quoted lines

> +				meld_has_output_option=true
> +				;;
> +			*)
> +				meld_has_output_option=false
> +				;;
> +			esac
> +			;;
> +		esac
> +	fi

Nicely done up to this point.

> +	# Check whether we should use 'meld --auto-merge ...'
> +	if test -z "${meld_use_auto_merge_option:+set}"

Likewise about "test -z ${var:+set}".

So, if we do not know yet, then...

Show 5 quoted lines

>  	then
> +		meld_use_auto_merge_option=$(git config mergetool.meld.useAutoMerge)
> +		case "$meld_use_auto_merge_option" in
> +		[Tt]rue|[Yy]es|[Oo]n|1)
> +			meld_use_auto_merge_option=true

This is sloppy.  TRUE is also a valid way to spell 'yes'.

    if o=$(git config --bool 2>/dev/null mergetool.meld.useautomerge)
    then
    	meld_use_auto_merge_option=$o
    elif test auto = "$(git config mergetool.meld.useautomerge)"
    then
	... auto detect ...
    else
	meld_use_auto_merge_option=false
    fi

Maybe somebody else has a clever idea to reduce the two calls into one without breaking correctness, but unfortunately I do not offhand think of a way to do this with just a single "git config" call.

Show 20 quoted lines

> +			;;
> +		auto)
> +			# testing the "--auto-merge" option only if config is "auto"
> +			init_meld_help_msg
> +
> +			case "$meld_help_msg" in
> +			*"--auto-merge"*)
> +				meld_use_auto_merge_option=true
> +				;;
> +			*)
> +				meld_use_auto_merge_option=false
> +				;;
> +			esac
> +			;;
> +		*)
> +			meld_use_auto_merge_option=false
> +			;;
> +		esac
>  	fi
>  }

Thanks.