Merge topic 'bad-commits-impls'

7347e7f6 Merge branch 'bad-commits-impls-3.9.1' into bad-commits-impls
ae0a91c9 bad_commits: add a more efficient TopicCheck impl
cec0f603 bad_commits: add a more efficient TopicCheck impl
Acked-by: Kitware Robot <kwrobot@kitware.com>
Merge-request: !135

git-checks/CHANGELOG.md

@@ -27,6 +27,13 @@
     the mode on a changelog file is no longer sufficient.
   * The `InvalidPaths` check now supports enforcing Windows filename rules.
 
+# v3.9.1
+
+## Updated checks
+
+  * The `BadCommits` check now has a `TopicCheck` implementation which is
+    equivalent, but more efficient, than its `Check` implementation.
+
 # v3.9.0
 
 ## Updated checks

git-checks/src/bad_commits.rs

@@ -49,6 +49,43 @@ impl Check for BadCommits {
     }
 }
 
+impl TopicCheck for BadCommits {
+    fn name(&self) -> &str {
+        "bad-commits-topic"
+    }
+
+    fn check(&self, ctx: &CheckGitContext, topic: &Topic) -> Result<CheckResult> {
+        let rev_list = ctx.git()
+            .arg("rev-list")
+            .arg("--reverse")
+            .arg("--topo-order")
+            .arg(&topic.sha1.as_str())
+            .arg(&format!("^{}", topic.base))
+            .output()
+            .chain_err(|| "failed to construct rev-list command")?;
+        if !rev_list.status.success() {
+            bail!(ErrorKind::Git(format!("failed to list all topic refs: {}",
+                                         String::from_utf8_lossy(&rev_list.stderr))));
+        }
+
+        let refs = String::from_utf8_lossy(&rev_list.stdout);
+
+        Ok(refs.lines()
+           .map(CommitId::new)
+           .fold(CheckResult::new(), |mut result, commit| {
+                if self.bad_commits.contains(&commit) {
+                    result.add_error(format!("commit {} is a known-bad commit that was removed from the \
+                                              server.",
+                                             commit))
+                        .add_alert(format!("commit {} was pushed to the server.", commit),
+                                   true);
+                }
+
+                result
+           }))
+    }
+}
+
 #[cfg(test)]
 mod tests {
     use BadCommits;
@@ -104,4 +141,51 @@ mod tests {
         assert_eq!(result.allowed(), false);
         assert_eq!(result.pass(), false);
     }
+
+    #[test]
+    fn test_bad_commits_topic_good_commit() {
+        let check = BadCommits::new(&[
+            BAD_COMMIT,
+        ]);
+        run_topic_check_ok("test_bad_commits_topic_good_commit", GOOD_COMMIT, check);
+    }
+
+    #[test]
+    fn test_bad_commits_topic_no_bad_commit() {
+        let check = BadCommits::new(&[
+            // This commit should never exist.
+            "0000000000000000000000000000000000000000",
+        ]);
+        run_topic_check_ok("test_bad_commits_topic_no_bad_commit", BAD_TOPIC, check);
+    }
+
+    #[test]
+    fn test_bad_commits_topic_already_in_history() {
+        let check = BadCommits::new(&[
+            // This commit is in the shared history.
+            FILLER_COMMIT,
+        ]);
+        run_topic_check_ok("test_bad_commits_topic_already_in_history", BAD_TOPIC, check);
+    }
+
+    #[test]
+    fn test_bad_commits_topic_not_already_in_history() {
+        let check = BadCommits::new(&[
+            // This commit is on the topic being brought in.
+            BAD_COMMIT,
+        ]);
+        let result = run_topic_check("test_bad_commits_topic_not_already_in_history", BAD_TOPIC, check);
+
+        assert_eq!(result.warnings().len(), 0);
+        assert_eq!(result.alerts().len(), 1);
+        assert_eq!(result.alerts()[0],
+                   "commit 029a00428913ee915ce5ee7250c023abfbc2aca3 was pushed to the server.");
+        assert_eq!(result.errors().len(), 1);
+        assert_eq!(result.errors()[0],
+                   "commit 029a00428913ee915ce5ee7250c023abfbc2aca3 is a known-bad commit that \
+                    was removed from the server.");
+        assert_eq!(result.temporary(), false);
+        assert_eq!(result.allowed(), false);
+        assert_eq!(result.pass(), false);
+    }
 }