[v3] Enable auto-merge for meld to follow the vim-diff beharior
diff mbox series

Message ID pull.781.v3.git.git.1593414441313.gitgitgadget@gmail.com
State New
Headers show
Series
  • [v3] Enable auto-merge for meld to follow the vim-diff beharior
Related show

Commit Message

Abhishek Kumar via GitGitGadget June 29, 2020, 7:07 a.m. UTC
From: "lin.sun" <lin.sun@zoom.us>

The mergetool "meld" does NOT merge the no-conflict changes, while the
mergetool "vimdiff" will merge the no-conflict parts and highlight the
conflict parts.
This patch will make the mergetool "meld" similar to "vimdiff",
auto-merge the no-conflict parts, highlight conflict parts.

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-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-781/sunlin7/master-v3
Pull-Request: https://github.com/git/git/pull/781

Range-diff vs v2:

 1:  6e98a10bfa ! 1:  3b70fd0bfc Enable auto-merge for meld to follow the vim-diff beharior
     @@ Commit message
      
       ## mergetools/meld ##
      @@ mergetools/meld: merge_cmd () {
     + 	then
     + 		check_meld_for_output_version
     + 	fi
     ++	if test -z "${meld_has_auto_merge_option:+set}"
     ++	then
     ++		check_meld_for_auto_merge_version
     ++	fi
     ++
     ++	option_auto_merge=
     ++	if test "$meld_has_auto_merge_option" = true
     ++	then
     ++		option_auto_merge="--auto-merge"
     ++	fi
       
       	if test "$meld_has_output_option" = true
       	then
      -		"$merge_tool_path" --output="$MERGED" \
     -+		"$merge_tool_path" --auto-merge --output="$MERGED" \
     ++		"$merge_tool_path" $option_auto_merge --output="$MERGED" \
       			"$LOCAL" "$BASE" "$REMOTE"
       	else
      -		"$merge_tool_path" "$LOCAL" "$MERGED" "$REMOTE"
     -+		"$merge_tool_path" --auto-merge "$LOCAL" "$MERGED" "$REMOTE"
     ++		"$merge_tool_path" $option_auto_merge "$LOCAL" "$MERGED" "$REMOTE"
       	fi
       }
       
     +@@ mergetools/meld: check_meld_for_output_version () {
     + 		meld_has_output_option=false
     + 	fi
     + }
     ++
     ++# Check whether we should use 'meld --auto-merge ...'
     ++check_meld_for_auto_merge_version () {
     ++	meld_path="$(git config mergetool.meld.path)"
     ++	meld_path="${meld_path:-meld}"
     ++
     ++	if meld_has_auto_merge_option=$(git config --bool mergetool.meld.hasAutoMerge)
     ++	then
     ++		: use configured value
     ++	elif "$meld_path" --help 2>&1 |
     ++		grep -e '--auto-merge' -e '\[OPTION\.\.\.\]' >/dev/null
     ++	then
     ++		: old ones mention --auto-merge and new ones just say OPTION...
     ++		meld_has_auto_merge_option=true
     ++	else
     ++		meld_has_auto_merge_option=false
     ++	fi
     ++}


 mergetools/meld | 32 ++++++++++++++++++++++++++++++--
 1 file changed, 30 insertions(+), 2 deletions(-)


base-commit: 07d8ea56f2ecb64b75b92264770c0a664231ce17

Comments

Junio C Hamano June 30, 2020, 12:06 a.m. UTC | #1
"sunlin via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: "lin.sun" <lin.sun@zoom.us>

The way this "name <addr>" is spelled must match the name used on
the Signed-off-by: line below.

> The mergetool "meld" does NOT merge the no-conflict changes, while the
> mergetool "vimdiff" will merge the no-conflict parts and highlight the
> conflict parts.

OK.

Have a blank line between paragraphs here.

> This patch will make the mergetool "meld" similar to "vimdiff",
> auto-merge the no-conflict parts, highlight conflict parts.

Give an order to the codebase to be like so, e.g.:

    Make the mergetool used with "meld" backend behave similarly to
    how "vimdiff" beheaves by telling it to auto-merge parts without
    conflicts and highlight the parts with conflicts.

