Improve merge algorithm

Handle the case where we have -

base - 1.md 2.md
a - 1.md 2.md 3.md
b - 1.md

In this case the file was deleted in 2.md and should not appear in the
final merged entries. This represents a common case where the file was
deleted in the remote branch but not locally.

Fixes GitJournal/GitJournal#962

bin/commands/merge.dart

@@ -53,13 +53,16 @@ class MergeCommand extends Command<int> {
 
     var authorDate = Platform.environment['GIT_AUTHOR_DATE'];
     if (authorDate != null) {
-      user.date = GDateTime.parse(authorDate);
+      var date = GDateTime.parse(authorDate);
+      user = GitAuthor(name: user.name, email: user.email, date: date);
     }
 
     var committer = user;
     var comitterDate = Platform.environment['GIT_COMMITTER_DATE'];
     if (comitterDate != null) {
-      committer.date = GDateTime.parse(comitterDate);
+      var date = GDateTime.parse(comitterDate);
+      committer =
+          GitAuthor(name: committer.name, email: committer.email, date: date);
     }
 
     var msg = argResults!['message'] ?? "Merge branch '$branchName'\n";

lib/merge.dart

@@ -57,6 +57,7 @@ extension Merge on GitRepository {
       }
     }
 
+    var baseTree = objStorage.readTree(bases.first.treeHash);
     var headTree = objStorage.readTree(headCommit.treeHash);
     var bTree = objStorage.readTree(commitB.treeHash);
 
@@ -70,68 +71,64 @@ extension Merge on GitRepository {
       committer: committer,
       parents: parents,
       message: message,
-      treeHash: _combineTrees(headTree, bTree),
+      treeHash: _combineTrees(headTree, bTree, baseTree),
     );
     objStorage.writeObject(commit);
     return resetHard(commit.hash);
-
-    // - unborn ?
-
-    // Full 3 way
-    // https://stackoverflow.com/questions/4129049/why-is-a-3-way-merge-advantageous-over-a-2-way-merge
   }
 
   /// throws exceptions
