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

[Junio C Hamano <gitster@pobox.com>]

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

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

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

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

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

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

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