[Clippy] Address most clippy issues in production code by DanielEScherzer · Pull Request #1291 · rust-lang/git2-rs

DanielEScherzer · 2026-07-15T01:23:47Z

Commits are split by file and lint for ease of review. In most cases, the lints are addressed by fixing the reported issue as suggested.

API changes

Notably, the following public API changes were made:

New `Default` implementations

The following types now implement the Default trait:

ApplyOptions
CherrypickOptions
DiffPatchidOptions
RepositoryInitOptions
RevertOptions
WorktreeAddOptions
WorktreePruneOptions

New `is_empty()` methods

The following types have a new is_empty() method to go along with the existing len() method:

MessageTrailersStrs
MessageTrailersBytes
OdbObject
Rebase

Other

the CloneLocal enum is marked as #[non_exhaustive], and the __Nonexhaustive variant was removed
the implementations of the Into trait for converting TreeWalkResult to i32 and to raw::git_treewalk_mode were replaced with implementations of the From trait

Lints expected

In a few places, addressing the clippy issues would have required breaking changes to the existing API. Accordingly, instead of addressing the issues, the module-level #[allow] attributes were replaced with method-level #[expect] attributes, ensuring that new functions do not have the same issues while maintaining backwards compatibility. The follwing methods are expected to trigger clippy lints:

`clippy::should_implement_trait`

ConfigEntries::next()
Cred::default()
Error::from_str()
ObjectType::from_str()
Oid::from_str()

`clippy::too_many_arguments`

Email::from_diff()
Repository::diff_blobs()

Remaining `#[allow]` attributes

This PR does not address code for tests (code in src/test.rs, code in mod tests inline modules, or code in the tests directory). It also does not address code for examples (the examples) directory. When a file-wide #[allow] turned out to also be needed for the inline test modules, it was added to the tests module.

The library-wide clippy::unnecessary_cast allowing specified in src/lib.rs is also not addressed. Given that some of the casts are needed on unix but not windows, and others are needed on windows but not unix, that lint requires further attention. A dedicated PR already exists at #1238 reduce the unnecessary casts.

DanielEScherzer · 2026-07-15T01:49:37Z

For the claim about from implementations for TreeWalkResult: https://play.rust-lang.org/?version=stable&mode=debug&edition=2024&gist=14aafe81106484b31590080fdd2386ae

weihanglo

Thanks. This a huge work!

BTW, is it possible to convert all #[allow] to #[expect]?

View changes since this review

weihanglo · 2026-07-15T01:41:10Z

    }
 }

+impl<'cb> Default for ApplyOptions<'cb> {


I am actually not fond of this. This may not be useful if we don't really need it. And that was why rust-lang/cargo only turn on a subset of lint group https://github.com/rust-lang/cargo/blob/371a35405026fc16dd9968589f13cd746b96e024/Cargo.toml#L139-L145.

Some of them are too pedantic.

weihanglo · 2026-07-15T01:42:34Z

 }

 #[test]
+#[allow(clippy::explicit_auto_deref)]


I forgot in the last PR. we should use #[expect] instead for all #[allow].

weihanglo · 2026-07-15T01:44:49Z


 #[cfg(test)]
 #[cfg(feature = "cred")]
+#[allow(clippy::needless_borrows_for_generic_args)]


#[expect]

weihanglo · 2026-07-15T01:45:44Z

@@ -1,3 +1,5 @@
+#![allow(clippy::should_implement_trait)]


#[expect]

this gets removed later in the same PR

weihanglo · 2026-07-15T01:46:53Z

