diff mbox series

[1/2] mergetool: do not enable hideResolved by default

Message ID YEx6GNybrU5mrlNi@google.com (mailing list archive)
State Accepted
Commit b2a51c1b03c44bd888e2f65c764dced571266482
Headers show
Series mergetool: do not enable hideResolved by default | expand

Commit Message

Jonathan Nieder March 13, 2021, 8:38 a.m. UTC
When 98ea309b3f (mergetool: add hideResolved configuration,
2021-02-09) introduced the mergetool.hideResolved setting to reduce
the clutter in viewing non-conflicted sections of files in a
mergetool, it enabled it by default, explaining:

    No adverse effects were noted in a small survey of popular mergetools[1]
    so this behavior defaults to `true`.

In practice, alas, adverse effects do appear.  A few issues:

1. No indication is shown in the UI that the base, local, and remote
   versions shown have been modified by additional resolution.  This
   is inherent in the design: the idea of mergetool.hideResolved is to
   convince a mergetool that expects pristine local, base, and remote
   files to show partially resolved verisons of those files instead;
   there is no additional source of information accessible to the
   mergetool to see where the resolution has happened.

   (By contrast, a mergetool generating the partial resolution from
   conflict markers for itself would be able to hilight the resolved
   sections with a different color.)

   A user accustomed to seeing the files without partial resolution
   gets no indication that this behavior has changed when they upgrade
   Git.

2. If the computed merge did not line up the files correctly (for
   example due to repeated sections in the file), the partially
   resolved files can be misleading and do not have enough information
   to reconstruct what happened and compute the correct merge result.

3. Resolving a conflict can involve information beyond the textual
   conflict.  For example, if the local and remote versions added
   overlapping functionality in different ways, seeing the full
   unresolved versions of each alongside the base gives information
   about each side's intent that makes it possible to come up with a
   resolution that combines those two intents.  By contrast, when
   starting with partially resolved versions of those files, one can
   produce a subtly wrong resolution that includes redundant extra
   code added by one side that is not needed in the approach taken
   on the other.

All that said, a user wanting to focus on textual conflicts with
reduced clutter can still benefit from mergetool.hideResolved=true as
a way to deemphasize sections of the code that resolve cleanly without
requiring any changes to the invoked mergetool.  The caveats described
above are reduced when the user has explicitly turned this on, because
then the user is aware of them.

Flip the default to 'false'.

Reported-by: Dana Dahlstrom <dahlstrom@google.com>
Helped-by: Seth House <seth@eseth.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Only difference from v1 is the commit message.

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

Patch

diff --git a/Documentation/config/mergetool.txt b/Documentation/config/mergetool.txt
index 90f76f5b9ba..cafbbef46ae 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 911470a5b2c..f751d9cfe20 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