Re: [PATCH v16] Support auto-merge for meld to follow the vim-diff behavior

[Junio C Hamano <gitster@pobox.com>]

"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.