| Message ID | pull.781.v6.git.git.1593650687697.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v6] Support auto-merge for meld to follow the vim-diff behavior | expand |
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
"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.
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
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.
Đ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 --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 }