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 53db123d34c1137b2e764d76ae341c75724d08b7..97130f288b5e858c395a7712b6574b0cf8b386b1 100644 (file)
--- 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/resources/org/eclipse/jgit/internal/JGitText.properties b/org.eclipse.jgit/resources/org/eclipse/jgit/internal/JGitText.properties
index 627007dc043f3aa8ce5e1a52bb5d56e914b88741..6e793dab75217441cf8448ea9fb619fe1d068479 100644 (file)
--- 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 2ba3b8fd1cc1e3bdb7add53f2b1c27261b86fadf..ea752b950deb2b1e2c42dbdc7774c4a2e88a6e8f 100644 (file)
--- 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/transport/ReceiveCommand.java b/org.eclipse.jgit/src/org/eclipse/jgit/transport/ReceiveCommand.java
index 2b21c4a8fe855c239c89408a225f2f0ef4d2612d..a3f75010ebd0fb745bea3906ad196fb2c8a80fad 100644 (file)
--- 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;
        }