Sane clippy linting

.github/workflows/rust.yml

@@ -25,9 +25,5 @@ jobs:
  target
  patchparser/target
  key: ${{ runner.os }}-cargo-${{ hashFiles('**/Cargo.lock') }}-withleakchecker
- - name: Run tests, deny warnings
- run: make test_deny_warnings
- - name: Run tests with leak checker
- run: make leak_test
- - name: Check formatting
- run: make check_formatting
+ - name: Run tests & deny warnings // check leaks // check formatting, in parallel
+ run: make ci

Cargo.toml

@@ -5,6 +5,22 @@ edition = "2021"
 description = "Split patch files on file, hunk or change boundaries"
 repository = "https://github.com/IntermediateResults/split-patch"
 
+[lints.clippy]
+# Naming lifetimes can be explanatory; even if it's not really, we
+# generally prioritize moving on with the project.
+needless_lifetimes = "allow"
+# Nesting if statements can be clearer
+collapsible_else_if = "allow"
+collapsible_if = "allow"
+# Having to write "x" as 'x' while another branch uses "yz" is
+# silly. It might even be less efficient.
+single_char_add_str = "allow"
+# We should inherently strive for simple types anyway. Having to
+# explicitly allow the cases that are not easy to simplify seems
+# wasteful on our time. (We may run clippy with `-W clippy::all` from
+# time to time.)
+type_complexity = "allow"
+
 [dependencies]
 patchparser = { path = "patchparser" }
 

Makefile

@@ -17,6 +17,19 @@ cargo_check:
 	( cd patchparser && cargo $(OUR_CARGO_FLAGS) test --color=always )
 	cargo $(OUR_CARGO_FLAGS) test --color=always
 
+# This target is to abstract running clippy on everything (and can be run manually)
+clippy:
+	( cd patchparser && cargo clippy --color=always --all-targets --all-features $(CLIPPY_ARGS) )
+	cargo clippy --color=always --all-targets --all-features $(CLIPPY_ARGS)
+
+# This is for use in CI
+clippy_deny:
+	CLIPPY_ARGS="-- -D warnings" make clippy
+
+# This is for manual use
+clippy_fix:
+	CLIPPY_ARGS="--fix" make clippy
+
 test_integration:
 	@echo "++ Run tests on test/div"
 	OUR_CARGO_BUILD_FLAGS="" test/run-test-for-input-dir test/div
@@ -34,6 +47,7 @@ test: cargo_test test_integration
 leak_test:
 	RUSTFLAGS="-Z sanitizer=leak" OUR_CARGO_FLAGS=+nightly make test
 
+# This is for use in CI
 test_deny_warnings:
 	RUSTFLAGS="--deny warnings" make cargo_test
 
@@ -44,3 +58,7 @@ miri_run:
 	SPLIT_PATCH=test/miri-split-patch test/run-test-for-input-dir test/div
 
 miri: miri_test miri_run
+
+# Run in Github CI
+ci:
+	test/ci-make test_deny_warnings clippy_deny leak_test check_formatting

README.md

@@ -50,3 +50,31 @@ repository, configure the remote correspondingly via:
 
  git config --add remote.origin.fetch '+refs/notes/commits:refs/notes/commits'
 
+### Clippy
+
+A number of warnings deemed not useful enough have been configured as
+"allow" in Cargo.toml.
+
+To run clippy the same way that the CI runs it (except CI runs it in
+error mode, via `make clippy_deny`):
+
+ make clippy
+
+In case you want to see all of the default clippy warnings,
+i.e. ignore the ignores:
+
+ CLIPPY_ARGS="-- -W clippy::all" make clippy
+
+To have clippy fix the warnings according to the project desires:
+
+ make clippy_fix
+
+### Testing / CI
+
+GitHub CI runs `make ci`. You can run this locally before
+(re)submitting the PR to speed up checking. This does run `cargo fmt`
+and will output the diff from the last commit.
+
+During development, `make test` is likely what you usually want to
+run. For other, more finegrained test choices, have a look at the
+`Makefile`.

patchparser/Cargo.toml

