Re: [PATCH v10 3/3] mergetool: add per-tool support and overrides for the hideResolved flag

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

Seth House <seth@eseth.com> writes:

> Keep the global mergetool flag and add a per-tool override flag so that
> users may enable the flag for one tool and disable it for another. In
> addition, the author or maintainer of a mergetool may optionally elect
> to set the default `hideResolved` value for that mergetool.

OK.

> To disable the feature for a specific tool, edit the `mergetools/<tool>`
> shell script for that tool and add a `hide_resolved_enabled` function:
>
>     hide_resolved_enabled () {
>         return 1
>     }
>
> Disabling may be desirable if the mergetool wants or needs access to the
> original, unmodified 'LOCAL' and 'REMOTE' versions of the conflicted
> file.

The above sounds as if it is a hint/help for end users, but it is
unreasonable to expect all end users of a particular <tool> to edit
part of their Git installation.  I suspect that you didn't mean it
that way, and instead it is meant to advise (new) tool authors who
will add mergetools/<tool> for their own tool, and when read with
that in mind, it does make sort-of sense (except that when you are
author of this thing, you won't "edit" as if you are modifying
something that already exists---you'd be the one who is adding the
<tool> under mergetools/ directory).

For an end-user, to disable the feature for a tool, you'd just
configure mergetool.<tool>.hideResolved to 'false', right?

> For example:
>
> - A tool may use a custom conflict resolution algorithm and prefer to
>   ignore the results of Git's conflict resolution.
> - A tool may want to visually compare/constrast the version of the file
>   from before the merge (saved to 'LOCAL', 'REMOTE', and 'BASE') with
>   Git's conflict resolution results (saved to 'MERGED').
>
> Helped-by: Johannes Sixt <j6t@kdbg.org>
> Helped-by: Junio C Hamano <gitster@pobox.com>
> Signed-off-by: Seth House <seth@eseth.com>
> ---
>  Documentation/config/mergetool.txt |  6 ++++++
>  git-mergetool--lib.sh              |  4 ++++
>  git-mergetool.sh                   | 14 ++++++++++++--
>  3 files changed, 22 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/config/mergetool.txt b/Documentation/config/mergetool.txt
> index 3171bacf91..046816fb07 100644
> --- a/Documentation/config/mergetool.txt
> +++ b/Documentation/config/mergetool.txt
> @@ -13,6 +13,12 @@ mergetool.<tool>.cmd::
>  	merged; 'MERGED' contains the name of the file to which the merge
>  	tool should write the results of a successful merge.
>  
> +mergetool.<tool>.hideResolved::
> +	A mergetool-specific override for the global `mergetool.hideResolved`
> +	configuration flag. This allows individual mergetools to enable or
> +	disable the flag regardless of the global setting. See
> +	`mergetool.hideResolved` for the full description.

This description is iffy.  

The configuration allows "users" to enable or disable the feature
for individual mergetools, overriding the 'mergetool.hideResolved'
global setting, no?  The above paragraph reads as if the tool author
is enabling/disabling it.

> diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
> index e059b3559e..11f00dde41 100644
> --- a/git-mergetool--lib.sh
> +++ b/git-mergetool--lib.sh
> @@ -164,6 +164,10 @@ setup_tool () {
>  		return 1
>  	}
>  
> +	hide_resolved_enabled () {
> +		return 0
> +	}
> +
>  	translate_merge_tool_path () {
>  		echo "$1"
>  	}
> diff --git a/git-mergetool.sh b/git-mergetool.sh
> index 865f12551a..6cf3884277 100755
> --- a/git-mergetool.sh
> +++ b/git-mergetool.sh
> @@ -333,9 +333,19 @@ merge_file () {
>  	checkout_staged_file 2 "$MERGED" "$LOCAL"
>  	checkout_staged_file 3 "$MERGED" "$REMOTE"
>  
> -	if test "$(git config --get mergetool.hideResolved)" != "false"
> +	# hideResolved preferences hierarchy:
> +	# First respect user's tool-specific configuration if exists.
> +	if test "$(git config --get "mergetool.$merge_tool.hideResolved")" != "false"

The same "--type=bool" comment applies to this step, too.

>  	then
> +		# Next respect tool-specified configuration.
> +		if hide_resolved_enabled
> +		then
> +			# Finally respect if user has a global disable.
> +			if test "$(git config --get "mergetool.hideResolved")" != "false"
> +			then
> +				hide_resolved
> +			fi
> +		fi
>  	fi

I am not sure if I understand this logic.

If the user says "for the tool <tool>, set hideresolved to
true/false" explicitly, I think it should be final.  Even if the
tool's author expresses that s/he prefers not to have to work on a
pre-munged input by setting hide_resolved_enabled to false, if the
end-user says s/he wants to use it on that tool, we do not want to
help the tool to override the user's wish.

If the user says "use hideresolved feature, as I like it in general"
by setting mergetool.hideResolved, on the other hand, it may be also
reasonable to heed "no, I recommend against it for this tool" for
individual tool whose hide_resolved_enabled returns false.  And if
the global is set to 'false', the user says "I do not want it", and
it may be iffy to let individual tool to countermand it.

WHen dealing with either of these variables, therefore, you'd need
to know if the variable is not set at all, or if the variable is set
to true, or to false.  Even if we default to enabled, we need to be
able to tell if the user didn't say anything (and we enabled the
feature for the user because of our default choice), or if the user
explicitly said s/he wants it.

In other words, you'd need to treat mergetool.hideResolved and
mergetool.$merge_tool.hideResolved as tristates.

Here is how "git config --type=bool" can be used to normalize
various ways to spell true/false and tell between "not set" and "set
to some value":

    $ git -c a.b config --type=bool a.b; echo $?
    true
    0
    $ git -c a.b=yes config --type=bool a.b; echo $?
    true
    0
    $ git -c a.b=0 config --type=bool a.b; echo $?
    false
    0
    $ git config --type=bool a.b; echo $?
    1

IOW, if "git config --type=bool" fails, the user does not have the
variable set.  If it succeeds, you'd get normalized 'true/false'
string on its standard output.

Using that technique, here is my attempt to rewrite the above logic,
with commentary.

    global_config=mergetool.hideResolved
    tool_config=mergetool.$merge_tool.hideResolved

    if enabled=$(git config --type=bool "$tool_config")
    then
	# The user explicitly says true or false, so there
	# is no point in asking any other source of preferences
	;
    elif enabled=$(git config --type=bool "$global_config")
    then
	# There is a blanket preference for all tools, and 'true'
	# means "I like the hide-resolved in general, so use it
	# when appropriate" by the user.  We can let the tool
	# author to override and disable, though.
	#
	# On the other hand, when set to 'false', it is "I really
	# don't like the feature in general, so do not use it
	# anywhere", which we take it as final, without letting
	# the tool override it.
        if test "$enabled" = true && hide_resolved_enabled
	then
		enabled=true
	else
		enabled=false
	fi
    else
	# The user does not have preference.  Ask the tool
	if hide_resolved_enabled
	then
		enabled=true
	else
		enabled=false
	fi
    fi

    # Now we know if the feature should be used.
    if test "$enabled" = true
    then
	hide_resolved
    fi