Browse Source

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
stable-4.9
Dave Borowitz 7 years ago
parent
commit
cf9e3fad52
  1. 49
      org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/RefDirectoryTest.java
  2. 46
      org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/RefUpdateTest.java
  3. 26
      org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/T0003_BasicTest.java
  4. 53
      org.eclipse.jgit.test/tst/org/eclipse/jgit/submodule/SubmoduleStatusTest.java
  5. 19
      org.eclipse.jgit/src/org/eclipse/jgit/lib/BatchRefUpdate.java
  6. 84
      org.eclipse.jgit/src/org/eclipse/jgit/lib/RefUpdate.java
  7. 8
      org.eclipse.jgit/src/org/eclipse/jgit/transport/ReceiveCommand.java

49
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<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");
}

46
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,

26
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

53
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<String, SubmoduleStatus> 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<String, SubmoduleStatus> 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());
}
}

19
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.
*

84
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.
* <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 @@ 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.
* <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 @@ public abstract class RefUpdate {
* 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 @@ 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;
}
}

8
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;

Loading…
Cancel
Save