Refactor composeGitlinksCommit in SubmoduleOp

* Separate the cases for create a new commit and amend an existing
commit, since some logics are totally different in these two cases.
* Fix a few glitches when gitlink is already updated.
* Prettify commit message.

Change-Id: If1c66fb5d183abe0e4729cf4183323ab354123d6

gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/SubmoduleSubscriptionsIT.java

@@ -215,21 +215,18 @@ public class SubmoduleSubscriptionsIT extends AbstractSubmoduleSubscription {
 
     // The first update doesn't include the rev log
     RevWalk rw = subRepo.getRevWalk();
-    RevCommit subCommitMsg = rw.parseCommit(subHEAD);
     expectToHaveCommitMessage(superRepo, "master",
         "Update git submodules\n\n" +
-        "Project: " + name("subscribed-to-project")
+            "* Update " + name("subscribed-to-project") + " from branch 'master'");
-            + " master " + subHEAD.name() + "\n\n");
 
     // The next commit should generate only its commit message,
     // omitting previous commit logs
     subHEAD = pushChangeTo(subRepo, "master");
-    subCommitMsg = rw.parseCommit(subHEAD);
+    RevCommit subCommitMsg = rw.parseCommit(subHEAD);
     expectToHaveCommitMessage(superRepo, "master",
         "Update git submodules\n\n" +
-        "Project: " + name("subscribed-to-project")
+            "* Update " + name("subscribed-to-project") + " from branch 'master'"
-            + " master " + subHEAD.name() + "\n\n" +
+             + "\n  - " + subCommitMsg.getFullMessage().replace("\n", "\n    "));
-        subCommitMsg.getFullMessage() + "\n\n");
   }
 
   @Test

gerrit-server/src/main/java/com/google/gerrit/server/git/SubmoduleOp.java

@@ -28,7 +28,6 @@ import com.google.gerrit.server.GerritPersonIdent;
 import com.google.gerrit.server.config.GerritServerConfig;
 import com.google.gerrit.server.git.BatchUpdate.Listener;
 import com.google.gerrit.server.git.BatchUpdate.RepoContext;
-import com.google.gerrit.server.git.CodeReviewCommit.CodeReviewRevWalk;
 import com.google.gerrit.server.git.MergeOpRepoManager.OpenRepo;
 import com.google.gerrit.server.project.NoSuchProjectException;
 import com.google.gerrit.server.project.ProjectCache;
@@ -46,10 +45,8 @@ import org.eclipse.jgit.lib.CommitBuilder;
 import org.eclipse.jgit.lib.Config;
 import org.eclipse.jgit.lib.FileMode;
 import org.eclipse.jgit.lib.ObjectId;
-import org.eclipse.jgit.lib.ObjectInserter;
 import org.eclipse.jgit.lib.PersonIdent;
 import org.eclipse.jgit.lib.Ref;
-import org.eclipse.jgit.lib.Repository;
 import org.eclipse.jgit.revwalk.RevCommit;
 import org.eclipse.jgit.revwalk.RevWalk;
 import org.eclipse.jgit.transport.ReceiveCommand;
@@ -66,6 +63,7 @@ import java.util.Deque;
 import java.util.HashMap;
 import java.util.LinkedHashSet;
 import java.util.Map;
