motiejus
/
jgit
1
Fork 0
Code Releases Activity

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
This commit is contained in:
Dave Borowitz 2017-07-05 14:07:17 -04:00
parent 00a72e22e6
commit 21ec281f3e
4 changed files with 68 additions and 22 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

31
org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/RefDirectoryTest.java
View File

@ -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 @@ public void testBatchRefUpdateSimpleNoForce() throws IOException {
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 void testBatchRefUpdateSimpleForce() throws IOException {
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 void testBatchRefUpdateNonFastForwardDoesNotDoExpensiveMergeCheck()
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 void testBatchRefUpdateConflict() throws IOException {
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 void testBatchRefUpdateConflictThanksToDelete() throws IOException {
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 void testBatchRefUpdateConflictThanksToDelete() throws IOException {
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");
}

6
org.eclipse.jgit/resources/org/eclipse/jgit/internal/JGitText.properties
View File

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

6
org.eclipse.jgit/src/org/eclipse/jgit/internal/JGitText.java
View File

@ -267,12 +267,14 @@ public static JGitText get() {
/***/ 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 static JGitText get() {
/***/ 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 static JGitText get() {
/***/ 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 static JGitText get() {
/***/ 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 static JGitText get() {
/***/ public String unsupportedPackIndexVersion;
/***/ public String unsupportedPackVersion;
/***/ public String unsupportedRepositoryDescription;
/***/ public String updateRequiresOldIdAndNewId;
/***/ public String updatingHeadFailed;
/***/ public String updatingReferences;
/***/ public String updatingRefFailed;

47
org.eclipse.jgit/src/org/eclipse/jgit/transport/ReceiveCommand.java
View File

@ -210,7 +210,7 @@ public static void abort(Iterable<ReceiveCommand> commands) {
* 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 static void abort(Iterable<ReceiveCommand> commands) {
*/
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 ReceiveCommand(final ObjectId oldId, final ObjectId newId,
* @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;
}