| Message ID | YEbdj27CmjNKSWf4@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | mergetool: do not enable hideResolved by default | expand |
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?
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
Jonathan Nieder <jrnieder@gmail.com> writes: > diff --git a/git-mergetool.sh b/git-mergetool.sh > index 911470a5b2..f751d9cfe2 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 OK. So the logic used to be - If the user has preference for a specific backend, use it; - If the user says the feature is unwanted, that is final; - If the user says it generally is OK to use the feature, let each backend set the preference; - If there is no preference, let each backend set the preference. As we want to disable the feature for any backend when the user does not explicitly say the feature is wanted (either in general, or for a specific backend), the change in the above hunk is exactly want we want to see. Looking good. Let's not revert the series and disable by default. Should I expect an updated log message, though? What was in the proposed log message sounded more unsubstantiated complaint than giving readable reasons why the feature is unwanted, but both the response by Seth and your response to Seth's response had material that made it more convincing why we would want to disable this by default, e.g. "with little to no explanation", "We don't have a way to communicate to the end-user" (both by Seth), "when ... didn't end up lining up the files correctly", "no way to visually distinguish" (yours) are all good ingredients to explain why this feature is prone to subtly and silently give wrong information to the end-users. Thanks.
Junio C Hamano <gitster@pobox.com> writes: > As we want to disable the feature for any backend when the user does > not explicitly say the feature is wanted (either in general, or for > a specific backend), the change in the above hunk is exactly want we > want to see. > > Looking good. Let's not revert the series and disable by default. > > Should I expect an updated log message, though? What was in the > proposed log message sounded more unsubstantiated complaint than > giving readable reasons why the feature is unwanted, but both the > response by Seth and your response to Seth's response had material > that made it more convincing why we would want to disable this by > default, e.g. "with little to no explanation", "We don't have a way > to communicate to the end-user" (both by Seth), "when ... didn't end > up lining up the files correctly", "no way to visually distinguish" > (yours) are all good ingredients to explain why this feature is > prone to subtly and silently give wrong information to the > end-users. For tonight's pushout, I'll use the patch as-is and merge it in 'seen'. Thanks.
Junio C Hamano <gitster@pobox.com> writes: > Junio C Hamano <gitster@pobox.com> writes: > >> As we want to disable the feature for any backend when the user does >> not explicitly say the feature is wanted (either in general, or for >> a specific backend), the change in the above hunk is exactly want we >> want to see. >> >> Looking good. Let's not revert the series and disable by default. >> >> Should I expect an updated log message, though? What was in the >> proposed log message sounded more unsubstantiated complaint than >> giving readable reasons why the feature is unwanted, but both the >> response by Seth and your response to Seth's response had material >> that made it more convincing why we would want to disable this by >> default, e.g. "with little to no explanation", "We don't have a way >> to communicate to the end-user" (both by Seth), "when ... didn't end >> up lining up the files correctly", "no way to visually distinguish" >> (yours) are all good ingredients to explain why this feature is >> prone to subtly and silently give wrong information to the >> end-users. > > For tonight's pushout, I'll use the patch as-is and merge it in > 'seen'. Any progress here?
Junio C Hamano wrote: > Junio C Hamano <gitster@pobox.com> writes: > > Junio C Hamano <gitster@pobox.com> writes: >>> As we want to disable the feature for any backend when the user does >>> not explicitly say the feature is wanted (either in general, or for >>> a specific backend), the change in the above hunk is exactly want we >>> want to see. >>> >>> Looking good. Let's not revert the series and disable by default. >>> >>> Should I expect an updated log message, though? [...] >> For tonight's pushout, I'll use the patch as-is and merge it in >> 'seen'. > > Any progress here? Sorry for the delay. I should be able to send out an improved log message (more concise and summarizing the supporting info from this thread) later this afternoon. Thanks, Jonathan
Jonathan Nieder <jrnieder@gmail.com> writes: >> Any progress here? > > Sorry for the delay. I should be able to send out an improved log > message (more concise and summarizing the supporting info from this > thread) later this afternoon. Thanks. I think this is the last known regression in the -rc, and an update before the final happens on coming Monday is very much appreciated.
diff --git a/Documentation/config/mergetool.txt b/Documentation/config/mergetool.txt index 90f76f5b9b..cafbbef46a 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 911470a5b2..f751d9cfe2 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
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. Sometimes, though, the exact content of these three competing versions of a file is not so important. Especially if the mergetool supports folding unchanged lines, the new 'mergetool.hideResolved' feature can be helpful for allowing a person resolving a merge to focus on the portion with conflicts. For sections of the file where BASE matched LOCAL or REMOTE, this feature makes all three versions match the resolved version, so that the user resolving can focus exclusively on the portions with conflicts. In other words, hideResolved makes a multi-pane merge tool show a similar amount of information to the file with conflict markers with conflictstyle=diff3, saving the operator from having to pay attention to parts that resolved cleanly. 98ea309b3f (mergetool: add hideResolved configuration, 2021-02-09) which introduced this setting enabled it by default, explaining: 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. Especially in cases where conflicts involve elements beyond textual conflict, it has resulted in incorrect resolutions and wasted work to figure out what happened. Flip the default back to the traditional behavior of `false`: although the old behavior involves slightly slower merges in the only-textual-conflicts case, it prevents this kind of painful moment of betrayal by one's tools, which is more important. Should we want to migrate to hideResolved=true in the future, we still can. It just requires a more careful migration, including a period where "git mergetool" shows a warning or errors out in affected cases. Reported-by: Dana Dahlstrom <dahlstrom@google.com> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com> --- Hi, Seth House wrote: > 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`. Thanks much for protecting this by a flag. We tried this out internally at Google when it hit "next" and not too long later realized that the new default of "true" is not workable for us. I don't believe it's the right default for Git, either, hence this patch. Thanks for working on the merge resolution workflow; it's much appreciated. Sincerely, Jonathan Documentation/config/mergetool.txt | 2 +- git-mergetool.sh | 9 ++------- 2 files changed, 3 insertions(+), 8 deletions(-)