On Wed, Jul 01, 2020 at 03:23:56PM +0800, lin.sun@xxxxxxx wrote: > Hi Danh, > > The [PATH v5] is changed to following the comments from you, Junio, David. > Please review this newer version. Thank you. > The raw files is https://github.com/git/git/blob/344817d57970e3ac0910cdd8ad47bf97334ab2a6/mergetools/meld > > Regards > Lin > -----Original Message----- > From: sunlin via GitGitGadget <gitgitgadget@xxxxxxxxx> > Sent: Wednesday, July 1, 2020 15:07 > To: git@xxxxxxxxxxxxxxx > Cc: sunlin <sunlin7@xxxxxxxxx>; Lin Sun <lin.sun@xxxxxxx> > Subject: [PATCH v5] Enable auto-merge for meld to follow the vim-diff beharior > > From: Lin Sun <lin.sun@xxxxxxx> > > 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. Please word-wrap commit messages to 72 chars or less. > > Signed-off-by: Lin Sun <lin.sun@xxxxxxx> > --- > Documentation/config/mergetool.txt | 8 ++++ > mergetools/meld | 72 +++++++++++++++++++++++------- > 2 files changed, 64 insertions(+), 16 deletions(-) > > diff --git a/Documentation/config/mergetool.txt b/Documentation/config/mergetool.txt > index 09ed31dbfa..9a74bd98dc 100644 > --- a/Documentation/config/mergetool.txt > +++ b/Documentation/config/mergetool.txt > @@ -30,6 +30,14 @@ mergetool.meld.hasOutput:: > to `true` tells Git to unconditionally use the `--output` option, > and `false` avoids using `--output`. > > +mergetool.meld.hasAutoMerge:: > + 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. > + Now that we've decided that "false" should be the default behavior, the naming of this option don't make sense. "hasAutoMerge" doesn't really convey what this option does anymore. I would suggest calling it "mergetool.meld.automerge". Perhaps something like this? mergetool.meld.automerge:: Older versions of `meld` do not support the `--auto-merge` option. Setting `mergetool.meld.automerge` 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. > @@ -3,34 +3,74 @@ diff_cmd () { > } > > merge_cmd () { > - if test -z "${meld_has_output_option:+set}" > + check_meld_for_features > + > + option_auto_merge= > + if test "$meld_has_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 > +get_meld_help_msg () { This comment seems redundant when the function is called get_meld_help_msg(). > + meld_path="$(git config mergetool.meld.path || echo meld)" > + $meld_path --help 2>&1 > +} > > - if meld_has_output_option=$(git config --bool mergetool.meld.hasOutput) > +# Check the features and set flags > +check_meld_for_features () { Ditto for this comment. > + # 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) > + if test "$meld_has_output_option" = true -o \ > + "$meld_has_output_option" = false Documentation/CodingGuidelines mentions: - We do not write our "test" command with "-a" and "-o" and use "&&" or "||" to concatenate multiple "test" commands instead, because the use of "-a/-o" is often error-prone. E.g. This would be written as: if test "$meld_has_output_option" = true || test "$meld_has_output_option" = false then ... fi Instead of two checks, would it be better to instead just check: if test -n "$meld_has_output_option" ? git config --bool will only ever produce true/false, so we really only need to check that it's a non-empty value, perhaps? > + 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 ;; > + esac We typically avoid the extra indentation level for the case labels. eg. this would be written like this (with ";;" on its own line): 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 ;; esac Also, if you're matching [OPTION ...] would it work to just use a single-quoted value in the case label? eg: *'[OPTION...]'* ? > + if test -z "${meld_has_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 > + # testing the "--auto-merge" option only if config is "auto" > + if test -z "$meld_help_msg" > + then > + meld_help_msg="$(get_meld_help_msg)" This can just be: meld_help_msg=$(get_meld_help_msg) and can do without the surrounding " " double quotes. > + 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 ;; > + esac As above -- unindent the case statements, and put the ";;" case statement terminators on their own line. cheers, -- David
