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

On Mon, Mar 08, 2021 at 06:29:35PM -0800, Jonathan Nieder wrote:
> 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.

Well said. Agreed on all counts.

The very early days of these patch sets touched on this exact discussion
point. (I'd link to it but that early discussion was a tad...unfocused.)
I make semi-frequent reference of those versions of the conflicted file
in the way you describe and have disabled hideResolved for a merge tool
I maintain for that reason.

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

Compelling point. This flag drastically changes what LOCAL and REMOTE
represent with little to no explanation.

There are three options to achieve the same end-goal of hideResolved
that I've thought of:

1.  Individual merge tools should do this work, not Git.

    A merge tool already has all the information needed to hide
    already-resolved conflicts since that is what MERGED represents.
    Conflict markers *are* a two-way diff and a merge tool should
    display them as such, rather than display the textual markers
    verbatim.

    In many ways this is the ideal approach -- all merge tools could be
    doing this with existing Git right now but none have seemingly
    thought of doing so yet.

2.  Git could pass six versions of the conflicted file to a merge tool,
    rather than the current four.

    Merge tools could accept LOCAL, REMOTE, BASE, MERGED (as most
    currently do), and also LCONFL and RCONFL files. The latter two
    being copies of MERGED but "pre split" by Git into the left
    conflicts and the right conflicts.

    This would spare the merge tool the work of splitting MERGED. It may
    encourage them to continue displaying LOCAL and REMOTE as useful
    context but also make it easy to diff LCONFL with RCONFL and use
    that diff to actually resolve the conflict. It could also make
    things worse, as many tools simply diff _every_ file Git gives them
    regardless if that makes sense or not (>_<).

3.  Git could overwrite LOCAL and REMOTE to display only unresolved
    conflicts.

    (The current hideResolved addition.) This has the pragmatic benefit
    of requiring the least amount of change for all merge tools, but to
    your point above, *destroys* valuable data -- the additional context
    to help understand where the conflicts came from -- and that data
    can't be viewd without running additional Git commands to fetch it.

Defaulting hideResolved to off is a fine change IMO. We don't have a way
to communicate to the end-user that LOCAL and REMOTE represent something
markedly different than what they have traditionally represented, so
having this be an opt-in will force the user to read the docs and
understand the ramifications.

I really appreciate your thoughts that accompanied this patch. Sorry for
the long response but your email made me want to ask the question:

Does the need to default hideResolved to off mean that it is the wrong
approach?

Thinking through an end-user's workflow: would a user want to configure
two copies of the same merge tool -- one with hideResolved and one
without? An easy conflict could benefit from the former but if it's
a tricky conflict the user would have to exit the tool and reopen the
same tool without the flag. That sounds like an annoying workflow, and
although the user would now have that extra, valuable context it would
also put them squarely back into the current state of viewing
already-resolved conflicts.

I know the Option 3, hideResolved, is merged and has that momentum and
this patch looks good to me -- but perhaps Option 2 is more "correct",
or Option 1, or yet another option I haven't thought of. Thoughts?


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