or somethin glike that.

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

That repeats the log message and does not add much useful info.

>  mergetools/meld | 32 ++++++++++++++++++++++++++++++--
>  1 file changed, 30 insertions(+), 2 deletions(-)
>
> diff --git a/mergetools/meld b/mergetools/meld
> index 7a08470f88..91b65ff22c 100644
> --- a/mergetools/meld
> +++ b/mergetools/meld
> @@ -7,13 +7,23 @@ merge_cmd () {
>  	then
>  		check_meld_for_output_version
>  	fi
> +	if test -z "${meld_has_auto_merge_option:+set}"
> +	then
> +		check_meld_for_auto_merge_version
> +	fi

The detection part looks clumsy and inefficient.  More about it later.

> +	option_auto_merge=
> +	if test "$meld_has_auto_merge_option" = true
> +	then
> +		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
>  }

The part that chooses whether to pass --auto-merge or not and
adjusts the command line options does look sensible.

I wonder if the same "hasAutoMerge" option can be used by those who
do *not* want the new --auto-merge behaviour to opt out of this
change.  Is there a reason why "meld" offers the --auto-merge as an
optional behaviour (which tells, at least to me, that the default
behaviour is not to auto-merge and makes me assume that the default
must be chosen by some sound reasoning, hence some users would prefer
not to use this new behaviour with good reasons)?

I guess what I am trying to get at is, if --auto-merge is an optional
and non-default behaviour for "meld" users, perhaps it is not a good
idea to change the behaviour on them only because the version of meld
they run happens to support the --auto-merge as an optional behaviour.

IOW, wouldn't it make more sense, and certainly make it safer
without surprises to existing users, if we made the logic to

    * If mergetool.meld.useAutoMerge is not set, do not pass
      --auto-merge whether "meld" supports the option or not.

    * If mergetool.meld.useAutoMerge is 'true', always pass it
      without checking.

    * If mergetool.meld.useAutoMerge is 'when-able' (or come up with
      a better name if you want, perhaps 'auto'), check if "meld"
      accepts "--auto-merge" and decide whether to pass it or not.

perhaps?

> +# Check whether we should use 'meld --auto-merge ...'
> +check_meld_for_auto_merge_version () {
> +	meld_path="$(git config mergetool.meld.path)"
> +	meld_path="${meld_path:-meld}"
> +
> +	if meld_has_auto_merge_option=$(git config --bool mergetool.meld.hasAutoMerge)
> +	then
> +		: use configured value
> +	elif "$meld_path" --help 2>&1 |
> +		grep -e '--auto-merge' -e '\[OPTION\.\.\.\]' >/dev/null
> +	then
> +		: old ones mention --auto-merge and new ones just say OPTION...
> +		meld_has_auto_merge_option=true
> +	else
> +		meld_has_auto_merge_option=false
> +	fi
> +}

When not configured, we end up running "meld --help" twice for two
options, which is not great, don't you think?  I actually think the
part that runs "meld --help" and parses its output should be split
out of the helper function "check_meld_for_output_version" and
called "check_meld_supported_options" or something, so that the
logic to see if the --output and --auto-merge options should be
passed can be made with at most one invocation of "meld --help".
Which may involve *NOT* having two separate helper functions
check_meld_for_*_version for the tested features.

