Four seams that reported a pass over something they never looked at

docs/REFERENCE.md

@@ -350,9 +350,9 @@ stamped on it, the range about to be pushed.
 
 | guard | refuses |
 |---|---|
-| `prevent-ai-author` | a commit message carrying AI-authorship markers |
+| `prevent-ai-author` | AI-authorship markers in the message being written — and at a push, in **every commit message the push publishes** |
 | `prevent-author-mismatch` | an identity that is not your global one |
-| `prevent-unusual-unicode` | unusual characters in a commit message |
+| `prevent-unusual-unicode` | unusual characters in the same set of messages |
 | `prevent-unusual-unicode-in-files` | characters that draw nothing, in committed content **and in the paths that carry it** |
 | `no-private-repo-names` | a private repository named in a public one's message |
 | `no-private-repo-names-staged` | the same, in the lines a commit adds |

src/audit.rs

@@ -42,9 +42,8 @@
 //! still carries the caveat in its body.
 
 use std::collections::BTreeSet;
-use std::io::{Read as _, Write as _};
 use std::path::Path;
-use std::process::{Command, Stdio};
+use std::process::Command;
 
 use crate::config::{Check, Policy, Rule};
 use crate::error::{Exit, Fatal, Result};
@@ -394,83 +393,11 @@ fn forge_conversations(root: &Path) -> (Vec<Surface>, Vec<String>) {
 }
 
 /// Which of these objects git says are blobs, asked once rather than once each.
+///
+/// `git::blob_shas` holds the pumping and the exit-status refusal, because the
+/// pre-push guard asks the identical question and had the version that hangs.
 fn blob_shas(root: &Path, shas: &[String]) -> Result<BTreeSet<String>> {
-    let mut blobs = BTreeSet::new();
-    if shas.is_empty() {
-        return Ok(blobs);
-    }
-    let mut child = Command::new("git")
-        .args(["cat-file", "--batch-check=%(objectname) %(objecttype)"])
-        .current_dir(root)
-        .stdin(Stdio::piped())
-        .stdout(Stdio::piped())
-        .stderr(Stdio::null())
-        .spawn()
-        .map_err(|error| Fatal::new(format!("git cat-file: {error}")))?;
-    let mut stdin = child
-        .stdin
-        .take()
-        .ok_or_else(|| Fatal::new("git cat-file: no stdin"))?;
-    let mut stdout = child
-        .stdout
-        .take()
-        .ok_or_else(|| Fatal::new("git cat-file: no stdout"))?;
-    // Both pipes move at once, for the reason `selection::not_text_paths` gives
-    // at greater length: `--batch-check` answers each object as it reads it, at
-    // roughly fifty bytes an answer, so it fills its stdout pipe -- 64 KiB on
-    // Linux -- somewhere near the thirteen-hundredth object and stops reading
-    // stdin. A parent that writes the whole list first is then blocked on a full
-    // stdin pipe while the child is blocked on a stdout pipe nobody is draining.
-    // Every repository this audit is meant for is far past that count, so
-    // writing first is not a rare hang, it is the ordinary case.
-    let mut answered: Vec<u8> = Vec::new();
-    let written = std::thread::scope(|scope| {
-        let writer = scope.spawn(move || {
-            for sha in shas {
-                writeln!(stdin, "{sha}")?;
-            }
-            // Dropped here, and closing stdin is what tells `--batch-check` the
-            // list is finished. Without it the child waits for more input that
-            // is never coming and the read below never sees end of file.
-            drop(stdin);
-            Ok::<(), std::io::Error>(())
-        });
-        let drained = stdout.read_to_end(&mut answered);
-        // The writer's own error outranks the drain's: a child that died early
-        // shows up here as a broken pipe, and the drain merely stops.
-        writer.join().map_or_else(
-            |_| {
-                Err(std::io::Error::other(
-                    "git cat-file: writer thread panicked",
-                ))
-            },
-            |result| result.and_then(|()| drained.map(|_| ())),
-        )
-    });
-    written.map_err(|error| Fatal::new(format!("git cat-file: {error}")))?;
-    let status = child
-        .wait()
-        .map_err(|error| Fatal::new(format!("git cat-file: {error}")))?;
-    // Reported rather than swallowed, for the reason `keep_blobs` in
-    // `guard::scope` gives: no stdout means no known kinds, the filter below
-    // then keeps nothing, and a repository whose objects could not be identified
-    // would be audited as a repository with nothing in it.
-    if !status.success() {
-        return Err(Fatal::new(format!(
-            "git cat-file --batch-check exited {}: cannot tell which of {} reachable \
-             object(s) are blobs, and reading none of them would report as a clean tree",
-            status.code().unwrap_or(-1),
-            shas.len()
-        )));
-    }
-    let text = String::from_utf8_lossy(&answered);
-    for line in text.lines() {
-        let fields: Vec<&str> = line.split_whitespace().collect();
-        if let [name, "blob", ..] = fields.as_slice() {
-            blobs.insert((*name).to_owned());
-        }
-    }
-    Ok(blobs)
+    crate::git::blob_shas(root, shas)
 }
 
 /// Every blob a flip would serve, not every path HEAD still names.

