From: Seth House <firstname.lastname@example.org> To: Jonathan Nieder <email@example.com> Cc: Junio C Hamano <firstname.lastname@example.org>, email@example.com, Felipe Contreras <firstname.lastname@example.org>, Dana Dahlstrom <email@example.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 > 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?
next prev parent 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 \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.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).