sublimehq / sublime_merge

Issue tracker for Sublime Merge
https://www.sublimemerge.com
276 stars 14 forks source link

Sublime Merge 3-way diff view fails the "Eat A Closer Avoidance" test, makes "take both sides" resolution difficult #1079

Open ferdnyc opened 3 years ago

ferdnyc commented 3 years ago

Background

The Guiffy SureMerge whitepaper by Bill Ritcher (developer of Guiffy) presents a set of interesting (as it terms them) "Pathological cases" faced by diff/merge tools/algorithms, complete with a set of example files for each test case provided in a zip archive.

I find the test files extremely helpful in exploring the mechanics of each case, as they allow me to view the problem from all angles and attempt to really grok its complexities. The test case files are also a useful tool for evaluating existing diff/merge tools' handling of the pathological cases. IMHO running through each of Ritcher's 8 test cases should be part of any such software's QA plan.

Problem description

Sublime Merge currently fails test case 7 from the white paper, a criterion Ritcher calls "Eat a Closer Avoidance".

A demonstration of test case 7: Eat a Closer Avoidance

Basically, consider the following two commits, applied to the same (byte-identical) base file on two different branches of a repo.

diff --git a/ReleaseNotes 9_4.html b/ReleaseNotes 9_4.html
index 02fa088..c092fab 100644
--- a/ReleaseNotes 9_4.html 
+++ b/ReleaseNotes 9_4.html 
@@ -46,6 +46,8 @@
              </LI>
              <LI>Upgraded Quaqua FileChooser for latest Snow Leopard LAF on MacOS X.
              </LI>
