diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/RefDirectoryTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/RefDirectoryTest.java index 97130f288..29f4a577a 100644 --- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/RefDirectoryTest.java +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/RefDirectoryTest.java @@ -75,6 +75,7 @@ import org.eclipse.jgit.junit.TestRepository; import org.eclipse.jgit.lib.AnyObjectId; import org.eclipse.jgit.lib.BatchRefUpdate; import org.eclipse.jgit.lib.NullProgressMonitor; +import org.eclipse.jgit.lib.ObjectId; import org.eclipse.jgit.lib.ProgressMonitor; import org.eclipse.jgit.lib.Ref; import org.eclipse.jgit.lib.Ref.Storage; @@ -1409,6 +1410,54 @@ public class RefDirectoryTest extends LocalDiskRepositoryTestCase { assertEquals(A.getId(), refs.get("refs/heads/masters/x").getObjectId()); } + @Test + public void testBatchRefUpdateUpdateToMissingObject() throws IOException { + writeLooseRef("refs/heads/master", A); + ObjectId bad = + ObjectId.fromString("deadbeefdeadbeefdeadbeefdeadbeefdeadbeef"); + List commands = Arrays.asList( + new ReceiveCommand(A, bad, "refs/heads/master", + ReceiveCommand.Type.UPDATE), + new ReceiveCommand(zeroId(), B, "refs/heads/foo2", + ReceiveCommand.Type.CREATE)); + BatchRefUpdate batchUpdate = refdir.newBatchUpdate(); + batchUpdate.setAtomic(false); + batchUpdate.setAllowNonFastForwards(true); + batchUpdate.addCommand(commands); + batchUpdate.execute(new RevWalk(diskRepo), NullProgressMonitor.INSTANCE); + Map refs = refdir.getRefs(RefDatabase.ALL); + assertEquals(ReceiveCommand.Result.REJECTED_MISSING_OBJECT, + commands.get(0).getResult()); + assertEquals(ReceiveCommand.Result.OK, commands.get(1).getResult()); + assertEquals("[HEAD, refs/heads/foo2, refs/heads/master]", refs.keySet() + .toString()); + assertEquals(A.getId(), refs.get("refs/heads/master").getObjectId()); + assertEquals(B.getId(), refs.get("refs/heads/foo2").getObjectId()); + } + + @Test + public void testBatchRefUpdateAddMissingObject() throws IOException { + writeLooseRef("refs/heads/master", A); + ObjectId bad = + ObjectId.fromString("deadbeefdeadbeefdeadbeefdeadbeefdeadbeef"); + List commands = Arrays.asList( + new ReceiveCommand(A, B, "refs/heads/master", + ReceiveCommand.Type.UPDATE), + new ReceiveCommand(zeroId(), bad, "refs/heads/foo2", + ReceiveCommand.Type.CREATE)); + BatchRefUpdate batchUpdate = refdir.newBatchUpdate(); + batchUpdate.setAllowNonFastForwards(true); + batchUpdate.addCommand(commands); + batchUpdate.execute(new RevWalk(diskRepo), NullProgressMonitor.INSTANCE); + Map refs = refdir.getRefs(RefDatabase.ALL); + assertEquals(ReceiveCommand.Result.OK, commands.get(0).getResult()); + assertEquals(ReceiveCommand.Result.REJECTED_MISSING_OBJECT, + commands.get(1).getResult()); + assertEquals("[HEAD, refs/heads/master]", refs.keySet() + .toString()); + assertEquals(B.getId(), refs.get("refs/heads/master").getObjectId()); + } + private void writeLooseRef(String name, AnyObjectId id) throws IOException { writeLooseRef(name, id.name() + "\n"); } diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/RefUpdateTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/RefUpdateTest.java index 1203e83dc..34f9eb95d 100644 --- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/RefUpdateTest.java +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/RefUpdateTest.java @@ -58,12 +58,10 @@ import static org.junit.Assert.fail; import java.io.File; import java.io.IOException; -import java.util.Arrays; import java.util.List; import java.util.Map; import java.util.Map.Entry; -import org.eclipse.jgit.errors.MissingObjectException; import org.eclipse.jgit.lib.AnyObjectId; import org.eclipse.jgit.lib.Constants; import org.eclipse.jgit.lib.ObjectId; @@ -969,27 +967,10 @@ public class RefUpdateTest extends SampleDataRepositoryTestCase { RefUpdate ru = db.updateRef(name); ru.setNewObjectId(bad); Result update = ru.update(); - assertEquals(Result.NEW, update); + assertEquals(Result.REJECTED_MISSING_OBJECT, update); Ref ref = db.exactRef(name); - assertNotNull(ref); - assertFalse(ref.isPeeled()); - assertEquals(bad, ref.getObjectId()); - - try (RevWalk rw = new RevWalk(db)) { - rw.parseAny(ref.getObjectId()); - fail("Expected MissingObjectException"); - } catch (MissingObjectException expected) { - assertEquals(bad, expected.getObjectId()); - } - - RefDirectory refdir = (RefDirectory) db.getRefDatabase(); - try { - // Packing requires peeling, which fails. - refdir.pack(Arrays.asList(name)); - } catch (MissingObjectException expected) { - assertEquals(bad, expected.getObjectId()); - } + assertNull(ref); } @Test @@ -1005,7 +986,7 @@ public class RefUpdateTest extends SampleDataRepositoryTestCase { ru = db.updateRef(name); ru.setNewObjectId(bad); update = ru.update(); - assertEquals(Result.REJECTED, update); + assertEquals(Result.REJECTED_MISSING_OBJECT, update); Ref ref = db.exactRef(name); assertNotNull(ref); @@ -1018,33 +999,18 @@ public class RefUpdateTest extends SampleDataRepositoryTestCase { RefUpdate ru = updateRef(name); Result update = ru.update(); assertEquals(Result.NEW, update); + ObjectId oldId = ru.getNewObjectId(); ObjectId bad = ObjectId.fromString("deadbeefdeadbeefdeadbeefdeadbeefdeadbeef"); ru = db.updateRef(name); ru.setNewObjectId(bad); update = ru.forceUpdate(); - assertEquals(Result.FORCED, update); + assertEquals(Result.REJECTED_MISSING_OBJECT, update); Ref ref = db.exactRef(name); assertNotNull(ref); - assertFalse(ref.isPeeled()); - assertEquals(bad, ref.getObjectId()); - - try (RevWalk rw = new RevWalk(db)) { - rw.parseAny(ref.getObjectId()); - fail("Expected MissingObjectException"); - } catch (MissingObjectException expected) { - assertEquals(bad, expected.getObjectId()); - } - - RefDirectory refdir = (RefDirectory) db.getRefDatabase(); - try { - // Packing requires peeling, which fails. - refdir.pack(Arrays.asList(name)); - } catch (MissingObjectException expected) { - assertEquals(bad, expected.getObjectId()); - } + assertEquals(oldId, ref.getObjectId()); } private static void writeReflog(Repository db, ObjectId newId, String msg, diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/T0003_BasicTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/T0003_BasicTest.java index ae1e531b8..9d23d8334 100644 --- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/T0003_BasicTest.java +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/T0003_BasicTest.java @@ -661,33 +661,39 @@ public class T0003_BasicTest extends SampleDataRepositoryTestCase { @Test public void test028_LockPackedRef() throws IOException { + ObjectId id1; + ObjectId id2; + try (ObjectInserter ins = db.newObjectInserter()) { + id1 = ins.insert( + Constants.OBJ_BLOB, "contents1".getBytes(Constants.CHARSET)); + id2 = ins.insert( + Constants.OBJ_BLOB, "contents2".getBytes(Constants.CHARSET)); + ins.flush(); + } + writeTrashFile(".git/packed-refs", - "7f822839a2fe9760f386cbbbcb3f92c5fe81def7 refs/heads/foobar"); + id1.name() + " refs/heads/foobar"); writeTrashFile(".git/HEAD", "ref: refs/heads/foobar\n"); BUG_WorkAroundRacyGitIssues("packed-refs"); BUG_WorkAroundRacyGitIssues("HEAD"); ObjectId resolve = db.resolve("HEAD"); - assertEquals("7f822839a2fe9760f386cbbbcb3f92c5fe81def7", resolve.name()); + assertEquals(id1, resolve); RefUpdate lockRef = db.updateRef("HEAD"); - ObjectId newId = ObjectId - .fromString("07f822839a2fe9760f386cbbbcb3f92c5fe81def"); - lockRef.setNewObjectId(newId); + lockRef.setNewObjectId(id2); assertEquals(RefUpdate.Result.FORCED, lockRef.forceUpdate()); assertTrue(new File(db.getDirectory(), "refs/heads/foobar").exists()); - assertEquals(newId, db.resolve("refs/heads/foobar")); + assertEquals(id2, db.resolve("refs/heads/foobar")); // Again. The ref already exists RefUpdate lockRef2 = db.updateRef("HEAD"); - ObjectId newId2 = ObjectId - .fromString("7f822839a2fe9760f386cbbbcb3f92c5fe81def7"); - lockRef2.setNewObjectId(newId2); + lockRef2.setNewObjectId(id1); assertEquals(RefUpdate.Result.FORCED, lockRef2.forceUpdate()); assertTrue(new File(db.getDirectory(), "refs/heads/foobar").exists()); - assertEquals(newId2, db.resolve("refs/heads/foobar")); + assertEquals(id1, db.resolve("refs/heads/foobar")); } @Test diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/submodule/SubmoduleStatusTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/submodule/SubmoduleStatusTest.java index 61df9d93f..5832518f8 100644 --- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/submodule/SubmoduleStatusTest.java +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/submodule/SubmoduleStatusTest.java @@ -59,11 +59,11 @@ import org.eclipse.jgit.dircache.DirCacheEditor; import org.eclipse.jgit.dircache.DirCacheEditor.PathEdit; import org.eclipse.jgit.dircache.DirCacheEntry; import org.eclipse.jgit.junit.RepositoryTestCase; +import org.eclipse.jgit.junit.TestRepository; import org.eclipse.jgit.lib.ConfigConstants; import org.eclipse.jgit.lib.Constants; import org.eclipse.jgit.lib.FileMode; import org.eclipse.jgit.lib.ObjectId; -import org.eclipse.jgit.lib.RefUpdate; import org.eclipse.jgit.lib.Repository; import org.eclipse.jgit.lib.StoredConfig; import org.eclipse.jgit.storage.file.FileBasedConfig; @@ -256,11 +256,16 @@ public class SubmoduleStatusTest extends RepositoryTestCase { } @Test - public void repositoryWithInitializedSubmodule() throws IOException, - GitAPIException { - final ObjectId id = ObjectId - .fromString("abcd1234abcd1234abcd1234abcd1234abcd1234"); - final String path = "sub"; + public void repositoryWithInitializedSubmodule() throws Exception { + String path = "sub"; + Repository subRepo = Git.init().setBare(false) + .setDirectory(new File(db.getWorkTree(), path)).call() + .getRepository(); + assertNotNull(subRepo); + + TestRepository subTr = new TestRepository<>(subRepo); + ObjectId id = subTr.branch(Constants.HEAD).commit().create().copy(); + DirCache cache = db.lockDirCache(); DirCacheEditor editor = cache.editor(); editor.add(new PathEdit(path) { @@ -287,15 +292,6 @@ public class SubmoduleStatusTest extends RepositoryTestCase { ConfigConstants.CONFIG_KEY_URL, url); modulesConfig.save(); - Repository subRepo = Git.init().setBare(false) - .setDirectory(new File(db.getWorkTree(), path)).call() - .getRepository(); - assertNotNull(subRepo); - - RefUpdate update = subRepo.updateRef(Constants.HEAD, true); - update.setNewObjectId(id); - update.forceUpdate(); - SubmoduleStatusCommand command = new SubmoduleStatusCommand(db); Map statuses = command.call(); assertNotNull(statuses); @@ -312,11 +308,16 @@ public class SubmoduleStatusTest extends RepositoryTestCase { } @Test - public void repositoryWithDifferentRevCheckedOutSubmodule() - throws IOException, GitAPIException { - final ObjectId id = ObjectId - .fromString("abcd1234abcd1234abcd1234abcd1234abcd1234"); - final String path = "sub"; + public void repositoryWithDifferentRevCheckedOutSubmodule() throws Exception { + String path = "sub"; + Repository subRepo = Git.init().setBare(false) + .setDirectory(new File(db.getWorkTree(), path)).call() + .getRepository(); + assertNotNull(subRepo); + + TestRepository subTr = new TestRepository<>(subRepo); + ObjectId id = subTr.branch(Constants.HEAD).commit().create().copy(); + DirCache cache = db.lockDirCache(); DirCacheEditor editor = cache.editor(); editor.add(new PathEdit(path) { @@ -343,15 +344,7 @@ public class SubmoduleStatusTest extends RepositoryTestCase { ConfigConstants.CONFIG_KEY_URL, url); modulesConfig.save(); - Repository subRepo = Git.init().setBare(false) - .setDirectory(new File(db.getWorkTree(), path)).call() - .getRepository(); - assertNotNull(subRepo); - - RefUpdate update = subRepo.updateRef(Constants.HEAD, true); - update.setNewObjectId(ObjectId - .fromString("aaaa0000aaaa0000aaaa0000aaaa0000aaaa0000")); - update.forceUpdate(); + ObjectId newId = subTr.branch(Constants.HEAD).commit().create().copy(); SubmoduleStatusCommand command = new SubmoduleStatusCommand(db); Map statuses = command.call(); @@ -365,7 +358,7 @@ public class SubmoduleStatusTest extends RepositoryTestCase { assertNotNull(status); assertEquals(path, status.getPath()); assertEquals(id, status.getIndexId()); - assertEquals(update.getNewObjectId(), status.getHeadId()); + assertEquals(newId, status.getHeadId()); assertEquals(SubmoduleStatusType.REV_CHECKED_OUT, status.getType()); } } diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/lib/BatchRefUpdate.java b/org.eclipse.jgit/src/org/eclipse/jgit/lib/BatchRefUpdate.java index 3c5ecfb1c..4a4db12fb 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/lib/BatchRefUpdate.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/lib/BatchRefUpdate.java @@ -59,6 +59,7 @@ import java.util.List; import java.util.concurrent.TimeoutException; import org.eclipse.jgit.annotations.Nullable; +import org.eclipse.jgit.errors.MissingObjectException; import org.eclipse.jgit.internal.JGitText; import org.eclipse.jgit.lib.RefUpdate.Result; import org.eclipse.jgit.revwalk.RevWalk; @@ -410,6 +411,11 @@ public class BatchRefUpdate { for (ReceiveCommand cmd : commands) { try { if (cmd.getResult() == NOT_ATTEMPTED) { + if (isMissing(walk, cmd.getOldId()) + || isMissing(walk, cmd.getNewId())) { + cmd.setResult(ReceiveCommand.Result.REJECTED_MISSING_OBJECT); + continue; + } cmd.updateType(walk); switch (cmd.getType()) { case CREATE: @@ -481,6 +487,19 @@ public class BatchRefUpdate { monitor.endTask(); } + private static boolean isMissing(RevWalk walk, ObjectId id) + throws IOException { + if (id.equals(ObjectId.zeroId())) { + return false; // Explicit add or delete is not missing. + } + try { + walk.parseAny(id); + return false; + } catch (MissingObjectException e) { + return true; + } + } + /** * Wait for timestamps to be in the past, aborting commands on timeout. * diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/lib/RefUpdate.java b/org.eclipse.jgit/src/org/eclipse/jgit/lib/RefUpdate.java index 61fda943c..07786450a 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/lib/RefUpdate.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/lib/RefUpdate.java @@ -58,7 +58,13 @@ import org.eclipse.jgit.transport.PushCertificate; * Creates, updates or deletes any reference. */ public abstract class RefUpdate { - /** Status of an update request. */ + /** + * Status of an update request. + *

+ * New values may be added to this enum in the future. Callers may assume that + * unknown values are failures, and may generally treat them the same as + * {@link #REJECTED_OTHER_REASON}. + */ public static enum Result { /** The ref update/delete has not been attempted by the caller. */ NOT_ATTEMPTED, @@ -114,6 +120,10 @@ public abstract class RefUpdate { * merged into the new value. The configuration did not allow a forced * update/delete to take place, so ref still contains the old value. No * previous history was lost. + *

+ * Note: Despite the general name, this result only refers to the + * non-fast-forward case. For more general errors, see {@link + * #REJECTED_OTHER_REASON}. */ REJECTED, @@ -139,7 +149,25 @@ public abstract class RefUpdate { * The ref was renamed from another name *

*/ - RENAMED + RENAMED, + + /** + * One or more objects aren't in the repository. + *

+ * This is severe indication of either repository corruption on the + * server side, or a bug in the client wherein the client did not supply + * all required objects during the pack transfer. + * + * @since 4.9 + */ + REJECTED_MISSING_OBJECT, + + /** + * Rejected for some other reason not covered by another enum value. + * + * @since 4.9 + */ + REJECTED_OTHER_REASON; } /** New value the caller wants this ref to have. */ @@ -637,34 +665,47 @@ public abstract class RefUpdate { RevObject oldObj; // don't make expensive conflict check if this is an existing Ref - if (oldValue == null && checkConflicting && getRefDatabase().isNameConflicting(getName())) + if (oldValue == null && checkConflicting + && getRefDatabase().isNameConflicting(getName())) { return Result.LOCK_FAILURE; + } try { // If we're detaching a symbolic reference, we should update the reference // itself. Otherwise, we will update the leaf reference, which should be // an ObjectIdRef. - if (!tryLock(!detachingSymbolicRef)) + if (!tryLock(!detachingSymbolicRef)) { return Result.LOCK_FAILURE; + } if (expValue != null) { final ObjectId o; o = oldValue != null ? oldValue : ObjectId.zeroId(); - if (!AnyObjectId.equals(expValue, o)) + if (!AnyObjectId.equals(expValue, o)) { return Result.LOCK_FAILURE; + } + } + try { + newObj = safeParseNew(walk, newValue); + } catch (MissingObjectException e) { + return Result.REJECTED_MISSING_OBJECT; } - if (oldValue == null) + + if (oldValue == null) { return store.execute(Result.NEW); + } - newObj = safeParse(walk, newValue); - oldObj = safeParse(walk, oldValue); - if (newObj == oldObj && !detachingSymbolicRef) + oldObj = safeParseOld(walk, oldValue); + if (newObj == oldObj && !detachingSymbolicRef) { return store.execute(Result.NO_CHANGE); + } - if (isForceUpdate()) + if (isForceUpdate()) { return store.execute(Result.FORCED); + } if (newObj instanceof RevCommit && oldObj instanceof RevCommit) { - if (walk.isMergedInto((RevCommit) oldObj, (RevCommit) newObj)) + if (walk.isMergedInto((RevCommit) oldObj, (RevCommit) newObj)) { return store.execute(Result.FAST_FORWARD); + } } return Result.REJECTED; @@ -684,16 +725,23 @@ public abstract class RefUpdate { checkConflicting = check; } - private static RevObject safeParse(final RevWalk rw, final AnyObjectId id) + private static RevObject safeParseNew(RevWalk rw, AnyObjectId newId) + throws IOException { + if (newId == null || ObjectId.zeroId().equals(newId)) { + return null; + } + return rw.parseAny(newId); + } + + private static RevObject safeParseOld(RevWalk rw, AnyObjectId oldId) throws IOException { try { - return id != null ? rw.parseAny(id) : null; + return oldId != null ? rw.parseAny(oldId) : null; } catch (MissingObjectException e) { - // We can expect some objects to be missing, like if we are - // trying to force a deletion of a branch and the object it - // points to has been pruned from the database due to freak - // corruption accidents (it happens with 'git new-work-dir'). - // + // We can expect some old objects to be missing, like if we are trying to + // force a deletion of a branch and the object it points to has been + // pruned from the database due to freak corruption accidents (it happens + // with 'git new-work-dir'). return null; } } diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/transport/ReceiveCommand.java b/org.eclipse.jgit/src/org/eclipse/jgit/transport/ReceiveCommand.java index a3f75010e..01ceef189 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/ReceiveCommand.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/ReceiveCommand.java @@ -467,6 +467,14 @@ public class ReceiveCommand { setResult(Result.REJECTED_CURRENT_BRANCH); break; + case REJECTED_MISSING_OBJECT: + setResult(Result.REJECTED_MISSING_OBJECT); + break; + + case REJECTED_OTHER_REASON: + setResult(Result.REJECTED_OTHER_REASON); + break; + default: setResult(Result.REJECTED_OTHER_REASON, r.name()); break;