Separate RefUpdate.Result.REJECTED_{MISSING_OBJECT,OTHER_REASON}
ReceiveCommand.Result has a slightly richer set of possibilities, so it
makes sense for RefUpdate.Result to have more values in order to match.
In particular, this allows us to return REJECTED_MISSING_OBJECT from
RefUpdate when an object is missing.
The comment in RefUpdate#safeParse about expecting some old objects to be
missing is only applicable to the old ID, not the new ID. A missing new
ID is a bug or programmer error, and we should not update a ref to point
to one.
Fix various tests that started failing because they depended for no good
reason on setting refs to point to nonexistent objects; it's always easy
to create a real object when necessary.
It is possible that some downstream users of RefUpdate.Result might
choose to handle one of the new statuses differently, for example by
providing a more user-readable error message; that is not done in this
change.
Change-Id: I734b1c32d5404752447d9e20329471436ffe05fc
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 97130f2..29f4a57 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.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 @@
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<ReceiveCommand> 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<String, Ref> 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<ReceiveCommand> 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<String, Ref> 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 1203e83..34f9eb9 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 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 @@
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 @@
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 @@
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 ae1e531..9d23d83 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 @@
@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 61df9d9..5832518 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.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 @@
}
@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 @@
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<String, SubmoduleStatus> statuses = command.call();
assertNotNull(statuses);
@@ -312,11 +308,16 @@
}
@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 @@
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<String, SubmoduleStatus> statuses = command.call();
@@ -365,7 +358,7 @@
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 3c5ecfb..4a4db12 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.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 @@
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 @@
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 61fda94..0778645 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 @@
* Creates, updates or deletes any reference.
*/
public abstract class RefUpdate {
- /** Status of an update request. */
+ /**
+ * Status of an update request.
+ * <p>
+ * 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 @@
* 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.
+ * <p>
+ * <em>Note:</em> 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 @@
* The ref was renamed from another name
* <p>
*/
- RENAMED
+ RENAMED,
+
+ /**
+ * One or more objects aren't in the repository.
+ * <p>
+ * 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 @@
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;
+ }
}
- if (oldValue == null)
+ try {
+ newObj = safeParseNew(walk, newValue);
+ } catch (MissingObjectException e) {
+ return Result.REJECTED_MISSING_OBJECT;
+ }
+
+ 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 @@
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 a3f7501..01ceef1 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 @@
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;
