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

6 files changed, 90 insertions, 7 deletions

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