From 69928a6d068442c945e996f529c9e809edc66074 Mon Sep 17 00:00:00 2001 From: Shawn Pearce Date: Sun, 24 Feb 2013 18:01:27 -0800 Subject: [PATCH] Add options to refs/for/ magic branch syntax Git doesn't want to modify the network protocol to support passing data from the git push client to the server. Work around this by embedding option data into a new style of reference specification: refs/for/master%r=alice,cc=bob,cc=charlie,topic=options is now parsed by the server as: - set topic to "options" - CC charlie and bob - add reviewer alice - for branch refs/heads/master If % is used the "extra information" after the branch name is parsed as options with args4j. Each option is delimited by ",". Selecting publish vs. draft should be done with options draft or publish, appearing anywhere in the refspec after the % marker: refs/for/master%draft refs/for/master%draft,r=alice refs/for/master%r=alice,draft refs/for/master%r=alice,publish Change-Id: I895bd1218c2099b5b45cac943039bbd12565370c --- Documentation/cmd-receive-pack.txt | 36 ++--- Documentation/user-upload.txt | 27 ++-- .../com/google/gerrit/httpd/WebModule.java | 2 + .../server/config/GerritRequestModule.java | 2 - .../gerrit/server/git/ReceiveCommits.java | 135 ++++++++++++------ .../com/google/gerrit/sshd/SshModule.java | 2 + .../google/gerrit/util/cli/CmdLineParser.java | 53 ++++--- 7 files changed, 157 insertions(+), 100 deletions(-) diff --git a/Documentation/cmd-receive-pack.txt b/Documentation/cmd-receive-pack.txt index 68f686d859..92bb65e2ff 100644 --- a/Documentation/cmd-receive-pack.txt +++ b/Documentation/cmd-receive-pack.txt @@ -32,28 +32,12 @@ OPTIONS --reviewer
:: --re
:: - Automatically add
as a reviewer to any change - created or updated by the pushed commit objects. These - changes will appear in the reviewer's dashboard, and will - also be emailed to the reviewer. -+ -May be specified more than once to request multiple reviewers. -+ -This is a Gerrit Code Review specific extension. + Automatically add
as a reviewer to any change. + Deprecated, use `refs/for/branch%r=address` instead. --cc
:: - Carbon-copy
on the created or updated changes, - but don't request them to perform a review. Like with - --reviewer the changes will appear in the CC'd user's - dashboard, and will be emailed to them. -+ -May be specified more than once to specify multiple CCs. -+ -This is a Gerrit Code Review specific extension. - -Above
may be the complete email address, or, if Gerrit is -configured with HTTP authentication (e.g. within a single domain), -just the local part (typically username). + Carbon-copy
on the created or updated changes. + Deprecated, use `refs/for/branch%cc=address` instead. ACCESS ------ @@ -64,32 +48,30 @@ EXAMPLES Send a review for a change on the master branch to charlie@example.com: ===== - git push --receive-pack='git receive-pack --reviewer charlie@example.com' ssh://review.example.com:29418/project HEAD:refs/for/master + git push ssh://review.example.com:29418/project HEAD:refs/for/master%r=charlie@example.com ===== Send reviews, but tagging them with the topic name 'bug42': ===== - git push --receive-pack='git receive-pack --reviewer charlie@example.com' ssh://review.example.com:29418/project HEAD:refs/for/master/bug42 + git push ssh://review.example.com:29418/project HEAD:refs/for/master%r=charlie@example.com,topic=bug42 ===== Also CC two other parties: ===== - git push --receive-pack='git receive-pack --reviewer charlie@example.com --cc alice@example.com --cc bob@example.com' ssh://review.example.com:29418/project HEAD:refs/for/master + git push ssh://review.example.com:29418/project HEAD:refs/for/master%r=charlie@example.com,cc=alice@example.com,cc=bob@example.com ===== Configure a push macro to perform the last action: ==== git config remote.charlie.url ssh://review.example.com:29418/project - git config remote.charlie.push HEAD:refs/for/master - git config remote.charlie.receivepack 'git receive-pack --reviewer charlie@example.com --cc alice@example.com --cc bob@example.com' + git config remote.charlie.push HEAD:refs/for/master%r=charlie@example.com,cc=alice@example.com,cc=bob@example.com ==== afterwards `.git/config` contains the following: ---- [remote "charlie"] url = ssh://review.example.com:29418/project - push = HEAD:refs/for/master - receivepack = git receive-pack --reviewer charlie@example.com --cc alice@example.com --cc bob@example.com + push = HEAD:refs/for/master%r=charlie@example.com,cc=alice@example.com,cc=bob@example.com ---- and now sending a new change for review to charlie, CC'ing both diff --git a/Documentation/user-upload.txt b/Documentation/user-upload.txt index 26de14a75e..d6b90ddae6 100644 --- a/Documentation/user-upload.txt +++ b/Documentation/user-upload.txt @@ -130,7 +130,7 @@ To create new changes for review, simply push to the project's magical `refs/for/'branch'` ref using any Git client tool: ==== - git push ssh://sshusername@hostname:29418/projectname HEAD:refs/for/branchname + git push ssh://sshusername@hostname:29418/projectname HEAD:refs/for/branch ==== E.g. `john.doe` can use git push to upload new changes for the @@ -152,12 +152,12 @@ message when the push is completed. To include a short tag associated with all of the changes in the same group, such as the local topic branch name, append it after -the destination branch name. In this example the short topic tag +the destination branch name. In this example the short topic tag 'driver/i42' will be saved on each change this push creates or updates: ==== - git push ssh://john.doe@git.example.com:29418/kernel/common HEAD:refs/for/experimental/driver/i42 + git push ssh://john.doe@git.example.com:29418/kernel/common HEAD:refs/for/experimental%topic=driver/i42 ==== If you are frequently uploading changes to the same Gerrit server, @@ -178,17 +178,17 @@ shorter URLs on the command line, such as: Specific reviewers can be requested and/or additional 'carbon copies' of the notification message may be sent by including these -as arguments to `git receive-pack`: +as options in the reference ==== - git push --receive-pack='git receive-pack --reviewer=a@a.com --cc=b@o.com' tr:kernel/common HEAD:refs/for/experimental + git push tr:kernel/common HEAD:refs/for/experimental%r=a@a.com,cc=b@o.com ==== -The `--reviewer='email'` and `--cc='email'` options may be -specified as many times as necessary to cover all interested -parties. Gerrit will automatically avoid sending duplicate email -notifications, such as if one of the specified reviewers or CC -addresses had also requested to receive all new change notifications. +The `r='email'` and `cc='email'` options may be specified as many +times as necessary to cover all interested parties. Gerrit will +automatically avoid sending duplicate email notifications, such as +if one of the specified reviewers or CC addresses had also requested +to receive all new change notifications. If you are frequently sending changes to the same parties and/or branches, consider adding a custom remote block to your project's @@ -197,12 +197,11 @@ branches, consider adding a custom remote block to your project's ==== $ cat .git/config ... - [remote "for-a-exp"] + [remote "exp"] url = tr:kernel/common - receivepack = git receive-pack --reviewer=a@a.com --cc=b@o.com - push = HEAD:refs/for/experimental + push = HEAD:refs/for/experimental%r=a@a.com,cc=b@o.com - $ git push for-a-exp + $ git push exp ==== diff --git a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/WebModule.java b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/WebModule.java index 29686ceb3c..dd82dde3d4 100644 --- a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/WebModule.java +++ b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/WebModule.java @@ -37,6 +37,7 @@ import com.google.gerrit.server.config.FactoryModule; import com.google.gerrit.server.config.GerritRequestModule; import com.google.gerrit.server.contact.ContactStore; import com.google.gerrit.server.contact.ContactStoreProvider; +import com.google.gerrit.server.git.AsyncReceiveCommits; import com.google.gerrit.server.util.GuiceRequestScopePropagator; import com.google.gerrit.server.util.RequestScopePropagator; import com.google.inject.AbstractModule; @@ -136,6 +137,7 @@ public class WebModule extends FactoryModule { DynamicSet.setOf(binder(), WebUiPlugin.class); factory(ClearPassword.Factory.class); + install(new AsyncReceiveCommits.Module()); install(new CmdLineParserModule()); factory(GeneratePassword.Factory.class); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/config/GerritRequestModule.java b/gerrit-server/src/main/java/com/google/gerrit/server/config/GerritRequestModule.java index 0779cec9b3..18e5e1f8a4 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/config/GerritRequestModule.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/config/GerritRequestModule.java @@ -21,7 +21,6 @@ import com.google.gerrit.server.IdentifiedUser; import com.google.gerrit.server.RequestCleanup; import com.google.gerrit.server.changedetail.DeleteDraftPatchSet; import com.google.gerrit.server.changedetail.PublishDraft; -import com.google.gerrit.server.git.AsyncReceiveCommits; import com.google.gerrit.server.git.BanCommit; import com.google.gerrit.server.git.MergeOp; import com.google.gerrit.server.git.MetaDataUpdate; @@ -50,7 +49,6 @@ public class GerritRequestModule extends FactoryModule { factory(SubmoduleOp.Factory.class); factory(MergeOp.Factory.class); - install(new AsyncReceiveCommits.Module()); // Not really per-request, but dammit, I don't know where else to // easily park this stuff. 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 5149ebaa70..322e363340 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 @@ -27,6 +27,7 @@ import static org.eclipse.jgit.transport.ReceiveCommand.Result.REJECTED_OTHER_RE import com.google.common.base.Function; import com.google.common.base.Predicate; +import com.google.common.base.Splitter; import com.google.common.base.Strings; import com.google.common.collect.Iterables; import com.google.common.collect.LinkedListMultimap; @@ -81,6 +82,7 @@ import com.google.gerrit.server.project.RefControl; import com.google.gerrit.server.ssh.SshInfo; import com.google.gerrit.server.util.MagicBranch; import com.google.gerrit.server.util.RequestScopePropagator; +import com.google.gerrit.util.cli.CmdLineParser; import com.google.gwtorm.server.AtomicUpdate; import com.google.gwtorm.server.OrmException; import com.google.gwtorm.server.ResultSet; @@ -113,10 +115,13 @@ import org.eclipse.jgit.transport.ReceiveCommand; import org.eclipse.jgit.transport.ReceiveCommand.Result; import org.eclipse.jgit.transport.ReceivePack; import org.eclipse.jgit.transport.UploadPack; +import org.kohsuke.args4j.CmdLineException; +import org.kohsuke.args4j.Option; import org.slf4j.Logger; import org.slf4j.LoggerFactory; import java.io.IOException; +import java.io.StringWriter; import java.sql.Timestamp; import java.util.ArrayList; import java.util.Collection; @@ -234,6 +239,7 @@ public class ReceiveCommits { private final ReviewDb db; private final SchemaFactory schemaFactory; private final AccountResolver accountResolver; + private final CmdLineParser.Factory optionParserFactory; private final CreateChangeSender.Factory createChangeSenderFactory; private final MergedSender.Factory mergedSenderFactory; private final ReplacePatchSetSender.Factory replacePatchSetFactory; @@ -284,6 +290,7 @@ public class ReceiveCommits { ReceiveCommits(final ReviewDb db, final SchemaFactory schemaFactory, final AccountResolver accountResolver, + final CmdLineParser.Factory optionParserFactory, final CreateChangeSender.Factory createChangeSenderFactory, final MergedSender.Factory mergedSenderFactory, final ReplacePatchSetSender.Factory replacePatchSetFactory, @@ -312,6 +319,7 @@ public class ReceiveCommits { this.db = db; this.schemaFactory = schemaFactory; this.accountResolver = accountResolver; + this.optionParserFactory = optionParserFactory; this.createChangeSenderFactory = createChangeSenderFactory; this.mergedSenderFactory = mergedSenderFactory; this.replacePatchSetFactory = replacePatchSetFactory; @@ -971,15 +979,38 @@ public class ReceiveCommits { } private static class MagicBranchInput { + private static final Splitter COMMAS = Splitter.on(',').omitEmptyStrings(); + final ReceiveCommand cmd; Branch.NameKey dest; RefControl ctl; - String topic; Set reviewer = Sets.newLinkedHashSet(); Set cc = Sets.newLinkedHashSet(); + @Option(name = "--topic", metaVar = "NAME", usage = "attach topic to changes") + String topic; + + @Option(name = "--draft", usage = "mark new/update changes as draft") + boolean draft; + + @Option(name = "-r", metaVar = "EMAIL", usage = "add reviewer to changes") + void reviewer(Account.Id id) { + reviewer.add(id); + } + + @Option(name = "--cc", metaVar = "EMAIL", usage = "notify user by CC") + void cc(Account.Id id) { + cc.add(id); + } + + @Option(name = "--publish", usage = "publish new/updated changes") + void publish(boolean publish) { + draft = !publish; + } + MagicBranchInput(ReceiveCommand cmd) { this.cmd = cmd; + this.draft = cmd.getRefName().startsWith(MagicBranch.NEW_DRAFT_CHANGE); } boolean isRef(Ref ref) { @@ -987,66 +1018,59 @@ public class ReceiveCommits { } boolean isDraft() { - return cmd.getRefName().startsWith(MagicBranch.NEW_DRAFT_CHANGE); + return draft; } MailRecipients getMailRecipients() { return new MailRecipients(reviewer, cc); } - String parse(Repository repo, Map advertisedRefs) { - String destBranchName = MagicBranch.getDestBranchName(cmd.getRefName()); - if (!destBranchName.startsWith(Constants.R_REFS)) { - destBranchName = Constants.R_HEADS + destBranchName; + String parse(CmdLineParser clp, Repository repo, Set refs) + throws CmdLineException { + String ref = MagicBranch.getDestBranchName(cmd.getRefName()); + if (!ref.startsWith(Constants.R_REFS)) { + ref = Constants.R_HEADS + ref; } - String head; - try { - head = repo.getFullBranch(); - } catch (IOException e) { - log.error("Cannot read HEAD symref", e); - cmd.setResult(REJECTED_OTHER_REASON, "internal error"); - return null; + int optionStart = ref.indexOf('%'); + if (0 < optionStart) { + ListMultimap options = LinkedListMultimap.create(); + for (String s : COMMAS.split(ref.substring(optionStart + 1))) { + int e = s.indexOf('='); + if (0 < e) { + options.put(s.substring(0, e), s.substring(e + 1)); + } else { + options.put(s, ""); + } + } + clp.parseOptionMap(options); + ref = ref.substring(0, optionStart); } - // Split the destination branch by branch and topic. The topic + // Split the destination branch by branch and topic. The topic // suffix is entirely optional, so it might not even exist. - int split = destBranchName.length(); + String head = readHEAD(repo); + int split = ref.length(); for (;;) { - String name = destBranchName.substring(0, split); - - if (advertisedRefs.containsKey(name)) { - // We advertised the branch to the client so we know - // the branch exists. Target this branch for the upload. - break; - } else if (head.equals(name)) { - // We didn't advertise the branch, because it doesn't exist yet. - // Allow it anyway as HEAD is a symbolic reference to the name. + String name = ref.substring(0, split); + if (refs.contains(name) || name.equals(head)) { break; } split = name.lastIndexOf('/', split - 1); if (split <= Constants.R_REFS.length()) { - String n = destBranchName; - if (n.startsWith(Constants.R_HEADS)) { - n = n.substring(Constants.R_HEADS.length()); - } - cmd.setResult(REJECTED_OTHER_REASON, "branch " + n + " not found"); - return null; + return ref; } } - - if (split < destBranchName.length()) { - String t = destBranchName.substring(split + 1); - topic = Strings.emptyToNull(t); + if (split < ref.length()) { + topic = Strings.emptyToNull(ref.substring(split + 1)); } - return destBranchName.substring(0, split); + return ref.substring(0, split); } } private void parseMagicBranch(final ReceiveCommand cmd) { // Permit exactly one new change request per push. - // if (magicBranch != null) { reject(cmd, "duplicate request"); return; @@ -1056,10 +1080,32 @@ public class ReceiveCommits { magicBranch.reviewer.addAll(reviewersFromCommandLine); magicBranch.cc.addAll(ccFromCommandLine); - String ref = magicBranch.parse(repo, rp.getAdvertisedRefs()); - if (ref == null) { - // Command was already rejected, but progress needs to update. - commandProgress.update(1); + String ref; + CmdLineParser clp = optionParserFactory.create(magicBranch); + try { + ref = magicBranch.parse(clp, repo, rp.getAdvertisedRefs().keySet()); + } catch (CmdLineException e) { + if (!clp.wasHelpRequestedByOption()) { + reject(cmd, e.getMessage()); + return; + } + ref = null; // never happen + } + if (clp.wasHelpRequestedByOption()) { + StringWriter w = new StringWriter(); + w.write("\nHelp for refs/for/branch:\n\n"); + clp.printUsage(w, null); + addMessage(w.toString()); + reject(cmd, "see help"); + return; + } + if (!rp.getAdvertisedRefs().containsKey(ref) && !ref.equals(readHEAD(repo))) { + if (ref.startsWith(Constants.R_HEADS)) { + String n = ref.substring(Constants.R_HEADS.length()); + reject(cmd, "branch " + n + " not found"); + } else { + reject(cmd, ref + " not found"); + } return; } @@ -1108,6 +1154,15 @@ public class ReceiveCommits { } } + private static String readHEAD(Repository repo) { + try { + return repo.getFullBranch(); + } catch (IOException e) { + log.error("Cannot read HEAD symref", e); + return null; + } + } + /** * Loads a list of commits to reject from {@code refs/meta/reject-commits}. * diff --git a/gerrit-sshd/src/main/java/com/google/gerrit/sshd/SshModule.java b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/SshModule.java index 7e76b466bd..6fad42bcda 100644 --- a/gerrit-sshd/src/main/java/com/google/gerrit/sshd/SshModule.java +++ b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/SshModule.java @@ -25,6 +25,7 @@ import com.google.gerrit.server.RemotePeer; import com.google.gerrit.server.config.FactoryModule; import com.google.gerrit.server.config.GerritRequestModule; import com.google.gerrit.server.config.GerritServerConfig; +import com.google.gerrit.server.git.AsyncReceiveCommits; import com.google.gerrit.server.git.QueueProvider; import com.google.gerrit.server.git.WorkQueue; import com.google.gerrit.server.plugins.ModuleGenerator; @@ -65,6 +66,7 @@ public class SshModule extends FactoryModule { bind(SshScope.class).in(SINGLETON); configureRequestScope(); + install(new AsyncReceiveCommits.Module()); install(new CmdLineParserModule()); configureAliases(); diff --git a/gerrit-util-cli/src/main/java/com/google/gerrit/util/cli/CmdLineParser.java b/gerrit-util-cli/src/main/java/com/google/gerrit/util/cli/CmdLineParser.java index 4d94e6a6f0..69598ce12e 100644 --- a/gerrit-util-cli/src/main/java/com/google/gerrit/util/cli/CmdLineParser.java +++ b/gerrit-util-cli/src/main/java/com/google/gerrit/util/cli/CmdLineParser.java @@ -36,6 +36,7 @@ package com.google.gerrit.util.cli; import com.google.common.collect.LinkedHashMultimap; import com.google.common.collect.Lists; +import com.google.common.collect.Maps; import com.google.common.collect.Multimap; import com.google.inject.Inject; import com.google.inject.Injector; @@ -77,6 +78,9 @@ public class CmdLineParser { private final Injector injector; private final MyParser parser; + @SuppressWarnings("rawtypes") + private Map options; + /** * Creates a new command line owner that parses arguments/options and set them * into the given object. @@ -225,16 +229,9 @@ public class CmdLineParser { throws CmdLineException { List tmp = Lists.newArrayListWithCapacity(2 * params.size()); for (final String key : params.keySet()) { - String name = key; - if (!name.startsWith("-")) { - if (name.length() == 1) { - name = "-" + name; - } else { - name = "--" + name; - } - } + String name = makeOption(key); - if (findHandler(name) instanceof BooleanOptionHandler) { + if (isBoolean(name)) { boolean on = false; for (String value : params.get(key)) { on = toBoolean(key, value); @@ -252,22 +249,44 @@ public class CmdLineParser { parser.parseArgument(tmp.toArray(new String[tmp.size()])); } + public boolean isBoolean(String name) { + return findHandler(makeOption(name)) instanceof BooleanOptionHandler; + } + + private String makeOption(String name) { + if (!name.startsWith("-")) { + if (name.length() == 1) { + name = "-" + name; + } else { + name = "--" + name; + } + } + return name; + } + @SuppressWarnings("rawtypes") private OptionHandler findHandler(String name) { - for (OptionHandler handler : parser.options) { + if (options == null) { + options = index(parser.options); + } + return options.get(name); + } + + @SuppressWarnings("rawtypes") + private static Map index(List in) { + Map m = Maps.newHashMap(); + for (OptionHandler handler : in) { if (handler.option instanceof NamedOptionDef) { NamedOptionDef def = (NamedOptionDef) handler.option; - if (name.equals(def.name())) { - return handler; - } - for (String alias : def.aliases()) { - if (name.equals(alias)) { - return handler; + if (!def.isArgument()) { + m.put(def.name(), handler); + for (String alias : def.aliases()) { + m.put(alias, handler); } } } } - return null; + return m; } private boolean toBoolean(String name, String value) throws CmdLineException {