| Message ID | pull.781.v13.git.git.1594254906647.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v13] Support auto-merge for meld to follow the vim-diff behavior | expand |
Hi Danh,
The [PATCH v13] add the missing "*" back at the back of *'[OPTION...]'.
-+ *"--auto-merge"*|*'[OPTION...]')
++ *"--auto-merge"*|*'[OPTION...]'*)
https://lore.kernel.org/git/pull.781.v13.git.git.1594254906647.gitgitgadget@gmail.com
Thanks
Regards
Lin
"sunlin via GitGitGadget" <gitgitgadget@gmail.com> writes: > From: Lin Sun <lin.sun@zoom.us> > Subject: Re: [PATCH v13] Support auto-merge for meld to follow the vim-diff behavior I'd retitle to match the recommended structure, e.g. Subject: [PATCH] mergetool: update meld backend to allow using '--auto-merge' while queuing. Thanks. I might have more comments later.
"sunlin via GitGitGadget" <gitgitgadget@gmail.com> writes: > + case "$meld_help_msg" in > + *"--output="*|*'[OPTION...]'*) This may be personal preference, but I think the way you spelled this line with SP on both sides of '|' was much easier to read. > + # 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 > + true|false) > + : use well formatted boolean value > + ;; OK. > + auto) > + # testing the "--auto-merge" option only if config is "auto" > + init_meld_help_msg > + > + case "$meld_help_msg" in > + *"--auto-merge"*|*'[OPTION...]'*) > + meld_use_auto_merge_option=true > + ;; > + *) > + meld_use_auto_merge_option=false > + ;; > + esac > + ;; OK. > + *) This is the case where we saw "", "True", "treu", etc. from the string "git config --get". > + if meld_use_auto_merge_option=$(\ > + git config --bool mergetool.meld.useAutoMerge) You do not need that backslash, do you? Perhaps if meld_use_auto_merge_option=$( git config --bool mergetool.meld.useAutoMerge ) then if you really want to spread them into multiple lines. > + then > + : use normalized boolean value Sure, we got a valid boolean. > + else > + meld_use_auto_merge_option=false Why? Shoudln't we loudly complain to let the user know of a misspelt value in the configuration? > + fi > + ;; > + esac > fi > } > > base-commit: 07d8ea56f2ecb64b75b92264770c0a664231ce17
Hi Junio, I'll add SP on both side of '|', and remove the backslash, thank you. This line assign value with `git config --bool...` is over 80 characters, so there is a break. >> + else >> + meld_use_auto_merge_option=false >Why? Shoudln't we loudly complain to let the user know of a misspelt value in the configuration? The command line `git config --bool ...` without "2>&/dev/null" will print error message, just passthrough to user. $ git config mergetool.meld.useAutoMerge hello $ git mergetool --tool=meld fatal: bad numeric config value 'hello' for 'mergetool.meld.useautomerge' in .git/config: invalid unit I'll upload the changes soon. Regards Lin
<lin.sun@zoom.us> writes: > Hi Junio, > > I'll add SP on both side of '|', and remove the backslash, thank you. > > This line assign value with `git config --bool...` is over 80 characters, so there is a break. > >>> + else >>> + meld_use_auto_merge_option=false >>Why? Shoudln't we loudly complain to let the user know of a misspelt value in the configuration? > > The command line `git config --bool ...` without "2>&/dev/null" will print error message, just passthrough to user. > $ git config mergetool.meld.useAutoMerge hello > $ git mergetool --tool=meld > fatal: bad numeric config value 'hello' for 'mergetool.meld.useautomerge' in .git/config: invalid unit Yes, but you do not exit(1) here, so the user will keep going without having a chance to stop, think and correct the misspelt value in the configuration file, no? > > I'll upload the changes soon. > > Regards > Lin
Junio C Hamano <gitster@pobox.com> writes: >>>> + else >>>> + meld_use_auto_merge_option=false >>>Why? Shoudln't we loudly complain to let the user know of a misspelt value in the configuration? >> >> The command line `git config --bool ...` without "2>&/dev/null" will print error message, just passthrough to user. >> $ git config mergetool.meld.useAutoMerge hello >> $ git mergetool --tool=meld >> fatal: bad numeric config value 'hello' for 'mergetool.meld.useautomerge' in .git/config: invalid unit > > Yes, but you do not exit(1) here, so the user will keep going > without having a chance to stop, think and correct the misspelt > value in the configuration file, no? Hmph, I've been forgetting an important case you are handling well, which is that the user does not have the variable defined anywhere in the configuration. "git config --bool var.ia.ble" fails silently in such a case, and you do want to declare that the user does not want to use the "--auto-merge" in that case, which is what this silent else clause does very well. So, let's leave this part as-is. Thanks, I'll push out a new iteration of today's integration before going to bed. > >> >> I'll upload the changes soon. >> >> Regards >> Lin
Hi Junio, I had add a new c function ` git_config_bool_or_str()` to support get the configured value for true/false/auto with git. It will make the `mergetool/meld` only calling `git config` once for getting the `useAutoMerge`. Please review the [PATH v16], thanks. https://lore.kernel.org/git/pull.781.v16.git.git.1594544903477.gitgitgadget@gmail.com Regards Lin
Hi Junio, I had rollback the changes, which is submit as [PATCH v17], it's same as [PATCH v14]. You can just ignore it. Thanks https://lore.kernel.org/git/pull.781.v17.git.git.1594596738929.gitgitgadget@gmail.com https://lore.kernel.org/git/pull.781.v14.git.git.1594268906195.gitgitgadget@gmail.com Regards Lin
diff --git a/Documentation/config/mergetool.txt b/Documentation/config/mergetool.txt index 09ed31dbfa..16a27443a3 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 wait 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..5e5872e1c3 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 + true|false) + : use well formatted boolean value + ;; + auto) + # testing the "--auto-merge" option only if config is "auto" + init_meld_help_msg + + case "$meld_help_msg" in + *"--auto-merge"*|*'[OPTION...]'*) + meld_use_auto_merge_option=true + ;; + *) + meld_use_auto_merge_option=false + ;; + esac + ;; + *) + if meld_use_auto_merge_option=$(\ + git config --bool mergetool.meld.useAutoMerge) + then + : use normalized boolean value + else + meld_use_auto_merge_option=false + fi + ;; + esac fi }