| Message ID | pull.781.v3.git.git.1593414441313.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] Enable auto-merge for meld to follow the vim-diff beharior | expand |
"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.
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.
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
The last change also available in https://github.com/git/git/pull/781/files.
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.
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 +}