| Message ID | pull.781.v9.git.git.1594002423685.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v9] Support auto-merge for meld to follow the vim-diff behavior | expand |
"sunlin via GitGitGadget" <gitgitgadget@gmail.com> writes: > From: Lin Sun <lin.sun@zoom.us> > > 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. > 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.
Hi Junio, Thank you for your comments, and I made the changes in [PATH v10]. For `git config --bool ...`, with a `=` appending to the rerere.enabled will get reverse result, here are the commands. $ echo [rerere] enabled >> .git/config $ git config --bool rerere.enabled true $ echo [rerere] enabled = >> .git/config $ git config --bool rerere.enabled false So in [PATH v10], it still try get the string value first, then detecting the ""(empty)/true/false/auto. That will cover most cases with one `git config...` calling for useAutoMerge is not configured or is configured properly. Otherwise, it will try call `git config --bool ...` for checking more boolean configuration values. Please review the [PATH v10], thank you. https://lore.kernel.org/git/pull.781.v10.git.git.1594102679750.gitgitgadget@gmail.com/ Regards Lin
<lin.sun@zoom.us> writes:
> So in [PATH v10], it still try get the string value first, then detecting the ""(empty)/true/false/auto.
You cannot interpret an empty output from "git config section.variable";
it could be "[section] variable" (which is true), or it could be a sign
that there is no "[section] variable = value" in the configuration (which
you treat as false).
Catching common spellings of 'true' and 'false' in the output of the
string version of "git config", while checking for 'auto' at the
same time, may not be too bad as an optimization to save an extra
call to "git config --bool 2>/dev/null" (and ignoring errors), so I
am OK with that as long as you leave the empty output alone.
Hi Junio, > You cannot interpret an empty output from "git config section.variable"; it could be "[section] variable" (which is true), or it could be a sign that there is no "[section] variable = value" in the configuration (which you treat as false) Yes, I notice that both "[section] variable " and "[section] variable = " output empty string but have reverse Boolean value. > Catching common spellings of 'true' and 'false' in the output of the string version of "git config", while checking for 'auto' at the same time, may not be too bad as an optimization to save an extra call to "git config --bool 2>/dev/null" (and ignoring errors)... With preview point, ` git config --bool ...` would better than "not be too bad" : ) With the return code and return string from ` git config --bool ...`, follow pseudo-code maybe good for current situation. if meld_use_auto_merge_option =` git config --bool ...` then # success, maybe "(empty)"/true/false if test -z "$ meld_use_auto_merge_option" then meld_use_auto_merge_option=false fi else # failed, the option is not empty or Boolean if test "auto" = ` git config ` then # detect the "--auto-merge" option else meld_use_auto_merge_option=false fi fi Changes will upload with the [PATCH v12]. I'll update to you when the patch is ready. Regards Lin
<lin.sun@zoom.us> writes: > if meld_use_auto_merge_option =` git config --bool ...` > then > # success, maybe "(empty)"/true/false > if test -z "$ meld_use_auto_merge_option" > then > meld_use_auto_merge_option=false > fi The inner test does not make any sense to me; I doubt that you can get an empty output from a "git config --bool" that succeeds. $ cat >config <<\EOF [x] a b=true c=1 d=false e=no f=bogus EOF $ for i in a b c d e f do if v=$(git config --file=config --bool x.$i 2>/dev/null) then echo "succeeds (x.$i) => '$v'" else echo "fails (x.$i) => '$v'" fi done You should see succeeds (x.a) => 'true' succeeds (x.b) => 'true' succeeds (x.c) => 'true' succeeds (x.d) => 'false' succeeds (x.e) => 'false' fails (x.f) => '' if you run the above. The most interesting is how all boolean values are normalized to true and false regardless of how they are originally spelled. > else > # failed, the option is not empty or Boolean > if test "auto" = ` git config ` > then > # detect the "--auto-merge" option > else > meld_use_auto_merge_option=false I think review by Đoàn pointed out that we would want to warn/fail loudly in this "else" case, as it is likely a misspelt configuration value? To recap, I think something along the following lines would be the most readable and acceptably efficient with a short-cut for the common cases. var=mergetool.meld.whatever val=$(git config $var) case "$val" in true | false) # we can take a short-cut without asking config --bool ... use $val ... ;; auto) ... auto detect ... ;; *) if val=$(git config --bool $var 2>/dev/null) then # succeeded, so val must be a normalized boolean ... use $val ... else die cannot parse $var fi easc Thanks.
diff --git a/Documentation/config/mergetool.txt b/Documentation/config/mergetool.txt index 09ed31dbfa..031c78aa95 100644 --- a/Documentation/config/mergetool.txt +++ b/Documentation/config/mergetool.txt @@ -30,6 +30,16 @@ mergetool.meld.hasOutput:: to `true` tells Git to unconditionally use the `--output` option, and `false` avoids using `--output`. +mergetool.meld.useAutoMerge:: + When the `--auto-merge` is given, meld will merge all non-conflicting + parts automatically, highlight the conflicting parts and waiting for + 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. + 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..ba6607a088 100644 --- a/mergetools/meld +++ b/mergetools/meld @@ -3,34 +3,88 @@ 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" + 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" 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 + ;; + *) + : 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 + ;; + esac + fi + # Check whether we should use 'meld --auto-merge ...' + if test -z "$meld_use_auto_merge_option" 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 + "") + meld_use_auto_merge_option=false + ;; + [Aa]uto) + # 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 + ;; + *) + 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 + ;; + esac fi }