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 {