Re: [PATCH v9] Support auto-merge for meld to follow the vim-diff behavior
From: Junio C Hamano <hidden>
Date: 2020-07-06 22:31:37
"sunlin via GitGitGadget" [off-list ref] writes:
From: Lin Sun <redacted> Make the mergetool used with "meld" backend behave similarly to "vimdiff" by telling it to auto-merge non-conflicting parts and highlight the conflicting parts when `mergetool.meld.useAutoMerge` is configured with `true`, or `auto` for detecting the `--auto-merge` option automatically.
This reads well.
+mergetool.meld.useAutoMerge:: + When the `--auto-merge` is given, meld will merge all non-conflicting + parts automatically, highlight the conflicting parts and waiting for
s/waiting/wait/, I presume, i.e. "merge", "highlight" and "wait" all share the "meld will" that starts the sentence.
+ user decision. 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.
Other than that, this reads well, too.
quoted hunk
diff --git a/mergetools/meld b/mergetools/meld index 7a08470f88..ba6607a088 100644 --- a/mergetools/meld +++ b/mergetools/meld@@ -3,34 +3,88 @@ 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 }
OK.
+# Get meld help message
+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
+}OK.
+# 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"
then
+ meld_has_output_option=$(git config --bool mergetool.meld.hasOutput)
+ case "$meld_has_output_option" in
+ true|false)
+ : use configured value
+ ;;I think the assignment before "case" would have yielded a non-zero exist status, so that would be another way to tell if we got a proper boolean value, but this is also good.
+ *) + : empty or invalid configured value, detecting "--output" automatically + 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 + ;;
OK.
+ esac + fi + # Check whether we should use 'meld --auto-merge ...' + if test -z "$meld_use_auto_merge_option" then + meld_use_auto_merge_option=$(git config mergetool.meld.useAutoMerge) + case "$meld_use_auto_merge_option" in + "") + meld_use_auto_merge_option=false
This is wrong, isn't it? I have
[rerere] enabled
in my .git/config and I'd certainly get an empty string if you ask
about it in a non-bool context:
$ o=$(git config --bool rerere.enabled); echo $o
true
$ o=$(git config rerere.enabled); echo $o
$
In short, I think you can just drop this test for an empty string here.
+ ;; + [Aa]uto)
Does any other configuration variable that takes "auto" take "Auto", "AUTO", etc. as syonyms? It's not like we said "you must create a file whose name is auto to trigger this feature" and some people live on a case insensitive filesystem that may make it impossible for them to do so, so I do not see much point in doing this.
+ # testing the "--auto-merge" option only if config is "[Aa]uto" + init_meld_help_msg + + case "$meld_help_msg" in + *"--auto-merge"*) + meld_use_auto_merge_option=true + ;; + *) + meld_use_auto_merge_option=false + ;; + esac
I didn't notice this until now, but for "--auto-merge", the "if your meld is new enough, the option name may not appear and you would only see [OPTION...]" rule we had for the "--output" does not apply? I am not a meld user, and "yes, my code is correct" is a perfectly good answer (in other word, the above question is NOT a rhetorical one that points out an error in the code being reviewed).
+ ;; + *) + bool_value=$(git config --bool mergetool.meld.useAutoMerge 2>/dev/null) + if test -n "$bool_value" + then + meld_use_auto_merge_option="$bool_value" + else + meld_use_auto_merge_option=false + fi + ;;
Perhaps the whole thing is easier to read if it is structured more
like so:
varname=mergetool.meld.useAutoMerge
if meld_use_auto_merge_option=$(git config --bool "$varname" 2>/dev/null)
then
: happy
else
# not a boolean. Is it 'auto'?
val=$(git config "$varname")
case "$val" in
auto)
;;
*)
die "unrecognized value '$val' for $varname"
;;;
esac
... here we auto detect ...
fi
Thanks.