| Message ID | pull.781.v5.git.git.1593587206520.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v5] Enable auto-merge for meld to follow the vim-diff beharior | expand |
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@gmail.com> Sent: Wednesday, July 1, 2020 15:07 To: git@vger.kernel.org Cc: sunlin <sunlin7@yahoo.com>; Lin Sun <lin.sun@zoom.us> Subject: [PATCH v5] Enable auto-merge for meld to follow the vim-diff beharior 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.hasAutoMerge` with `true`, or `auto` for automatically detecting the option. 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-v5 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-781/sunlin7/master-v5 Pull-Request: https://github.com/git/git/pull/781 Range-diff vs v4: 1: 93ae3ec011 ! 1: 344817d579 Enable auto-merge for meld to follow the vim-diff beharior @@ ## Metadata ## -Author: lin.sun <lin.sun@zoom.us> +Author: Lin Sun <lin.sun@zoom.us> ## Commit message ## Enable auto-merge for meld to follow the vim-diff beharior 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. + 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. - Signed-off-by: lin.sun <lin.sun@zoom.us> + 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:: ++ 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. ++ + 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 ## mergetools/meld ## @@ mergetools/meld: diff_cmd () { @@ mergetools/meld: diff_cmd () { +# Get meld help message +get_meld_help_msg () { + meld_path="$(git config mergetool.meld.path || echo meld)" -+ $meld_path --help 2>&1 ++ $meld_path --help 2>&1 +} - if meld_has_output_option=$(git config --bool mergetool.meld.hasOutput) @@ mergetools/meld: diff_cmd () { - 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 ++ "$meld_has_output_option" = false + then + : use configured value -+ else # treat meld_has_output_option as "auto" ++ else ++ # treat meld_has_output_option as "auto" + if test -z "$meld_help_msg" + then + meld_help_msg="$(get_meld_help_msg)" + fi + -+ if echo "$meld_help_msg" | -+ grep -e '--output=' -e '\[OPTION\.\.\.\]' >/dev/null -+ then -+ : old ones mention --output and new ones just say OPTION... -+ meld_has_output_option=true -+ else -+ meld_has_output_option=false -+ fi ++ 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 + fi + fi + # Check whether we should use 'meld --auto-merge ...' @@ mergetools/meld: diff_cmd () { - meld_has_output_option=true - else - meld_has_output_option=false -+ meld_has_auto_merge_option=$(git config --bool mergetool.meld.hasAutoMerge) -+ if test "$meld_has_auto_merge_option" = true -o \ -+ "$meld_has_auto_merge_option" = false ++ meld_has_auto_merge_option=$(git config mergetool.meld.hasAutoMerge) ++ if test "$meld_has_auto_merge_option" = auto + then -+ : use configured value -+ else # treat meld_has_auto_merge_option as "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 + -+ if echo "$meld_help_msg" | grep -e '--auto-merge' >/dev/null -+ then -+ meld_has_auto_merge_option=true -+ else -+ meld_has_auto_merge_option=false -+ fi ++ 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 + fi fi } 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. + 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..9ee835b1e5 100644 --- a/mergetools/meld +++ b/mergetools/meld @@ -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 () { + 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 () { + # 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 + then + : 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 + + 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 + fi + fi + # Check whether we should use 'meld --auto-merge ...' + 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)" + fi + + 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 + fi fi } base-commit: 07d8ea56f2ecb64b75b92264770c0a664231ce17 -- gitgitgadget
On 2020-07-01 07:06:46+0000, sunlin via GitGitGadget <gitgitgadget@gmail.com> wrote: > 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.hasAutoMerge` with `true`, or `auto` for > automatically detecting the option. > > Signed-off-by: Lin Sun <lin.sun@zoom.us> > --- > 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 s/hasOutput/hasAutoMerge/ Bikeshed opinion: I don't know if hasAutoMerge is a good name :) > + 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. > + > 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..9ee835b1e5 100644 > --- a/mergetools/meld > +++ b/mergetools/meld > @@ -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 () { > + meld_path="$(git config mergetool.meld.path || echo meld)" > + $meld_path --help 2>&1 > +} I'm actually prefer this change in 2 separated patches to reduce noise. But given that mergetool/meld doesn't attract much attention, I don't know. > > - 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) > + if test "$meld_has_output_option" = true -o \ > + "$meld_has_output_option" = false The coding guideline seems to not like "test -o". I think it's acceptable in this case since we control its input. The output is comming out of "git config --bool" anyway, so meld_has_output_option must be either "", "true", or "false" I think we're better to do this instead: if test -n "$meld_has_output_option" then : use configured output else : messing with help fi > + then > + : 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 If I were writing this patch, I probably changed get_meld_help_msg to init_meld_help_msg () { if test -z "$meld_help_msg" then meld_path="$(git config mergetool.meld.path || echo meld)" meld_help_msg=$($meld_path --help 2>&1) fi } And call init_meld_help_msg unconditionally here, (and in --auto-merge arm below, maybe other arms in the future). I'm writing without much thought into this, please take my word with a grain of salt :) > +} > + > + case "$meld_help_msg" in > + *"--output="* | *"[OPTION"???"]"*) I think Git project prefer aligning case arm with case, IOW, move left 1 TAB. > + # old ones mention --output and new ones just say OPTION... > + meld_has_output_option=true ;; It's nice to see this update, good. The comment is no longer correct, though. The version 3.20.2 has --output but not OPTIONS. It's not introduced by your change, but I think it's better to say: # All versions that has [OPTIONS???] supports --output > + *) > + meld_has_output_option=false ;; > + esac > + fi > + fi > + # Check whether we should use 'meld --auto-merge ...' > + 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 Since we don't canonicallise to bool output of mergetool.meld.hasAutoMerge, I think we would need: case "$meld_has_auto_merge_option" in [Tt]rue|[Yy]es|[Oo]n) meld_has_auto_merge_option=true ;; auto) : this shenanigan ;; esac But, that's a bit messy. Let's see other's opinions. > + 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)" > + fi > + > + case "$meld_help_msg" in > + *"--auto-merge"*) > + : old ones mention --output and new ones just say OPTION... This comment doesn't apply here. > + meld_has_auto_merge_option=true ;; > + *) > + meld_has_auto_merge_option=false ;; > + esac > + fi > fi > } > > base-commit: 07d8ea56f2ecb64b75b92264770c0a664231ce17 > -- > gitgitgadget
Hi Danh, Thank you for your comments, and changes apply to the last patch with your suggestions. Please refer the attachment for [PATH v6]. Thank you. Regards Lin -----Original Message----- From: Đoàn Trần Công Danh <congdanhqx@gmail.com> Sent: Wednesday, July 1, 2020 22:18 To: sunlin via GitGitGadget <gitgitgadget@gmail.com> Cc: git@vger.kernel.org; sunlin <sunlin7@yahoo.com>; Lin Sun <lin.sun@zoom.us> Subject: Re: [PATCH v5] Enable auto-merge for meld to follow the vim-diff beharior On 2020-07-01 07:06:46+0000, sunlin via GitGitGadget <gitgitgadget@gmail.com> wrote: > 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.hasAutoMerge` with `true`, or `auto` for automatically > detecting the option. > > Signed-off-by: Lin Sun <lin.sun@zoom.us> > --- > 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 s/hasOutput/hasAutoMerge/ Bikeshed opinion: I don't know if hasAutoMerge is a good name :) > + 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. > + > 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..9ee835b1e5 100644 > --- a/mergetools/meld > +++ b/mergetools/meld > @@ -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 () { > + meld_path="$(git config mergetool.meld.path || echo meld)" > + $meld_path --help 2>&1 > +} I'm actually prefer this change in 2 separated patches to reduce noise. But given that mergetool/meld doesn't attract much attention, I don't know. > > - 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) > + if test "$meld_has_output_option" = true -o \ > + "$meld_has_output_option" = false The coding guideline seems to not like "test -o". I think it's acceptable in this case since we control its input. The output is comming out of "git config --bool" anyway, so meld_has_output_option must be either "", "true", or "false" I think we're better to do this instead: if test -n "$meld_has_output_option" then : use configured output else : messing with help fi > + then > + : 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 If I were writing this patch, I probably changed get_meld_help_msg to init_meld_help_msg () { if test -z "$meld_help_msg" then meld_path="$(git config mergetool.meld.path || echo meld)" meld_help_msg=$($meld_path --help 2>&1) fi } And call init_meld_help_msg unconditionally here, (and in --auto-merge arm below, maybe other arms in the future). I'm writing without much thought into this, please take my word with a grain of salt :) > +} > + > + case "$meld_help_msg" in > + *"--output="* | *"[OPTION"???"]"*) I think Git project prefer aligning case arm with case, IOW, move left 1 TAB. > + # old ones mention --output and new ones just say OPTION... > + meld_has_output_option=true ;; It's nice to see this update, good. The comment is no longer correct, though. The version 3.20.2 has --output but not OPTIONS. It's not introduced by your change, but I think it's better to say: # All versions that has [OPTIONS???] supports --output > + *) > + meld_has_output_option=false ;; > + esac > + fi > + fi > + # Check whether we should use 'meld --auto-merge ...' > + 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 Since we don't canonicallise to bool output of mergetool.meld.hasAutoMerge, I think we would need: case "$meld_has_auto_merge_option" in [Tt]rue|[Yy]es|[Oo]n) meld_has_auto_merge_option=true ;; auto) : this shenanigan ;; esac But, that's a bit messy. Let's see other's opinions. > + 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)" > + fi > + > + case "$meld_help_msg" in > + *"--auto-merge"*) > + : old ones mention --output and new ones just say OPTION... This comment doesn't apply here. > + meld_has_auto_merge_option=true ;; > + *) > + meld_has_auto_merge_option=false ;; > + esac > + fi > fi > } > > base-commit: 07d8ea56f2ecb64b75b92264770c0a664231ce17 > -- > gitgitgadget -- Danh
On Wed, Jul 01, 2020 at 03:23:56PM +0800, lin.sun@zoom.us 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@gmail.com> > Sent: Wednesday, July 1, 2020 15:07 > To: git@vger.kernel.org > Cc: sunlin <sunlin7@yahoo.com>; Lin Sun <lin.sun@zoom.us> > Subject: [PATCH v5] Enable auto-merge for meld to follow the vim-diff beharior > > 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.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@zoom.us> > --- > 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,
Đoàn Trần Công Danh <congdanhqx@gmail.com> writes: >> +mergetool.meld.hasAutoMerge:: >> + Older versions of `meld` do not support the `--auto-merge` option. >> + Setting `mergetool.meld.hasOutput` to `true` tells Git to > > s/hasOutput/hasAutoMerge/ > > Bikeshed opinion: I don't know if hasAutoMerge is a good name :) I do not think "has" is a good choice for this one, even though hasOutput may very well be a good one for "output". As the paragraph describes (below), this lets the user "avoid" using "--auto-merge", even if the version of "meld" the user has does have the automerge capability. So perhaps say "mergetool.meld.useAutoMerge"; that will let the users express exactly their wish---don't use it (false), do forcibly try to use it (true), or use it when able (auto).
Hi Danh, I'll push the patch to GitGitGadget so that it's will sending from the system mail. Sorry for this noise. Regards Lin -----Original Message----- From: lin.sun@zoom.us <lin.sun@zoom.us> Sent: Wednesday, July 1, 2020 23:32 To: 'Đoàn Trần Công Danh' <congdanhqx@gmail.com>; 'sunlin via GitGitGadget' <gitgitgadget@gmail.com> Cc: git@vger.kernel.org; 'Lin Sun' <lin.sun@zoom.us> Subject: RE: [PATCH v5] Enable auto-merge for meld to follow the vim-diff beharior Hi Danh, Thank you for your comments, and changes apply to the last patch with your suggestions. Please refer the attachment for [PATH v6]. Thank you. Regards Lin -----Original Message----- From: Đoàn Trần Công Danh <congdanhqx@gmail.com> Sent: Wednesday, July 1, 2020 22:18 To: sunlin via GitGitGadget <gitgitgadget@gmail.com> Cc: git@vger.kernel.org; sunlin <sunlin7@yahoo.com>; Lin Sun <lin.sun@zoom.us> Subject: Re: [PATCH v5] Enable auto-merge for meld to follow the vim-diff beharior On 2020-07-01 07:06:46+0000, sunlin via GitGitGadget <gitgitgadget@gmail.com> wrote: > 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.hasAutoMerge` with `true`, or `auto` for automatically > detecting the option. > > Signed-off-by: Lin Sun <lin.sun@zoom.us> > --- > 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 s/hasOutput/hasAutoMerge/ Bikeshed opinion: I don't know if hasAutoMerge is a good name :) > + 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. > + > 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..9ee835b1e5 100644 > --- a/mergetools/meld > +++ b/mergetools/meld > @@ -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 () { > + meld_path="$(git config mergetool.meld.path || echo meld)" > + $meld_path --help 2>&1 > +} I'm actually prefer this change in 2 separated patches to reduce noise. But given that mergetool/meld doesn't attract much attention, I don't know. > > - 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) > + if test "$meld_has_output_option" = true -o \ > + "$meld_has_output_option" = false The coding guideline seems to not like "test -o". I think it's acceptable in this case since we control its input. The output is comming out of "git config --bool" anyway, so meld_has_output_option must be either "", "true", or "false" I think we're better to do this instead: if test -n "$meld_has_output_option" then : use configured output else : messing with help fi > + then > + : 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 If I were writing this patch, I probably changed get_meld_help_msg to init_meld_help_msg () { if test -z "$meld_help_msg" then meld_path="$(git config mergetool.meld.path || echo meld)" meld_help_msg=$($meld_path --help 2>&1) fi } And call init_meld_help_msg unconditionally here, (and in --auto-merge arm below, maybe other arms in the future). I'm writing without much thought into this, please take my word with a grain of salt :) > +} > + > + case "$meld_help_msg" in > + *"--output="* | *"[OPTION"???"]"*) I think Git project prefer aligning case arm with case, IOW, move left 1 TAB. > + # old ones mention --output and new ones just say OPTION... > + meld_has_output_option=true ;; It's nice to see this update, good. The comment is no longer correct, though. The version 3.20.2 has --output but not OPTIONS. It's not introduced by your change, but I think it's better to say: # All versions that has [OPTIONS???] supports --output > + *) > + meld_has_output_option=false ;; > + esac > + fi > + fi > + # Check whether we should use 'meld --auto-merge ...' > + 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 Since we don't canonicallise to bool output of mergetool.meld.hasAutoMerge, I think we would need: case "$meld_has_auto_merge_option" in [Tt]rue|[Yy]es|[Oo]n) meld_has_auto_merge_option=true ;; auto) : this shenanigan ;; esac But, that's a bit messy. Let's see other's opinions. > + 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)" > + fi > + > + case "$meld_help_msg" in > + *"--auto-merge"*) > + : old ones mention --output and new ones just say OPTION... This comment doesn't apply here. > + meld_has_auto_merge_option=true ;; > + *) > + meld_has_auto_merge_option=false ;; > + esac > + fi > fi > } > > base-commit: 07d8ea56f2ecb64b75b92264770c0a664231ce17 > -- > gitgitgadget -- Danh
On 2020-07-02 06:02:27+0800, lin.sun@zoom.us wrote: > Hi Danh, Hi Lin, > > I'll push the patch to GitGitGadget so that it's will sending from > the system mail. Sorry for this noise. While you're doing it, do you mind changing hasAutoMerge to useAutoMerge? I think that name is better :) c.f: https://lore.kernel.org/git/xmqq7dvndnf0.fsf@gitster.c.googlers.com/ Thanks,
Hi Danh, > While you're doing it, > do you mind changing hasAutoMerge to useAutoMerge? > I think that name is better :) Okay, no problem, it will be ` useAutoMerge`. Regards Lin
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. + 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..9ee835b1e5 100644 --- a/mergetools/meld +++ b/mergetools/meld @@ -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 () { + 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 () { + # 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 + then + : 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 + + 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 + fi + fi + # Check whether we should use 'meld --auto-merge ...' + 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)" + fi + + 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 + fi fi }