Re: [PATCH] mergetool: do not enable hideResolved by default

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Mar 10, 2021, 01:23 UTC
Message-ID: <YEgfhYSz7VaCtvH1@google.com>
In-Reply-To: <YEcKy83ZmvGTAfxq@ellen.lan>

Hi,

Seth House wrote:

Show 5 quoted lines

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

Thanks. Do you have a public example of a merge that was produced in such a way? It might help focus the discussion.

For concreteness' sake: in the repository that Dana mentioned, one can see some merges from before hideResolved at https://android.googlesource.com/platform/tools/idea/+log/mirror-goog-studio-master-dev/build.txt.

The xml files there (I'm not sure these are the right ones for me to focus on, just commenting as I observe) remind me of other routine conflicts with xml I've had to resolve in the past, e.g. at https://git.eclipse.org/r/c/jgit/jgit/+/134451/3. Having information from each side of the merge and not a mixture can be very helpful in this kind of case. That's especially true when the three-way merge algorithm didn't end up lining up the files correctly, which has happened from time to time to me in files with repetitive structure.

[...]

Show 14 quoted lines

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

One obstacle to this is that a merge tool can't count on the file in the worktree containing pristine conflict markers, because the user may have already started to work on the merge resolution.

Show 14 quoted lines

> 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 (>_<).

Interesting! I kind of like this, especially if it were something the tool could opt in to. That said, I'm not the best person to ask, since I never ended up finding a good workflow using mergetool for my own use; instead, I tend to do the work of a merge tool "by hand":

- gradually resolving the merge in each diff3-style conflict hunk by
  removing common lines from base+local and base+remote until there is
  nothing left in base

- in harder cases, making the worktree match the local version,
  putting the diff from base to remote in a temporary file, and then
  hunk by hunk applying it

- in even harder cases, using git-imerge
  <https://github.com/mhagger/git-imerge>

[...]

Show 5 quoted lines

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

That's a good argument for having the option available, *as long as the user explicitly turns it on*.

[...]

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

One disadvantage relative to (1) is that the mergetool has no way to visually distinguish the automatically resolved portion. For that reason, I suspect this will never be something we can make the default. But in principle I'm not against it existing.

The implementation is concise and maintainable. The documentation adds a little user-facing complexity; I think as long as we're able to keep it clear and well maintained, that should be okay.

git-mergetool.txt probably ought to mention the hideResolved setting. Otherwise, users can have a confusing experience if they set the config once and forget about it later.

[...]

Show 12 quoted lines

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

I suspect option 1 is indeed more correct. Dana mentions that some mergetools (p4merge?) use different colors to highlight the 'automatically resolved' portions, something that isn't possible using option 3.

Thanks, Jonathan