Oh, also, check_meld_for_*_version is nonsensical as a name for
these helper functions (it is not the fault of this patch---it is
mimicking the existing practice, but that does not make the function
name not nonsensical).  The helpers do not actually want to check a
"version", they only want to see if a feature is supported.  So
having "feature" in the name would be good, but "version" is not.
David Aguilar June 30, 2020, 7:42 a.m. UTC | #2
On Mon, Jun 29, 2020 at 05:06:13PM -0700, Junio C Hamano wrote:
> "sunlin via GitGitGadget" <gitgitgadget@gmail.com> writes:
> >  mergetools/meld | 32 ++++++++++++++++++++++++++++++--
> >  1 file changed, 30 insertions(+), 2 deletions(-)
> >
> > diff --git a/mergetools/meld b/mergetools/meld
> > index 7a08470f88..91b65ff22c 100644
> > --- a/mergetools/meld
> > +++ b/mergetools/meld
> > @@ -7,13 +7,23 @@ merge_cmd () {
> >  	then
> >  		check_meld_for_output_version
> >  	fi
> > +	if test -z "${meld_has_auto_merge_option:+set}"
> > +	then
> > +		check_meld_for_auto_merge_version
> > +	fi
> 
> The detection part looks clumsy and inefficient.  More about it later.


Sorry for not noticing your reply here earlier.  I agree with everything
you wrote here, and rescind my earlier sign-off.  Combining as you
suggested below is best IMO as well.

> 
> > +	option_auto_merge=
> > +	if test "$meld_has_auto_merge_option" = true
> > +	then
> > +		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
> >  }
> 
> The part that chooses whether to pass --auto-merge or not and
> adjusts the command line options does look sensible.
> 
> I wonder if the same "hasAutoMerge" option can be used by those who
> do *not* want the new --auto-merge behaviour to opt out of this
> change.  Is there a reason why "meld" offers the --auto-merge as an
> optional behaviour (which tells, at least to me, that the default
> behaviour is not to auto-merge and makes me assume that the default
> must be chosen by some sound reasoning, hence some users would prefer
> not to use this new behaviour with good reasons)?
> 
> I guess what I am trying to get at is, if --auto-merge is an optional
> and non-default behaviour for "meld" users, perhaps it is not a good
> idea to change the behaviour on them only because the version of meld
> they run happens to support the --auto-merge as an optional behaviour.
> 
> IOW, wouldn't it make more sense, and certainly make it safer
> without surprises to existing users, if we made the logic to
> 
>     * If mergetool.meld.useAutoMerge is not set, do not pass
>       --auto-merge whether "meld" supports the option or not.
> 
>     * If mergetool.meld.useAutoMerge is 'true', always pass it
>       without checking.
> 
>     * If mergetool.meld.useAutoMerge is 'when-able' (or come up with
>       a better name if you want, perhaps 'auto'), check if "meld"
>       accepts "--auto-merge" and decide whether to pass it or not.
> 
> perhaps?


I like the idea of having it be auto/true/false, and perhaps "auto"
would be a sensible default if more users benefit from it than not.

Sunlin, do you have an opinion on what the default should be?



> > +# Check whether we should use 'meld --auto-merge ...'
> > +check_meld_for_auto_merge_version () {
> > +	meld_path="$(git config mergetool.meld.path)"

Small sug -- this command substitution doesn't need the enclosing
quotes.

	meld_path=$(git config mergetool.meld.path || echo meld)

should be sufficient.  Are we okay with `|| echo meld`?
If so, it would let us drop this line below.

> > +	meld_path="${meld_path:-meld}"


> > +
> > +	if meld_has_auto_merge_option=$(git config --bool mergetool.meld.hasAutoMerge)
> > +	then
> > +		: use configured value
> > +	elif "$meld_path" --help 2>&1 |
> > +		grep -e '--auto-merge' -e '\[OPTION\.\.\.\]' >/dev/null
> > +	then
> > +		: old ones mention --auto-merge and new ones just say OPTION...
> > +		meld_has_auto_merge_option=true
> > +	else
> > +		meld_has_auto_merge_option=false
> > +	fi
> > +}
> 
> When not configured, we end up running "meld --help" twice for two
> options, which is not great, don't you think?  I actually think the
> part that runs "meld --help" and parses its output should be split
> out of the helper function "check_meld_for_output_version" and
> called "check_meld_supported_options" or something, so that the
> logic to see if the --output and --auto-merge options should be
> passed can be made with at most one invocation of "meld --help".
> Which may involve *NOT* having two separate helper functions
> check_meld_for_*_version for the tested features.


I'm 100% on board with this suggestion.

> 
> Oh, also, check_meld_for_*_version is nonsensical as a name for
> these helper functions (it is not the fault of this patch---it is
> mimicking the existing practice, but that does not make the function
> name not nonsensical).  The helpers do not actually want to check a
> "version", they only want to see if a feature is supported.  So
> having "feature" in the name would be good, but "version" is not.


