From 7e0cd90cf7945006426b689774fd6ccdaf2c9af0 Mon Sep 17 00:00:00 2001 From: Matthias Sohn Date: Tue, 3 Sep 2019 01:27:02 +0200 Subject: [PATCH 1/5] Silence API warnings Change-Id: I27fd62de51ca0eedcc7e2e256487bda1e18bce8a Signed-off-by: Matthias Sohn --- org.eclipse.jgit/.settings/.api_filters | 59 +++++++++++++++++++++---- 1 file changed, 51 insertions(+), 8 deletions(-) diff --git a/org.eclipse.jgit/.settings/.api_filters b/org.eclipse.jgit/.settings/.api_filters index 1630c3fcd..3d1e656a1 100644 --- a/org.eclipse.jgit/.settings/.api_filters +++ b/org.eclipse.jgit/.settings/.api_filters @@ -1,10 +1,10 @@ - + - - + + @@ -23,12 +23,24 @@ + + + + + + + + + + + + @@ -62,12 +74,43 @@ - - + + - - - + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + From 3712b0a3b1b428ec446d83f13e544f605d26220e Mon Sep 17 00:00:00 2001 From: Masaya Suzuki Date: Sun, 23 Dec 2018 19:31:10 -0800 Subject: [PATCH 2/5] Change RacyGitTests to create a racy git situation in a stable way By using File#setLastModified, we can create a racy git situation stably. Tested with --runs_per_test=100 Bug: 526111 Change-Id: I60b3632d353e19f335668325aa603640be423f58 Signed-off-by: Masaya Suzuki (cherry picked from commit df637928d2ef4b9ee06af7e37344c7848f870ce4) --- .../org/eclipse/jgit/lib/RacyGitTests.java | 49 +++++++++---------- 1 file changed, 24 insertions(+), 25 deletions(-) diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/lib/RacyGitTests.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/lib/RacyGitTests.java index 9236b4e82..8b1d5cc37 100644 --- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/lib/RacyGitTests.java +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/lib/RacyGitTests.java @@ -61,8 +61,8 @@ public class RacyGitTests extends RepositoryTestCase { @Test - public void testIterator() throws IllegalStateException, IOException, - InterruptedException { + public void testIterator() + throws IllegalStateException, IOException, InterruptedException { TreeSet modTimes = new TreeSet<>(); File lastFile = null; for (int i = 0; i < 10; i++) { @@ -126,9 +126,6 @@ public void testIterator() throws IllegalStateException, IOException, @Test public void testRacyGitDetection() throws Exception { - TreeSet modTimes = new TreeSet<>(); - File lastFile; - // Reset to force creation of index file try (Git git = new Git(db)) { git.reset().call(); @@ -136,42 +133,44 @@ public void testRacyGitDetection() throws Exception { // wait to ensure that modtimes of the file doesn't match last index // file modtime - modTimes.add(valueOf(fsTick(db.getIndexFile()))); + fsTick(db.getIndexFile()); // create two files - addToWorkDir("a", "a"); - lastFile = addToWorkDir("b", "b"); + File a = addToWorkDir("a", "a"); + File b = addToWorkDir("b", "b"); + assertTrue(a.setLastModified(b.lastModified())); + assertTrue(b.setLastModified(b.lastModified())); // wait to ensure that file-modTimes and therefore index entry modTime // doesn't match the modtime of index-file after next persistance - modTimes.add(valueOf(fsTick(lastFile))); + fsTick(b); // now add both files to the index. No racy git expected - resetIndex(new FileTreeIteratorWithTimeControl(db, modTimes)); + resetIndex(new FileTreeIterator(db)); assertEquals( - "[a, mode:100644, time:t0, length:1, content:a]" + - "[b, mode:100644, time:t0, length:1, content:b]", + "[a, mode:100644, time:t0, length:1, content:a]" + + "[b, mode:100644, time:t0, length:1, content:b]", indexState(SMUDGE | MOD_TIME | LENGTH | CONTENT)); - // Remember the last modTime of index file. All modifications times of - // further modification are translated to this value so it looks that - // files have been modified in the same time slot as the index file - modTimes.add(Long.valueOf(db.getIndexFile().lastModified())); + // wait to ensure the file 'a' is updated at t1. + fsTick(db.getIndexFile()); - // modify one file - addToWorkDir("a", "a2"); - // now update the index the index. 'a' has to be racily clean -- because - // it's modification time is exactly the same as the previous index file - // mod time. - resetIndex(new FileTreeIteratorWithTimeControl(db, modTimes)); + // Create a racy git situation. This is a situation that the index is + // updated and then a file is modified within a second. By changing the + // index file artificially, we create a fake racy situation. + File updatedA = addToWorkDir("a", "a2"); + assertTrue(updatedA.setLastModified(updatedA.lastModified() + 100)); + resetIndex(new FileTreeIterator(db)); + assertTrue(db.getIndexFile() + .setLastModified(updatedA.lastModified() + 90)); db.readDirCache(); // although racily clean a should not be reported as being dirty assertEquals( - "[a, mode:100644, time:t1, smudged, length:0, content:a2]" + - "[b, mode:100644, time:t0, length:1, content:b]", - indexState(SMUDGE|MOD_TIME|LENGTH|CONTENT)); + "[a, mode:100644, time:t1, smudged, length:0, content:a2]" + + "[b, mode:100644, time:t0, length:1, content:b]", + indexState(SMUDGE | MOD_TIME | LENGTH | CONTENT)); } private File addToWorkDir(String path, String content) throws IOException { From 1de117780036b14db025ee3a5fa4ebd285a826cf Mon Sep 17 00:00:00 2001 From: Matthias Sohn Date: Tue, 18 Jun 2019 11:32:59 +0200 Subject: [PATCH 3/5] Fix RacyGitTests#testRacyGitDetection This test case assumed file system timestamp resolution of 1 second. On filesystems with a finer resolution this test fails since the index entry is only smudged if the file index entry's lastModified and the lastModified of the git index itself are within the same filesystem timer tick. Fix this by ensuring that these timestamps are identical which should work for any filesystem timer resolution. Bug: 548188 Change-Id: Id84d59e1cfeb48fa008f8f27f2f892c4f73985de Signed-off-by: Matthias Sohn Signed-off-by: Christian Halstrick (cherry picked from commit 1159f9dd7c80a53c2509cd75d997a6afed37f9a6) --- .../org/eclipse/jgit/lib/RacyGitTests.java | 37 +++++++++++++------ 1 file changed, 26 insertions(+), 11 deletions(-) diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/lib/RacyGitTests.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/lib/RacyGitTests.java index 8b1d5cc37..27939b212 100644 --- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/lib/RacyGitTests.java +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/lib/RacyGitTests.java @@ -44,6 +44,7 @@ import static java.lang.Long.valueOf; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertTrue; import java.io.File; @@ -52,10 +53,12 @@ import java.util.TreeSet; import org.eclipse.jgit.api.Git; +import org.eclipse.jgit.dircache.DirCache; import org.eclipse.jgit.junit.RepositoryTestCase; import org.eclipse.jgit.treewalk.FileTreeIterator; import org.eclipse.jgit.treewalk.FileTreeIteratorWithTimeControl; import org.eclipse.jgit.treewalk.NameConflictTreeWalk; +import org.eclipse.jgit.treewalk.WorkingTreeOptions; import org.eclipse.jgit.util.FileUtils; import org.junit.Test; @@ -136,8 +139,8 @@ public void testRacyGitDetection() throws Exception { fsTick(db.getIndexFile()); // create two files - File a = addToWorkDir("a", "a"); - File b = addToWorkDir("b", "b"); + File a = writeToWorkDir("a", "a"); + File b = writeToWorkDir("b", "b"); assertTrue(a.setLastModified(b.lastModified())); assertTrue(b.setLastModified(b.lastModified())); @@ -157,23 +160,35 @@ public void testRacyGitDetection() throws Exception { fsTick(db.getIndexFile()); // Create a racy git situation. This is a situation that the index is - // updated and then a file is modified within a second. By changing the - // index file artificially, we create a fake racy situation. - File updatedA = addToWorkDir("a", "a2"); - assertTrue(updatedA.setLastModified(updatedA.lastModified() + 100)); + // updated and then a file is modified within the same tick of the + // filesystem timestamp resolution. By changing the index file + // artificially, we create a fake racy situation. + File updatedA = writeToWorkDir("a", "a2"); + long newLastModified = updatedA.lastModified() + 100; + assertTrue(updatedA.setLastModified(newLastModified)); resetIndex(new FileTreeIterator(db)); - assertTrue(db.getIndexFile() - .setLastModified(updatedA.lastModified() + 90)); + assertTrue(db.getIndexFile().setLastModified(newLastModified)); - db.readDirCache(); - // although racily clean a should not be reported as being dirty + DirCache dc = db.readDirCache(); + // check index state: although racily clean a should not be reported as + // being dirty since we forcefully reset the index to match the working + // tree assertEquals( "[a, mode:100644, time:t1, smudged, length:0, content:a2]" + "[b, mode:100644, time:t0, length:1, content:b]", indexState(SMUDGE | MOD_TIME | LENGTH | CONTENT)); + + // compare state of files in working tree with index to check that + // FileTreeIterator.isModified() works as expected + FileTreeIterator f = new FileTreeIterator(db.getWorkTree(), db.getFS(), + db.getConfig().get(WorkingTreeOptions.KEY)); + assertTrue(f.findFile("a")); + try (ObjectReader reader = db.newObjectReader()) { + assertFalse(f.isModified(dc.getEntry("a"), false, reader)); + } } - private File addToWorkDir(String path, String content) throws IOException { + private File writeToWorkDir(String path, String content) throws IOException { File f = new File(db.getWorkTree(), path); FileOutputStream fos = new FileOutputStream(f); try { From 400342bbc1cff644ee9d7fd1076e62be89bb54f4 Mon Sep 17 00:00:00 2001 From: Matthias Sohn Date: Thu, 11 Jul 2019 10:00:23 +0200 Subject: [PATCH 4/5] Delete unused FileTreeIteratorWithTimeControl The only usage of this test iterator was removed in df637928d. Hence delete this iterator and associated test. Change-Id: I47710133ec3edc675c21db210960c024982668c6 Signed-off-by: Matthias Sohn (cherry picked from commit a024759cf5bf1cd6b9beb4f790d484943761a7e1) --- .../org/eclipse/jgit/lib/RacyGitTests.java | 68 ----------- .../FileTreeIteratorWithTimeControl.java | 109 ------------------ 2 files changed, 177 deletions(-) delete mode 100644 org.eclipse.jgit.test/tst/org/eclipse/jgit/treewalk/FileTreeIteratorWithTimeControl.java diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/lib/RacyGitTests.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/lib/RacyGitTests.java index 27939b212..552b7a1e9 100644 --- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/lib/RacyGitTests.java +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/lib/RacyGitTests.java @@ -42,7 +42,6 @@ */ package org.eclipse.jgit.lib; -import static java.lang.Long.valueOf; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertTrue; @@ -50,82 +49,15 @@ import java.io.File; import java.io.FileOutputStream; import java.io.IOException; -import java.util.TreeSet; import org.eclipse.jgit.api.Git; import org.eclipse.jgit.dircache.DirCache; import org.eclipse.jgit.junit.RepositoryTestCase; import org.eclipse.jgit.treewalk.FileTreeIterator; -import org.eclipse.jgit.treewalk.FileTreeIteratorWithTimeControl; -import org.eclipse.jgit.treewalk.NameConflictTreeWalk; import org.eclipse.jgit.treewalk.WorkingTreeOptions; -import org.eclipse.jgit.util.FileUtils; import org.junit.Test; public class RacyGitTests extends RepositoryTestCase { - @Test - public void testIterator() - throws IllegalStateException, IOException, InterruptedException { - TreeSet modTimes = new TreeSet<>(); - File lastFile = null; - for (int i = 0; i < 10; i++) { - lastFile = new File(db.getWorkTree(), "0." + i); - FileUtils.createNewFile(lastFile); - if (i == 5) - fsTick(lastFile); - } - modTimes.add(valueOf(fsTick(lastFile))); - for (int i = 0; i < 10; i++) { - lastFile = new File(db.getWorkTree(), "1." + i); - FileUtils.createNewFile(lastFile); - } - modTimes.add(valueOf(fsTick(lastFile))); - for (int i = 0; i < 10; i++) { - lastFile = new File(db.getWorkTree(), "2." + i); - FileUtils.createNewFile(lastFile); - if (i % 4 == 0) - fsTick(lastFile); - } - FileTreeIteratorWithTimeControl fileIt = new FileTreeIteratorWithTimeControl( - db, modTimes); - try (NameConflictTreeWalk tw = new NameConflictTreeWalk(db)) { - tw.addTree(fileIt); - tw.setRecursive(true); - FileTreeIterator t; - long t0 = 0; - for (int i = 0; i < 10; i++) { - assertTrue(tw.next()); - t = tw.getTree(0, FileTreeIterator.class); - if (i == 0) { - t0 = t.getEntryLastModified(); - } else { - assertEquals(t0, t.getEntryLastModified()); - } - } - long t1 = 0; - for (int i = 0; i < 10; i++) { - assertTrue(tw.next()); - t = tw.getTree(0, FileTreeIterator.class); - if (i == 0) { - t1 = t.getEntryLastModified(); - assertTrue(t1 > t0); - } else { - assertEquals(t1, t.getEntryLastModified()); - } - } - long t2 = 0; - for (int i = 0; i < 10; i++) { - assertTrue(tw.next()); - t = tw.getTree(0, FileTreeIterator.class); - if (i == 0) { - t2 = t.getEntryLastModified(); - assertTrue(t2 > t1); - } else { - assertEquals(t2, t.getEntryLastModified()); - } - } - } - } @Test public void testRacyGitDetection() throws Exception { diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/treewalk/FileTreeIteratorWithTimeControl.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/treewalk/FileTreeIteratorWithTimeControl.java deleted file mode 100644 index ff5730e72..000000000 --- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/treewalk/FileTreeIteratorWithTimeControl.java +++ /dev/null @@ -1,109 +0,0 @@ -/* - * Copyright (C) 2010, Christian Halstrick - * and other copyright owners as documented in the project's IP log. - * - * This program and the accompanying materials are made available - * under the terms of the Eclipse Distribution License v1.0 which - * accompanies this distribution, is reproduced below, and is - * available at http://www.eclipse.org/org/documents/edl-v10.php - * - * All rights reserved. - * - * Redistribution and use in source and binary forms, with or - * without modification, are permitted provided that the following - * conditions are met: - * - * - Redistributions of source code must retain the above copyright - * notice, this list of conditions and the following disclaimer. - * - * - Redistributions in binary form must reproduce the above - * copyright notice, this list of conditions and the following - * disclaimer in the documentation and/or other materials provided - * with the distribution. - * - * - Neither the name of the Eclipse Foundation, Inc. nor the - * names of its contributors may be used to endorse or promote - * products derived from this software without specific prior - * written permission. - * - * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND - * CONTRIBUTORS "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, - * INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES - * OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE - * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR - * CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, - * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT - * NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; - * LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER - * CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, - * STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) - * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF - * ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. - */ -package org.eclipse.jgit.treewalk; - -import java.io.File; -import java.util.SortedSet; -import java.util.TreeSet; - -import org.eclipse.jgit.lib.Config; -import org.eclipse.jgit.lib.ObjectReader; -import org.eclipse.jgit.lib.Repository; -import org.eclipse.jgit.util.FS; - -/** - * A {@link FileTreeIterator} used in tests which allows to specify explicitly - * what will be returned by {@link #getEntryLastModified()}. This allows to - * write tests where certain files have to have the same modification time. - *

