Add check-shebang-scripts-are-executable builtin hook (#1847)

Author: j178

crates/prek/src/hooks/builtin_hooks/mod.rs

@@ -35,6 +35,7 @@ pub(crate) enum BuiltinHooks {
     CheckJson,
     CheckJson5,
     CheckMergeConflict,
+    CheckShebangScriptsAreExecutable,
     CheckSymlinks,
     CheckToml,
     CheckVcsPermalinks,
@@ -74,6 +75,9 @@ impl BuiltinHooks {
             Self::CheckMergeConflict => {
                 pre_commit_hooks::check_merge_conflict(hook, filenames).await
             }
+            Self::CheckShebangScriptsAreExecutable => {
+                pre_commit_hooks::check_shebang_scripts_are_executable(hook, filenames).await
+            }
             Self::CheckSymlinks => pre_commit_hooks::check_symlinks(hook, filenames).await,
             Self::CheckToml => pre_commit_hooks::check_toml(hook, filenames).await,
             Self::CheckVcsPermalinks => {
@@ -195,6 +199,21 @@ impl BuiltinHook {
                     ..Default::default()
                 },
             },
+            BuiltinHooks::CheckShebangScriptsAreExecutable => BuiltinHook {
+                id: "check-shebang-scripts-are-executable".to_string(),
+                name: "check that scripts with shebangs are executable".to_string(),
+                entry: "check-shebang-scripts-are-executable".to_string(),
+                priority: None,
+                options: HookOptions {
+                    description: Some(
+                        "ensures that (non-binary) files with a shebang are executable."
+                            .to_string(),
+                    ),
+                    types: Some(tags::TAG_SET_TEXT),
+                    stages: Some([Stage::PreCommit, Stage::PrePush, Stage::Manual].into()),
+                    ..Default::default()
+                },
+            },
             BuiltinHooks::CheckSymlinks => BuiltinHook {
                 id: "check-symlinks".to_string(),
                 name: "check for broken symlinks".to_string(),

crates/prek/src/hooks/pre_commit_hooks/check_executables_have_shebangs.rs

@@ -1,14 +1,16 @@
+use std::fmt::Write as _;
 use std::path::Path;
 
-use futures::StreamExt;
 use owo_colors::OwoColorize;
-use rustc_hash::FxHashSet;
-use tokio::io::AsyncReadExt;
 
 use crate::git;
 use crate::hook::Hook;
+use crate::hooks::pre_commit_hooks::shebangs::{
+    file_has_shebang, git_index_stage_output, matching_git_index_paths_by_executable_bit,
+};
 use crate::hooks::run_concurrent_file_checks;
 use crate::run::CONCURRENCY;
+use rustc_hash::FxHashSet;
 
 pub(crate) async fn check_executables_have_shebangs(
     hook: &Hook,
@@ -47,166 +49,67 @@ async fn os_check_shebangs(
         if has_shebang {
             anyhow::Ok((0, Vec::new()))
         } else {
-            let msg = print_shebang_warning(file);
+            let msg = build_missing_shebang_warning(file)?;
             Ok((1, msg.into_bytes()))
         }
     })
     .await
 }
 
-fn print_shebang_warning(path: &Path) -> String {
+fn build_missing_shebang_warning(path: &Path) -> Result<String, std::fmt::Error> {
     let path_str = path.display();
-
-    format!(
-        "{}\n\
-         {}\n\
-         {}\n\
-         {}\n",
+    let mut warning = String::new();
+    writeln!(
+        warning,
+        "{}",
         format!(
             "{} marked executable but has no (or invalid) shebang!",
             path_str.yellow()
         )
-        .bold(),
-        format!("  If it isn't supposed to be executable, try: 'chmod -x {path_str}'").dimmed(),
-        format!("  If on Windows, you may also need to: 'git add --chmod=-x {path_str}'").dimmed(),
-        "  If it is supposed to be executable, double-check its shebang.".dimmed(),
-    )
+        .bold()
+    )?;
+    writeln!(
+        warning,
+        "{}",
+        format!("  If it isn't supposed to be executable, try: 'chmod -x {path_str}'").dimmed()
+    )?;
+    writeln!(
+        warning,
+        "{}",
+        format!("  If on Windows, you may also need to: 'git add --chmod=-x {path_str}'").dimmed()
+    )?;
+    writeln!(
+        warning,
+        "{}",
+        "  If it is supposed to be executable, double-check its shebang.".dimmed()
+    )?;
+    Ok(warning)
 }
 
 async fn git_check_shebangs(
     file_base: &Path,
     filenames: &[&Path],
 ) -> Result<(i32, Vec<u8>), anyhow::Error> {
-    let filenames: FxHashSet<_> = filenames.iter().collect();
+    let stdout = git_index_stage_output(file_base).await?;
+    let filenames: FxHashSet<_> = filenames.iter().copied().collect();
+    let entries = matching_git_index_paths_by_executable_bit(&stdout, file_base, &filenames, true);
 
-    let output = git::git_cmd("git ls-files")?
-        .arg("ls-files")
-        // Show staged contents' mode bits, object name and stage number in the output.
-        .arg("--stage")
-        .arg("-z")
-        .arg("--")
-        .arg(if file_base.as_os_str().is_empty() {
-            Path::new(".")
+    run_concurrent_file_checks(entries, *CONCURRENCY, |file| async move {
+        let file_path = file_base.join(file);
+        if file_has_shebang(&file_path).await? {
+            Ok((0, Vec::new()))
         } else {
-            file_base
-        })
-        .check(true)
-        .output()
-        .await?;
-
-    let entries = output.stdout.split(|&b| b == b'\0').filter_map(|entry| {
-        let entry = str::from_utf8(entry).ok()?;
-        if entry.is_empty() {
-            return None;
+            Ok((1, build_missing_shebang_warning(file)?.into_bytes()))
         }
-
-        let mut parts = entry.split('\t');
-        let metadata = parts.next()?;
-        let file_name = parts.next()?;
-        let file_name = Path::new(file_name);
-        if !filenames.contains(&file_name) {
-            return None;
-        }
-
-        let mode_str = metadata.split_whitespace().next()?;
-        let mode_bits = u32::from_str_radix(mode_str, 8).ok()?;
-        let is_executable = (mode_bits & 0o111) != 0;
-        Some((file_name, is_executable))
-    });
-
-    let mut tasks = futures::stream::iter(entries)
-        .map(async |(file_name, is_executable)| {
-            if is_executable {
-                let has_shebang = file_has_shebang(file_name).await?;
-                if has_shebang {
-                    anyhow::Ok((0, Vec::new()))
-                } else {
-                    let stripped = file_name.strip_prefix(file_base).unwrap_or(file_name);
-                    let msg = print_shebang_warning(stripped);
-                    Ok((1, msg.into_bytes()))
-                }
-            } else {
-                Ok((0, Vec::new()))
-            }
-        })
-        .buffered(*CONCURRENCY);
-
-    let mut code = 0;
-    let mut output = Vec::new();
-
-    while let Some(result) = tasks.next().await {
-        let (c, o) = result?;
-        code |= c;
-        output.extend(o);
-    }
-
-    Ok((code, output))
-}
-
-/// Check first 2 bytes for shebang (#!)
-async fn file_has_shebang(path: &Path) -> Result<bool, anyhow::Error> {
-    let mut file = fs_err::tokio::File::open(path).await?;
-    let mut buf = [0u8; 2];
-    let n = file.read(&mut buf).await?;
-    Ok(n >= 2 && buf[0] == b'#' && buf[1] == b'!')
+    })
+    .await
 }
 
 #[cfg(test)]
 mod tests {
     use super::*;
     use tempfile::NamedTempFile;
 
-    #[tokio::test]
-    async fn test_file_with_shebang() -> Result<(), anyhow::Error> {
-        let file = NamedTempFile::new()?;
-        tokio::fs::write(file.path(), b"#!/bin/bash\necho Hello World\n").await?;
-
-        assert!(file_has_shebang(file.path()).await?);
-        Ok(())
-    }
-
-    #[tokio::test]
-    async fn test_file_without_shebang() -> Result<(), anyhow::Error> {
-        let file = NamedTempFile::new()?;
-        tokio::fs::write(file.path(), b"echo Hello World\n").await?;
-
-        assert!(!file_has_shebang(file.path()).await?);
-        Ok(())
-    }
-
-    #[tokio::test]
-    async fn test_empty_file() -> Result<(), anyhow::Error> {
-        let file = NamedTempFile::new()?;
-        tokio::fs::write(file.path(), b"").await?;
-
-        assert!(!file_has_shebang(file.path()).await?);
-        Ok(())
-    }
-
-    #[tokio::test]
-    async fn test_file_with_partial_shebang() -> Result<(), anyhow::Error> {
-        let file = NamedTempFile::new()?;
-        tokio::fs::write(file.path(), b"#\n").await?;
-        assert!(!file_has_shebang(file.path()).await?);
-        Ok(())
-    }
-
-    #[tokio::test]
-    async fn test_file_with_shebang_and_spaces() -> Result<(), anyhow::Error> {
-        let file = NamedTempFile::new()?;
-        tokio::fs::write(file.path(), b"#! /bin/bash\necho Test\n").await?;
-        assert!(file_has_shebang(file.path()).await?);
-        Ok(())
-    }
-
-    #[tokio::test]
-    async fn test_file_with_non_shebang_start() -> Result<(), anyhow::Error> {
-        let file = NamedTempFile::new()?;
-        tokio::fs::write(file.path(), b"##!/bin/bash\n").await?;
-        assert!(!file_has_shebang(file.path()).await?);
-        Ok(())
-    }
-
     #[tokio::test]
     async fn test_os_check_shebangs_with_shebang() -> Result<(), anyhow::Error> {
         let file = NamedTempFile::new()?;

crates/prek/src/hooks/pre_commit_hooks/check_shebang_scripts_are_executable.rs

@@ -0,0 +1,75 @@
+use std::fmt::Write as _;
+use std::path::Path;
+
+use owo_colors::OwoColorize;
+
+use crate::hook::Hook;
+use crate::hooks::pre_commit_hooks::shebangs::{
+    file_has_shebang, git_index_stage_output, matching_git_index_paths_by_executable_bit,
+};
+use crate::hooks::run_concurrent_file_checks;
+use crate::run::CONCURRENCY;
+use rustc_hash::FxHashSet;
+
+pub(crate) async fn check_shebang_scripts_are_executable(
+    hook: &Hook,
+    filenames: &[&Path],
+) -> Result<(i32, Vec<u8>), anyhow::Error> {
+    let file_base = hook.project().relative_path();
+    let stdout = git_index_stage_output(file_base).await?;
+    let filenames: FxHashSet<_> = filenames.iter().copied().collect();
+    let entries = matching_git_index_paths_by_executable_bit(&stdout, file_base, &filenames, false);
+
+    run_concurrent_file_checks(entries, *CONCURRENCY, |file| async move {
+        let file_path = file_base.join(file);
+        if file_has_shebang(&file_path).await? {
+            Ok((1, build_non_executable_shebang_warning(file)?.into_bytes()))
+        } else {
+            Ok((0, Vec::new()))
+        }
+    })
+    .await
+}
+
+fn build_non_executable_shebang_warning(path: &Path) -> Result<String, std::fmt::Error> {
+    let path_str = path.display();
+    let mut warning = String::new();
+    writeln!(
+        warning,
+        "{}",
+        format!(
+            "{} has a shebang but is not marked executable!",
+            path_str.yellow()
+        )
+        .bold()
+    )?;
+    writeln!(
+        warning,
+        "{}",
+        format!("  If it is supposed to be executable, try: 'chmod +x {path_str}'").dimmed()
+    )?;
+    writeln!(
+        warning,
+        "{}",
+        format!("  If on Windows, you may also need to: 'git add --chmod=+x {path_str}'").dimmed()
+    )?;
+    writeln!(
+        warning,
+        "{}",
+        "  If it is not supposed to be executable, double-check its shebang is wanted.".dimmed()
+    )?;
+    Ok(warning)
+}
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    #[test]
+    fn non_executable_warning_mentions_chmod_and_git_add() {
+        let warning = build_non_executable_shebang_warning(Path::new("script.sh")).unwrap();
+
+        assert!(warning.contains("chmod +x script.sh"));
+        assert!(warning.contains("git add --chmod=+x script.sh"));
+    }
+}

crates/prek/src/hooks/pre_commit_hooks/mod.rs

@@ -10,6 +10,7 @@ mod check_case_conflict;
 mod check_executables_have_shebangs;
 pub(crate) mod check_json;
 mod check_merge_conflict;
+mod check_shebang_scripts_are_executable;
 mod check_symlinks;
 mod check_toml;
 mod check_vcs_permalinks;
@@ -22,12 +23,14 @@ mod fix_end_of_file;
 mod fix_trailing_whitespace;
 mod mixed_line_ending;
 mod no_commit_to_branch;
+mod shebangs;
 
 pub(crate) use check_added_large_files::check_added_large_files;
 pub(crate) use check_case_conflict::check_case_conflict;
 pub(crate) use check_executables_have_shebangs::check_executables_have_shebangs;
 pub(crate) use check_json::check_json;
 pub(crate) use check_merge_conflict::check_merge_conflict;
+pub(crate) use check_shebang_scripts_are_executable::check_shebang_scripts_are_executable;
 pub(crate) use check_symlinks::check_symlinks;
 pub(crate) use check_toml::check_toml;
 pub(crate) use check_vcs_permalinks::check_vcs_permalinks;
@@ -48,6 +51,7 @@ pub(crate) enum PreCommitHooks {
     CheckAddedLargeFiles,
     CheckCaseConflict,
     CheckExecutablesHaveShebangs,
+    CheckShebangScriptsAreExecutable,
     CheckVcsPermalinks,
     FileContentsSorter,
     EndOfFileFixer,
@@ -81,6 +85,9 @@ impl PreCommitHooks {
             Self::CheckExecutablesHaveShebangs => {
                 check_executables_have_shebangs(hook, filenames).await
             }
+            Self::CheckShebangScriptsAreExecutable => {
+                check_shebang_scripts_are_executable(hook, filenames).await
+            }
             Self::CheckVcsPermalinks => check_vcs_permalinks(hook, filenames).await,
             Self::FileContentsSorter => file_contents_sorter(hook, filenames).await,
             Self::EndOfFileFixer => fix_end_of_file(hook, filenames).await,