+             <LI>Bugs 412-417 fixed (Refer to BugReports on Guiffy's web site for details).
+             </LI>
             </UL>

 <h2>9.3 Release Notes: Build 293 - September 1, 2010</h2>
diff --git a/ReleaseNotes 9_4.html b/ReleaseNotes 9_4.html
index 02fa088..b38d01a 100644
--- a/ReleaseNotes 9_4.html 
+++ b/ReleaseNotes 9_4.html 
@@ -46,6 +46,10 @@
              </LI>
              <LI>Upgraded Quaqua FileChooser for latest Snow Leopard LAF on MacOS X.
              </LI>
+             <LI>Upgraded to JavaHelp 2 - improved search and performance.
+             </LI>
+             <LI>Upgraded to InstallAnywhere 2010 - improved installers and application launchers.
+             </LI>
             </UL>

 <h2>9.3 Release Notes: Build 293 - September 1, 2010</h2>

When the branches containing these two changes are merged, they will conflict and need to be resolved. Currently, Sublime Merge will bring up the conflict resolution view like so:

image

As you can see, the last line has been trimmed off both sides. Since both commits made the same change there (inserted a </LI>), that line is considered pre-resolved. Fairly standard. In fact, it's exactly how git merge itself treats the conflict, at least by default (more on that shortly).

However, Ritcher makes a very compelling argument for why this is the wrong approach to resolving a conflict like this. Pre-resolving only part of these conflicting edits means that if the user were to include both sides' changes in the merge, the result would be a combined file that's missing a closing </LI> tag. Properly managing the conflict requires that blocks of edited lines be kept intact, so that they remain syntactically valid.

Isn't this Git's fault?

There's a fair argument to be made here that this is really Git's problem. As I noted, the default conflict strategy applied by git merge will insert conflict markers in exactly the same configuration used by Sublime Merge:

git checkout edit2
git -c merge.conflictStyle=merge merge edit1
cat ReleaseNotes\ 9_4.html|sed -e '1,45d;60,$d'
             </LI>
             <LI>Upgraded Quaqua FileChooser for latest Snow Leopard LAF on MacOS X.
             </LI>
<<<<<<< HEAD
             <LI>Bugs 412-417 fixed (Refer to BugReports on Guiffy's web site for details).
=======
             <LI>Upgraded to JavaHelp 2 - improved search and performance.
             </LI>
             <LI>Upgraded to InstallAnywhere 2010 - improved installers and application launchers.
>>>>>>> edit1
             </LI>
            </UL>

<h2>9.3 Release Notes: Build 293 - September 1, 2010</h2>

So, what more can Sublime Merge do than what it already does, which is accurately reproduce the results of Git's own merge tools?

Preferred solution

Ah, but not so fast! Git supports other conflictStrategy modes than just merge. There's also diff3, which adds in the contents of the base file. As previously raised in at least #818, diff3 comparisons are more robust, and therefore a better fit for Sublime Merge. When Git marks up the same conflict in diff3 style, the closing </LI> on both sides of the diff is not eaten:

git merge --abort
git -c merge.conflictStyle=diff3 merge edit1
cat ReleaseNotes\ 9_4.html|sed -e '1,45d;62,$d'
             </LI>
             <LI>Upgraded Quaqua FileChooser for latest Snow Leopard LAF on MacOS X.
             </LI>
<<<<<<< HEAD
             <LI>Bugs 412-417 fixed (Refer to BugReports on Guiffy's web site for details).
             </LI>
||||||| 742717a
=======
             <LI>Upgraded to JavaHelp 2 - improved search and performance.
             </LI>
             <LI>Upgraded to InstallAnywhere 2010 - improved installers and application launchers.
             </LI>
>>>>>>> edit1
            </UL>

<h2>9.3 Release Notes: Build 293 - September 1, 2010</h2>

Unfortunately, even if a repo is configured for merge.conflictStrategy=diff3, the conflict resolution view of Sublime Merge is unaffected. It continues to eat the closer of those two changes.

For better management and interleaving of edits during conflict resolution, it would be extremely valuable if Sublime Merge were to take a diff3-style (or perhaps even directly diff3-driven) approach to edit conflicts that avoids eating the closer.

Alternatives

None really, I guess just continue to eat the closer in the merge tool view? It's not the end of the world if nothing changes, which is why I marked this a feature request rather than a bug. It's just something that would be Nice To Have™.

ferdnyc commented 3 years ago

Closer-consumption avoidance in the Github Atom editor

I guess just as a data point, though not a hugely relevant one, Github's Atom text editor provides a conflict-resolution tool integrated into its editor panes. When a file with conflict markup is loaded, it will overlay some styling and context options directly onto the text markup. It's rudimentary, but serviceable for minor fixup:

image

One area where Atom's tool excels, though, is in its support for diff3 style conflict markup. If a file with diff3-style markup is loaded, Atom will recognize it and enhance the overlay interface with additional detail and functionality:

image

Atom even offers pre-defined resolution options in the overlay's context menu, so that with just one click the user can choose to combine the changes in either order: "Theirs Then Ours", or "Ours Then Theirs".

Screenshot from 2021-03-02 11-06-05

When used with standard merge-style conflict markup, just like in Sublime Merge this will result in a combined file that's missing a </LI> tag.

But UN-like Sublime Merge, when used with diff3 conflict markup that avoids eating the closer, Atom's "Theirs Then Ours" and "Ours Then Theirs" operations do the right thing. Selecting either of them will combine both sets of changes, including all necessary closer lines. The result is a merged file that contains fully valid HTML source.

dpjohnst commented 3 years ago

Hi @ferdnyc,

Thank you for sharing this feedback with the Sublime Merge team - we currently don't support selecting alternative merge algorithms (as you have noted already). That being said, we are definitely open to adding more merge / diff algorithms to suit different use-cases / languages.

Related issues: https://github.com/sublimehq/sublime_merge/issues/613 https://github.com/sublimehq/sublime_merge/issues/312

I'll keep you updated on any progress that gets made in this domain.

Thanks, - Dylan

ferdnyc commented 3 years ago

@dpjohnst

Thanks for responding! Honestly, I'm not really asking that the conflict-resolution algorithm be selectable: It should always be diff3-style, and Sublime Merge should never pre-resolve trailing lines in conflict blocks.

There's no down side to Sublime Merge showing the </LI> addition on both the left and right sides of the merge tool, instead of pre-resolving it into the combined text. It's exactly the same number of clicks to incorporate either change into the merged file.

But there are significant advantages to making sure the closing line isn't eaten as a pre-resolved edit. As demonstrated, the merged output can contain only the left change (same as before), only the right change (same as before), or both changes. The latter can't be done using the current conflict view, without breaking the file's syntax or having to hand-edit the results to fix it up.

Pre-resolving lines in diffs is slightly defensible when the conflicts are being displayed inline, the way they are in Git's original markup. It makes the total file length smaller, and the merged file is expected to need hand-editing regardless.

But with side-by-side conflict resolution, it's just always better to leave the conflict blocks on both sides as intact as possible.

ferdnyc commented 3 years ago

(Note that the conflict style selection is different from the merge algorithm. The merge algorithm controls how Git attempts to resolve conflicts for itself, and changing it can significantly alter the resulting merged file's contents. The conflict style only affects how conflicts are formatted in the source, once Git determines that there are conflicting edits. The change from merge to diff3 style tweaks what lines are included in which branch of the conflict (Theirs vs. Base vs. Ours), but the actual content of the changes is unaffected.)

dpjohnst commented 3 years ago

Hi @ferdnyc,

Thank you for your response - to clarify, Git includes an extra step when detecting merge conflicts (by default). If it detects two conflicting hunks, it will diff both hunks against each other to determine what has actually changed.

In the above case, that would leave the last line of each changed hunk (<\li>) as marked as common. Clearly this isn't ideal in the above scenario.

That being said, there are definitely some scenarios where shrinking the conflict is ideal.

For example:

base:

start
end

left:

start
1
2
3
X
4
5
6
end

right:

start
1
2
3
Y
4
5
6
end

conflict:

====
1:2,8c
  1
  2
  3
  X
  4
  5
  6
2:1a
3:2,8c
  1
  2
  3
  Y
  4
  5
  6

In the above, you can see that the only thing that is conflicting is actually X and Y. If we don't simplify the common sections, you'll end up having the entire hunk from 1 to 6 marked as a conflict. This is less than ideal in many situations.

That being said, different conflict handling will produce better / worse results in different contexts. I don't believe there's a one-size-fits-all solution, and so one of the solutions would be to allow people to configure the merge / conflict algorithm to suit their needs.

Thanks, - Dylan