src/git.rs

@@ -5,6 +5,7 @@
 //! is safe; it has established nothing, and returning an empty answer would
 //! make that look like a pass.
 
+use std::collections::BTreeSet;
 use std::path::{Path, PathBuf};
 use std::process::Command;
 
@@ -33,6 +34,102 @@ pub(crate) fn run(root: &Path, args: &[&str]) -> Result<String> {
     })
 }
 
+/// Which of these objects git says are blobs, asked once rather than once each.
+///
+/// The two pipes move at the same time, on two threads, and that is not a style
+/// preference. `--batch-check` answers each object as it reads it, at roughly
+/// fifty bytes an answer, so it fills its stdout pipe -- 64 KiB on Linux --
+/// somewhere near the thirteen-hundredth object and stops reading stdin. A
+/// parent that writes the whole list first is then blocked on a full stdin pipe
+/// while the child is blocked on a stdout pipe nobody is draining, and neither
+/// ever moves again: no output, no exit code, the push simply stops. Every
+/// repository these callers are meant for is far past that count, so writing
+/// first is not a rare hang, it is the ordinary case.
+///
+/// This lived twice, and the third caller is why it lives here instead: the
+/// audit and the selection pass each grew their own writer thread while the
+/// pre-push guard kept the version that hangs. One copy is the only shape in
+/// which that cannot happen again.
+pub(crate) fn blob_shas(root: &Path, shas: &[String]) -> Result<BTreeSet<String>> {
+    use std::io::{Read, Write};
+    use std::process::Stdio;
+
+    let mut blobs = BTreeSet::new();
+    if shas.is_empty() {
+        return Ok(blobs);
+    }
+
+    let mut child = Command::new("git")
+        .args(["cat-file", "--batch-check=%(objectname) %(objecttype)"])
+        .current_dir(root)
+        .stdin(Stdio::piped())
+        .stdout(Stdio::piped())
+        .stderr(Stdio::null())
+        .spawn()
+        .map_err(|error| Fatal::new(format!("git cat-file: {error}")))?;
+    let mut stdin = child
+        .stdin
+        .take()
+        .ok_or_else(|| Fatal::new("git cat-file: no stdin"))?;
+    let mut stdout = child
+        .stdout
+        .take()
+        .ok_or_else(|| Fatal::new("git cat-file: no stdout"))?;
+
+    let mut answered: Vec<u8> = Vec::new();
+    let written = std::thread::scope(|scope| {
+        let writer = scope.spawn(move || {
+            for sha in shas {
+                writeln!(stdin, "{sha}")?;
+            }
+            // Dropped here, and closing stdin is what tells `--batch-check` the
+            // list is finished. Without it the child waits for more input that
+            // is never coming and the read below never sees end of file.
+            drop(stdin);
+            Ok::<(), std::io::Error>(())
+        });
+        let drained = stdout.read_to_end(&mut answered);
+        // The writer's own error outranks the drain's: a child that died early
+        // shows up here as a broken pipe, and the drain merely stops.
+        writer.join().map_or_else(
+            |_| {
+                Err(std::io::Error::other(
+                    "git cat-file: writer thread panicked",
+                ))
+            },
+            |result| result.and_then(|()| drained.map(|_| ())),
+        )
+    });
+    written.map_err(|error| Fatal::new(format!("git cat-file: {error}")))?;
+
+    let status = child
+        .wait()
+        .map_err(|error| Fatal::new(format!("git cat-file: {error}")))?;
+    // Reported rather than swallowed: no stdout means no known kinds, every
+    // caller's filter then keeps nothing, and a set of objects that could not be
+    // identified would read as a set with no blobs in it.
+    //
+    // A missing object is not this case. `--batch-check` writes "<sha> missing"
+    // and still exits 0, so a non-zero status means git itself could not run.
+    if !status.success() {
+        return Err(Fatal::new(format!(
+            "git cat-file --batch-check exited {}: cannot tell which of {} object(s) \
+             are blobs, and reporting none of them would read as nothing to check",
+            status.code().unwrap_or(-1),
+            shas.len()
+        )));
+    }
+
+    let text = String::from_utf8_lossy(&answered);
+    for line in text.lines() {
+        let fields: Vec<&str> = line.split_whitespace().collect();
+        if let [name, "blob", ..] = fields.as_slice() {
+            blobs.insert((*name).to_owned());
+        }
+    }
+    Ok(blobs)
+}
+
 /// NUL-separated output, for the paths git will not quote.
 pub(crate) fn run_z(root: &Path, args: &[&str]) -> Result<Vec<String>> {
     Ok(run(root, args)?
@@ -135,4 +232,80 @@ mod tests {
             ("Ada Lovelace".to_owned(), "ada@example.test".to_owned())
         );
     }
+
+    #[test]
+    fn several_thousand_objects_are_asked_about_without_deadlocking() {
+        // The proof for the pipe. `--batch-check` answers each object as it
+        // reads it, at roughly fifty bytes an answer, so 4000 objects is 160 KiB
+        // of stdin and 200 KiB back -- several times over the 64 KiB a pipe
+        // holds in each direction. A caller that wrote the whole list before
+        // reading a byte stopped somewhere past the fifteen-hundredth and never
+        // came back, which is what `guard::scope::keep_blobs` did to every push
+        // of a range this size.
+        //
+        // On a thread with a deadline, because a test that proves a deadlock is
+        // gone has to FAIL when it is not, and a test that hangs reports nothing
+        // at all -- it stops the suite with no failing test named.
+        let root = std::env::temp_dir().join(format!("uphold-git-batch-{}", std::process::id()));
+        std::fs::remove_dir_all(&root).ok();
+        std::fs::create_dir_all(&root).unwrap();
+        for args in [
+            &["init", "-q", "-b", "main"][..],
+            &["config", "user.name", "Test"][..],
+            &["config", "user.email", "test@example.test"][..],
+        ] {
+            let status = Command::new("git")
+                .args(args)
+                .current_dir(&root)
+                .stdout(std::process::Stdio::null())
+                .stderr(std::process::Stdio::null())
+                .status()
+                .unwrap();
+            assert!(status.success(), "git {args:?} failed");
+        }
+
+        // Written and staged in one `git add`, because the point of the test is
+        // the pipe and not the fixture: 4000 `hash-object` processes cost a
+        // minute of suite time to produce the same 4000 shas.
+        for index in 0..4000_u32 {
+            std::fs::write(
+                root.join(format!("blob-{index:05}.txt")),
+                format!("blob number {index}\n"),
+            )
+            .unwrap();
+        }
+        let status = Command::new("git")
+            .args(["add", "-A", "."])
+            .current_dir(&root)
+            .stdout(std::process::Stdio::null())
+            .stderr(std::process::Stdio::null())
+            .status()
+            .unwrap();
+        assert!(status.success(), "git add failed");
+        let staged = run(&root, &["ls-files", "-s"]).unwrap();
+        let shas: Vec<String> = staged
+            .lines()
+            .filter_map(|line| line.split_whitespace().nth(1).map(str::to_owned))
+            .collect();
+        assert_eq!(shas.len(), 4000, "the fixture did not stage");
+
+        let (sender, receiver) = std::sync::mpsc::channel();
+        let asked = root.clone();
+        let listed = shas.clone();
+        std::thread::spawn(move || {
+            sender.send(blob_shas(&asked, &listed)).ok();
+        });
+        let blobs = receiver
+            .recv_timeout(std::time::Duration::from_secs(60))
+            .expect("`git cat-file --batch-check` did not answer: the pipes deadlocked")
+            .unwrap();
+
+        assert_eq!(blobs.len(), shas.len());
+        std::fs::remove_dir_all(&root).ok();
+    }
+
+    #[test]
+    fn an_empty_list_asks_git_nothing() {
+        assert!(blob_shas(&std::env::temp_dir(), &[]).unwrap().is_empty());
+    }
 }

