ReceiveCommand: Explicitly check constructor preconditions
Some downstream code checks whether a ReceiveCommand is a create or a
delete based on the type field. Other downstream code (in particular a
good chunk of Gerrit code I wrote) checks the same thing by comparing
oldId/newId to zeroId. Unfortunately, there were no strict checks in the
constructor that ensures that zeroId is only set for oldId/newId if the
type argument corresponds, so a caller that passed mismatched IDs and
types would observe completely undefined behavior as a result. This is
and always has been a misuse of the API; throw IllegalArgumentException
so the caller knows that it is a misuse.
Similarly, throw from the constructor if oldId/newId are null. The
non-nullness requirement was already documented. Fix RefDirectoryTest to
not do the wrong thing.
Change-Id: Ie2d0bfed8a2d89e807a41925d548f0f0ce243ecf
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 53db123..97130f2 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 @@
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.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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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/resources/org/eclipse/jgit/internal/JGitText.properties b/org.eclipse.jgit/resources/org/eclipse/jgit/internal/JGitText.properties
index 627007d..6e793da 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 @@
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 @@
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 @@
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 @@
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 @@
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 2ba3b8f..ea752b9 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 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 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 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 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 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/transport/ReceiveCommand.java b/org.eclipse.jgit/src/org/eclipse/jgit/transport/ReceiveCommand.java
index 2b21c4a..a3f7501 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 @@
* 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 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 @@
* @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;
}