@@ -18,6 +18,22 @@ keywords = [
 [lib]
 name = "patchparser"
 
+[lints.clippy]
+# Naming lifetimes can be explanatory; even if it's not really, we
+# generally prioritize moving on with the project.
+needless_lifetimes = "allow"
+# Nesting if statements can be clearer
+collapsible_else_if = "allow"
+collapsible_if = "allow"
+# Having to write "x" as 'x' while another branch uses "yz" is
+# silly. It might even be less efficient.
+single_char_add_str = "allow"
+# We should inherently strive for simple types anyway. Having to
+# explicitly allow the cases that are not easy to simplify seems
+# wasteful on our time. (We may run clippy with `-W clippy::all` from
+# time to time.)
+type_complexity = "allow"
+
 [dependencies]
 anyhow = { version = "1.0", features = ["backtrace"] }
 bstr = { version = "1.10", default-features = false, features = ["alloc"] }

patchparser/src/bumpalo_cow.rs

@@ -75,7 +75,7 @@ impl<'b, B: ?Sized + ToOwnedIn<'b>> CloneIn<'b> for BumpaloCow<'b, '_, B> {
  fn clone_from_in(&mut self, source: &Self, bump: &'b Bump) {
  use BumpaloCow::*;
  match (self, source) {
- (&mut Owned(ref mut dest), &Owned(ref o)) => o.borrow().clone_into_in(dest, bump),
+ (&mut Owned(ref mut dest), Owned(o)) => o.borrow().clone_into_in(dest, bump),
  (t, s) => *t = s.clone_in(bump),
  }
  }
@@ -96,7 +96,7 @@ where
  fn clone_from(&mut self, source: &Self) {
  use BumpaloCow::*;
  match (self, source) {
- (&mut Owned(ref mut dest), &Owned(ref o)) => o.clone_into(dest),
+ (&mut Owned(ref mut dest), Owned(o)) => o.clone_into(dest),
  (t, s) => *t = s.clone(),
  }
  }

patchparser/src/patch/diff.rs

@@ -41,9 +41,9 @@ where
  hunks,
  } = self;
  DiffDifferences {
- index_line: index_line.clone(),
- minus_line: minus_line.clone(),
- plus_line: plus_line.clone(),
+ index_line: *index_line,
+ minus_line: *minus_line,
+ plus_line: *plus_line,
  hunks: hunks.iter().map(|v| v.reborrow_in(bump)).collect_in(bump),
  }
  }
@@ -123,13 +123,13 @@ where
  } = self;
  Diff {
  diff_line: *diff_line,
- diff_path_a_full: diff_path_a_full.clone(),
- diff_path_b_full: diff_path_b_full.clone(),
- newfile_line: newfile_line.clone(),
- deleted_line: deleted_line.clone(),
- similarity_line: similarity_line.clone(),
- rename_from_line: rename_from_line.clone(),
- rename_to_line: rename_to_line.clone(),
+ diff_path_a_full: *diff_path_a_full,
+ diff_path_b_full: *diff_path_b_full,
+ newfile_line: *newfile_line,
+ deleted_line: *deleted_line,
+ similarity_line: *similarity_line,
+ rename_from_line: *rename_from_line,
+ rename_to_line: *rename_to_line,
  differences: differences.as_ref().map(|v| v.reborrow_in(bump)),
  }
  }
