From 1c57530e2fb8a319c64c353f468e0be2d712b05a Mon Sep 17 00:00:00 2001
From: Ben Boeckel <ben.boeckel@kitware.com>
Date: Fri, 8 Sep 2017 17:56:00 -0400
Subject: [PATCH 1/8] handlers/commands: remove unnecessary argument

---
 src/handlers/common/handlers/commands.rs       | 6 +++---
 src/handlers/common/handlers/merge_requests.rs | 2 +-
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/src/handlers/common/handlers/commands.rs b/src/handlers/common/handlers/commands.rs
index 640880d8..1c634a8e 100644
--- a/src/handlers/common/handlers/commands.rs
+++ b/src/handlers/common/handlers/commands.rs
@@ -179,7 +179,7 @@ impl<'a> Commands<'a> {
     }
 
     /// Error messages from the commands.
-    pub fn error_messages(&self, author: &User, branch: &Branch) -> Vec<String> {
+    pub fn error_messages(&self, author: &User) -> Vec<String> {
         let mut messages = vec![];
 
         if !self.unrecognized.is_empty() {
@@ -192,7 +192,7 @@ impl<'a> Commands<'a> {
         if !unavailable.is_empty() {
             messages.push(format!("@{}: the following commands are not supported for `{}`: `{}`.",
                                   author.handle,
-                                  branch.name,
+                                  self.branch.name,
                                   unavailable.join("`, `")));
         }
 
@@ -200,7 +200,7 @@ impl<'a> Commands<'a> {
         if !disallowed.is_empty() {
             messages.push(format!("@{}: insufficient privileges for the commands for `{}`: `{}`.",
                                   author.handle,
-                                  branch.name,
+                                  self.branch.name,
                                   disallowed.join("`, `")));
         }
 
diff --git a/src/handlers/common/handlers/merge_requests.rs b/src/handlers/common/handlers/merge_requests.rs
index d4f9505e..42eba072 100644
--- a/src/handlers/common/handlers/merge_requests.rs
+++ b/src/handlers/common/handlers/merge_requests.rs
@@ -432,7 +432,7 @@ pub fn handle_merge_request_note(data: &Value, host: &Host, mr_note: MergeReques
                                                            mr.merge_request.url))));
     }
 
-    let errors = commands.error_messages(&note.author, branch);
+    let errors = commands.error_messages(&note.author);
     let defer = if errors.is_empty() {
         handle_commands(data,
                         host,
-- 
GitLab


From 0f5815c99df9555332b6e3c78037e8bc29155a39 Mon Sep 17 00:00:00 2001
From: Ben Boeckel <ben.boeckel@kitware.com>
Date: Fri, 8 Sep 2017 18:27:29 -0400
Subject: [PATCH 2/8] nits: use Vec::new() rather than vec![]

---
 src/config/types.rs                            | 4 ++--
 src/handlers/common/handlers/commands.rs       | 8 ++++----
 src/handlers/common/handlers/merge_requests.rs | 6 +++---
 src/handlers/common/handlers/push.rs           | 2 +-
 src/main.rs                                    | 2 +-
 5 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/src/config/types.rs b/src/config/types.rs
index a127865c..85c0fb04 100644
--- a/src/config/types.rs
+++ b/src/config/types.rs
@@ -412,13 +412,13 @@ impl merge::MergePolicy for WorkflowMergePolicy {
     type Filter = WorkflowMergePolicyFilter;
 
     fn for_mr(&self, _: &MergeRequest) -> Self::Filter {
-        let mut trailers = vec![];
+        let mut trailers = Vec::new();
 
         trailers.push(Trailer::new("Acked-by", format!("{}", self.identity)));
 
         WorkflowMergePolicyFilter {
             trailers: trailers,
-            errors: vec![],
+            errors: Vec::new(),
         }
     }
 }
diff --git a/src/handlers/common/handlers/commands.rs b/src/handlers/common/handlers/commands.rs
index 1c634a8e..8e3822d9 100644
--- a/src/handlers/common/handlers/commands.rs
+++ b/src/handlers/common/handlers/commands.rs
@@ -139,7 +139,7 @@ macro_rules! check_actions {
 
 macro_rules! check_action_ok {
     { $method:ident, $( $action:expr => $name:expr, )* } => {
-        let mut results = vec![];
+        let mut results = Vec::new();
 
         $( if $action.$method() {
             results.push($name);
@@ -151,7 +151,7 @@ macro_rules! check_action_ok {
 
 macro_rules! check_action_consistency {
     { $( ($action1:expr, $action2:expr) => ($name1:expr, $name2:expr), )* } => {
-        let mut results = vec![];
+        let mut results = Vec::new();
 
         $( if $action1.requested() && $action2.requested() {
             results.push(concat!($name1, "` and `", $name2));
@@ -174,13 +174,13 @@ impl<'a> Commands<'a> {
             test: CommandState::Unrequested,
             reformat: CommandState::Unrequested,
 
-            unrecognized: vec![],
+            unrecognized: Vec::new(),
         }
     }
 
     /// Error messages from the commands.
     pub fn error_messages(&self, author: &User) -> Vec<String> {
-        let mut messages = vec![];
+        let mut messages = Vec::new();
 
         if !self.unrecognized.is_empty() {
             messages.push(format!("@{}: the following commands are not recognized at all: `{}`.",
diff --git a/src/handlers/common/handlers/merge_requests.rs b/src/handlers/common/handlers/merge_requests.rs
index 42eba072..f6eaa3ba 100644
--- a/src/handlers/common/handlers/merge_requests.rs
+++ b/src/handlers/common/handlers/merge_requests.rs
@@ -35,7 +35,7 @@ pub fn handle_merge_request_update(data: &Value, host: &Host, mr: MergeRequestIn
                                                           &mr.merge_request.target_repo.name,
                                                           &mr.merge_request.target_branch));
 
-    let mut comment_notes = vec![];
+    let mut comment_notes = Vec::new();
 
     if let Some(test_action) = branch.test() {
         handle_test_on_update(data, test_action, &mr);
@@ -423,7 +423,7 @@ pub fn handle_merge_request_note(data: &Value, host: &Host, mr_note: MergeReques
         return Ok(HandlerResult::Reject("no commands present".to_string()));
     }
 
-    let mut comment_notes = vec![];
+    let mut comment_notes = Vec::new();
 
     let fetch_res = host.service.fetch_mr(&project.context, &mr.merge_request);
     if let Err(err) = fetch_res {
@@ -668,7 +668,7 @@ fn handle_test_command(data: &mut CommandData, action: &TestAction, arguments: &
                 ("arguments".to_string(), args),
                 ("merge_request".to_string(), data.data.clone())
             ])));
-            (vec![],
+            (Vec::new(),
              Some(test_jobs.test_mr(&mr.merge_request, job_data)
                  .map_err(Into::into)))
         },
diff --git a/src/handlers/common/handlers/push.rs b/src/handlers/common/handlers/push.rs
index 1b6df98d..6c641272 100644
--- a/src/handlers/common/handlers/push.rs
+++ b/src/handlers/common/handlers/push.rs
@@ -79,7 +79,7 @@ pub fn handle_push(data: &Value, host: &Host, push: PushInfo) -> JobResult<Handl
         }
     }
 
-    let mut comment_notes = vec![];
+    let mut comment_notes = Vec::new();
 
     let fetch_res = project.context.force_fetch_into("origin", refname, refname);
     if let Err(err) = fetch_res {
diff --git a/src/main.rs b/src/main.rs
index 6f8c864e..1e3233dc 100644
--- a/src/main.rs
+++ b/src/main.rs
@@ -75,7 +75,7 @@ fn run_director(config_path: &Path) -> Result<RunResult> {
     let mut config = Config::from_path(&config_path, handlers::connect_to_host)?;
 
     let watchdog = DirectorWatchdog {};
-    let mut handlers = vec![];
+    let mut handlers = Vec::new();
 
     for (name, host) in config.hosts.drain() {
         handlers.push(handlers::create_handler(host, &name)?)
-- 
GitLab


From 5b742639419a15eb3fc56331fe0b624d10941b72 Mon Sep 17 00:00:00 2001
From: Ben Boeckel <ben.boeckel@kitware.com>
Date: Fri, 8 Sep 2017 18:27:46 -0400
Subject: [PATCH 3/8] gitlab: simplify the token extraction

---
 src/handlers/gitlab/handler.rs | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/src/handlers/gitlab/handler.rs b/src/handlers/gitlab/handler.rs
index ae6a9358..8553c228 100644
--- a/src/handlers/gitlab/handler.rs
+++ b/src/handlers/gitlab/handler.rs
@@ -27,9 +27,10 @@ use std::sync::Arc;
 /// Connect to a Gitlab from its configuration block.
 pub fn connect_to_host(url: &Option<String>, secrets: Value) -> Result<Arc<HostingService>> {
     let host = url.as_ref().map_or("gitlab.com", |u| u.as_str());
-    let token = match secrets.pointer("/token").and_then(|t| t.as_str()) {
-        Some(t) => t,
-        None => return Err("gitlab requires a token to be provided".into()),
+    let token = if let Some(token) = secrets.pointer("/token").and_then(|t| t.as_str()) {
+        token
+    } else {
+        bail!("gitlab requires a token to be provided");
     };
     let use_ssl = secrets.pointer("/use_ssl").and_then(Value::as_bool).unwrap_or(true);
 
-- 
GitLab


From 273f355d0358582430db94f6334a155f7fbcc38e Mon Sep 17 00:00:00 2001
From: Ben Boeckel <ben.boeckel@kitware.com>
Date: Fri, 8 Sep 2017 18:24:00 -0400
Subject: [PATCH 4/8] commands: make unrecognized private

There's no need to have it be public.
---
 src/handlers/common/handlers/commands.rs | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/handlers/common/handlers/commands.rs b/src/handlers/common/handlers/commands.rs
index 8e3822d9..9ea06584 100644
--- a/src/handlers/common/handlers/commands.rs
+++ b/src/handlers/common/handlers/commands.rs
@@ -99,7 +99,7 @@ pub struct Commands<'a> {
     pub reformat: CommandState,
 
     /// List of unrecognized commands.
-    pub unrecognized: Vec<String>,
+    unrecognized: Vec<String>,
 }
 
 macro_rules! check_action {
-- 
GitLab


From a261aca0fb13c3de71e4742d1c3ed5002daf7876 Mon Sep 17 00:00:00 2001
From: Ben Boeckel <ben.boeckel@kitware.com>
Date: Sat, 9 Sep 2017 22:34:38 -0400
Subject: [PATCH 5/8] merge_requests: use the json! macro

---
 src/handlers/common/handlers/merge_requests.rs | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/src/handlers/common/handlers/merge_requests.rs b/src/handlers/common/handlers/merge_requests.rs
index f6eaa3ba..b8134262 100644
--- a/src/handlers/common/handlers/merge_requests.rs
+++ b/src/handlers/common/handlers/merge_requests.rs
@@ -17,7 +17,7 @@ use crates::git_workarea::{CommitId, GitContext};
 use crates::itertools::Itertools;
 use crates::json_job_dispatch::HandlerResult;
 use crates::json_job_dispatch::Result as JobResult;
-use crates::serde_json::{Map, Value};
+use crates::serde_json::Value;
 
 use config::{Branch, Host, CheckAction, MergeAction, Project, ReformatAction, StageAction,
              StageUpdatePolicy, TestAction, TestBackend};
@@ -26,7 +26,7 @@ use handlers::common::handlers::commands::Commands;
 use handlers::common::handlers::error::Error;
 use handlers::common::handlers::utils;
 
-use std::iter::{self, FromIterator};
+use std::iter;
 
 /// Handle an update to a merge request.
 pub fn handle_merge_request_update(data: &Value, host: &Host, mr: MergeRequestInfo)
@@ -664,10 +664,10 @@ fn handle_test_command(data: &mut CommandData, action: &TestAction, arguments: &
                 .map(|arg| Value::String(arg.clone()))
                 .collect());
             let job_data = utils::test_job("test_action",
-                                           &Value::Object(Map::from_iter(vec![
-                ("arguments".to_string(), args),
-                ("merge_request".to_string(), data.data.clone())
-            ])));
+                                           &json!({
+                "arguments": args,
+                "merge_request": data.data.clone(),
+            }));
             (Vec::new(),
              Some(test_jobs.test_mr(&mr.merge_request, job_data)
                  .map_err(Into::into)))
-- 
GitLab


From 2ede23cc43ac2a8afcec35cea39796b8db4e9f7a Mon Sep 17 00:00:00 2001
From: Ben Boeckel <ben.boeckel@kitware.com>
Date: Sat, 9 Sep 2017 22:34:54 -0400
Subject: [PATCH 6/8] merge_requests: fix a typo in an error message

---
 src/handlers/common/handlers/merge_requests.rs | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/handlers/common/handlers/merge_requests.rs b/src/handlers/common/handlers/merge_requests.rs
index b8134262..32203823 100644
--- a/src/handlers/common/handlers/merge_requests.rs
+++ b/src/handlers/common/handlers/merge_requests.rs
@@ -338,7 +338,7 @@ fn check_mr(project: &Project, branch: &Branch, mr: &MergeRequestInfo) -> check:
                 Ok(commit) => commit,
                 Err(err) => {
                     error!(target: "ghostflow-director/handler",
-                           "failed to determin ethe commit to backport into {}: {:?}",
+                           "failed to determine the commit to backport into {}: {:?}",
                            backport.branch(),
                            err);
 
-- 
GitLab


From bc28dbe1fbbd26d9e221c2df306f3b07d225086c Mon Sep 17 00:00:00 2001
From: Ben Boeckel <ben.boeckel@kitware.com>
Date: Sat, 9 Sep 2017 22:35:17 -0400
Subject: [PATCH 7/8] merge_requests: mention that the backport is what's wrong

---
 src/handlers/common/handlers/merge_requests.rs | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/handlers/common/handlers/merge_requests.rs b/src/handlers/common/handlers/merge_requests.rs
index 32203823..b1a8ec3d 100644
--- a/src/handlers/common/handlers/merge_requests.rs
+++ b/src/handlers/common/handlers/merge_requests.rs
@@ -356,7 +356,7 @@ fn check_mr(project: &Project, branch: &Branch, mr: &MergeRequestInfo) -> check:
         .map(|results| results.into_iter().all(|status| status == check::CheckStatus::Pass));
 
     if !comment_notes.is_empty() {
-        let comment = format!("The branch configuration is not valid:\n\
+        let comment = format!("The backport configuration is not valid:\n\
                                \n  - {}",
                               comment_notes.into_iter().join("\n  - "));
         if let Err(err) = project.service.post_mr_comment(&mr.merge_request, &comment) {
-- 
GitLab


From ddf5cb38f10bb5321a4a99a6ba92467bdcf639f1 Mon Sep 17 00:00:00 2001
From: Ben Boeckel <ben.boeckel@kitware.com>
Date: Sat, 9 Sep 2017 22:35:39 -0400
Subject: [PATCH 8/8] merge_requests: use clap for `Do: test` argument parsing

---
 .../common/handlers/merge_requests.rs         | 68 ++++++++-----------
 1 file changed, 27 insertions(+), 41 deletions(-)

diff --git a/src/handlers/common/handlers/merge_requests.rs b/src/handlers/common/handlers/merge_requests.rs
index b1a8ec3d..d5f48ce7 100644
--- a/src/handlers/common/handlers/merge_requests.rs
+++ b/src/handlers/common/handlers/merge_requests.rs
@@ -658,7 +658,7 @@ fn handle_test_command(data: &mut CommandData, action: &TestAction, arguments: &
     let mr = &data.mr_note.merge_request;
     let test = action.test();
 
-    let (arguments, res): (Vec<String>, Option<test::Result<_>>) = match *test {
+    let res: test::Result<_> = match *test {
         TestBackend::Jobs(ref test_jobs) => {
             let args = Value::Array(arguments.iter()
                 .map(|arg| Value::String(arg.clone()))
@@ -668,54 +668,40 @@ fn handle_test_command(data: &mut CommandData, action: &TestAction, arguments: &
                 "arguments": args,
                 "merge_request": data.data.clone(),
             }));
-            (Vec::new(),
-             Some(test_jobs.test_mr(&mr.merge_request, job_data)
-                 .map_err(Into::into)))
+            test_jobs.test_mr(&mr.merge_request, job_data)
+                .map_err(Into::into)
         },
         TestBackend::Refs(ref test_refs) => {
-            let mut stop = false;
-
-            let arguments = arguments.iter()
-                .filter_map(|arg| {
-                    if arg == "--stop" {
-                        stop = true;
-                        None
+            let matches = command_app("test-action")
+                .arg(Arg::with_name("STOP")
+                    .long("stop")
+                    .takes_value(false))
+                .get_matches_from_safe(arguments);
+
+            match matches {
+                Ok(matches) => {
+                    let res = if matches.is_present("STOP") {
+                        test_refs.untest_mr(&mr.merge_request)
                     } else {
-                        Some(arg.to_string())
-                    }
-                })
-                .collect::<Vec<_>>();
+                        test_refs.test_mr(&mr.merge_request)
+                    };
 
-            let res = if arguments.is_empty() {
-                let res = if stop {
-                    test_refs.untest_mr(&mr.merge_request)
-                } else {
-                    test_refs.test_mr(&mr.merge_request)
-                };
-
-                Some(res.map_err(Into::into))
-            } else {
-                None
-            };
+                    res.map_err(Into::into)
+                },
+                Err(err) => {
+                    data.notes.push(format!("Unrecognized `merge` arguments: `{}`.",
+                                            err.message.lines().next().unwrap()));
 
-            (arguments, res)
+                    Ok(())
+                },
+            }
         },
     };
 
-    if arguments.is_empty() != res.is_some() {
-        error!(target: "ghostflow-director/handler",
-               "a test action occurred even though some arguments were unparsed");
-    }
-
-    if arguments.is_empty() {
-        if let Some(Err(err)) = res {
-            data.notes.push(format!("Error occurred during test action ({}): `{:?}`",
-                                    data.host.maintainers.join(" "),
-                                    err));
-        }
-    } else {
-        data.notes.push(format!("Unrecognized `test` arguments: `{}`.",
-                                arguments.iter().join("`, `")));
+    if let Err(err) = res {
+        data.notes.push(format!("Error occurred during test action ({}): `{:?}`",
+                                data.host.maintainers.join(" "),
+                                err));
     }
 
     CommandResult::Continue
-- 
GitLab