Merge changes from topic 'packed-batch-ref-update'

* changes:
  Add tests for updating single refs to missing objects
  Fix deleting symrefs
  RefDirectory: Throw exception if CAS of packed ref list fails
  ReceiveCommand: Explicitly check constructor preconditions
  BatchRefUpdate: Document when getPushOptions is null

9 files changed, 269 insertions, 44 deletions

diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/GcPackRefsTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/GcPackRefsTest.java
index 11a2a22edb..c43bdbd298 100644
--- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/GcPackRefsTest.java
+++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/GcPackRefsTest.java
@@ -43,12 +43,13 @@
 
 package org.eclipse.jgit.internal.storage.file;
 
-import static java.lang.Integer.valueOf;
+import static org.hamcrest.Matchers.lessThanOrEqualTo;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertNotNull;
 import static org.junit.Assert.assertNull;
 import static org.junit.Assert.assertSame;
+import static org.junit.Assert.assertThat;
 
 import java.io.File;
 import java.io.IOException;
@@ -74,6 +75,7 @@ import org.eclipse.jgit.revwalk.RevCommit;
 import org.eclipse.jgit.storage.file.FileBasedConfig;
 import org.junit.Test;
 
+@SuppressWarnings("boxing")
 public class GcPackRefsTest extends GcTestCase {
 	@Test
 	public void looseRefPacked() throws Exception {
@@ -100,27 +102,23 @@ public class GcPackRefsTest extends GcTestCase {
 		RevBlob a = tr.blob("a");
 		tr.lightweightTag("t", a);
 
-		final CyclicBarrier syncPoint = new CyclicBarrier(2);
+		CyclicBarrier syncPoint = new CyclicBarrier(2);
 
-		Callable<Integer> packRefs = new Callable<Integer>() {
-
-			/** @return 0 for success, 1 in case of error when writing pack */
-			@Override
-			public Integer call() throws Exception {
-				syncPoint.await();
-				try {
-					gc.packRefs();
-					return valueOf(0);
-				} catch (IOException e) {
-					return valueOf(1);
-				}
+		// Returns 0 for success, 1 in case of error when writing pack.
+		Callable<Integer> packRefs = () -> {
+			syncPoint.await();
+			try {
+				gc.packRefs();
+				return 0;
+			} catch (IOException e) {
+				return 1;
 			}
 		};
 		ExecutorService pool = Executors.newFixedThreadPool(2);
 		try {
 			Future<Integer> p1 = pool.submit(packRefs);
 			Future<Integer> p2 = pool.submit(packRefs);
-			assertEquals(1, p1.get().intValue() + p2.get().intValue());
+			assertThat(p1.get() + p2.get(), lessThanOrEqualTo(1));
 		} finally {
 			pool.shutdown();
 			pool.awaitTermination(Long.MAX_VALUE, TimeUnit.SECONDS);
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 53db123d34..97130f288b 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
@@ -46,6 +46,7 @@ package org.eclipse.jgit.internal.storage.file;
 import static org.eclipse.jgit.lib.Constants.HEAD;
 import static org.eclipse.jgit.lib.Constants.R_HEADS;
 import static org.eclipse.jgit.lib.Constants.R_TAGS;
+import static org.eclipse.jgit.lib.ObjectId.zeroId;
 import static org.eclipse.jgit.lib.Ref.Storage.LOOSE;
 import static org.eclipse.jgit.lib.Ref.Storage.NEW;
 import static org.junit.Assert.assertEquals;
@@ -83,7 +84,6 @@ import org.eclipse.jgit.revwalk.RevCommit;
 import org.eclipse.jgit.revwalk.RevTag;
 import org.eclipse.jgit.revwalk.RevWalk;
 import org.eclipse.jgit.transport.ReceiveCommand;
-import org.eclipse.jgit.transport.ReceiveCommand.Type;
 import org.junit.Before;
 import org.junit.Test;
 
@@ -1297,9 +1297,9 @@ public class RefDirectoryTest extends LocalDiskRepositoryTestCase {
 		writeLooseRef("refs/heads/master", A);
 		writeLooseRef("refs/heads/masters", B);
 		List<ReceiveCommand> commands = Arrays.asList(
-				newCommand(A, B, "refs/heads/master",
+				new ReceiveCommand(A, B, "refs/heads/master",
 						ReceiveCommand.Type.UPDATE),
-				newCommand(B, A, "refs/heads/masters",
+				new ReceiveCommand(B, A, "refs/heads/masters",
 						ReceiveCommand.Type.UPDATE_NONFASTFORWARD));
 		BatchRefUpdate batchUpdate = refdir.newBatchUpdate();
 		batchUpdate.addCommand(commands);
@@ -1319,9 +1319,9 @@ public class RefDirectoryTest extends LocalDiskRepositoryTestCase {
 		writeLooseRef("refs/heads/master", A);
 		writeLooseRef("refs/heads/masters", B);
 		List<ReceiveCommand> commands = Arrays.asList(
-				newCommand(A, B, "refs/heads/master",
+				new ReceiveCommand(A, B, "refs/heads/master",
 						ReceiveCommand.Type.UPDATE),
-				newCommand(B, A, "refs/heads/masters",
+				new ReceiveCommand(B, A, "refs/heads/masters",
 						ReceiveCommand.Type.UPDATE_NONFASTFORWARD));
 		BatchRefUpdate batchUpdate = refdir.newBatchUpdate();
 		batchUpdate.setAllowNonFastForwards(true);
@@ -1341,7 +1341,7 @@ public class RefDirectoryTest extends LocalDiskRepositoryTestCase {
 			throws IOException {
 		writeLooseRef("refs/heads/master", B);
 		List<ReceiveCommand> commands = Arrays.asList(
-				newCommand(B, A, "refs/heads/master",
+				new ReceiveCommand(B, A, "refs/heads/master",
 						ReceiveCommand.Type.UPDATE_NONFASTFORWARD));
 		BatchRefUpdate batchUpdate = refdir.newBatchUpdate();
 		batchUpdate.setAllowNonFastForwards(true);
@@ -1362,11 +1362,12 @@ public class RefDirectoryTest extends LocalDiskRepositoryTestCase {
 		writeLooseRef("refs/heads/master", A);
 		writeLooseRef("refs/heads/masters", B);
 		List<ReceiveCommand> commands = Arrays.asList(
-				newCommand(A, B, "refs/heads/master",
+				new ReceiveCommand(A, B, "refs/heads/master",
 						ReceiveCommand.Type.UPDATE),
-				newCommand(null, A, "refs/heads/master/x",
+				new ReceiveCommand(zeroId(), A, "refs/heads/master/x",
 						ReceiveCommand.Type.CREATE),
-				newCommand(null, A, "refs/heads", ReceiveCommand.Type.CREATE));
+				new ReceiveCommand(zeroId(), A, "refs/heads",
+						ReceiveCommand.Type.CREATE));
 		BatchRefUpdate batchUpdate = refdir.newBatchUpdate();
 		batchUpdate.setAllowNonFastForwards(true);
 		batchUpdate.addCommand(commands);
@@ -1389,11 +1390,11 @@ public class RefDirectoryTest extends LocalDiskRepositoryTestCase {
 		writeLooseRef("refs/heads/master", A);
 		writeLooseRef("refs/heads/masters", B);
 		List<ReceiveCommand> commands = Arrays.asList(
-				newCommand(A, B, "refs/heads/master",
+				new ReceiveCommand(A, B, "refs/heads/master",
 						ReceiveCommand.Type.UPDATE),
-				newCommand(null, A, "refs/heads/masters/x",
+				new ReceiveCommand(zeroId(), A, "refs/heads/masters/x",
 						ReceiveCommand.Type.CREATE),
-				newCommand(B, null, "refs/heads/masters",
+				new ReceiveCommand(B, zeroId(), "refs/heads/masters",
 						ReceiveCommand.Type.DELETE));
 		BatchRefUpdate batchUpdate = refdir.newBatchUpdate();
 		batchUpdate.setAllowNonFastForwards(true);
@@ -1408,12 +1409,6 @@ public class RefDirectoryTest extends LocalDiskRepositoryTestCase {
 		assertEquals(A.getId(), refs.get("refs/heads/masters/x").getObjectId());
 	}
 
-	private static ReceiveCommand newCommand(RevCommit a, RevCommit b,
-			String string, Type update) {
-		return new ReceiveCommand(a != null ? a.getId() : null,
-				b != null ? b.getId() : null, string, update);
-	}
-
 	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 daef91d53c..1203e83dce 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
@@ -45,6 +45,7 @@
 
 package org.eclipse.jgit.internal.storage.file;
 
+import static java.nio.charset.StandardCharsets.UTF_8;
 import static org.eclipse.jgit.junit.Assert.assertEquals;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
@@ -57,13 +58,16 @@ 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;
+import org.eclipse.jgit.lib.ObjectInserter;
 import org.eclipse.jgit.lib.PersonIdent;
 import org.eclipse.jgit.lib.Ref;
 import org.eclipse.jgit.lib.RefRename;
@@ -240,14 +244,73 @@ public class RefUpdateTest extends SampleDataRepositoryTestCase {
 	@Test
 	public void testDeleteHeadInBareRepo() throws IOException {
 		Repository bareRepo = createBareRepository();
+		String master = "refs/heads/master";
+		Ref head = bareRepo.exactRef(Constants.HEAD);
+		assertNotNull(head);
+		assertTrue(head.isSymbolic());
+		assertEquals(master, head.getLeaf().getName());
+		assertNull(head.getObjectId());
+		assertNull(bareRepo.exactRef(master));
+
+		ObjectId blobId;
+		try (ObjectInserter ins = bareRepo.newObjectInserter()) {
+			blobId = ins.insert(Constants.OBJ_BLOB, "contents".getBytes(UTF_8));
+			ins.flush();
+		}
+
+		// Create master via HEAD, so we delete it.
 		RefUpdate ref = bareRepo.updateRef(Constants.HEAD);
-		ref.setNewObjectId(ObjectId
-				.fromString("0123456789012345678901234567890123456789"));
-		// Create the HEAD ref so we can delete it.
+		ref.setNewObjectId(blobId);
 		assertEquals(Result.NEW, ref.update());
+
+		head = bareRepo.exactRef(Constants.HEAD);
+		assertTrue(head.isSymbolic());
+		assertEquals(master, head.getLeaf().getName());
+		assertEquals(blobId, head.getLeaf().getObjectId());
+		assertEquals(blobId, bareRepo.exactRef(master).getObjectId());
+
+		// Unlike in a non-bare repo, deleting the HEAD is allowed, and leaves HEAD
+		// back in a dangling state.
 		ref = bareRepo.updateRef(Constants.HEAD);
-		delete(bareRepo, ref, Result.NO_CHANGE, true, true);
+		ref.setExpectedOldObjectId(blobId);
+		ref.setForceUpdate(true);
+		delete(bareRepo, ref, Result.FORCED, true, true);
+
+		head = bareRepo.exactRef(Constants.HEAD);
+		assertNotNull(head);
+		assertTrue(head.isSymbolic());
+		assertEquals(master, head.getLeaf().getName());
+		assertNull(head.getObjectId());
+		assertNull(bareRepo.exactRef(master));
 	}
+
+	@Test
+	public void testDeleteSymref() throws IOException {
+		RefUpdate dst = updateRef("refs/heads/abc");
+		assertEquals(Result.NEW, dst.update());
+		ObjectId id = dst.getNewObjectId();
+
+		RefUpdate u = db.updateRef("refs/symref");
+		assertEquals(Result.NEW, u.link(dst.getName()));
+
+		Ref ref = db.exactRef(u.getName());
+		assertNotNull(ref);
+		assertTrue(ref.isSymbolic());
+		assertEquals(dst.getName(), ref.getLeaf().getName());
+		assertEquals(id, ref.getLeaf().getObjectId());
+
+		u = db.updateRef(u.getName());
+		u.setDetachingSymbolicRef();
+		u.setForceUpdate(true);
+		assertEquals(Result.FORCED, u.delete());
+
+		assertNull(db.exactRef(u.getName()));
+		ref = db.exactRef(dst.getName());
+		assertNotNull(ref);
+		assertFalse(ref.isSymbolic());
+		assertEquals(id, ref.getObjectId());
+	}
+
 	/**
 	 * Delete a loose ref and make sure the directory in refs is deleted too,
 	 * and the reflog dir too
@@ -898,6 +961,92 @@ public class RefUpdateTest extends SampleDataRepositoryTestCase {
 				"HEAD").getReverseEntries().get(0).getComment());
 	}
 
+	@Test
+	public void testCreateMissingObject() throws IOException {
+		String name = "refs/heads/abc";
+		ObjectId bad =
+				ObjectId.fromString("deadbeefdeadbeefdeadbeefdeadbeefdeadbeef");
+		RefUpdate ru = db.updateRef(name);
+		ru.setNewObjectId(bad);
+		Result update = ru.update();
+		assertEquals(Result.NEW, 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());
+		}
+	}
+
+	@Test
+	public void testUpdateMissingObject() throws IOException {
+		String name = "refs/heads/abc";
+		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.update();
+		assertEquals(Result.REJECTED, update);
+
+		Ref ref = db.exactRef(name);
+		assertNotNull(ref);
+		assertEquals(oldId, ref.getObjectId());
+	}
+
+	@Test
+	public void testForceUpdateMissingObject() throws IOException {
+		String name = "refs/heads/abc";
+		RefUpdate ru = updateRef(name);
+		Result update = ru.update();
+		assertEquals(Result.NEW, update);
+
+		ObjectId bad =
+				ObjectId.fromString("deadbeefdeadbeefdeadbeefdeadbeefdeadbeef");
+		ru = db.updateRef(name);
+		ru.setNewObjectId(bad);
+		update = ru.forceUpdate();
+		assertEquals(Result.FORCED, 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());
+		}
+	}
+
 	private static void writeReflog(Repository db, ObjectId newId, String msg,
 			String refName) throws IOException {
 		RefDirectory refs = (RefDirectory) db.getRefDatabase();
diff --git a/org.eclipse.jgit/resources/org/eclipse/jgit/internal/JGitText.properties b/org.eclipse.jgit/resources/org/eclipse/jgit/internal/JGitText.properties
index 627007dc04..6e793dab75 100644
--- a/org.eclipse.jgit/resources/org/eclipse/jgit/internal/JGitText.properties
+++ b/org.eclipse.jgit/resources/org/eclipse/jgit/internal/JGitText.properties
@@ -208,12 +208,14 @@ corruptPack=Pack file {0} is corrupt, removing it from pack list
 createBranchFailedUnknownReason=Create branch failed for unknown reason
 createBranchUnexpectedResult=Create branch returned unexpected result {0}
 createNewFileFailed=Could not create new file {0}
+createRequiresZeroOldId=Create requires old ID to be zero
 credentialPassword=Password
 credentialUsername=Username
 daemonAlreadyRunning=Daemon already running
 daysAgo={0} days ago
 deleteBranchUnexpectedResult=Delete branch returned unexpected result {0}
 deleteFileFailed=Could not delete file {0}
+deleteRequiresZeroNewId=Delete requires new ID to be zero
 deleteTagUnexpectedResult=Delete tag returned unexpected result {0}
 deletingNotSupported=Deleting {0} not supported.
 destinationIsNotAWildcard=Destination is not a wildcard.
@@ -242,6 +244,7 @@ encryptionError=Encryption error: {0}
 encryptionOnlyPBE=Encryption error: only password-based encryption (PBE) algorithms are supported.
 endOfFileInEscape=End of file in escape
 entryNotFoundByPath=Entry not found by path: {0}
+enumValueNotSupported0=Invalid value: {0}
 enumValueNotSupported2=Invalid value: {0}.{1}={2}
 enumValueNotSupported3=Invalid value: {0}.{1}.{2}={3}
 enumValuesNotAvailable=Enumerated values of type {0} not available
@@ -425,6 +428,7 @@ need2Arguments=Need 2 arguments
 needPackOut=need packOut
 needsAtLeastOneEntry=Needs at least one entry
 needsWorkdir=Needs workdir
+newIdMustNotBeNull=New ID must not be null
 newlineInQuotesNotAllowed=Newline in quotes not allowed
 noApplyInDelete=No apply in delete
 noClosingBracket=No closing {0} found for {1} at index {2}.
@@ -458,6 +462,7 @@ objectNotFound=Object {0} not found.
 objectNotFoundIn=Object {0} not found in {1}.
 obtainingCommitsForCherryPick=Obtaining commits that need to be cherry-picked
 offsetWrittenDeltaBaseForObjectNotFoundInAPack=Offset-written delta base for object not found in a pack
+oldIdMustNotBeNull=Expected old ID must not be null
 onlyAlreadyUpToDateAndFastForwardMergesAreAvailable=only already-up-to-date and fast forward merges are available
 onlyOneFetchSupported=Only one fetch supported
 onlyOneOperationCallPerConnectionIsSupported=Only one operation call per connection is supported.
@@ -684,6 +689,7 @@ unsupportedOperationNotAddAtEnd=Not add-at-end: {0}
 unsupportedPackIndexVersion=Unsupported pack index version {0}
 unsupportedPackVersion=Unsupported pack version {0}.
 unsupportedRepositoryDescription=Repository description not supported
+updateRequiresOldIdAndNewId=Update requires both old ID and new ID to be nonzero
 updatingHeadFailed=Updating HEAD failed
 updatingReferences=Updating references
 updatingRefFailed=Updating the ref {0} to {1} failed. ReturnCode from RefUpdate.update() was {2}
diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/JGitText.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/JGitText.java
index 2ba3b8fd1c..ea752b950d 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/JGitText.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/JGitText.java
@@ -267,12 +267,14 @@ public class JGitText extends TranslationBundle {
 	/***/ public String createBranchFailedUnknownReason;
 	/***/ public String createBranchUnexpectedResult;
 	/***/ public String createNewFileFailed;
+	/***/ public String createRequiresZeroOldId;
 	/***/ public String credentialPassword;
 	/***/ public String credentialUsername;
 	/***/ public String daemonAlreadyRunning;
 	/***/ public String daysAgo;
 	/***/ public String deleteBranchUnexpectedResult;
 	/***/ public String deleteFileFailed;
+	/***/ public String deleteRequiresZeroNewId;
 	/***/ public String deleteTagUnexpectedResult;
 	/***/ public String deletingNotSupported;
 	/***/ public String destinationIsNotAWildcard;
@@ -301,6 +303,7 @@ public class JGitText extends TranslationBundle {
 	/***/ public String encryptionOnlyPBE;
 	/***/ public String endOfFileInEscape;
 	/***/ public String entryNotFoundByPath;
+	/***/ public String enumValueNotSupported0;
 	/***/ public String enumValueNotSupported2;
 	/***/ public String enumValueNotSupported3;
 	/***/ public String enumValuesNotAvailable;
@@ -484,6 +487,7 @@ public class JGitText extends TranslationBundle {
 	/***/ public String needPackOut;
 	/***/ public String needsAtLeastOneEntry;
 	/***/ public String needsWorkdir;
+	/***/ public String newIdMustNotBeNull;
 	/***/ public String newlineInQuotesNotAllowed;
 	/***/ public String noApplyInDelete;
 	/***/ public String noClosingBracket;
@@ -517,6 +521,7 @@ public class JGitText extends TranslationBundle {
 	/***/ public String objectNotFoundIn;
 	/***/ public String obtainingCommitsForCherryPick;
 	/***/ public String offsetWrittenDeltaBaseForObjectNotFoundInAPack;
+	/***/ public String oldIdMustNotBeNull;
 	/***/ public String onlyAlreadyUpToDateAndFastForwardMergesAreAvailable;
 	/***/ public String onlyOneFetchSupported;
 	/***/ public String onlyOneOperationCallPerConnectionIsSupported;
@@ -743,6 +748,7 @@ public class JGitText extends TranslationBundle {
 	/***/ public String unsupportedPackIndexVersion;
 	/***/ public String unsupportedPackVersion;
 	/***/ public String unsupportedRepositoryDescription;
+	/***/ public String updateRequiresOldIdAndNewId;
 	/***/ public String updatingHeadFailed;
 	/***/ public String updatingReferences;
 	/***/ public String updatingRefFailed;
diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/RefDirectory.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/RefDirectory.java
index 5d66a4fbd3..c8c2dd5867 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/RefDirectory.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/RefDirectory.java
@@ -580,6 +580,9 @@ public class RefDirectory extends RefDatabase {
 
 	void delete(RefDirectoryUpdate update) throws IOException {
 		Ref dst = update.getRef();
+		if (!update.isDetachingSymbolicRef()) {
+			dst = dst.getLeaf();
+		}
 		String name = dst.getName();
 
 		// Write the packed-refs file using an atomic update. We might
@@ -914,8 +917,24 @@ public class RefDirectory extends RefDatabase {
 					throw new ObjectWritingException(MessageFormat.format(JGitText.get().unableToWrite, name));
 
 				byte[] digest = Constants.newMessageDigest().digest(content);
-				packedRefs.compareAndSet(oldPackedList, new PackedRefList(refs,
-						lck.getCommitSnapshot(), ObjectId.fromRaw(digest)));
+				PackedRefList newPackedList = new PackedRefList(
+						refs, lck.getCommitSnapshot(), ObjectId.fromRaw(digest));
+
+				// This thread holds the file lock, so no other thread or process should
+				// be able to modify the packed-refs file on disk. If the list changed,
+				// it means something is very wrong, so throw an exception.
+				//
+				// However, we can't use a naive compareAndSet to check whether the
+				// update was successful, because another thread might _read_ the
+				// packed refs file that was written out by this thread while holding
+				// the lock, and update the packedRefs reference to point to that. So
+				// compare the actual contents instead.
+				PackedRefList afterUpdate = packedRefs.updateAndGet(
+						p -> p.id.equals(oldPackedList.id) ? newPackedList : p);
+				if (!afterUpdate.id.equals(newPackedList.id)) {
+					throw new ObjectWritingException(
+							MessageFormat.format(JGitText.get().unableToWrite, name));
+				}
 			}
 		}.writePackedRefs();
 	}
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 3043d4fcda..3c5ecfb1c8 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/lib/BatchRefUpdate.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/lib/BatchRefUpdate.java
@@ -58,6 +58,7 @@ import java.util.HashSet;
 import java.util.List;
 import java.util.concurrent.TimeoutException;
 
+import org.eclipse.jgit.annotations.Nullable;
 import org.eclipse.jgit.internal.JGitText;
 import org.eclipse.jgit.lib.RefUpdate.Result;
 import org.eclipse.jgit.revwalk.RevWalk;
@@ -323,9 +324,11 @@ public class BatchRefUpdate {
 	/**
 	 * Gets the list of option strings associated with this update.
 	 *
-	 * @return pushOptions
+	 * @return push options that were passed to {@link #execute}; prior to calling
+	 *         {@link #execute}, always returns null.
 	 * @since 4.5
 	 */
+	@Nullable
 	public List<String> getPushOptions() {
 		return pushOptions;
 	}
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 fc334f0275..61fda943c0 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/lib/RefUpdate.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/lib/RefUpdate.java
@@ -278,6 +278,16 @@ public abstract class RefUpdate {
 	}
 
 	/**
+	 * Return whether this update is actually detaching a symbolic ref.
+	 *
+	 * @return true if detaching a symref.
+	 * @since 4.9
+	 */
+	public boolean isDetachingSymbolicRef() {
+		return detachingSymbolicRef;
+	}
+
+	/**
 	 * Set the new value the ref will update to.
 	 *
 	 * @param id
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 2b21c4a8fe..a3f75010eb 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/ReceiveCommand.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/ReceiveCommand.java
@@ -210,7 +210,7 @@ public class ReceiveCommand {
 	 * Create a new command for {@link BaseReceivePack}.
 	 *
 	 * @param oldId
-	 *            the old object id; must not be null. Use
+	 *            the expected old object id; must not be null. Use
 	 *            {@link ObjectId#zeroId()} to indicate a ref creation.
 	 * @param newId
 	 *            the new object id; must not be null. Use
@@ -220,15 +220,23 @@ public class ReceiveCommand {
 	 */
 	public ReceiveCommand(final ObjectId oldId, final ObjectId newId,
 			final String name) {
+		if (oldId == null) {
+			throw new IllegalArgumentException(JGitText.get().oldIdMustNotBeNull);
+		}
+		if (newId == null) {
+			throw new IllegalArgumentException(JGitText.get().newIdMustNotBeNull);
+		}
 		this.oldId = oldId;
 		this.newId = newId;
 		this.name = name;
 
 		type = Type.UPDATE;
-		if (ObjectId.zeroId().equals(oldId))
+		if (ObjectId.zeroId().equals(oldId)) {
 			type = Type.CREATE;
-		if (ObjectId.zeroId().equals(newId))
+		}
+		if (ObjectId.zeroId().equals(newId)) {
 			type = Type.DELETE;
+		}
 	}
 
 	/**
@@ -243,14 +251,45 @@ public class ReceiveCommand {
 	 * @param name
 	 *            name of the ref being affected.
 	 * @param type
-	 *            type of the command.
+	 *            type of the command. Must be {@link Type#CREATE} if {@code
+	 *            oldId} is zero, or {@link Type#DELETE} if {@code newId} is zero.
 	 * @since 2.0
 	 */
 	public ReceiveCommand(final ObjectId oldId, final ObjectId newId,
 			final String name, final Type type) {
+		if (oldId == null) {
+			throw new IllegalArgumentException(JGitText.get().oldIdMustNotBeNull);
+		}
+		if (newId == null) {
+			throw new IllegalArgumentException(JGitText.get().newIdMustNotBeNull);
+		}
 		this.oldId = oldId;
 		this.newId = newId;
 		this.name = name;
+		switch (type) {
+		case CREATE:
+			if (!ObjectId.zeroId().equals(oldId)) {
+				throw new IllegalArgumentException(
+						JGitText.get().createRequiresZeroOldId);
+			}
+			break;
+		case DELETE:
+			if (!ObjectId.zeroId().equals(newId)) {
+				throw new IllegalArgumentException(
+						JGitText.get().deleteRequiresZeroNewId);
+			}
+			break;
+		case UPDATE:
+		case UPDATE_NONFASTFORWARD:
+			if (ObjectId.zeroId().equals(newId)
+					|| ObjectId.zeroId().equals(oldId)) {
+				throw new IllegalArgumentException(
+						JGitText.get().updateRequiresOldIdAndNewId);
+			}
+			break;
+		default:
+			throw new IllegalStateException(JGitText.get().enumValueNotSupported0);
+		}
 		this.type = type;
 	}