@@ -244,7 +244,7 @@ impl<'a> Diff<'a> {
 
  pub fn diff_path_b(&self) -> Result<&BStr> {
  strip_leading_path_segment(
-&self.diff_path_b_full.with_context(|| {
+ self.diff_path_b_full.with_context(|| {
  format!("missing second path in 'diff' line {}", self.diff_line)
  })?,
  )
@@ -272,12 +272,12 @@ impl<'a> WriteTo for Diff<'a> {
 
 impl<'a> FromLines<'a> for Diff<'a> {
  fn from_lines(lines_slice: &'a [Line<'a>], bump: &'a Bump) -> Result<Diff<'a>> {
- let mut lines = lines_slice.into_iter();
+ let mut lines = lines_slice.iter();
 
  let diff_line = *lines
  .next()
  .filter(|l| l.starts_with(b"diff "))
- .with_context(|| format!("missing `diff ` line"))?;
+ .context("missing `diff ` line")?;
  let (diff_path_a_full, diff_path_b_full);
  {
  let mut parts = diff_line.split(|b| *b == b' ');

patchparser/src/patch/mod.rs

@@ -2,4 +2,6 @@ pub mod change;
 pub mod change_line;
 pub mod diff;
 pub mod hunk;
+#[allow(clippy::module_inception)]
+// XXX rename or re-export
 pub mod patch;

patchparser/src/patch/patch.rs

@@ -24,7 +24,7 @@ pub fn string_equal_ci<A: AsRef<[u8]>, B: AsRef<[u8]>>(a: A, b: B) -> bool {
  let b = b.as_ref();
  a.len() == b.len() && {
  for (ac, bc) in a.iter().zip(b) {
- if ac.to_ascii_lowercase() != bc.to_ascii_lowercase() {
+ if !ac.eq_ignore_ascii_case(bc) {
  return false;
  }
  }
@@ -352,7 +352,7 @@ impl<'a> FromLines<'a> for Patch<'a> {
  (&[], &chunks)
  } else {
  // First part is head
- (&chunks[0], &chunks[1..])
+ (chunks[0], &chunks[1..])
  };
  if diff_lines_groups.is_empty() {
  // bail!("file does not appear to contain any diffs");
@@ -366,7 +366,7 @@ impl<'a> FromLines<'a> for Patch<'a> {
  .iter()
  .enumerate()
  .map(|(diff_i, diff_lines)| -> Result<_> {
- Diff::from_lines(*diff_lines, bump).with_context(|| {
+ Diff::from_lines(diff_lines, bump).with_context(|| {
  format!(
  "parsing diff no. {}/{}",
  diff_i + 1,

src/bin/split-patch.rs

@@ -34,7 +34,7 @@ fn main() -> Result<()> {
  let split_options = args.split_args.into();
 
  for patch_file in &args.patch_file {
- let written = split_patch(&patch_file, &split_options)
+ let written = split_patch(patch_file, &split_options)
  .with_context(|| anyhow!("splitting the patch file {patch_file:?}"))?;
 
  if !args.quiet {

src/bin/test-split-patches-in-dir.rs

@@ -93,7 +93,7 @@ fn main() -> Result<()> {
  let mut split_args = split_args.clone();
  split_args.output_dir = Some(full_output_dir.clone());
 
- let written = split_patch(&file, &split_args.into())
+ let written = split_patch(file, &split_args.into())
  .with_context(|| anyhow!("splitting the patch file {file:?}"))?;
 
  let list_path = full_output_dir.join("_list");

src/core.rs

@@ -39,7 +39,7 @@ fn split_diff_in<'a, 'h>(
  let path = {
  let path_in_source_dir = add_suffix(
  original_path,
- OsStr::from_bytes(&*make_bstring!({ b"-" } + { b_path.replace("/", b"_") })),
+ OsStr::from_bytes(&make_bstring!({ b"-" } + { b_path.replace("/", b"_") })),
  )?;
  if let Some(output_dir) = &split_options.output_dir {
  output_dir.join(
@@ -81,9 +81,9 @@ fn split_diff_in<'a, 'h>(
  };
 
  let written_path = write_patch_file(
-&head_with_prefix(&prefix_part),
+ head_with_prefix(&prefix_part),
  diff_with_hunk!(change.to_hunk(bump)),
- add_suffix(&path, format!("-{prefix_part}"))?.into(),
+ add_suffix(&path, format!("-{prefix_part}"))?,
  )?;
 
  written_paths.push(written_path);
@@ -173,9 +173,8 @@ pub fn split_patch(patch_file_path: &Path, split_options: &SplitOptions) -> Resu
  // Write the diffs to individual (separate) files
  let mut written = Vec::new();
  for (diff_i, diff) in diffs.iter().enumerate() {
- let written_paths =
- split_diff_in(&patch.head, &diff, patch_file_path, split_options, &bump)
- .with_context(|| format!("splitting diff no. {}/{}", diff_i + 1, diffs.len()))?;
+ let written_paths = split_diff_in(&patch.head, diff, patch_file_path, split_options, &bump)
+ .with_context(|| format!("splitting diff no. {}/{}", diff_i + 1, diffs.len()))?;
 
  written.extend(written_paths);
  }

src/split_options.rs

@@ -44,7 +44,7 @@ impl Default for SplitArgs {
 #[test]
 fn t_default_split_args() {
  let d = SplitArgs::default();
-assert_eq!(d.no_insert_after_patch, false);
+assert!(!d.no_insert_after_patch);
 }
 
 pub enum SplitMode {

test/ci-make

@@ -0,0 +1,53 @@
+#!/bin/bash
+
+# do *not* specify -o pipefail, due to "| grep -q" below
+set -meu
+IFS=
+
+usage() {
+ echo "Usage: $0 make-targets..."
+ echo " Runs the given make targets in parallel but with separated outputs"
+ exit 1
+}
+
+if [ $# -eq 0 ] || [ "$1" = -h -o "$1" = --help ]; then
+ usage
+fi
+
+targets=("$@")
+
+run() {
+ local target=$1
+ echo "+ make $target"
+ if time make "$target"; then
+ echo -e "\n$target: Ok."
+ else
+ echo -e "\n$target: Err: $?"
+ fi
+}
+
+tmpfiles=()
+
+for target in "${targets[@]}"; do
+ tmp=$(mktemp)
+ tmpfiles+=("$tmp")
+ run "$target" > "$tmp" 2>&1 &
+done
+
+for target in "${targets[@]}"; do
+ wait || true
+done
+
+exit_code=0
+
+for tmp in "${tmpfiles[@]}"; do
+ echo "========================================================================================"
+ cat "$tmp"
+ if ! tail -1 "$tmp" | grep -q ": Ok."; then
+ exit_code=$((exit_code + 1))
+ fi
+done
+
+echo "========================================================================================"
+
+exit "$exit_code"