From b961a7d03d42de6ebc3d269b88f8837f93c4ea0a Mon Sep 17 00:00:00 2001 From: Edwin Kempin Date: Mon, 2 May 2016 12:53:49 +0200 Subject: [PATCH] Always set VisibleRefFilter on UploadPack There was an optimization to not add VisibleRefFilter if all refs of the project were visible to the calling user, because in this case it is not needed to evaluate all rules. This optimization is unneeded because a similar optimization exists within VisibleRefFilter. VisibleRefFilter checks if all refs except refs/meta/config are visible to the calling user and if yes only one separate permission check for refs/meta/config is done. Removing the optimization for the case where all refs are visible to the calling user makes this case only slightly slower, since there is only one additional permission check for the refs/meta/config branch now. On the other hand the case where a user can see all refs except refs/meta/config is a little faster now, since the check if all refs are visible is removed. By having VisibleRefFilter invoked always, we can now use it to add magic refs to the advertised ref set, which is e.g. required to support a magic refs/users/self ref (implemented in the follow-up change). Change-Id: Ic8f29b41f3abf23f99b59260956d543f815253cc Signed-off-by: Edwin Kempin --- .../com/google/gerrit/acceptance/InProcessProtocol.java | 7 ++----- .../java/com/google/gerrit/httpd/GitOverHttpServlet.java | 6 ++---- .../java/com/google/gerrit/server/git/ReceiveCommits.java | 6 ++++-- .../main/java/com/google/gerrit/sshd/commands/Upload.java | 6 ++---- 4 files changed, 10 insertions(+), 15 deletions(-) diff --git a/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/InProcessProtocol.java b/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/InProcessProtocol.java index c16eed7b1a..14188bde74 100644 --- a/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/InProcessProtocol.java +++ b/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/InProcessProtocol.java @@ -259,11 +259,8 @@ class InProcessProtocol extends TestProtocol { UploadPack up = new UploadPack(repo); up.setPackConfig(transferConfig.getPackConfig()); up.setTimeout(transferConfig.getTimeout()); - - if (!ctl.allRefsAreVisible()) { - up.setAdvertiseRefsHook(new VisibleRefFilter( - tagCache, changeCache, repo, ctl, dbProvider.get(), true)); - } + up.setAdvertiseRefsHook(new VisibleRefFilter( + tagCache, changeCache, repo, ctl, dbProvider.get(), true)); List hooks = Lists.newArrayList(preUploadHooks); hooks.add(uploadValidatorsFactory.create( ctl.getProject(), repo, "localhost-test")); diff --git a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/GitOverHttpServlet.java b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/GitOverHttpServlet.java index bf8fe01f0a..063f22bd50 100644 --- a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/GitOverHttpServlet.java +++ b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/GitOverHttpServlet.java @@ -256,10 +256,8 @@ public class GitOverHttpServlet extends GitServlet { uploadValidatorsFactory.create(pc.getProject(), repo, request.getRemoteHost()); up.setPreUploadHook(PreUploadHookChain.newChain( Lists.newArrayList(up.getPreUploadHook(), uploadValidators))); - if (!pc.allRefsAreVisible()) { - up.setAdvertiseRefsHook(new VisibleRefFilter(tagCache, changeCache, - repo, pc, db.get(), true)); - } + up.setAdvertiseRefsHook(new VisibleRefFilter(tagCache, changeCache, + repo, pc, db.get(), true)); next.doFilter(request, response); } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java index dc3ff6eba2..fc8be68035 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java @@ -474,9 +474,11 @@ public class ReceiveCommits { }); if (!projectControl.allRefsAreVisible()) { - rp.setCheckReferencedObjectsAreReachable(receiveConfig.checkReferencedObjectsAreReachable); - rp.setAdvertiseRefsHook(new VisibleRefFilter(tagCache, changeCache, repo, projectControl, db, false)); + rp.setCheckReferencedObjectsAreReachable( + receiveConfig.checkReferencedObjectsAreReachable); } + rp.setAdvertiseRefsHook(new VisibleRefFilter(tagCache, changeCache, repo, + projectControl, db, false)); List advHooks = new ArrayList<>(3); advHooks.add(new AdvertiseRefsHook() { @Override diff --git a/gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/Upload.java b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/Upload.java index 39efb26282..b420a5faa2 100644 --- a/gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/Upload.java +++ b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/Upload.java @@ -68,10 +68,8 @@ final class Upload extends AbstractGitCommand { } final UploadPack up = new UploadPack(repo); - if (!projectControl.allRefsAreVisible()) { - up.setAdvertiseRefsHook(new VisibleRefFilter(tagCache, changeCache, repo, - projectControl, db, true)); - } + up.setAdvertiseRefsHook(new VisibleRefFilter(tagCache, changeCache, repo, + projectControl, db, true)); up.setPackConfig(config.getPackConfig()); up.setTimeout(config.getTimeout()); up.setPostUploadHook(uploadMetrics);