src/guard/message.rs

@@ -53,6 +53,37 @@ fn message_text(request: &Request<'_>) -> Result<(PathBuf, String)> {
     Ok((path, String::from_utf8_lossy(&bytes).into_owned()))
 }
 
+/// Every message this run is actually about, labelled.
+///
+/// At `pre-push` that is the messages of the commits being published, and NOT
+/// `.git/COMMIT_EDITMSG`. Reading the fallback there is the same mistake the
+/// paragraph above `message_text` describes, arriving by the other door: the
+/// file exists, it holds whatever the last `git commit` wrote, and it is clean
+/// -- so a push carrying a marker in a commit made by `git commit-tree`, a
+/// rebase, a cherry-pick, `git am`, `--no-verify`, or a fast-forward out of a
+/// hookless clone was reported as one guard passed, exit 0.
+///
+/// `no-private-repo-names` already reads the pushed range for exactly this
+/// reason; these two guards were the ones left asking the wrong file.
+fn message_subjects(request: &Request<'_>) -> Result<Vec<(String, String)>> {
+    if request.stage == super::Stage::PrePush {
+        return Ok(super::scope::pushed_messages(
+            request.root,
+            request.stage,
+            request.push_refs,
+            request.push_source,
+        )?
+        .into_iter()
+        .map(|(sha, body)| {
+            let short: String = sha.chars().take(12).collect();
+            (format!("commit {short} (its MESSAGE)"), body)
+        })
+        .collect());
+    }
+    let (path, text) = message_text(request)?;
+    Ok(vec![(path.display().to_string(), text)])
+}
+
 /// The judgment, over text that may never have been a file.
 pub(crate) fn ai_author_in(rule: &crate::config::Rule, label: &str, text: &str) -> Option<Refusal> {
     let mut found: Vec<&str> = Vec::new();
@@ -129,12 +160,12 @@ fn unusual_findings(label: &str, text: &str) -> Vec<String> {
 }
 
 pub(crate) fn prevent_ai_author(request: &Request<'_>) -> Result<Option<Refusal>> {
-    let (path, text) = message_text(request)?;
-    Ok(ai_author_in(
-        request.rule,
-        &path.display().to_string(),
-        &text,
-    ))
+    for (label, text) in message_subjects(request)? {
+        if let Some(refusal) = ai_author_in(request.rule, &label, &text) {
+            return Ok(Some(refusal));
+        }
+    }
+    Ok(None)
 }
 
 /// Characters refused in a commit message.
@@ -163,10 +194,10 @@ fn message_character_is_ordinary(character: char) -> bool {
 }
 
 pub(crate) fn prevent_unusual_unicode(request: &Request<'_>) -> Result<Option<Refusal>> {
-    let (path, text) = message_text(request)?;
-    Ok(unusual_unicode_in(
-        request.rule,
-        &path.display().to_string(),
-        &text,
-    ))
+    for (label, text) in message_subjects(request)? {
+        if let Some(refusal) = unusual_unicode_in(request.rule, &label, &text) {
+            return Ok(Some(refusal));
+        }
+    }
+    Ok(None)
 }