From: Jonathan Nieder <firstname.lastname@example.org> To: Seth House <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: Tue, 9 Mar 2021 17:23:17 -0800 [thread overview] Message-ID: <YEgfhYSz7VaCtvH1@google.com> (raw) In-Reply-To: <YEcKy83ZmvGTAfxq@ellen.lan> Hi, Seth House wrote: > 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. [...] > 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. > 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> [...] > 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. [...] > 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
next prev parent reply other threads:[~2021-03-10 1:24 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 2021-03-10 1:23 ` Jonathan Nieder [this message] 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=YEgfhYSz7VaCtvH1@google.com \ --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).