UploadPack: use want-refs as advertised set in fetch v2

Protocol v2 introduced refs-in-wants and ls-remote with
prefixes. UploadPack already uses prefixes provided by the client
during a v2 ref advertisement (ls-refs). However, when the client
consequently sends another request to fetch a previously advertised
ref (with want-ref lines), the server uses the whole set of advertised
refs to compute reachability.

In repos with many refs, this slows down the reachability checks
setting up and walking through unnecessary refs. For gerrit it can
also break valid requests because in gerrit "all" means "recent" and
the wanted-ref could fall out of the "recent" range when reloading all
refs at fetch time.

Treat wanted-refs like a ref-prefix when calculating the advertised
refs on v2 fetch command. Less refs means a faster setup and less walk
for the reachability checks. Note that wanted-refs filters only over
the refs visible to the user, so this doesn't give any extra
visibility to the caller.

If the request contains also "want <oid>" lines, we cannot use this
optimization. Those objects could be reachable from any visible
branch, not necessarily in the wanted-refs.

Google-Bug: b/122888978
Change-Id: I2a4ae171d4fc5d4cb30b020cb073ad23dd5a66c4

diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/UploadPackTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/UploadPackTest.java
index 278c9f86163d725282ff34b946a6e7da839f2b91..d61403f97e3fe61c5f531f85018bf9381f37db77 100644 (file)
--- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/UploadPackTest.java
+++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/UploadPackTest.java
@@ -3000,7 +3000,7 @@ public class UploadPackTest {
                assertThat(pckIn.readString(), is("packfile"));
                parsePack(recvStream);
                assertTrue(client.getObjectDatabase().has(one.toObjectId()));
-               assertEquals(1, ((RefCallsCountingRepository)server).numRefCalls());
+               assertEquals(0, ((RefCallsCountingRepository)server).numRefCalls());
        }
 
        @Test

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 33aa5855693ca8ec7f0d1570d7a8208789c11c98..9318871520839f61a607e583d5c7c926b0fc2e3e 100644 (file)
--- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java
@@ -1190,6 +1190,11 @@ public class UploadPack implements Closeable {
        }
 
        private void fetchV2(PacketLineOut pckOut) throws IOException {
+               ProtocolV2Parser parser = new ProtocolV2Parser(transferConfig);
+               FetchV2Request req = parser.parseFetchRequest(pckIn);
+               currentRequest = req;
+               Map<String, ObjectId> wantedRefs = wantedRefs(req);
+
                // Depending on the requestValidator, #processHaveLines may
                // require that advertised be set. Set it only in the required
                // circumstances (to avoid a full ref lookup in the case that
@@ -1199,16 +1204,25 @@ public class UploadPack implements Closeable {
                                requestValidator instanceof AnyRequestValidator) {
                        advertised = Collections.emptySet();
                } else {
-                       advertised = refIdSet(getAdvertisedOrDefaultRefs().values());
+                       if (req.wantIds.isEmpty()) {
+                               // Only refs-in-wants in request. These ref-in-wants where used as
+                               // filters already in the ls-refs, there is no need to use a full
+                               // advertisement now in fetch. This improves performance and also
+                               // accuracy: when the ref db prioritize and truncates the returned
+                               // refs (e.g. Gerrit hides too old refs), applying a filter can
+                               // return different results than a plain listing.
+                               advertised = refIdSet(getFilteredRefs(wantedRefs.keySet()).values());
+                       } else {
+                               // At least one SHA1 in wants, so we need to take the full
+                               // advertisement as base for a reachability check.
+                               advertised = refIdSet(getAdvertisedOrDefaultRefs().values());
+                       }
                }
 
                PackStatistics.Accumulator accumulator = new PackStatistics.Accumulator();
                Instant negotiateStart = Instant.now();
                accumulator.advertised = advertised.size();
 
-               ProtocolV2Parser parser = new ProtocolV2Parser(transferConfig);
-               FetchV2Request req = parser.parseFetchRequest(pckIn);
-               currentRequest = req;
                rawOut.stopBuffering();
 
                protocolV2Hook.onFetch(req);
@@ -1221,7 +1235,6 @@ public class UploadPack implements Closeable {
                // copying data back to class fields
                List<ObjectId> deepenNots = parseDeepenNots(req.getDeepenNots());
 
-               Map<String, ObjectId> wantedRefs = wantedRefs(req);
                // TODO(ifrade): Avoid mutating the parsed request.
                req.getWantIds().addAll(wantedRefs.values());
                wantIds = req.getWantIds();