"sunlin via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes: > From: Lin Sun <lin.sun@xxxxxxx> > > 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@xxxxxxxxx> > Helped-by: David Aguilar <davvid@xxxxxxxxx> > Signed-off-by: Lin Sun <lin.sun@xxxxxxx> > Signed-off-by: Junio C Hamano <gitster@xxxxxxxxx> > --- > 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.