-  GitHash _combineTrees(GitTree a, GitTree b) {
+  GitHash _combineTrees(GitTree a, GitTree b, GitTree base) {
     // Get all the paths
     var names = a.entries.map((e) => e.name).toSet();
     names.addAll(b.entries.map((e) => e.name));
 
     var entries = <GitTreeEntry>[];
-    for (var name in names) {
+    for (var baseEntry in base.entries) {
+      var name = baseEntry.name;
       var aIndex = a.entries.indexWhere((e) => e.name == name);
       var bIndex = b.entries.indexWhere((e) => e.name == name);
 
       var aContains = aIndex != -1;
       var bContains = bIndex != -1;
 
-      if (aContains && !bContains) {
-        var aEntry = a.entries[aIndex];
-        entries.add(aEntry);
+      if (!aContains && !bContains) {
+        // both don't contain it!
+        continue;
+      } else if (aContains && !bContains) {
+        // Entry deleted in 'b', but exists in 'a'
+        // Delete this entry in the merged result
+        continue;
       } else if (!aContains && bContains) {
+        // Entry deleted in 'a', but exists in 'b'
         var bEntry = b.entries[bIndex];
         entries.add(bEntry);
       } else {
         // both contain it!
         var aEntry = a.entries[aIndex];
         var bEntry = b.entries[bIndex];
 
-        if (aEntry.mode == GitFileMode.Dir && bEntry.mode == GitFileMode.Dir) {
-          var aEntryTree = objStorage.readTree(aEntry.hash);
-          var bEntryTree = objStorage.readTree(bEntry.hash);
-
-          var newTreeHash = _combineTrees(
-            aEntryTree,
-            bEntryTree,
-          );
-
-          var entry = GitTreeEntry(
-            mode: GitFileMode.Dir,
-            name: aEntry.name,
-            hash: newTreeHash,
-          );
-          entries.add(entry);
-          continue;
-        } else if (aEntry.mode != GitFileMode.Dir &&
-            bEntry.mode != GitFileMode.Dir) {
-          // FIXME: Which one to pick?
-          var aEntry = a.entries[aIndex];
-          entries.add(aEntry);
-          continue;
-        }
-
-        throw GitNotImplemented();
+        var newEntry = _resolvConflicts(aEntry, bEntry, baseEntry);
+        entries.add(newEntry);
+      }
+    }
+
+    for (var entry in [...a.entries, ...b.entries]) {
+      var name = entry.name;
+
+      // If the entry was already in the base
+      var baseIndex = base.entries.indexWhere((e) => e.name == name);
+      if (baseIndex != -1) {
+        continue;
       }
+
+      // If the entry was already in the merged entries
+      var mergedIndex = entries.indexWhere((e) => e.name == name);
+      if (mergedIndex != -1) {
+        continue;
+      }
+
+      entries.add(entry);
     }
 
     var newTree = GitTree.create(entries);
@@ -140,6 +137,40 @@ extension Merge on GitRepository {
     return newTree.hash;
   }
 
+  GitTreeEntry _resolvConflicts(
+      GitTreeEntry a, GitTreeEntry b, GitTreeEntry base) {
+    if (a.hash == b.hash) {
+      return a;
+    }
+
+    // Both are not Directories
+    if (a.mode != GitFileMode.Dir && b.mode != GitFileMode.Dir) {
+      return _resolveBlobConflict(a, b, base);
+    }
+
+    if (a.mode == GitFileMode.Dir && b.mode == GitFileMode.Dir) {
+      var aTree = objStorage.readTree(a.hash);
+      var bTree = objStorage.readTree(b.hash);
+      var baseTree = base.mode == GitFileMode.Dir
+          ? objStorage.readTree(base.hash)
+          : GitTree.create();
+
+      var newTreeHash = _combineTrees(aTree, bTree, baseTree);
+      return GitTreeEntry(
+        mode: GitFileMode.Dir,
+        name: a.name,
+        hash: newTreeHash,
+      );
+    }
+
+    throw GitNotImplemented();
+  }
+
+  GitTreeEntry _resolveBlobConflict(
+      GitTreeEntry a, GitTreeEntry b, GitTreeEntry base) {
+    return a;
+  }
+
   void mergeTrackingBranch({required GitAuthor author}) {
     var branch = currentBranch();
     var branchConfig = config.branch(branch);

test/commands/merge_delete_test.dart

@@ -0,0 +1,30 @@
+import 'package:test/test.dart';
+
+import 'common.dart';
+
+void main() {
+  late GitCommandSetupResult s;
+
+  setUpAll(() async {
+    s = await gitCommandTestFixtureSetupAll('merge-delete');
+  });
+
+  setUp(() async => gitCommandTestSetup(s));
+
+  var commands = [
+    'merge del2',
+  ];
+
+  for (var command in commands) {
+    test(
+        command,
+        () async => testGitCommand(s, command, ignoreOutput: true, env: {
+              'GIT_AUTHOR_DATE': '2020-02-15T09:08:07.000Z',
+              'GIT_AUTHOR_NAME': 'Vishesh Handa',
+              'GIT_AUTHOR_EMAIL': '[email protected]',
+              'GIT_COMMITTER_DATE': '2020-02-15T09:08:07.000Z',
+              'GIT_COMMITTER_NAME': 'Vishesh Handa',
+              'GIT_COMMITTER_EMAIL': '[email protected]',
+            }));
+  }
+}

test/commands/merge_test.dart

@@ -1,9 +1,8 @@
-// import 'package:test/test.dart';
+import 'package:test/test.dart';
 
-// import 'common.dart';
+import 'common.dart';
 
 void main() {
-  /*
   late GitCommandSetupResult s;
 
   setUpAll(() async {
@@ -31,7 +30,6 @@ void main() {
               'GIT_COMMITTER_EMAIL': '[email protected]',
             }));
   }
-  */
 }
 
 // FIXME: We aren't taking directories into account!

test/data/merge-delete/.gitted/HEAD

@@ -0,0 +1 @@
+ref: refs/heads/main

test/data/merge-delete/.gitted/config

@@ -0,0 +1,7 @@
+[core]
+	repositoryformatversion = 0
+	filemode = true
+	bare = false
+	logallrefupdates = true
+	ignorecase = true
+	precomposeunicode = true

test/data/merge-delete/.gitted/description

@@ -0,0 +1 @@
+Unnamed repository; edit this file 'description' to name the repository.

test/data/merge-delete/.gitted/packed-refs

@@ -0,0 +1,3 @@
+# pack-refs with: peeled fully-peeled sorted 
+138451d299ab7dfdb8ad27bc37098d953cb09a3c refs/heads/del2
+f6fde458426a0eb55fe19b74eb4d12f877b8f40a refs/heads/main

test/data/merge-delete/1.md

@@ -0,0 +1 @@
+1

test/data/merge-delete/2.md

@@ -0,0 +1 @@
+2

test/data/merge-delete/3.md

@@ -0,0 +1 @@
+3

test/lib.dart

@@ -98,7 +98,7 @@ done''';
   var repo2Objects =
       repo2Result.stdout.split('\n').where((String e) => e.isNotEmpty).toSet();
 
-  expect(repo1Objects, repo2Objects);
+  expect(repo1Objects, repo2Objects, reason: 'Objects are different');
 
   // Test if all the references are the same
   var listRefScript = 'git show-ref --head';
@@ -115,7 +115,7 @@ done''';
   var repo2Refs =
       repo2Result.stdout.split('\n').where((String e) => e.isNotEmpty).toSet();
 
-  expect(repo1Refs, repo2Refs);
+  expect(repo1Refs, repo2Refs, reason: 'Refs are different');
 
   // Test if the index is the same
   var listIndexScript = 'git ls-files --stage';
@@ -136,7 +136,7 @@ done''';
       .where((String e) => e.isNotEmpty)
       .toSet() as Set<String>?;
 
-  expect(repo1Index, repo2Index);
+  expect(repo1Index, repo2Index, reason: 'Index is different');
 
   // Test if the config is the same
   var config1Data = await File(p.join(repo1, '.git', 'config')).readAsString();
@@ -249,7 +249,7 @@ Future<List<String>> runDartGitCommand(
     reason: "Command ran with an exception. This shouldn't happen",
   );
   if (!shouldReturnError) {
-    expect(ret, 0);
+    expect(ret, 0, reason: 'Dart command `$command` failed in $workingDir');
   } else {
     expect(ret, isNot(0));
   }