| Message ID | pull.781.v16.git.git.1594544903477.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v16] 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. > > Helped-by: Đoàn Trần Công Danh <congdanhqx@gmail.com> > Helped-by: David Aguilar <davvid@gmail.com> > Signed-off-by: Lin Sun <lin.sun@zoom.us> > Signed-off-by: Junio C Hamano <gitster@pobox.com> > --- > 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-v16 > Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-781/sunlin7/master-v16 > Pull-Request: https://github.com/git/git/pull/781 > > Range-diff vs v15: > > 1: 02d849784f ! 1: d235a576b4 Support auto-merge for meld to follow the vim-diff behavior > @@ mergetools/meld: diff_cmd () { > - else > - meld_has_output_option=false > + meld_use_auto_merge_option=$( > -+ git --bool-or-str config mergetool.meld.useAutoMerge) > ++ git config --bool-or-str mergetool.meld.useAutoMerge) It is quite clear in this hunk that that the previous one was not even proofread before sending X-<. > + } else if (type == TYPE_BOOL_OR_STR) { > + int is_bool, v; > + v = git_config_bool_or_str(NULL, key_, value_, &is_bool); > + if (is_bool) > + strbuf_addstr(buf, v ? "true" : "false"); > + else > + strbuf_addstr(buf, value_); > } else if (type == TYPE_PATH) { > const char *v; > if (git_config_pathname(&v, key_, value_) < 0) > @@ -411,6 +422,14 @@ static char *normalize_value(const char *key, const char *value) > else > return xstrdup(v ? "true" : "false"); > } > + if (type == TYPE_BOOL_OR_STR) { > + int is_bool, v; > + v = git_config_bool_or_str(NULL, key, value, &is_bool); > + if (!is_bool) > + return xstrdup(value); > + else > + return xstrdup(v ? "true" : "false"); > + } That's unfortunate that we need almost identical code duplicated here and above. It probably is a tad larger than what we typcally call #leftoverbits, so please ignore it for now. > diff --git a/config.c b/config.c > index 8db9c77098..4c6c06d10b 100644 > --- a/config.c > +++ b/config.c > @@ -1100,6 +1100,20 @@ int git_config_bool_or_int(const char *name, const char *value, int *is_bool) > return git_config_int(name, value); > } > > +int git_config_bool_or_str(const char **dest, const char *name, const char *value, int *is_bool) > +{ > + int v = git_parse_maybe_bool_text(value); > + if (0 <= v) { > + *is_bool = 1; > + return v; > + } > + *is_bool = 0; > + if (dest != NULL) > + return git_config_string(dest, name, value); > + else > + return 0; > +} Wrong indentation. I do not think this is a good interface at all, from at least three points. - What happens when the value is set to "2"? git_config_bool() would say, because it calls git_config_bool_or_int() and learns that the value is an integer 2 and uses !! operator on it to normalize it to 1, we judge it as "true". Your implementation says it is not a bool and instead it is a string "2". When telling a boolean and an integer apart, saying 2 is not a bool makes sense, but given that "interpret this value as boolean" logic in git_config_bool() says "2" is a true, the logic to tell a boolean and a string apart probably should say that the user who wrote "2" there meant true, i.e. boolean. - What's the returned value from this function and how can the caller sensibly use it? If it happened to be (narrowly defined) bool, the returned value is 0 for false and 1 for true. Otherwise, the caller gets 0 if it forgets to pass dest, or 0 if value successfully gets returned as a string, or -1 upon an error. Hence it is impossible for the caller to use if (git_config_bool_or_str(...)) { ... do one thing ... } else { ... do something else ... } - There is no point to pass dest to this function. If it is not bool, then the caller can do strdup() the value. > diff --git a/config.h b/config.h > index 060874488f..175b88d9c5 100644 > --- a/config.h > +++ b/config.h > @@ -217,6 +217,13 @@ ssize_t git_config_ssize_t(const char *, const char *); > */ > int git_config_bool_or_int(const char *, const char *, int *); > > +/** > + * Same as `git_config_bool`, except that `is_bool` flag is unset, then if > + * `dest` parameter is non-NULL, it allocates and copies the value string > + * into the `dest`, if `dest` is NULL and `is_bool` flag is unset it return 0. > + */ is_bool is not an "in-parameter" flag but a pointer to point at where the result is stored, so the above description does not make much sense. I suspect, from the actual implementation, that you wanted to say Parse "value" to see if it is a boolean, and if so set *is_bool to true and leave *dest untouched. If it is not a boolean, set *is_bool to false and assign a copy of value to *dest. But again, I do not think this function is designed right, so let's not spend any more time polishing what you wrote for now. I would expect something like this in builtin/config.c would be sufficient: if (type == TYPE_BOOL_OR_STRING) { int v = git_parse_maybe_bool(value); if (v < 0) return xstrdup(value); else return xstrdup(v ? "true" : "false"); } i.e. we do not need a new helper in the lower level of the API stack. > + meld_use_auto_merge_option=$( > + git config --bool-or-str mergetool.meld.useAutoMerge) If the body is made on a separate line for readability, doing it more like so would be even more readable: meld_use_auto_merge_option=$( git config --bool-or-str 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 > + ;; > + *) > + meld_use_auto_merge_option=false Now that the --bool-or-string would be silent, you have to give an error message yourself here, no? Have you hand-tested the result of applying your patch to see if all the cases we care about (i.e. various scenarios we raised and thought together how the code should react to the situation during the review discussion so far)? We are not in a hurry, and we will not be paying too much attention on topics that are not yet in 'next' until the upcoming release is done anyway, so take your time to try polishing before sending anything out. Thanks.
Hi Junio, Apologize for the [PATCH v16] which is not well thoughtful, and I totally agree that we will not paying too much attention before upcoming release. I'll rollback changes to preview one, thank you so much. Best Regards Lin
Junio C Hamano <gitster@pobox.com> writes: > ... > Now that the --bool-or-string would be silent, you have to give an > error message yourself here, no? Have you hand-tested the result of > applying your patch to see if all the cases we care about (i.e. > various scenarios we raised and thought together how the code should > react to the situation during the review discussion so far)? > > We are not in a hurry, and we will not be paying too much attention > on topics that are not yet in 'next' until the upcoming release is > done anyway, so take your time to try polishing before sending > anything out. Just for fun, I've queued the following on top of v16 and merged the result to 'seen'. As this adds a new feature to "git config", it also needs updates to Documentation/git-config.txt and tests for the feature, and it probably makes sense to make it a two-patch series. Everything related to the "git config" enhancement as 1/2, and change to mergetools/meld as 2/2. builtin/config.c | 16 +++++++--------- config.c | 14 -------------- config.h | 7 ------- mergetools/meld | 16 ++++++++++------ 4 files changed, 17 insertions(+), 36 deletions(-) diff --git a/builtin/config.c b/builtin/config.c index 6f2ddadc80..7891e070a4 100644 --- a/builtin/config.c +++ b/builtin/config.c @@ -65,7 +65,7 @@ static int show_scope; #define TYPE_PATH 4 #define TYPE_EXPIRY_DATE 5 #define TYPE_COLOR 6 -#define TYPE_BOOL_OR_STR 7 +#define TYPE_BOOL_OR_STR 7 #define OPT_CALLBACK_VALUE(s, l, v, h, i) \ { OPTION_CALLBACK, (s), (l), (v), NULL, (h), PARSE_OPT_NOARG | \ @@ -255,12 +255,11 @@ static int format_config(struct strbuf *buf, const char *key_, const char *value else strbuf_addf(buf, "%d", v); } else if (type == TYPE_BOOL_OR_STR) { - int is_bool, v; - v = git_config_bool_or_str(NULL, key_, value_, &is_bool); - if (is_bool) - strbuf_addstr(buf, v ? "true" : "false"); - else + int v = git_parse_maybe_bool(value_); + if (v < 0) strbuf_addstr(buf, value_); + else + strbuf_addstr(buf, v ? "true" : "false"); } else if (type == TYPE_PATH) { const char *v; if (git_config_pathname(&v, key_, value_) < 0) @@ -423,9 +422,8 @@ static char *normalize_value(const char *key, const char *value) return xstrdup(v ? "true" : "false"); } if (type == TYPE_BOOL_OR_STR) { - int is_bool, v; - v = git_config_bool_or_str(NULL, key, value, &is_bool); - if (!is_bool) + int v = git_parse_maybe_bool(value); + if (v < 0) return xstrdup(value); else return xstrdup(v ? "true" : "false"); diff --git a/config.c b/config.c index 4c6c06d10b..8db9c77098 100644 --- a/config.c +++ b/config.c @@ -1100,20 +1100,6 @@ int git_config_bool_or_int(const char *name, const char *value, int *is_bool) return git_config_int(name, value); } -int git_config_bool_or_str(const char **dest, const char *name, const char *value, int *is_bool) -{ - int v = git_parse_maybe_bool_text(value); - if (0 <= v) { - *is_bool = 1; - return v; - } - *is_bool = 0; - if (dest != NULL) - return git_config_string(dest, name, value); - else - return 0; -} - int git_config_bool(const char *name, const char *value) { int discard; diff --git a/config.h b/config.h index 175b88d9c5..060874488f 100644 --- a/config.h +++ b/config.h @@ -217,13 +217,6 @@ ssize_t git_config_ssize_t(const char *, const char *); */ int git_config_bool_or_int(const char *, const char *, int *); -/** - * Same as `git_config_bool`, except that `is_bool` flag is unset, then if - * `dest` parameter is non-NULL, it allocates and copies the value string - * into the `dest`, if `dest` is NULL and `is_bool` flag is unset it return 0. - */ -int git_config_bool_or_str(const char **, const char *, const char *, int *); - /** * Parse a string into a boolean value, respecting keywords like "true" and * "false". Integer values are converted into true/false values (when they diff --git a/mergetools/meld b/mergetools/meld index bc2ea894d7..aab4ebb935 100644 --- a/mergetools/meld +++ b/mergetools/meld @@ -36,7 +36,7 @@ check_meld_for_features () { then meld_has_output_option=$(git config --bool mergetool.meld.hasOutput) case "$meld_has_output_option" in - true|false) + true | false) : use configured value ;; *) @@ -44,7 +44,7 @@ check_meld_for_features () { init_meld_help_msg case "$meld_help_msg" in - *"--output="*|*'[OPTION...]'*) + *"--output="* | *'[OPTION...]'*) # All version that has [OPTION...] supports --output meld_has_output_option=true ;; @@ -59,9 +59,10 @@ check_meld_for_features () { if test -z "$meld_use_auto_merge_option" then meld_use_auto_merge_option=$( - git config --bool-or-str mergetool.meld.useAutoMerge) + git config --bool-or-str mergetool.meld.useAutoMerge + ) case "$meld_use_auto_merge_option" in - true|false) + true | false) : use well formatted boolean value ;; auto) @@ -69,7 +70,7 @@ check_meld_for_features () { init_meld_help_msg case "$meld_help_msg" in - *"--auto-merge"*|*'[OPTION...]'*) + *"--auto-merge"* | *'[OPTION...]'*) meld_use_auto_merge_option=true ;; *) @@ -77,9 +78,12 @@ check_meld_for_features () { ;; esac ;; - *) + "") meld_use_auto_merge_option=false ;; + *) + die "unknown mergetool.meld.useAutoMerge: $meld_use_auto_merge_option" + ;; esac fi }
Hi Junio,
> As this adds a new feature to "git config", it also needs updates to Documentation/git-config.txt and tests for the feature, and it probably makes sense to make it a two-patch series. Everything related to the "git config" enhancement as 1/2, and change to mergetools/meld as 2/2.
Thank you for your comments, these are key points for adding new feature into git.
I'll try my best to make new pull-requests for option `--bool-or-string` after current feature is applied upstream.
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/builtin/config.c b/builtin/config.c index ee4aef6a35..6f2ddadc80 100644 --- a/builtin/config.c +++ b/builtin/config.c @@ -65,6 +65,7 @@ static int show_scope; #define TYPE_PATH 4 #define TYPE_EXPIRY_DATE 5 #define TYPE_COLOR 6 +#define TYPE_BOOL_OR_STR 7 #define OPT_CALLBACK_VALUE(s, l, v, h, i) \ { OPTION_CALLBACK, (s), (l), (v), NULL, (h), PARSE_OPT_NOARG | \ @@ -94,6 +95,8 @@ static int option_parse_type(const struct option *opt, const char *arg, new_type = TYPE_INT; else if (!strcmp(arg, "bool-or-int")) new_type = TYPE_BOOL_OR_INT; + else if (!strcmp(arg, "bool-or-str")) + new_type = TYPE_BOOL_OR_STR; else if (!strcmp(arg, "path")) new_type = TYPE_PATH; else if (!strcmp(arg, "expiry-date")) @@ -149,6 +152,7 @@ static struct option builtin_config_options[] = { OPT_CALLBACK_VALUE(0, "bool", &type, N_("value is \"true\" or \"false\""), TYPE_BOOL), OPT_CALLBACK_VALUE(0, "int", &type, N_("value is decimal number"), TYPE_INT), OPT_CALLBACK_VALUE(0, "bool-or-int", &type, N_("value is --bool or --int"), TYPE_BOOL_OR_INT), + OPT_CALLBACK_VALUE(0, "bool-or-str", &type, N_("value is --bool or string"), TYPE_BOOL_OR_STR), OPT_CALLBACK_VALUE(0, "path", &type, N_("value is a path (file or directory name)"), TYPE_PATH), OPT_CALLBACK_VALUE(0, "expiry-date", &type, N_("value is an expiry date"), TYPE_EXPIRY_DATE), OPT_GROUP(N_("Other")), @@ -250,6 +254,13 @@ static int format_config(struct strbuf *buf, const char *key_, const char *value strbuf_addstr(buf, v ? "true" : "false"); else strbuf_addf(buf, "%d", v); + } else if (type == TYPE_BOOL_OR_STR) { + int is_bool, v; + v = git_config_bool_or_str(NULL, key_, value_, &is_bool); + if (is_bool) + strbuf_addstr(buf, v ? "true" : "false"); + else + strbuf_addstr(buf, value_); } else if (type == TYPE_PATH) { const char *v; if (git_config_pathname(&v, key_, value_) < 0) @@ -411,6 +422,14 @@ static char *normalize_value(const char *key, const char *value) else return xstrdup(v ? "true" : "false"); } + if (type == TYPE_BOOL_OR_STR) { + int is_bool, v; + v = git_config_bool_or_str(NULL, key, value, &is_bool); + if (!is_bool) + return xstrdup(value); + else + return xstrdup(v ? "true" : "false"); + } if (type == TYPE_COLOR) { char v[COLOR_MAXLEN]; if (git_config_color(v, key, value)) diff --git a/config.c b/config.c index 8db9c77098..4c6c06d10b 100644 --- a/config.c +++ b/config.c @@ -1100,6 +1100,20 @@ int git_config_bool_or_int(const char *name, const char *value, int *is_bool) return git_config_int(name, value); } +int git_config_bool_or_str(const char **dest, const char *name, const char *value, int *is_bool) +{ + int v = git_parse_maybe_bool_text(value); + if (0 <= v) { + *is_bool = 1; + return v; + } + *is_bool = 0; + if (dest != NULL) + return git_config_string(dest, name, value); + else + return 0; +} + int git_config_bool(const char *name, const char *value) { int discard; diff --git a/config.h b/config.h index 060874488f..175b88d9c5 100644 --- a/config.h +++ b/config.h @@ -217,6 +217,13 @@ ssize_t git_config_ssize_t(const char *, const char *); */ int git_config_bool_or_int(const char *, const char *, int *); +/** + * Same as `git_config_bool`, except that `is_bool` flag is unset, then if + * `dest` parameter is non-NULL, it allocates and copies the value string + * into the `dest`, if `dest` is NULL and `is_bool` flag is unset it return 0. + */ +int git_config_bool_or_str(const char **, const char *, const char *, int *); + /** * Parse a string into a boolean value, respecting keywords like "true" and * "false". Integer values are converted into true/false values (when they diff --git a/mergetools/meld b/mergetools/meld index 7a08470f88..bc2ea894d7 100644 --- a/mergetools/meld +++ b/mergetools/meld @@ -3,34 +3,83 @@ 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 --bool-or-str 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 + ;; + *) + meld_use_auto_merge_option=false + ;; + esac fi }