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

@@ -32,28 +32,12 @@ OPTIONS
 
 --reviewer <address>::
 --re <address>::
-	Automatically add <address> as a reviewer to any change
+	Automatically add <address> as a reviewer to any change.
-	created or updated by the pushed commit objects.  These
+	Deprecated, use `refs/for/branch%r=address` instead.
-	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.
 
 --cc <address>::
-	Carbon-copy <address> on the created or updated changes,
+	Carbon-copy <address> on the created or updated changes.
-	but don't request them to perform a review.  Like with
+	Deprecated, use `refs/for/branch%cc=address` instead.
-	--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 <address> 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).
 
 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.push HEAD:refs/for/master%r=charlie@example.com,cc=alice@example.com,cc=bob@example.com
-	git config remote.charlie.receivepack 'git receive-pack --reviewer 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
+  push = HEAD:refs/for/master%r=charlie@example.com,cc=alice@example.com,cc=bob@example.com
-  receivepack = git receive-pack --reviewer charlie@example.com --cc alice@example.com --cc bob@example.com
 ----
 
 and now sending a new change for review to charlie, CC'ing both

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
+The `r='email'` and `cc='email'` options may be specified as many
-specified as many times as necessary to cover all interested
+times as necessary to cover all interested parties. Gerrit will
-parties.  Gerrit will automatically avoid sending duplicate email
+automatically avoid sending duplicate email notifications, such as
-notifications, such as if one of the specified reviewers or CC
+if one of the specified reviewers or CC addresses had also requested
-addresses had also requested to receive all new change notifications.
+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%r=a@a.com,cc=b@o.com
-    push = HEAD:refs/for/experimental
 
-  $ git push for-a-exp
+  $ git push exp
 ====
 
 

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);
 

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.

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<ReviewDb> 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<ReviewDb> 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<Account.Id> reviewer = Sets.newLinkedHashSet();
     Set<Account.Id> 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<String, Ref> advertisedRefs) {
+    String parse(CmdLineParser clp, Repository repo, Set<String> refs)
-      String destBranchName = MagicBranch.getDestBranchName(cmd.getRefName());
+        throws CmdLineException {
-      if (!destBranchName.startsWith(Constants.R_REFS)) {
+      String ref = MagicBranch.getDestBranchName(cmd.getRefName());
-        destBranchName = Constants.R_HEADS + destBranchName;
+      if (!ref.startsWith(Constants.R_REFS)) {
+        ref = Constants.R_HEADS + ref;
       }
 
-      String head;
+      int optionStart = ref.indexOf('%');
-      try {
+      if (0 < optionStart) {
-        head = repo.getFullBranch();
+        ListMultimap<String, String> options = LinkedListMultimap.create();
-      } catch (IOException e) {
+        for (String s : COMMAS.split(ref.substring(optionStart + 1))) {
-        log.error("Cannot read HEAD symref", e);
+          int e = s.indexOf('=');
-        cmd.setResult(REJECTED_OTHER_REASON, "internal error");
+          if (0 < e) {
-        return null;
+            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);
+        String name = ref.substring(0, split);
-
+        if (refs.contains(name) || name.equals(head)) {
-        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.
           break;
         }
 
         split = name.lastIndexOf('/', split - 1);
         if (split <= Constants.R_REFS.length()) {
-          String n = destBranchName;
+          return ref;
-          if (n.startsWith(Constants.R_HEADS)) {
-            n = n.substring(Constants.R_HEADS.length());
-          }
-          cmd.setResult(REJECTED_OTHER_REASON, "branch " + n + " not found");
-          return null;
         }
       }
-
+      if (split < ref.length()) {
-      if (split < destBranchName.length()) {
+        topic = Strings.emptyToNull(ref.substring(split + 1));
-        String t = destBranchName.substring(split + 1);
-        topic = Strings.emptyToNull(t);
       }
-      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());
+    String ref;
-    if (ref == null) {
+    CmdLineParser clp = optionParserFactory.create(magicBranch);
-      // Command was already rejected, but progress needs to update.
+    try {
-      commandProgress.update(1);
+      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}.
    *

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();
 

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<String, OptionHandler> 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<String> tmp = Lists.newArrayListWithCapacity(2 * params.size());
     for (final String key : params.keySet()) {
-      String name = key;
+      String name = makeOption(key);
-      if (!name.startsWith("-")) {
-        if (name.length() == 1) {
-          name = "-" + name;
-        } else {
-          name = "--" + name;
-        }
-      }
 
-      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<String, OptionHandler> index(List<OptionHandler> in) {
+    Map<String, OptionHandler> m = Maps.newHashMap();
+    for (OptionHandler handler : in) {
       if (handler.option instanceof NamedOptionDef) {
         NamedOptionDef def = (NamedOptionDef) handler.option;
-        if (name.equals(def.name())) {
+        if (!def.isArgument()) {
-          return handler;
+          m.put(def.name(), handler);
-        }
+          for (String alias : def.aliases()) {
-        for (String alias : def.aliases()) {
+            m.put(alias, handler);
-          if (name.equals(alias)) {
-            return handler;
           }
         }
       }
     }
-    return null;
+    return m;
   }
 
   private boolean toBoolean(String name, String value) throws CmdLineException {