-            match writepack.free {
-                Some(free) => free(self.raw),
-                None => (),
+            if let Some(free) = writepack.free {


Like, this one is pretty annoying. It generally doesn't really matter.

weihanglo · 2026-07-15T01:54:29Z

@@ -1,3 +1,5 @@
+#![allow(clippy::should_implement_trait)]


#[expect]

DanielEScherzer · 2026-07-15T02:39:47Z

Thanks. This a huge work!

BTW, is it possible to convert all #[allow] to #[expect]?

I didn't use expect because as I found out both here and on #1238 some lints might fire for one OS but not another
Plus the new allows that are added here are either

moved from the file to the tests, and I will deal with soon
temporarily added to src/oid.rs and I deal with it already in this PR

Should I remove the Default implementations? Disable the lint entirely?

weihanglo · 2026-07-15T03:06:47Z

Should I remove the Default implementations? Disable the lint entirely?

I am fine keeping them anyway. Just share with you my preference :)

moved from the file to the tests, and I will deal with soon

This might be better, if not too many churns.

Add safety documentation for `ApplyOptions::raw()` regarding using the raw pointer.

Add an implementation of the `Default` trait for `ApplyOptions` that delegates to `ApplyOptions::new()`.

Simplify `BlameHunk::summary()` to use the tuple variant `Some` rather than using a closure.

In the `std::io::Write` trait implementation for `BlobWriter`, use the helper function `std::io::Error::other()` for constructing an `std::io::Error` of kind `std::io::ErrorKind::other`.

Add the missing `# Safety` heading to the existing safety documentation for `Branches::from_raw()` and expand that documentation.

Remove the explicit dereferencing in `Buf::as_str()`.

Mark the `CloneLocal` enum as `#[non_exhaustive]` and remove the hidden `__Nonexhaustive` variant. Based on the libgit2 documentation of the `git_clone_local_t` type the `__Nonexhaustive` was not something that the upstream library used as an actual value.

Add the missing `# Safety` heading to the existing safety documentation for `CheckoutBuilder::configure()` and expand that documentation.

Remove needless lifetimes from the implementation of the `Convert` trait for - converting references (`&T`) to constant pointers (`*const T`) - converting mutable references (`&mut T`) to mutable pointers (`*mut T`)

Add an implementation of the `Default` trait for `CherrypickOptions` that delegates to `CherrypickOptions::new()`.

Simplify a few methods to use the tuple variant `Some` rather than using a closure: - `Commit::message_encoding()` - `Commit::summary()` - `Commit::body()`

Because the method with the confusing name (`ConfigEntries::next()`) is part of the public API of the crate, simply move the lint handling to that method with an `#[expect]` attribute. Doing so avoids breaking changes while also removing the `#![allow]` from the overall module, ensuring that new functions do not have confusing names.

Simplify `CredentialHelper::add_command()` using `str::strip_prefix()`.

Simplify `CredentialHelper::config_use_http_path()` by using if-let expressions with `Results` rather than calling `Result::ok()` and matching against the resulting `Option`.

Add safety documentation for `Cred::unwrap()`.

Simplify `CredentialHelper::url_key()` to avoid needless borrows.

…red.rs Simplify `CredentialHelper::execute_cmd()` to avoid a needless borrow.

Because the method with the confusing name (`Cred::default()`) is part of the public API of the crate, simply move the lint handling to that method with an `#[expect]` attribute. Doing so avoids breaking changes while also removing the `#![allow]` from the overall module, ensuring that new functions do not have confusing names.

Add actual documentation to the `Addition`, `Deletion`, and `HunkHeader` variants of the `DiffLineType` enum.

Add the missing `# Safety` heading to the existing safety documentation for `DiffOptions::raw()`. Add safety documentation for `DiffFindOptions::raw()`.

Add an implementation of the `Default` trait for `DiffPatchidOptions` that delegates to `DiffPatchidOptions::new()`.

Because the method that accepts too many arguments (`Email::from_diff()`) is part of the public API of the crate, simply move the lint handling to that method with an `#[expect]` attribute. Doing so avoids breaking changes while also removing the `#![allow]` from the overall module, ensuring that new functions do not have too many arguments.

In `Repository::revert()` use `std::ptr::null()` for the revert options pointer passed to `git_revert()` if no options are provided by the caller.

Add an implementation of the `Default` trait for `RevertOptions` that delegates to `RevertOptions::new()`.

Add the missing `# Safety` heading to the existing safety documentation for `StashSaveOptions::raw()`.

Add the missing `# Safety` heading to the existing safety documentation for `StatusOptions::raw()`.

Simplify `StringArray::get()` to use the tuple variant `Some` rather than using a closure.

…dule.rs Add an empty comment line (`///`) between the paragraphs of the documentation for `Submodule::repo_init()`.

Remove the unneeded assignment in `Submodule::raw()`.

Simplify a few methods to use the tuple variant `Some` rather than using a closure: - `Submodule::branch()` - `Submodule::url()`

Remove the `#![allow]` attribute, apparently something has changed since I initially added it and clippy no longer complains.

Simplify `Tag::message()` to use the tuple variant `Some` rather than using a closure.

Add the missing `# Safety` heading to the existing safety documentation for `register()`.

…port.rs Add the missing annotations to: - `subtransport_action()` - `subtransport_free()` - `stream_free()`

Convert the implementations of conversions from `TreeWalkResult` to `i32` and `raw::git_treewalk_mode` from the `Into` trait to the `From` trait.

Simplify `treewalk_cb()` to use `Option::unwrap_or()`.

Add safety documentation for `Binding::from_raw()` and `Binding::from_raw_opt()`.

Remove needless lifetimes from the `IntoCString` trait implementations for - references to clonable types implementing `IntoCString` (`&T`) - references to string slices - references to `std::path::Path` objects - references to `std::ffi::OsStr` objects - references to `u8` slices

Simplify the `fixup_windows_path()` function to iterate by mutable reference to the path bytes rather than using a range and reading and writing bytes by index.

Remove unneeded `return`s in `path_to_repo_path()`.

Add an implementation of the `Default` trait for `WorktreeAddOptions` that delegates to `WorktreeAddOptions::new()`. Add an implementation of the `Default` trait for `WorktreePruneOptions` that delegates to `WorktreePruneOptions::new()`.

Simplify `Worktree::name()` to use the tuple variant `Some` rather than using a closure.

rustbot · 2026-07-15T20:13:58Z

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

weihanglo

Thanks!

View changes since this review

rustbot added the S-waiting-on-review Status: Waiting on review label Jul 15, 2026

DanielEScherzer force-pushed the clippy-fixes branch 3 times, most recently from b8a4250 to e45e67c Compare July 15, 2026 01:49

weihanglo reviewed Jul 15, 2026

View reviewed changes

DanielEScherzer added 22 commits July 15, 2026 13:12

[Clippy] Address clippy::missing_safety_doc in src/apply.rs

4924fc6

Add safety documentation for `ApplyOptions::raw()` regarding using the raw pointer.

[Clippy] Address clippy::new_without_default in src/apply.rs

2acbfb7

Add an implementation of the `Default` trait for `ApplyOptions` that delegates to `ApplyOptions::new()`.

[Clippy] Address clippy::redundant_closure in src/blame.rs

8d2f956

Simplify `BlameHunk::summary()` to use the tuple variant `Some` rather than using a closure.

[Clippy] Address clippy::io_other_error in src/blob.rs

d39ed7b

In the `std::io::Write` trait implementation for `BlobWriter`, use the helper function `std::io::Error::other()` for constructing an `std::io::Error` of kind `std::io::ErrorKind::other`.

[Clippy] Address clippy::missing_safety_doc in src/branch.rs

b6ea28b

Add the missing `# Safety` heading to the existing safety documentation for `Branches::from_raw()` and expand that documentation.

[Clippy] Address clippy::explicit_auto_deref in src/buf.rs

6278c61

Remove the explicit dereferencing in `Buf::as_str()`.

[Clippy] Address clippy::missing_safety_doc in src/build.rs

e2a5755

Add the missing `# Safety` heading to the existing safety documentation for `CheckoutBuilder::configure()` and expand that documentation.

[Clippy] Address clippy::needless_lifetimes in src/call.rs

95fab73

Remove needless lifetimes from the implementation of the `Convert` trait for - converting references (`&T`) to constant pointers (`*const T`) - converting mutable references (`&mut T`) to mutable pointers (`*mut T`)

[Clippy] Address clippy::new_without_default in src/cherrypick.rs

5843bac

Add an implementation of the `Default` trait for `CherrypickOptions` that delegates to `CherrypickOptions::new()`.

[Clippy] Address clippy::redundant_closure in src/commit.rs

a9aaab1

Simplify a few methods to use the tuple variant `Some` rather than using a closure: - `Commit::message_encoding()` - `Commit::summary()` - `Commit::body()`

[Clippy] Address clippy::manual_strip in src/cred.rs

700a9cd

Simplify `CredentialHelper::add_command()` using `str::strip_prefix()`.

[Clippy] Address clippy::match_result_ok in src/cred.rs

493cd2f

Simplify `CredentialHelper::config_use_http_path()` by using if-let expressions with `Results` rather than calling `Result::ok()` and matching against the resulting `Option`.

[Clippy] Address clippy::missing_safety_doc in src/cred.rs

2f79fce

Add safety documentation for `Cred::unwrap()`.

[Clippy] Address clippy::needless_borrowed_reference in src/cred.rs

f96b066

Simplify `CredentialHelper::url_key()` to avoid needless borrows.

[Clippy] Address clippy::needless_borrows_for_generic_args in src/c…

4a235ba

…red.rs Simplify `CredentialHelper::execute_cmd()` to avoid a needless borrow.

[Clippy] Address clippy::empty_docs in src/diff.rs

bba606e

Add actual documentation to the `Addition`, `Deletion`, and `HunkHeader` variants of the `DiffLineType` enum.

[Clippy] Address clippy::missing_safety_doc in src/diff.rs

d7f4900

Add the missing `# Safety` heading to the existing safety documentation for `DiffOptions::raw()`. Add safety documentation for `DiffFindOptions::raw()`.

[Clippy] Address clippy::new_without_default in src/diff.rs

894770c

Add an implementation of the `Default` trait for `DiffPatchidOptions` that delegates to `DiffPatchidOptions::new()`.

DanielEScherzer added 20 commits July 15, 2026 13:13

[Clippy] Address clippy::zero_ptr in src/repo.rs

fe53f31

In `Repository::revert()` use `std::ptr::null()` for the revert options pointer passed to `git_revert()` if no options are provided by the caller.

[Clippy] Address clippy::new_without_default in src/revert.rs

7ed3393

Add an implementation of the `Default` trait for `RevertOptions` that delegates to `RevertOptions::new()`.

[Clippy] Address clippy::missing_safety_doc in src/revert.rs

15815dc

Add the missing `# Safety` heading to the existing safety documentation for `StashSaveOptions::raw()`.

[Clippy] Address clippy::missing_safety_doc in src/status.rs

bf01979

Add the missing `# Safety` heading to the existing safety documentation for `StatusOptions::raw()`.

[Clippy] Address clippy::redundant_closure in src/string_array.rs

e8a0834

Simplify `StringArray::get()` to use the tuple variant `Some` rather than using a closure.

[Clippy] Address clippy::empty_line_after_doc_comments in src/submo…

78570ef

…dule.rs Add an empty comment line (`///`) between the paragraphs of the documentation for `Submodule::repo_init()`.

[Clippy] Address clippy::let_and_return in src/submodule.rs

c5e2093

Remove the unneeded assignment in `Submodule::raw()`.

[Clippy] Address clippy::redundant_closure in src/submodule.rs

000ea52

Simplify a few methods to use the tuple variant `Some` rather than using a closure: - `Submodule::branch()` - `Submodule::url()`

[Clippy] Address clippy::from_over_into in src/tag.rs

205f3c3

Remove the `#![allow]` attribute, apparently something has changed since I initially added it and clippy no longer complains.

[Clippy] Address clippy::redundant_closure in src/tag.rs

3003bba

Simplify `Tag::message()` to use the tuple variant `Some` rather than using a closure.

[Clippy] Address clippy::missing_safety_doc in src/transport.rs

de0a19a

Add the missing `# Safety` heading to the existing safety documentation for `register()`.

[Clippy] Address clippy::missing_transmute_annotations in src/trans…

b84b7cb

…port.rs Add the missing annotations to: - `subtransport_action()` - `subtransport_free()` - `stream_free()`

[Clippy] Address clippy::from_over_into in src/tree.rs

d10b756

Convert the implementations of conversions from `TreeWalkResult` to `i32` and `raw::git_treewalk_mode` from the `Into` trait to the `From` trait.

[Clippy] Address clippy::manual_unwrap_or in src/tree.rs

9ff0abd

Simplify `treewalk_cb()` to use `Option::unwrap_or()`.

[Clippy] Address clippy::missing_safety_doc in src/util.rs

df57c81

Add safety documentation for `Binding::from_raw()` and `Binding::from_raw_opt()`.

[Clippy] Address clippy::needless_range_loop in src/util.rs

5721e99

Simplify the `fixup_windows_path()` function to iterate by mutable reference to the path bytes rather than using a range and reading and writing bytes by index.

[Clippy] Address clippy::needless_return in src/util.rs

b987823

Remove unneeded `return`s in `path_to_repo_path()`.

[Clippy] Address clippy::redundant_closure in src/worktree.rs

3aedab1

Simplify `Worktree::name()` to use the tuple variant `Some` rather than using a closure.

DanielEScherzer force-pushed the clippy-fixes branch from e45e67c to 3aedab1 Compare July 15, 2026 20:13

weihanglo approved these changes Jul 15, 2026

View reviewed changes

weihanglo enabled auto-merge July 15, 2026 20:16

weihanglo added this pull request to the merge queue Jul 15, 2026

Merged via the queue into rust-lang:main with commit fdf1a11 Jul 15, 2026
11 checks passed

rustbot removed the S-waiting-on-review Status: Waiting on review label Jul 15, 2026

DanielEScherzer deleted the clippy-fixes branch July 15, 2026 20:30

rustbot mentioned this pull request Jul 15, 2026

Pass byte slice as root to tree walk callback #1034

Open

Uh oh!

Conversation

DanielEScherzer commented Jul 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

API changes

New Default implementations

New is_empty() methods

Other

Lints expected

clippy::should_implement_trait

clippy::too_many_arguments

Remaining #[allow] attributes

Uh oh!

DanielEScherzer commented Jul 15, 2026

Uh oh!

weihanglo left a comment • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

weihanglo Jul 15, 2026

Choose a reason for hiding this comment

Uh oh!

weihanglo Jul 15, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

weihanglo Jul 15, 2026

Choose a reason for hiding this comment

Uh oh!

weihanglo Jul 15, 2026

Choose a reason for hiding this comment

Uh oh!

DanielEScherzer Jul 15, 2026

Choose a reason for hiding this comment

Uh oh!

weihanglo Jul 15, 2026

Choose a reason for hiding this comment

Uh oh!

weihanglo Jul 15, 2026

Choose a reason for hiding this comment

Uh oh!

DanielEScherzer commented Jul 15, 2026

Uh oh!

weihanglo commented Jul 15, 2026

Uh oh!

rustbot commented Jul 15, 2026

Uh oh!

weihanglo left a comment • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

DanielEScherzer commented Jul 15, 2026 •

edited

Loading

New `Default` implementations

New `is_empty()` methods

`clippy::should_implement_trait`

`clippy::too_many_arguments`

Remaining `#[allow]` attributes

weihanglo left a comment •

edited by rustbot

Loading

weihanglo left a comment •

edited by rustbot

Loading