From 68ccc5d2137322118ed2a55f355e014a3670293a Mon Sep 17 00:00:00 2001 From: Dave Borowitz Date: Sun, 23 Jun 2013 17:02:29 -0700 Subject: [PATCH] UploadPack: refactor want validation Associate each RequestPolicy with an implementation of a RequestValidator interface that contains the validation logic. The checkWants method is only called if there are wants that were not advertised, since clients may always request any advertised want according to the git protocol. Calling the method only once at the end of parsing the want list also means policy implementations can be stateful, unlike the previous switch statement inside a loop. For the special handling of unidirectional pipes, simply check isBiDirectional() and delegate to other implementations if necessary. Change-Id: I52a174999ac3a5aca46d3469cb0b81edd1710580 --- .../eclipse/jgit/transport/UploadPack.java | 172 +++++++++++------- 1 file changed, 105 insertions(+), 67 deletions(-) diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java b/org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java index f1028a0f8..43f854e17 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java @@ -141,6 +141,25 @@ public class UploadPack { ANY; } + /** Validator for client requests. */ + private interface RequestValidator { + /** + * Check a list of client wants against the request policy. + * + * @param up + * {@link UploadPack} instance. + * @param wants + * objects the client requested that were not advertised. + * + * @throws PackProtocolException + * if one or more wants is not valid. + * @throws IOException + * if a low-level exception occurred. + */ + void checkWants(UploadPack up, List wants) + throws PackProtocolException, IOException; + } + /** Data in the first line of a request, the line itself plus options. */ public static class FirstLine { private final String line; @@ -311,7 +330,7 @@ public class UploadPack { SAVE.add(COMMON); SAVE.add(SATISFIED); - transferConfig = new TransferConfig(db); + setTransferConfig(null); } /** @return the repository this upload is reading from. */ @@ -490,6 +509,8 @@ public class UploadPack { */ public void setTransferConfig(TransferConfig tc) { this.transferConfig = tc != null ? tc : new TransferConfig(db); + this.requestPolicy = transferConfig.isAllowTipSha1InWant() + ? RequestPolicy.TIP : RequestPolicy.ADVERTISED; } /** @return the configured logger. */ @@ -595,27 +616,7 @@ public class UploadPack { return refs; } - private RequestPolicy getEffectiveRequestPolicy() { - RequestPolicy rp; - if (requestPolicy != null) - rp = requestPolicy; - else if (transferConfig.isAllowTipSha1InWant()) - rp = RequestPolicy.TIP; - else - rp = RequestPolicy.ADVERTISED; - - if (!biDirectionalPipe) { - if (rp == RequestPolicy.ADVERTISED) - rp = RequestPolicy.REACHABLE_COMMIT; - else if (rp == RequestPolicy.TIP) - rp = RequestPolicy.REACHABLE_COMMIT_TIP; - } - return rp; - } - private void service() throws IOException { - requestPolicy = getEffectiveRequestPolicy(); - if (biDirectionalPipe) sendAdvertisedRefs(new PacketLineOutRefAdvertiser(pckOut)); else if (requestPolicy == RequestPolicy.ANY) @@ -988,49 +989,13 @@ public class UploadPack { private void parseWants() throws IOException { AsyncRevObjectQueue q = walk.parseAny(wantIds, true); try { - List checkReachable = null; - Set reachableFrom = null; - Set tips = null; + List notAdvertisedWants = null; RevObject obj; while ((obj = q.next()) != null) { if (!advertised.contains(obj)) { - switch (requestPolicy) { - case ADVERTISED: - default: - throw new PackProtocolException(MessageFormat.format( - JGitText.get().wantNotValid, obj)); - case REACHABLE_COMMIT: - if (!(obj instanceof RevCommit)) { - throw new PackProtocolException(MessageFormat.format( - JGitText.get().wantNotValid, obj)); - } - if (checkReachable == null) { - checkReachable = new ArrayList(); - reachableFrom = advertised; - } - checkReachable.add((RevCommit) obj); - break; - case TIP: - if (tips == null) - tips = refIdSet(db.getAllRefs().values()); - if (!tips.contains(obj)) - throw new PackProtocolException(MessageFormat.format( - JGitText.get().wantNotValid, obj)); - break; - case REACHABLE_COMMIT_TIP: - if (!(obj instanceof RevCommit)) { - throw new PackProtocolException(MessageFormat.format( - JGitText.get().wantNotValid, obj)); - } - if (checkReachable == null) { - checkReachable = new ArrayList(); - reachableFrom = refIdSet(db.getAllRefs().values()); - } - checkReachable.add((RevCommit) obj); - break; - case ANY: - break; - } + if (notAdvertisedWants == null) + notAdvertisedWants = new ArrayList(); + notAdvertisedWants.add(obj); } want(obj); @@ -1042,8 +1007,8 @@ public class UploadPack { want(obj); } } - if (checkReachable != null) - checkNotAdvertisedWants(checkReachable, reachableFrom); + if (notAdvertisedWants != null) + getRequestValidator().checkWants(this, notAdvertisedWants); wantIds.clear(); } catch (MissingObjectException notFound) { ObjectId id = notFound.getObjectId(); @@ -1061,8 +1026,77 @@ public class UploadPack { } } - private void checkNotAdvertisedWants(List notAdvertisedWants, - Set reachableFrom) + private RequestValidator getRequestValidator() { + switch (requestPolicy) { + case ADVERTISED: + default: + return new AdvertisedRequestValidator(); + case REACHABLE_COMMIT: + return new ReachableCommitRequestValidator(); + case TIP: + return new TipRequestValidator(); + case REACHABLE_COMMIT_TIP: + return new ReachableCommitTipRequestValidator(); + case ANY: + return new AnyRequestValidator(); + } + } + + private static class AdvertisedRequestValidator implements RequestValidator { + public void checkWants(UploadPack up, List wants) + throws PackProtocolException, IOException { + if (!up.isBiDirectionalPipe()) + new ReachableCommitRequestValidator().checkWants(up, wants); + else if (!wants.isEmpty()) + throw new PackProtocolException(MessageFormat.format( + JGitText.get().wantNotValid, wants.iterator().next().name())); + } + } + + private static class ReachableCommitRequestValidator + implements RequestValidator { + public void checkWants(UploadPack up, List wants) + throws PackProtocolException, IOException { + checkNotAdvertisedWants(up.getRevWalk(), wants, + refIdSet(up.getAdvertisedRefs().values())); + } + } + + private static class TipRequestValidator implements RequestValidator { + public void checkWants(UploadPack up, List wants) + throws PackProtocolException, IOException { + if (!up.isBiDirectionalPipe()) + new ReachableCommitTipRequestValidator().checkWants(up, wants); + else if (!wants.isEmpty()) { + Set refIds = + refIdSet(up.getRepository().getAllRefs().values()); + for (RevObject obj : wants) { + if (!refIds.contains(obj)) + throw new PackProtocolException(MessageFormat.format( + JGitText.get().wantNotValid, obj.name())); + } + } + } + } + + private static class ReachableCommitTipRequestValidator + implements RequestValidator { + public void checkWants(UploadPack up, List wants) + throws PackProtocolException, IOException { + checkNotAdvertisedWants(up.getRevWalk(), wants, + refIdSet(up.getRepository().getAllRefs().values())); + } + } + + private static class AnyRequestValidator implements RequestValidator { + public void checkWants(UploadPack up, List wants) + throws PackProtocolException, IOException { + // All requests are valid. + } + } + + private static void checkNotAdvertisedWants(RevWalk walk, + List notAdvertisedWants, Set reachableFrom) throws MissingObjectException, IncorrectObjectTypeException, IOException { // Walk the requested commits back to the provided set of commits. If any // commit exists, a branch was deleted or rewound and the repository owner @@ -1070,8 +1104,12 @@ public class UploadPack { // into an advertised branch it will be marked UNINTERESTING and no commits // return. - for (RevCommit c : notAdvertisedWants) - walk.markStart(c); + for (RevObject obj : notAdvertisedWants) { + if (!(obj instanceof RevCommit)) + throw new PackProtocolException(MessageFormat.format( + JGitText.get().wantNotValid, obj.name())); + walk.markStart((RevCommit) obj); + } for (ObjectId id : reachableFrom) { try { walk.markUninteresting(walk.parseCommit(id));