- * This iterator is configured by a list of strictly increasing long values - * t(0), t(1), ..., t(n). For each file with a modification between t(x) and - * t(x+1) [ t(x) <= time < t(x+1) ] this iterator will report t(x). For files - * with a modification time smaller t(0) a modification time of 0 is returned. - * For files with a modification time greater or equal t(n) t(n) will be - * returned. - *

- * This class was written especially to test racy-git problems - */ -public class FileTreeIteratorWithTimeControl extends FileTreeIterator { - private TreeSet modTimes; - - public FileTreeIteratorWithTimeControl(FileTreeIterator p, Repository repo, - TreeSet modTimes) { - super(p, repo.getWorkTree(), repo.getFS()); - this.modTimes = modTimes; - } - - public FileTreeIteratorWithTimeControl(FileTreeIterator p, File f, FS fs, - TreeSet modTimes) { - super(p, f, fs); - this.modTimes = modTimes; - } - - public FileTreeIteratorWithTimeControl(Repository repo, - TreeSet modTimes) { - super(repo); - this.modTimes = modTimes; - } - - public FileTreeIteratorWithTimeControl(File f, FS fs, - TreeSet modTimes) { - super(f, fs, new Config().get(WorkingTreeOptions.KEY)); - this.modTimes = modTimes; - } - - @Override - public AbstractTreeIterator createSubtreeIterator(final ObjectReader reader) { - return new FileTreeIteratorWithTimeControl(this, - ((FileEntry) current()).getFile(), fs, modTimes); - } - - @Override - public long getEntryLastModified() { - if (modTimes == null) - return 0; - Long cutOff = Long.valueOf(super.getEntryLastModified() + 1); - SortedSet head = modTimes.headSet(cutOff); - return head.isEmpty() ? 0 : head.last().longValue(); - } -} From 0e3d4a273f3d5b79c55205a25f21d688b6f131fc Mon Sep 17 00:00:00 2001 From: Han-Wen Nienhuys Date: Sat, 31 Aug 2019 21:33:58 +0200 Subject: [PATCH 5/5] BatchRefUpdate: repro racy atomic update, and fix it PackedBatchRefUpdate was creating a new packed-refs list that was potentially unsorted. This would be papered over when the list was read back from disk in parsePackedRef, which detects unsorted ref lists on reading, and sorts them. However, the BatchRefUpdate also installed the new (unsorted) list in-memory in RefDirectory#packedRefs. With the timestamp granularity code committed to stable-5.1, we can more often accurately decide that the packed-refs file is clean, and will return the erroneous unsorted data more often. Unluckily timed delays also cause the file to be clean, hence this problem was exacerbated under load. The symptom is that refs added by a BatchRefUpdate would stop being visible directly after they were added. In particular, the Gerrit integration tests uses BatchRefUpdate in its setup for creating the Admin group, and then tries to read it out directly afterward. The tests recreates one failure case. A better approach would be to revise RefList.Builder, so it detects out-of-order lists and automatically sorts them. Fixes https://bugs.eclipse.org/bugs/show_bug.cgi?id=548716 and https://bugs.chromium.org/p/gerrit/issues/detail?id=11373. Bug: 548716 Change-Id: I613c8059964513ce2370543620725b540b3cb6d1 Signed-off-by: Han-Wen Nienhuys Signed-off-by: Matthias Sohn --- .../storage/file/BatchRefUpdateTest.java | 29 +++++ .../storage/file/PackedBatchRefUpdate.java | 118 +++++++++--------- 2 files changed, 87 insertions(+), 60 deletions(-) diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/BatchRefUpdateTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/BatchRefUpdateTest.java index 3c4b8cf4b..2ac4a846e 100644 --- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/BatchRefUpdateTest.java +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/BatchRefUpdateTest.java @@ -43,6 +43,7 @@ package org.eclipse.jgit.internal.storage.file; +import static java.nio.charset.StandardCharsets.UTF_8; import static java.util.concurrent.TimeUnit.NANOSECONDS; import static java.util.concurrent.TimeUnit.SECONDS; import static org.eclipse.jgit.internal.storage.file.BatchRefUpdateTest.Result.LOCK_FAILURE; @@ -64,6 +65,7 @@ import java.io.File; import java.io.IOException; +import java.nio.file.Files; import java.util.Arrays; import java.util.Collection; import java.util.Collections; @@ -161,6 +163,33 @@ public void removeListener() { refsChangedEvents = 0; } + @Test + public void packedRefsFileIsSorted() throws IOException { + assumeTrue(atomic); + + for (int i = 0; i < 2; i++) { + BatchRefUpdate bu = diskRepo.getRefDatabase().newBatchUpdate(); + String b1 = String.format("refs/heads/a%d",i); + String b2 = String.format("refs/heads/b%d",i); + bu.setAtomic(atomic); + ReceiveCommand c1 = new ReceiveCommand(ObjectId.zeroId(), A, b1); + ReceiveCommand c2 = new ReceiveCommand(ObjectId.zeroId(), B, b2); + bu.addCommand(c1, c2); + try (RevWalk rw = new RevWalk(diskRepo)) { + bu.execute(rw, NullProgressMonitor.INSTANCE); + } + assertEquals(c1.getResult(), ReceiveCommand.Result.OK); + assertEquals(c2.getResult(), ReceiveCommand.Result.OK); + } + + File packed = new File(diskRepo.getDirectory(), "packed-refs"); + String packedStr = new String(Files.readAllBytes(packed.toPath()), UTF_8); + + int a2 = packedStr.indexOf("refs/heads/a1"); + int b1 = packedStr.indexOf("refs/heads/b0"); + assertTrue(a2 < b1); + } + @Test public void simpleNoForce() throws IOException { writeLooseRef("refs/heads/master", A); diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/PackedBatchRefUpdate.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/PackedBatchRefUpdate.java index c1f547649..9ab9a1bad 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/PackedBatchRefUpdate.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/PackedBatchRefUpdate.java @@ -50,10 +50,10 @@ import java.io.IOException; import java.text.MessageFormat; -import java.util.ArrayList; +import java.util.Collections; +import java.util.Comparator; import java.util.HashMap; import java.util.HashSet; -import java.util.LinkedHashMap; import java.util.List; import java.util.Map; import java.util.Set; @@ -347,65 +347,72 @@ private Map lockLooseRefs(List commands) private static RefList applyUpdates(RevWalk walk, RefList refs, List commands) throws IOException { - int nDeletes = 0; - List adds = new ArrayList<>(commands.size()); + // Construct a new RefList by merging the old list with the updates. + // This assumes that each ref occurs at most once as a ReceiveCommand. + Collections.sort(commands, new Comparator() { + @Override + public int compare(ReceiveCommand a, ReceiveCommand b) { + return a.getRefName().compareTo(b.getRefName()); + } + }); + + int delta = 0; for (ReceiveCommand c : commands) { - if (c.getType() == ReceiveCommand.Type.CREATE) { - adds.add(c); - } else if (c.getType() == ReceiveCommand.Type.DELETE) { - nDeletes++; + switch (c.getType()) { + case DELETE: + delta--; + break; + case CREATE: + delta++; + break; + default: } } - int addIdx = 0; - // Construct a new RefList by linearly scanning the old list, and merging in - // any updates. - Map byName = byName(commands); - RefList.Builder b = - new RefList.Builder<>(refs.size() - nDeletes + adds.size()); - for (Ref ref : refs) { - String name = ref.getName(); - ReceiveCommand cmd = byName.remove(name); - if (cmd == null) { + RefList.Builder b = new RefList.Builder<>(refs.size() + delta); + int refIdx = 0; + int cmdIdx = 0; + while (refIdx < refs.size() || cmdIdx < commands.size()) { + Ref ref = (refIdx < refs.size()) ? refs.get(refIdx) : null; + ReceiveCommand cmd = (cmdIdx < commands.size()) + ? commands.get(cmdIdx) + : null; + int cmp = 0; + if (ref != null && cmd != null) { + cmp = ref.getName().compareTo(cmd.getRefName()); + } else if (ref == null) { + cmp = 1; + } else if (cmd == null) { + cmp = -1; + } + + if (cmp < 0) { b.add(ref); - continue; - } - if (!cmd.getOldId().equals(ref.getObjectId())) { - lockFailure(cmd, commands); - return null; - } - - // Consume any adds between the last and current ref. - while (addIdx < adds.size()) { - ReceiveCommand currAdd = adds.get(addIdx); - if (currAdd.getRefName().compareTo(name) < 0) { - b.add(peeledRef(walk, currAdd)); - byName.remove(currAdd.getRefName()); - } else { - break; + refIdx++; + } else if (cmp > 0) { + assert cmd != null; + if (cmd.getType() != ReceiveCommand.Type.CREATE) { + lockFailure(cmd, commands); + return null; } - addIdx++; - } - if (cmd.getType() != ReceiveCommand.Type.DELETE) { b.add(peeledRef(walk, cmd)); + cmdIdx++; + } else { + assert cmd != null; + assert ref != null; + if (!cmd.getOldId().equals(ref.getObjectId())) { + lockFailure(cmd, commands); + return null; + } + + if (cmd.getType() != ReceiveCommand.Type.DELETE) { + b.add(peeledRef(walk, cmd)); + } + cmdIdx++; + refIdx++; } } - - // All remaining adds are valid, since the refs didn't exist. - while (addIdx < adds.size()) { - ReceiveCommand cmd = adds.get(addIdx++); - byName.remove(cmd.getRefName()); - b.add(peeledRef(walk, cmd)); - } - - // Any remaining updates/deletes do not correspond to any existing refs, so - // they are lock failures. - if (!byName.isEmpty()) { - lockFailure(byName.values().iterator().next(), commands); - return null; - } - return b.toRefList(); } @@ -484,15 +491,6 @@ private String toResultString(ReceiveCommand cmd) { } } - private static Map byName( - List commands) { - Map ret = new LinkedHashMap<>(); - for (ReceiveCommand cmd : commands) { - ret.put(cmd.getRefName(), cmd); - } - return ret; - } - private static Ref peeledRef(RevWalk walk, ReceiveCommand cmd) throws IOException { ObjectId newId = cmd.getNewId().copy();