Skip to content

Commit

Permalink
Merge pull request #1711 from EliahKagan/run-ci/mode-it
Browse files Browse the repository at this point in the history
Add internal-tools `check-mode`, replacing `check-mode.sh`
  • Loading branch information
Byron authored Nov 29, 2024
2 parents ee33221 + 7e8aedf commit c146b7a
Show file tree
Hide file tree
Showing 9 changed files with 141 additions and 61 deletions.
9 changes: 0 additions & 9 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -359,14 +359,6 @@ jobs:
- name: gix-pack with all features (including wasm)
run: cd gix-pack && cargo build --all-features --target "$TARGET"

check-mode:
runs-on: ubuntu-latest

steps:
- uses: actions/checkout@v4
- name: Find scripts with mode/shebang mismatch
run: etc/check-mode.sh

check-packetline:
strategy:
fail-fast: false
Expand Down Expand Up @@ -449,7 +441,6 @@ jobs:
- test-32bit-cross
- lint
- cargo-deny
- check-mode
- check-packetline
- check-blocking

Expand Down
2 changes: 2 additions & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

47 changes: 0 additions & 47 deletions etc/check-mode.sh

This file was deleted.

8 changes: 5 additions & 3 deletions justfile
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,10 @@ alias c := check
alias nt := nextest

# run all tests, clippy, including journey tests, try building docs
test: clippy check doc unit-tests journey-tests-pure journey-tests-small journey-tests-async journey-tests
test: clippy check doc unit-tests journey-tests-pure journey-tests-small journey-tests-async journey-tests check-mode

# run all tests, without clippy, and try building docs
ci-test: check doc unit-tests
ci-test: check doc unit-tests check-mode

# run all journey tests - should be run in a fresh clone or after `cargo clean`
ci-journey-tests: journey-tests-pure journey-tests-small journey-tests-async journey-tests
Expand Down Expand Up @@ -196,6 +196,7 @@ target_dir := `cargo metadata --format-version 1 | jq -r .target_directory`
ein := target_dir / "debug/ein"
gix := target_dir / "debug/gix"
jtt := target_dir / "debug/jtt"
it := target_dir / "debug/it"

# run journey tests (max)
journey-tests:
Expand Down Expand Up @@ -257,7 +258,8 @@ find-yanked:

# Find shell scripts whose +x/-x bits and magic bytes (e.g. `#!`) disagree
check-mode:
./etc/check-mode.sh
cargo build -p internal-tools
"{{ it }}" check-mode

# Delete gix-packetline-blocking/src and regenerate from gix-packetline/src
copy-packetline:
Expand Down
5 changes: 3 additions & 2 deletions tests/it/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,8 @@ name = "it"
path = "src/main.rs"

[dependencies]
clap = { version = "4.5.16", features = ["derive"] }
anyhow = "1.0.86"

clap = { version = "4.5.16", features = ["derive"] }
gix = { version = "^0.68.0", path = "../../gix", default-features = false, features = ["attributes", "revision"] }
once_cell = "1.20.2"
regex = { version = "1.11.1", default-features = false, features = ["std"] }
11 changes: 11 additions & 0 deletions tests/it/src/args.rs
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,17 @@ pub enum Subcommands {
#[clap(value_parser = AsPathSpec)]
patterns: Vec<gix::pathspec::Pattern>,
},
/// Check for executable bits that disagree with shebangs.
///
/// This checks committed and staged files, but not anything unstaged, to find shell scripts
/// that either begin with a `#!` but not `+x` permissions, or do not begin with `#!` but do
/// have `+x` permissions. Such mismatches are reported but not automatically corrected. Some
/// platforms (at least Windows) do not have such permissions, but Git still represents them.
///
/// This currently only checks files name with an `.sh` suffix, and only operates on the
/// current repository. Its main use is checking that fixture scripts are have correct modes.
#[clap(visible_alias = "cm")]
CheckMode {},
}