+import java.util.Objects;
 import java.util.Set;
 
 public class SubmoduleOp {
@@ -82,11 +80,13 @@ public class SubmoduleOp {
 
     @Override
     public void updateRepo(RepoContext ctx) throws Exception {
-      CodeReviewCommit c = composeGitlinksCommit(branch, null);
+      CodeReviewCommit c = composeGitlinksCommit(branch);
+      if (c != null) {
         ctx.addRefUpdate(new ReceiveCommand(c.getParent(0), c, branch.get()));
         addBranchTip(branch, c);
       }
     }
+  }
 
   public interface Factory {
     SubmoduleOp create(
@@ -321,67 +321,117 @@ public class SubmoduleOp {
   }
 
   /**
-   * Create a gitlink update commit on the tip of subscriber or modify the
+   * Create a separate gitlink commit
-   * baseCommit with gitlink update patch
    */
-  public CodeReviewCommit composeGitlinksCommit(
+  public CodeReviewCommit composeGitlinksCommit(final Branch.NameKey subscriber)
-      final Branch.NameKey subscriber, RevCommit baseCommit)
       throws IOException, SubmoduleException {
-    PersonIdent author = null;
-    StringBuilder msgbuf = new StringBuilder("Update git submodules\n\n");
-    boolean sameAuthorForAll = true;
-
     try {
       orm.openRepo(subscriber.getParentKey(), false);
     } catch (NoSuchProjectException | IOException e) {
       throw new SubmoduleException("Cannot access superproject", e);
     }
-
     OpenRepo or = orm.getRepo(subscriber.getParentKey());
+
+    CodeReviewCommit currentCommit;
     Ref r = or.repo.exactRef(subscriber.get());
     if (r == null) {
       throw new SubmoduleException(
           "The branch was probably deleted from the subscriber repository");
+    } else {
+      currentCommit = or.rw.parseCommit(r.getObjectId());
     }
 
-    RevCommit currentCommit = (baseCommit != null) ? baseCommit :
+    StringBuilder msgbuf = new StringBuilder("");
-        or.rw.parseCommit(or.repo.exactRef(subscriber.get()).getObjectId());
+    PersonIdent author = null;
-    or.rw.parseBody(currentCommit);
-
     DirCache dc = readTree(or.rw, currentCommit);
     DirCacheEditor ed = dc.editor();
-
     for (SubmoduleSubscription s : targets.get(subscriber)) {
+      RevCommit newCommit = updateSubmodule(dc, ed, msgbuf, s);
+      if (newCommit != null) {
+        if (author == null) {
+          author = newCommit.getAuthorIdent();
+        } else if (!author.equals(newCommit.getAuthorIdent())) {
+          author = myIdent;
+        }
+      }
+    }
+    ed.finish();
+    ObjectId newTreeId = dc.writeTree(or.ins);
+
+    // Gitlinks are already in the branch, return null
+    if (newTreeId.equals(currentCommit.getTree())) {
+      return null;
+    } else {
+      CommitBuilder commit = new CommitBuilder();
+      commit.setTreeId(newTreeId);
+      commit.setParentId(currentCommit);
+      StringBuilder commitMsg = new StringBuilder("Update git submodules\n\n");
+      if (verboseSuperProject) {
+        commitMsg.append(msgbuf);
+      }
+      commit.setMessage(commitMsg.toString());
+      commit.setAuthor(author);
+      commit.setCommitter(myIdent);
+      ObjectId id = or.ins.insert(commit);
+      return or.rw.parseCommit(id);
+    }
+  }
+
+  /**
+   * Amend an existing commit with gitlink updates
+   */
+  public CodeReviewCommit composeGitlinksCommit(
+      final Branch.NameKey subscriber, CodeReviewCommit currentCommit)
+      throws IOException, SubmoduleException {
+    try {
+      orm.openRepo(subscriber.getParentKey(), false);
+    } catch (NoSuchProjectException | IOException e) {
+      throw new SubmoduleException("Cannot access superproject", e);
+    }
+    OpenRepo or = orm.getRepo(subscriber.getParentKey());
+
+    StringBuilder msgbuf = new StringBuilder("");
+    DirCache dc = readTree(or.rw, currentCommit);
+    DirCacheEditor ed = dc.editor();
+    for (SubmoduleSubscription s : targets.get(subscriber)) {
+      updateSubmodule(dc, ed, msgbuf, s);
+    }
+    ed.finish();
+    ObjectId newTreeId = dc.writeTree(or.ins);
+
+    // Gitlinks are already updated, just return the commit
+    if (newTreeId.equals(currentCommit.getTree())) {
+      return currentCommit;
+    } else {
+      or.rw.parseBody(currentCommit);
+      CommitBuilder commit = new CommitBuilder();
+      commit.setTreeId(newTreeId);
+      commit.setParentIds(currentCommit.getParents());
+      if (verboseSuperProject) {
+        commit.setMessage(
+            currentCommit.getFullMessage() + "\n\n*submodules:\n" + msgbuf.toString());
+      } else {
+        commit.setMessage(currentCommit.getFullMessage());
+      }
+      commit.setAuthor(currentCommit.getAuthorIdent());
+      commit.setCommitter(myIdent);
+      ObjectId id = or.ins.insert(commit);
+      return or.rw.parseCommit(id);
+    }
+  }
+
+  private RevCommit updateSubmodule(DirCache dc, DirCacheEditor ed,
+      StringBuilder msgbuf, final SubmoduleSubscription s)
+      throws SubmoduleException, IOException {
     try {
       orm.openRepo(s.getSubmodule().getParentKey(), false);
     } catch (NoSuchProjectException | IOException e) {
       throw new SubmoduleException("Cannot access submodule", e);
     }
     OpenRepo subOr = orm.getRepo(s.getSubmodule().getParentKey());
-      Repository subRepo = subOr.repo;
-
-      Ref ref = subRepo.getRefDatabase().exactRef(s.getSubmodule().get());
-      if (ref == null) {
-        ed.add(new DeletePath(s.getPath()));
-        continue;
-      }
-
-      ObjectId updateTo = ref.getObjectId();
-      if (branchTips.containsKey(s.getSubmodule())) {
-        updateTo = branchTips.get(s.getSubmodule());
-      }
-      RevWalk subOrRw = subOr.rw;
-      final RevCommit newCommit = subOrRw.parseCommit(updateTo);
-
-      subOrRw.parseBody(newCommit);
-      if (author == null) {
-        author = newCommit.getAuthorIdent();
-      } else if (!author.equals(newCommit.getAuthorIdent())) {
-        sameAuthorForAll = false;
-      }
 
     DirCacheEntry dce = dc.getEntry(s.getPath());
-      ObjectId oldId;
+    RevCommit oldCommit = null;
     if (dce != null) {
       if (!dce.getFileMode().equals(FileMode.GITLINK)) {
         String errMsg = "Requested to update gitlink " + s.getPath() + " in "
@@ -389,13 +439,25 @@ public class SubmoduleOp {
             + "doesn't have gitlink file mode.";
         throw new SubmoduleException(errMsg);
       }
-        oldId = dce.getObjectId();
+      oldCommit = subOr.rw.parseCommit(dce.getObjectId());
-      } else {
-        // This submodule did not exist before. We do not want to add
-        // the full submodule history to the commit message, so omit it.
-        oldId = updateTo;
     }
 
+    final RevCommit newCommit;
+    if (branchTips.containsKey(s.getSubmodule())) {
+      newCommit = branchTips.get(s.getSubmodule());
+    } else {
+      Ref ref = subOr.repo.getRefDatabase().exactRef(s.getSubmodule().get());
+      if (ref == null) {
+        ed.add(new DeletePath(s.getPath()));
+        return null;
+      }
+      newCommit = subOr.rw.parseCommit(ref.getObjectId());
+    }
+
+    if (Objects.equals(newCommit, oldCommit)) {
+      // gitlink have already been updated for this submodule
+      return null;
+    } else {
       ed.add(new PathEdit(s.getPath()) {
         @Override
         public void apply(DirCacheEntry ent) {
@@ -403,55 +465,39 @@ public class SubmoduleOp {
           ent.setObjectId(newCommit.getId());
         }
       });
+
       if (verboseSuperProject) {
-        msgbuf.append("Project: " + s.getSubmodule().getParentKey().get());
+        createSubmoduleCommitMsg(msgbuf, s, subOr, newCommit, oldCommit);
-        msgbuf.append(" " + s.getSubmodule().getShortName());
+      }
-        msgbuf.append(" " + newCommit.getName());
+      subOr.rw.parseBody(newCommit);
-        msgbuf.append("\n\n");
+      return newCommit;
+    }
+  }
+
+  private void createSubmoduleCommitMsg(StringBuilder msgbuf,
+      SubmoduleSubscription s, OpenRepo subOr, RevCommit newCommit, RevCommit oldCommit)
+      throws SubmoduleException {
+    msgbuf.append("* Update " + s.getPath());
+    msgbuf.append(" from branch '" + s.getSubmodule().getShortName() + "'");
+
+    // newly created submodule gitlink, do not append whole history
+    if (oldCommit == null) {
+      return;
+    }
 
     try {
-          subOrRw.resetRetain(subOr.canMergeFlag);
+      subOr.rw.resetRetain(subOr.canMergeFlag);
-          subOrRw.markStart(newCommit);
+      subOr.rw.markStart(newCommit);
-          subOrRw.markUninteresting(subOrRw.parseCommit(oldId));
+      subOr.rw.markUninteresting(oldCommit);
-          for (RevCommit c : subOrRw) {
+      for (RevCommit c : subOr.rw) {
-            subOrRw.parseBody(c);
+        subOr.rw.parseBody(c);
-            msgbuf.append(c.getFullMessage() + "\n\n");
+        msgbuf.append("\n  - " + c.getFullMessage().replace("\n", "\n    "));
       }
     } catch (IOException e) {
       throw new SubmoduleException("Could not perform a revwalk to "
           + "create superproject commit message", e);
     }
   }
-    }
-    ed.finish();
-
-
-    ObjectInserter oi = or.ins;
-    CodeReviewRevWalk rw = or.rw;
-    ObjectId tree = dc.writeTree(oi);
-
-    if (!sameAuthorForAll || author == null) {
-      author = myIdent;
-    }
-
-    CommitBuilder commit = new CommitBuilder();
-    commit.setTreeId(tree);
-    if (baseCommit != null) {
-      // modify the baseCommit
-      commit.setParentIds(baseCommit.getParents());
-      commit.setMessage(baseCommit.getFullMessage() + "\n\n" + msgbuf.toString());
-      commit.setAuthor(baseCommit.getAuthorIdent());
-    } else {
-      // create a new commit
-      commit.setParentId(currentCommit);
-      commit.setMessage(msgbuf.toString());
-      commit.setAuthor(author);
-    }
-    commit.setCommitter(myIdent);
-
-    ObjectId id = oi.insert(commit);
-    return rw.parseCommit(id);
-  }
 
   private static DirCache readTree(RevWalk rw, ObjectId base)
       throws IOException {