diff mbox series

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

Message ID pull.781.v6.git.git.1593650687697.gitgitgadget@gmail.com
State New, archived
Headers show
Series [v6] Support auto-merge for meld to follow the vim-diff behavior | expand

Commit Message

Johannes Schindelin via GitGitGadget July 2, 2020, 12:44 a.m. UTC
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>
---
    Enable auto-merge for meld to follow the vimdiff beharior
    
    Hi, the mergetool "meld" does NOT merge the no-conflict changes, while
    the mergetool "vimdiff" will merge the no-conflict changes and highlight
    the conflict parts. This patch will make the mergetool "meld" similar to
    "vimdiff", auto-merge the no-conflict changes, highlight conflict parts.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-781%2Fsunlin7%2Fmaster-v6
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-781/sunlin7/master-v6
Pull-Request: https://github.com/git/git/pull/781

Range-diff vs v5:

 1:  344817d579 ! 1:  63ee3406d2 Enable auto-merge for meld to follow the vim-diff beharior
     @@ Metadata
      Author: Lin Sun <lin.sun@zoom.us>
      
       ## Commit message ##
     -    Enable auto-merge for meld to follow the vim-diff beharior
     +    Support auto-merge for meld to follow the vim-diff behavior
      
     -    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.hasAutoMerge` with `true`, or `auto` for
     -    automatically detecting the option.
     +    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>
      
       ## Documentation/config/mergetool.txt ##
     @@ Documentation/config/mergetool.txt: mergetool.meld.hasOutput::
       	to `true` tells Git to unconditionally use the `--output` option,
       	and `false` avoids using `--output`.
       
     -+mergetool.meld.hasAutoMerge::
     ++mergetool.meld.useAutoMerge::
      +	Older versions of `meld` do not support the `--auto-merge` option.
     -+	Setting `mergetool.meld.hasOutput` to `true` tells Git to
     -+	unconditionally use the `--auto-merge` option, and `false` avoids using
     -+	`--auto-merge`, and `auto` detect whether `meld` supports `--auto-merge`
     -+	by inspecting the output of `meld --help`, otherwise, follow meld's
     -+	default behavior.
     ++	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 () {
      +	check_meld_for_features
      +
      +	option_auto_merge=
     -+	if test "$meld_has_auto_merge_option" = true
     ++	if test "$meld_use_auto_merge_option" = true
       	then
      -		check_meld_for_output_version
      +		option_auto_merge="--auto-merge"
     @@ mergetools/meld: diff_cmd () {
      -	meld_path="$(git config mergetool.meld.path)"
      -	meld_path="${meld_path:-meld}"
      +# Get meld help message
     -+get_meld_help_msg () {
     -+	meld_path="$(git config mergetool.meld.path || echo meld)"
     -+	$meld_path --help 2>&1
     ++init_meld_help_msg () {
     ++	if test -z "${meld_help_msg:+set}"
     ++	then
     ++		meld_path="$(git config mergetool.meld.path || echo meld)"
     ++		meld_help_msg=$($meld_path --help 2>&1)
     ++	fi
      +}
       
      -	if meld_has_output_option=$(git config --bool mergetool.meld.hasOutput)
     @@ mergetools/meld: diff_cmd () {
      -	elif "$meld_path" --help 2>&1 |
      -		grep -e '--output=' -e '\[OPTION\.\.\.\]' >/dev/null
      +		meld_has_output_option=$(git config --bool mergetool.meld.hasOutput)
     -+		if test "$meld_has_output_option" = true -o \
     -+			"$meld_has_output_option" = false
     -+		then
     ++		case "$meld_has_output_option" in
     ++		true|false)
      +			: use configured value
     -+		else
     -+			# treat meld_has_output_option as "auto"
     -+			if test -z "$meld_help_msg"
     -+			then
     -+				meld_help_msg="$(get_meld_help_msg)"
     -+			fi
     ++			;;
     ++		*)
     ++			: treat meld_has_output_option as "auto"
     ++			init_meld_help_msg
      +
      +			case "$meld_help_msg" in
     -+				*"--output="* | *"[OPTION"???"]"*)
     -+					# old ones mention --output and new ones just say OPTION...
     -+					meld_has_output_option=true ;;
     -+				*)
     -+					meld_has_output_option=false ;;
     ++			*"--output="* | *'[OPTION...]'*)
     ++				# All version that has [OPTION...] supports --output
     ++				meld_has_output_option=true
     ++				;;
     ++			*)
     ++				meld_has_output_option=false
     ++				;;
      +			esac
     -+		fi
     ++			;;
     ++		esac
      +	fi
      +	# Check whether we should use 'meld --auto-merge ...'
     -+	if test -z "${meld_has_auto_merge_option:+set}"
     ++	if test -z "${meld_use_auto_merge_option:+set}"
       	then
      -		: old ones mention --output and new ones just say OPTION...
      -		meld_has_output_option=true
      -	else
      -		meld_has_output_option=false
     -+		meld_has_auto_merge_option=$(git config mergetool.meld.hasAutoMerge)
     -+		if test "$meld_has_auto_merge_option" = auto
     -+		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
     ++			;;
     ++		auto)
      +			# testing the "--auto-merge" option only if config is "auto"
     -+			if test -z "$meld_help_msg"
     -+			then
     -+					meld_help_msg="$(get_meld_help_msg)"
     -+			fi
     ++			init_meld_help_msg
      +
      +			case "$meld_help_msg" in
     -+				*"--auto-merge"*)
     -+					: old ones mention --output and new ones just say OPTION...
     -+					meld_has_auto_merge_option=true ;;
     -+				*)
     -+					meld_has_auto_merge_option=false ;;
     ++			*"--auto-merge"*)
     ++				meld_use_auto_merge_option=true
     ++				;;
     ++			*)
     ++				meld_use_auto_merge_option=false
     ++				;;
      +			esac
     -+		fi
     ++			;;
     ++		*)
     ++			meld_use_auto_merge_option=false
     ++			;;
     ++		esac
       	fi
       }


 Documentation/config/mergetool.txt |  9 ++++
 mergetools/meld                    | 80 ++++++++++++++++++++++++------
 2 files changed, 73 insertions(+), 16 deletions(-)


base-commit: 07d8ea56f2ecb64b75b92264770c0a664231ce17

Comments

Lin Sun July 2, 2020, 2:35 a.m. UTC | #1
Hi,

The [PATCH v6] changes to follow comments from Danh, Junio, David. 
Please review again. Thank you all.

https://lore.kernel.org/git/pull.781.v6.git.git.1593650687697.gitgitgadget@gmail.com/

Regards
Lin
Junio C Hamano July 3, 2020, 1:50 a.m. UTC | #2
"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.
Lin Sun July 3, 2020, 3:53 a.m. UTC | #3
Hi Junio,

Thank you for your comments, and the changes are made to follow your suggestion. 
In the [PATCH v7], the ":+set" is removed, and the "${var,,*}" is used to convert 
configuration to lower-case, it won't call git config twice for getting `useAutoMerge`.

Please review again, thank you.
https://lore.kernel.org/git/pull.781.v7.git.git.1593746805771.gitgitgadget@gmail.com

Regards
Lin
Đoàn Trần Công Danh July 3, 2020, 3:58 p.m. UTC | #4
On 2020-07-02 18:50:15-0700, Junio C Hamano <gitster@pobox.com> wrote:
> 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

Something like this should work if we don't write anything to stderr,
except the complain from git-config:

> fatal: bad numeric config value 'auto' for 'mergetool.meld.useautomerge': invalid unit

	if o=$(git config --bool mergetool.meld.useautomerge 2>&1)
	then
		meld_use_auto_merge_option=$o
	else
		case "$o" in
		*"'auto'"*) ... auto detect ... ;;
		esac
	fi

This code block is likely broken if git-config is chatty (trace).
I don't know if we have a reliable ways to tell Git to not chatty,
though.
Junio C Hamano July 6, 2020, 6:23 a.m. UTC | #5
Đoàn Trần Công Danh  <congdanhqx@gmail.com> writes:

>>     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
>
> Something like this should work if we don't write anything to stderr,
> except the complain from git-config:

I did say "Maybe somebody else has a clever idea to reduce the two
calls into one without breaking correctness" but the stress is on
the "without breaking correctness" part, not on "clever" part.  Two
"git config" call may be more expensive than one call, but at least
the resulting code is readable.

If we really wanted to, I think the way to go would actually be to
teach "git config" new options that allows us scripters to express
what we can already say internally like "maybe bool", "bool or int",
etc.  IOW, "git config --bool-or-string" that does something like

int git_config_bool_or_str(const char **dest,
			   const char *name, const char *value);
{
	int v = git_parse_maybe_bool_text(value);
	if (0 <= v)
        	*dest = v ? "true" : "false";
	else                
        	*dest = value;
	return 0;
}

may help normalizing various ways to spell boolean plus non-boolean
strings into canonical form, so that you can say

	if o=$(git config --bool-or-string mergetool.meld.useautomerge)"
	then
		case "$o" in
		true | false)
		        meld_use_auto_merge_option=$o # as specified
			;;
		auto)
			... auto detect ...
			;;
		esac
	else
		meld_use_auto_merge_option=false
	fi
	
But I think two calls to config is good enough and it certainly is
not worth making the script ugly with hackeries and/or manually
enumerating all the possible shapes of trues and falses.

Thanks.
diff mbox series

Patch

diff --git a/Documentation/config/mergetool.txt b/Documentation/config/mergetool.txt
index 09ed31dbfa..625ad34230 100644
--- a/Documentation/config/mergetool.txt
+++ b/Documentation/config/mergetool.txt
@@ -30,6 +30,15 @@  mergetool.meld.hasOutput::
 	to `true` tells Git to unconditionally use the `--output` option,
 	and `false` avoids using `--output`.
 
+mergetool.meld.useAutoMerge::
+	Older versions of `meld` do not support the `--auto-merge` option.
+	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
 	can be saved as a file with a `.orig` extension.  If this variable
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 () {
-	if test -z "${meld_has_output_option:+set}"
+	check_meld_for_features
+
+	option_auto_merge=
+	if test "$meld_use_auto_merge_option" = true
 	then
-		check_meld_for_output_version
+		option_auto_merge="--auto-merge"
 	fi
 
 	if test "$meld_has_output_option" = true
 	then
-		"$merge_tool_path" --output="$MERGED" \
+		"$merge_tool_path" $option_auto_merge --output="$MERGED" \
 			"$LOCAL" "$BASE" "$REMOTE"
 	else
-		"$merge_tool_path" "$LOCAL" "$MERGED" "$REMOTE"
+		"$merge_tool_path" $option_auto_merge "$LOCAL" "$MERGED" "$REMOTE"
 	fi
 }
 
-# Check whether we should use 'meld --output <file>'
-check_meld_for_output_version () {
-	meld_path="$(git config mergetool.meld.path)"
-	meld_path="${meld_path:-meld}"
+# Get meld help message
+init_meld_help_msg () {
+	if test -z "${meld_help_msg:+set}"
+	then
+		meld_path="$(git config mergetool.meld.path || echo meld)"
+		meld_help_msg=$($meld_path --help 2>&1)
+	fi
+}
 
-	if meld_has_output_option=$(git config --bool mergetool.meld.hasOutput)
+# 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}"
 	then
-		: use configured value
-	elif "$meld_path" --help 2>&1 |
-		grep -e '--output=' -e '\[OPTION\.\.\.\]' >/dev/null
+		meld_has_output_option=$(git config --bool mergetool.meld.hasOutput)
+		case "$meld_has_output_option" in
+		true|false)
+			: use configured value
+			;;
+		*)
+			: treat meld_has_output_option as "auto"
+			init_meld_help_msg
+
+			case "$meld_help_msg" in
+			*"--output="* | *'[OPTION...]'*)
+				# All version that has [OPTION...] supports --output
+				meld_has_output_option=true
+				;;
+			*)
+				meld_has_output_option=false
+				;;
+			esac
+			;;
+		esac
+	fi
+	# Check whether we should use 'meld --auto-merge ...'
+	if test -z "${meld_use_auto_merge_option:+set}"
 	then
-		: old ones mention --output and new ones just say OPTION...
-		meld_has_output_option=true
-	else
-		meld_has_output_option=false
+		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
+			;;
+		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
 }