Ditto.
lin.sun@zoom.us June 30, 2020, 11:25 a.m. UTC | #3
Hi David, Junio,

Appreciate for your comments, I rewrite the "mergetool/meld" to follow your comments and suggestions.
It will respect the git config first, then detect the options if no configuration for them, and also reduce the subprocess calling.
Both the modified-file and patch-file are appended.
Please review again. Thanks

Regards
Lin Sun
lin.sun@zoom.us June 30, 2020, 11:37 a.m. UTC | #4
The last change also available in https://github.com/git/git/pull/781/files.
Junio C Hamano June 30, 2020, 3:51 p.m. UTC | #5
David Aguilar <davvid@gmail.com> writes:

> On Mon, Jun 29, 2020 at 05:06:13PM -0700, Junio C Hamano wrote:
> ...
>> I guess what I am trying to get at is, if --auto-merge is an optional
>> and non-default behaviour for "meld" users, perhaps it is not a good
>> idea to change the behaviour on them only because the version of meld
>> they run happens to support the --auto-merge as an optional behaviour.
>> 
>> IOW, wouldn't it make more sense, and certainly make it safer
>> without surprises to existing users, if we made the logic to
>> 
>>     * If mergetool.meld.useAutoMerge is not set, do not pass
>>       --auto-merge whether "meld" supports the option or not.
>> 
>>     * If mergetool.meld.useAutoMerge is 'true', always pass it
>>       without checking.
>> 
>>     * If mergetool.meld.useAutoMerge is 'when-able' (or come up with
>>       a better name if you want, perhaps 'auto'), check if "meld"
>>       accepts "--auto-merge" and decide whether to pass it or not.
>> 
>> perhaps?
>
> I like the idea of having it be auto/true/false, and perhaps "auto"
> would be a sensible default if more users benefit from it than not.

The principle I've stuck to running this project for the past years
is to avoid hurting 1 user more than trying to please 10 others.  As
I do not use meld regularly myself, I do not know if there are use
cases that are helped by the fact that "--auto-merge" is not the
default behaviour among "meld" users (not limited to "users of
mergetool with meld backend").  And if there are, suddenly changing
the behaviour to those who have happily been using mergetool with
meld backend that does not "--auto-merge" can be disservice to our
users.  Until I hear something along the lines of "Even though meld
requires --auto-merge from users merely due to historical reasons,
these days everybody hates the fact that it is not the default", I'd
recommend against making 'auto' the default on our side, just to be
safe, even if our intuition is that majority of users may want the
"--auto-merge" behaviour.

But I am not a user of mergetool with meld backend, so...

Thanks.

Patch
diff mbox series

diff --git a/mergetools/meld b/mergetools/meld
index 7a08470f88..91b65ff22c 100644
--- a/mergetools/meld
+++ b/mergetools/meld
@@ -7,13 +7,23 @@  merge_cmd () {
 	then
 		check_meld_for_output_version
 	fi
+	if test -z "${meld_has_auto_merge_option:+set}"
+	then
+		check_meld_for_auto_merge_version
+	fi
+
+	option_auto_merge=
+	if test "$meld_has_auto_merge_option" = true
+	then
+		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
 }
 
@@ -34,3 +44,21 @@  check_meld_for_output_version () {
 		meld_has_output_option=false
 	fi
 }
+
+# Check whether we should use 'meld --auto-merge ...'
+check_meld_for_auto_merge_version () {
+	meld_path="$(git config mergetool.meld.path)"
+	meld_path="${meld_path:-meld}"
+
+	if meld_has_auto_merge_option=$(git config --bool mergetool.meld.hasAutoMerge)
+	then
+		: use configured value
+	elif "$meld_path" --help 2>&1 |
+		grep -e '--auto-merge' -e '\[OPTION\.\.\.\]' >/dev/null
+	then
+		: old ones mention --auto-merge and new ones just say OPTION...
+		meld_has_auto_merge_option=true
+	else
+		meld_has_auto_merge_option=false
+	fi
+}