git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Jonathan Nieder <jrnieder@gmail.com>
To: Junio C Hamano <gitster@pobox.com>
Cc: git@vger.kernel.org,
	Felipe Contreras <felipe.contreras@gmail.com>,
	Seth House <seth@eseth.com>,
	Dana Dahlstrom <dahlstrom@google.com>
Subject: [PATCH] mergetool: do not enable hideResolved by default
Date: Mon, 8 Mar 2021 18:29:35 -0800	[thread overview]
Message-ID: <YEbdj27CmjNKSWf4@google.com> (raw)
In-Reply-To: <20210209200712.156540-2-seth@eseth.com>

A typical mergetool uses four panes, showing the content of the file
being resolved from MERGE_BASE ('BASE'), HEAD ('LOCAL'), MERGE_HEAD
('REMOTE'), and the working copy.  This allows understanding the
conflicts in context: by seeing the entire content of the file from
MERGE_HEAD, say, we can see the full intent of the code we are pulling
in and understand what they were trying to do that conflicted with our
own changes.

Sometimes, though, the exact content of these three competing versions
of a file is not so important.  Especially if the mergetool supports
folding unchanged lines, the new 'mergetool.hideResolved' feature can
be helpful for allowing a person resolving a merge to focus on the
portion with conflicts.  For sections of the file where BASE matched
LOCAL or REMOTE, this feature makes all three versions match the
resolved version, so that the user resolving can focus exclusively on
the portions with conflicts.  In other words, hideResolved makes a
multi-pane merge tool show a similar amount of information to the file
with conflict markers with conflictstyle=diff3, saving the operator
from having to pay attention to parts that resolved cleanly.

98ea309b3f (mergetool: add hideResolved configuration, 2021-02-09)
which introduced this setting enabled it by default, explaining:

    No adverse effects were noted in a small survey of popular mergetools[1]
    so this behavior defaults to `true`. However it can be globally disabled
    by setting `mergetool.hideResolved` to `false`.

In practice, however, this has proved confusing for users.  No
indication is shown in the UI that the base, local, and remote
versions shown have been modified by additional resolution.
Especially in cases where conflicts involve elements beyond textual
conflict, it has resulted in incorrect resolutions and wasted work to
figure out what happened.  Flip the default back to the traditional
behavior of `false`: although the old behavior involves slightly
slower merges in the only-textual-conflicts case, it prevents this
kind of painful moment of betrayal by one's tools, which is more
important.

Should we want to migrate to hideResolved=true in the future, we still
can.  It just requires a more careful migration, including a period
where "git mergetool" shows a warning or errors out in affected cases.

Reported-by: Dana Dahlstrom <dahlstrom@google.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Hi,

Seth House wrote:

> No adverse effects were noted in a small survey of popular mergetools[1]
> so this behavior defaults to `true`. However it can be globally disabled
> by setting `mergetool.hideResolved` to `false`.

Thanks much for protecting this by a flag.  We tried this out
internally at Google when it hit "next" and not too long later
realized that the new default of "true" is not workable for us.  I
don't believe it's the right default for Git, either, hence this
patch.

Thanks for working on the merge resolution workflow; it's much
appreciated.

Sincerely,
Jonathan

 Documentation/config/mergetool.txt | 2 +-
 git-mergetool.sh                   | 9 ++-------
 2 files changed, 3 insertions(+), 8 deletions(-)

diff --git a/Documentation/config/mergetool.txt b/Documentation/config/mergetool.txt
index 90f76f5b9b..cafbbef46a 100644
--- a/Documentation/config/mergetool.txt
+++ b/Documentation/config/mergetool.txt
@@ -53,7 +53,7 @@ mergetool.hideResolved::
 	resolution. This flag causes 'LOCAL' and 'REMOTE' to be overwriten so
 	that only the unresolved conflicts are presented to the merge tool. Can
 	be configured per-tool via the `mergetool.<tool>.hideResolved`
-	configuration variable. Defaults to `true`.
+	configuration variable. Defaults to `false`.
 
 mergetool.keepBackup::
 	After performing a merge, the original file with conflict markers
diff --git a/git-mergetool.sh b/git-mergetool.sh
index 911470a5b2..f751d9cfe2 100755
--- a/git-mergetool.sh
+++ b/git-mergetool.sh
@@ -358,13 +358,8 @@ merge_file () {
 		    enabled=false
 		fi
 	else
-		# The user does not have a preference. Ask the tool.
-		if hide_resolved_enabled
-		then
-		    enabled=true
-		else
-		    enabled=false
-		fi
+		# The user does not have a preference. Default to disabled.
+		enabled=false
 	fi
 
 	if test "$enabled" = true
-- 
2.31.0.rc1.246.gcd05c9c855


  reply	other threads:[~2021-03-09  2:30 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
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               ` Jonathan Nieder [this message]
2021-03-09  5:42                 ` [PATCH] mergetool: do not enable hideResolved by default 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=YEbdj27CmjNKSWf4@google.com \
    --to=jrnieder@gmail.com \
    --cc=dahlstrom@google.com \
    --cc=felipe.contreras@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=seth@eseth.com \
    --subject='Re: [PATCH] mergetool: do not enable hideResolved by default' \
    /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).