diff options
author | Matthias Sohn <matthias.sohn@sap.com> | 2019-09-03 13:24:28 +0200 |
---|---|---|
committer | Matthias Sohn <matthias.sohn@sap.com> | 2019-09-03 14:20:37 +0200 |
commit | b525036e586c1e3e8ec5ab316cef1652603fe78e (patch) | |
tree | 39f3e9d3c3f665c47ba7c3ea58dd538cedbc409a | |
parent | 53014c0804e2aef8dce7ad535518b761114be6c7 (diff) | |
parent | 0e3d4a273f3d5b79c55205a25f21d688b6f131fc (diff) | |
download | jgit-b525036e586c1e3e8ec5ab316cef1652603fe78e.tar.gz jgit-b525036e586c1e3e8ec5ab316cef1652603fe78e.zip |
Merge branch 'stable-4.9' into stable-4.10
* stable-4.9:
BatchRefUpdate: repro racy atomic update, and fix it
Delete unused FileTreeIteratorWithTimeControl
Fix RacyGitTests#testRacyGitDetection
Change RacyGitTests to create a racy git situation in a stable way
Silence API warnings
Change-Id: Id5bf44645655fca40ad22bb1f1ad20a7c2e8f6db
Signed-off-by: Matthias Sohn <matthias.sohn@sap.com>
5 files changed, 165 insertions, 271 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 3c4b8cf4bc..2ac4a846ea 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 static org.junit.Assume.assumeTrue; 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; @@ -162,6 +164,33 @@ public class BatchRefUpdateTest extends LocalDiskRepositoryTestCase { } @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); writeLooseRef("refs/heads/masters", B); 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 9236b4e821..552b7a1e99 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,93 +42,25 @@ */ 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; 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.util.FileUtils; +import org.eclipse.jgit.treewalk.WorkingTreeOptions; import org.junit.Test; public class RacyGitTests extends RepositoryTestCase { - @Test - public void testIterator() throws IllegalStateException, IOException, - InterruptedException { - TreeSet<Long> 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 { - TreeSet<Long> modTimes = new TreeSet<>(); - File lastFile; - // Reset to force creation of index file try (Git git = new Git(db)) { git.reset().call(); @@ -136,45 +68,59 @@ public class RacyGitTests extends RepositoryTestCase { // 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 = writeToWorkDir("a", "a"); + File b = writeToWorkDir("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 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(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)); + "[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 { 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 717a2f3b2c..0000000000 --- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/treewalk/FileTreeIteratorWithTimeControl.java +++ /dev/null @@ -1,109 +0,0 @@ -/* - * Copyright (C) 2010, Christian Halstrick <christian.halstrick@sap.com> - * 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. - * <p> - * 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. - * <p> - * This class was written especially to test racy-git problems - */ -public class FileTreeIteratorWithTimeControl extends FileTreeIterator { - private TreeSet<Long> modTimes; - - public FileTreeIteratorWithTimeControl(FileTreeIterator p, Repository repo, - TreeSet<Long> modTimes) { - super(p, repo.getWorkTree(), repo.getFS()); - this.modTimes = modTimes; - } - - public FileTreeIteratorWithTimeControl(FileTreeIterator p, File f, FS fs, - TreeSet<Long> modTimes) { - super(p, f, fs); - this.modTimes = modTimes; - } - - public FileTreeIteratorWithTimeControl(Repository repo, - TreeSet<Long> modTimes) { - super(repo); - this.modTimes = modTimes; - } - - public FileTreeIteratorWithTimeControl(File f, FS fs, - TreeSet<Long> 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<Long> head = modTimes.headSet(cutOff); - return head.isEmpty() ? 0 : head.last().longValue(); - } -} diff --git a/org.eclipse.jgit/.settings/.api_filters b/org.eclipse.jgit/.settings/.api_filters index 726d670311..6a15442105 100644 --- a/org.eclipse.jgit/.settings/.api_filters +++ b/org.eclipse.jgit/.settings/.api_filters @@ -1,13 +1,5 @@ <?xml version="1.0" encoding="UTF-8" standalone="no"?> <component id="org.eclipse.jgit" version="2"> - <resource path="META-INF/MANIFEST.MF"> - <filter id="924844039"> - <message_arguments> - <message_argument value="4.10.1"/> - <message_argument value="4.10.0"/> - </message_arguments> - </filter> - </resource> <resource path="src/org/eclipse/jgit/errors/PackInvalidException.java" type="org.eclipse.jgit.errors.PackInvalidException"> <filter id="1142947843"> <message_arguments> @@ -22,6 +14,21 @@ </message_arguments> </filter> </resource> + <resource path="src/org/eclipse/jgit/lib/ConfigConstants.java" type="org.eclipse.jgit.lib.ConfigConstants"> + <filter id="336658481"> + <message_arguments> + <message_argument value="org.eclipse.jgit.lib.ConfigConstants"/> + <message_argument value="CONFIG_KEY_SUPPORTSATOMICFILECREATION"/> + </message_arguments> + </filter> + <filter id="1141899266"> + <message_arguments> + <message_argument value="4.5"/> + <message_argument value="4.10"/> + <message_argument value="CONFIG_KEY_SUPPORTSATOMICFILECREATION"/> + </message_arguments> + </filter> + </resource> <resource path="src/org/eclipse/jgit/lib/Constants.java" type="org.eclipse.jgit.lib.Constants"> <filter id="1141899266"> <message_arguments> @@ -47,9 +54,32 @@ </message_arguments> </filter> </resource> + <resource path="src/org/eclipse/jgit/merge/ResolveMerger.java" type="org.eclipse.jgit.merge.ResolveMerger"> + <filter id="1141899266"> + <message_arguments> + <message_argument value="3.5"/> + <message_argument value="4.10"/> + <message_argument value="processEntry(CanonicalTreeParser, CanonicalTreeParser, CanonicalTreeParser, DirCacheBuildIterator, WorkingTreeIterator, boolean)"/> + </message_arguments> + </filter> + </resource> <resource path="src/org/eclipse/jgit/util/FS.java" type="org.eclipse.jgit.util.FS"> <filter id="1141899266"> <message_arguments> + <message_argument value="4.5"/> + <message_argument value="4.10"/> + <message_argument value="createNewFile(File)"/> + </message_arguments> + </filter> + <filter id="1141899266"> + <message_arguments> + <message_argument value="4.5"/> + <message_argument value="4.10"/> + <message_argument value="supportsAtomicCreateNewFile()"/> + </message_arguments> + </filter> + <filter id="1141899266"> + <message_arguments> <message_argument value="4.7"/> <message_argument value="4.10"/> <message_argument value="createNewFileAtomic(File)"/> 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 200c63c17a..e45b53ea68 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 @@ -51,10 +51,10 @@ import static org.eclipse.jgit.transport.ReceiveCommand.Result.REJECTED_OTHER_RE 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; @@ -364,65 +364,72 @@ class PackedBatchRefUpdate extends BatchRefUpdate { private static RefList<Ref> applyUpdates(RevWalk walk, RefList<Ref> refs, List<ReceiveCommand> commands) throws IOException { - int nDeletes = 0; - List<ReceiveCommand> 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<ReceiveCommand>() { + @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<String, ReceiveCommand> byName = byName(commands); - RefList.Builder<Ref> 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) { - b.add(ref); - continue; - } - if (!cmd.getOldId().equals(ref.getObjectId())) { - lockFailure(cmd, commands); - return null; + + RefList.Builder<Ref> 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; } - // 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; + if (cmp < 0) { + b.add(ref); + 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)); - } - } - - // 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)); - } + cmdIdx++; + } else { + assert cmd != null; + assert ref != null; + if (!cmd.getOldId().equals(ref.getObjectId())) { + lockFailure(cmd, commands); + return null; + } - // 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; + if (cmd.getType() != ReceiveCommand.Type.DELETE) { + b.add(peeledRef(walk, cmd)); + } + cmdIdx++; + refIdx++; + } } - return b.toRefList(); } @@ -501,15 +508,6 @@ class PackedBatchRefUpdate extends BatchRefUpdate { } } - private static Map<String, ReceiveCommand> byName( - List<ReceiveCommand> commands) { - Map<String, ReceiveCommand> 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(); |