git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Seth House <seth@eseth.com>
Cc: git@vger.kernel.org, Johannes Sixt <j6t@kdbg.org>
Subject: Re: [PATCH v10 3/3] mergetool: add per-tool support and overrides for the hideResolved flag
Date: Sat, 30 Jan 2021 00:08:06 -0800	[thread overview]
Message-ID: <xmqqsg6iq23d.fsf@gitster.c.googlers.com> (raw)
In-Reply-To: <20210130054655.48237-4-seth@eseth.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


  reply	other threads:[~2021-01-30  9:15 UTC|newest]

Thread overview: 80+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-12-23  4:53 [PATCH v5 0/1] mergetool: remove unconflicted lines Felipe Contreras
2020-12-23  4:53 ` [PATCH v5 1/1] mergetool: add automerge configuration Felipe Contreras
2020-12-23 13:34   ` Junio C Hamano
2020-12-23 14:23     ` Felipe Contreras
2020-12-23 20:21       ` Junio C Hamano
2020-12-24  0:14         ` Felipe Contreras
2020-12-24  0:32           ` Junio C Hamano
2020-12-24  1:36             ` Felipe Contreras
2020-12-24  6:17               ` Junio C Hamano
2020-12-24 15:59                 ` Felipe Contreras
2020-12-24 22:32                   ` Junio C Hamano
2020-12-27 18:01                     ` Felipe Contreras
2020-12-24  9:24 ` [PATCH v5 0/1] mergetool: remove unconflicted lines Junio C Hamano
2020-12-24 16:16   ` Felipe Contreras
2020-12-30  5:47   ` Johannes Schindelin
2020-12-30 22:33     ` Felipe Contreras
2020-12-27 20:58 ` [PATCH v6 0/1] mergetool: add automerge configuration Seth House
2020-12-27 20:58   ` [PATCH v6 1/2] " Seth House
2020-12-27 22:06     ` Junio C Hamano
2020-12-27 22:29       ` Seth House
2020-12-27 22:59         ` Junio C Hamano
2020-12-27 20:58   ` [PATCH v6 2/2] mergetool: Add per-tool support for the autoMerge flag Seth House
2020-12-27 22:31     ` Junio C Hamano
2020-12-28  0:41   ` [PATCH v7 0/2] mergetool: add automerge configuration Seth House
2020-12-28  0:41     ` [PATCH v7 1/2] " Seth House
2020-12-28  0:41     ` [PATCH v7 2/2] mergetool: Add per-tool support for the autoMerge flag Seth House
2020-12-28  1:18       ` Felipe Contreras
2020-12-28  4:54     ` [PATCH v8 0/4] mergetool: add automerge configuration Seth House
2020-12-28  4:54       ` [PATCH v8 1/4] " Seth House
2020-12-28 11:30         ` Johannes Sixt
2020-12-28  4:54       ` [PATCH v8 2/4] mergetool: Add per-tool support for the autoMerge flag Seth House
2020-12-28 13:09         ` Junio C Hamano
2020-12-28  4:54       ` [PATCH v8 3/4] mergetool: Break setup_tool out into separate initialization function Seth House
2020-12-28 11:48         ` Johannes Sixt
2020-12-28  4:54       ` [PATCH v8 4/4] mergetool: Add automerge_enabled tool-specific override function Seth House
2020-12-28 11:57         ` Johannes Sixt
2020-12-28 13:19         ` Junio C Hamano
2020-12-28 19:29       ` [PATCH v9 0/5] mergetool: add automerge configuration Seth House
2020-12-28 19:29         ` [PATCH v9 1/5] " Seth House
2020-12-28 19:29         ` [PATCH v9 2/5] mergetool: alphabetize the mergetool config docs Seth House
2020-12-28 19:29         ` [PATCH v9 3/5] mergetool: add per-tool support for the autoMerge flag Seth House
2020-12-28 19:29         ` [PATCH v9 4/5] mergetool: break setup_tool out into separate initialization function Seth House
2020-12-29  8:50           ` Johannes Sixt
2020-12-29 17:23             ` Seth House
2020-12-28 19:29         ` [PATCH v9 5/5] mergetool: add automerge_enabled tool-specific override function Seth House
2020-12-29  2:01           ` Felipe Contreras
2021-01-06  5:55           ` Junio C Hamano
2021-01-07  3:58             ` Seth House
2021-01-07  6:38               ` Junio C Hamano
2021-01-07  9:27                 ` Seth House
2021-01-07 21:26                   ` Junio C Hamano
2021-01-08 15:04                     ` Johannes Schindelin
2021-01-30  5:46         ` [PATCH v10 0/3] mergetool: add hideResolved configuration (was automerge) Seth House
2021-01-30  5:46           ` [PATCH v10 1/3] mergetool: add hideResolved configuration Seth House
2021-01-30  8:08             ` Junio C Hamano
2021-01-30  5:46           ` [PATCH v10 2/3] mergetool: break setup_tool out into separate initialization function Seth House
2021-01-30  5:46           ` [PATCH v10 3/3] mergetool: add per-tool support and overrides for the hideResolved flag Seth House
2021-01-30  8:08             ` Junio C Hamano [this message]
2021-02-09 20:07           ` [PATCH v11 0/3] mergetool: add hideResolved configuration (was automerge) Seth House
2021-02-09 20:07             ` [PATCH v11 1/3] mergetool: add hideResolved configuration Seth House
2021-03-09  2:29               ` [PATCH] mergetool: do not enable hideResolved by default Jonathan Nieder
2021-03-09  5:42                 ` Seth House
2021-03-10  1:23                   ` Jonathan Nieder
2021-03-10  8:06                 ` Junio C Hamano
2021-03-11  1:57                   ` Junio C Hamano
2021-03-12 23:12                     ` Junio C Hamano
2021-03-12 23:29                       ` Jonathan Nieder
2021-03-12 23:36                         ` Junio C Hamano
2021-03-13  8:36                           ` [PATCH v2 0/2] " Jonathan Nieder
2021-03-13  8:38                             ` [PATCH 1/2] " Jonathan Nieder
2021-03-13  8:41                             ` [PATCH 2/2] doc: describe mergetool configuration in git-mergetool(1) Jonathan Nieder
2021-03-13 23:34                               ` Junio C Hamano
2021-03-13 23:37                               ` Junio C Hamano
2021-02-09 20:07             ` [PATCH v11 2/3] mergetool: break setup_tool out into separate initialization function Seth House
2021-02-09 20:07             ` [PATCH v11 3/3] mergetool: add per-tool support and overrides for the hideResolved flag Seth House
2021-02-09 22:11             ` [PATCH v11 0/3] mergetool: add hideResolved configuration (was automerge) Junio C Hamano
2021-02-09 23:27               ` Seth House
2020-12-28 10:29     ` [PATCH v7 0/2] mergetool: add automerge configuration Junio C Hamano
2020-12-28 14:40       ` Felipe Contreras
2020-12-28  1:02   ` [PATCH v6 0/1] " Felipe Contreras

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqqsg6iq23d.fsf@gitster.c.googlers.com \
    --to=gitster@pobox.com \
    --cc=git@vger.kernel.org \
    --cc=j6t@kdbg.org \
    --cc=seth@eseth.com \
    --subject='Re: [PATCH v10 3/3] mergetool: add per-tool support and overrides for the hideResolved flag' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).