#[derive(Clone)]
Expand Down
116 changes: 116 additions & 0 deletions tests/it/src/commands/check_mode.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,116 @@
pub(super) mod function {
use anyhow::{bail, Context};
use gix::bstr::ByteSlice;
use once_cell::sync::Lazy;
use regex::bytes::Regex;
use std::ffi::{OsStr, OsString};
use std::io::{BufRead, BufReader, Read};
use std::process::{Command, Stdio};

pub fn check_mode() -> anyhow::Result<()> {
let root = find_root()?;
let mut any_mismatch = false;

let mut child = git_on(&root)
.args(["ls-files", "-sz", "--", "*.sh"])
.stdout(Stdio::piped())
.spawn()
.context("Can't start `git` subprocess to list index")?;

let stdout = child.stdout.take().expect("should have captured stdout");
for result in BufReader::new(stdout).split(0) {
let record = result.context(r"Can't read '\0'-terminated record")?;
any_mismatch |= check_for_mismatch(&root, &record)?;
}

let status = child.wait().context("Failure running `git` subprocess to list index")?;
if !status.success() {
bail!("`git` subprocess to list index did not complete successfully");
}
if any_mismatch {
bail!("Mismatch found - scan completed, finding at least one `#!` vs. `+x` mismatch");
}
Ok(())
}

/// Find the top-level directory of the current repository working tree.
fn find_root() -> anyhow::Result<OsString> {
let output = Command::new(gix::path::env::exe_invocation())
.args(["rev-parse", "--show-toplevel"])
.output()
.context("Can't run `git` to find worktree root")?;

if !output.status.success() {
bail!("`git` failed to find worktree root");
}

let root = output
.stdout
.strip_suffix(b"\n")
.context("Can't parse worktree root")?
.to_os_str()?
.to_owned();
Ok(root)
}

/// Prepare a `git` command, passing `root` as an operand to `-C`.
///
/// This is suitable when `git` gave us the path `root`. Then it should already be in a form
/// where `git -C` will be able to use it, without alteration, regardless of the platform.
/// (Otherwise, it may be preferable to set `root` as the `cwd` of the `git` process instead.)
fn git_on(root: &OsStr) -> Command {
let mut cmd = Command::new(gix::path::env::exe_invocation());
cmd.arg("-C").arg(root);
cmd
}

/// On mismatch, report it and return `Some(true)`.
fn check_for_mismatch(root: &OsStr, record: &[u8]) -> anyhow::Result<bool> {
static RECORD_REGEX: Lazy<Regex> = Lazy::new(|| {
let pattern = r"(?-u)\A([0-7]+) ([[:xdigit:]]+) [[:digit:]]+\t(.+)\z";
Regex::new(pattern).expect("regex should be valid")
});

let fields = RECORD_REGEX.captures(record).context("Malformed record from `git`")?;
let mode = fields.get(1).expect("match should get mode").as_bytes();
let oid = fields
.get(2)
.expect("match should get oid")
.as_bytes()
.to_os_str()
.expect("oid field verified as hex digits, should be valid OsStr");
let path = fields.get(3).expect("match should get path").as_bytes().as_bstr();

match mode {
b"100644" if blob_has_shebang(root, oid)? => {
println!("mode -x but has shebang: {path:?}");
Ok(true)
}
b"100755" if !blob_has_shebang(root, oid)? => {
println!("mode +x but no shebang: {path:?}");
Ok(true)
}
_ => Ok(false),
}
}

fn blob_has_shebang(root: &OsStr, oid: &OsStr) -> anyhow::Result<bool> {
let mut buf = [0u8; 2];

let mut child = git_on(root)
.args(["cat-file", "blob"])
.arg(oid)
.stdout(Stdio::piped())
.spawn()
.context("Can't start `git` subprocess to read blob")?;

let mut stdout = child.stdout.take().expect("should have captured stdout");
let count = stdout.read(&mut buf).context("Error reading data from blob")?;
drop(stdout); // Let the pipe break rather than waiting for the rest of the blob.

// TODO: Maybe check status? On Unix, it should be 0 or SIGPIPE. Not sure about Windows.
_ = child.wait().context("Failure running `git` subprocess to read blob")?;

Ok(&buf[..count] == b"#!")
}
}
3 changes: 3 additions & 0 deletions tests/it/src/commands/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,3 +3,6 @@ pub use copy_royal::function::copy_royal;

pub mod git_to_sh;
pub use git_to_sh::function::git_to_sh;

pub mod check_mode;
pub use check_mode::function::check_mode;
1 change: 1 addition & 0 deletions tests/it/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ fn main() -> anyhow::Result<()> {
destination_dir,
patterns,
} => commands::copy_royal(dry_run, &worktree_root, destination_dir, patterns),
Subcommands::CheckMode {} => commands::check_mode(),
}
}

Expand Down

0 comments on commit c146b7a

Please sign in to comment.