refactor(args): centralize include-path resolution after repo discovery#1379
refactor(args): centralize include-path resolution after repo discovery#1379o1x3 wants to merge 1 commit intoorhun:mainfrom
Conversation
Extract `resolve_include_paths` to handle workdir, cwd, and config include paths in one place, after the repository root is known. This fixes --workdir producing empty output since v2.11.0 (orhun#1369). The previous logic set include_path in the workdir block before repo discovery, producing patterns that were absolute or cwd-relative instead of repo-relative. These never matched the repo-relative paths from git2's diff API. All three paths (root, cwd, workdir) are now canonicalized before comparison, fixing symlink edge cases on macOS.
|
hey @orhun @ognis1205 - draft is up for the refactor we discussed in #1369. main change is pulling the include-path resolution into one function that runs after repo discovery instead of the three scattered spots. still a draft so happy to rework anything based on feedback. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1379 +/- ##
==========================================
+ Coverage 47.53% 48.18% +0.66%
==========================================
Files 24 24
Lines 2119 2140 +21
==========================================
+ Hits 1007 1031 +24
+ Misses 1112 1109 -3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Thanks for the effort!
I'm quite cautious about this PR, especially around the directory handling changes.
Historically, this area has been fragile: fixing one bug has often led to unexpected behavior elsewhere, and we've gone back and forth with multiple fixes. Because of that history, I'm very hesitant to introduce substantial changes now.
On top of that, I believe the git-cliff crate as a whole needs a broader refactoring. Although this PR is labeled as a refactoring, I feel it also contains unclear functional changes, and I'm not convinced this is the right time for such a refactoring.
In particular, simply consolidating directory handling into a single function doesn't address the root issues. Given the past instability, I'm concerned we'll reintroduce bugs that we haven't fully anticipated.
Personally, I'd prefer to avoid major changes to the existing implementation for now, and instead fix the workdir issue based on the current code. I think that would let us isolate the problem more cleanly and reduce the risk of regressions.
cc: @orhun any thoughts on this?
| let canonical_root = fs::canonicalize(&root).unwrap_or(root); | ||
| let canonical_cwd = fs::canonicalize(&cwd).unwrap_or(cwd); |
There was a problem hiding this comment.
Regarding the use of unwrap on fs::canonicalize:
According to the documentation:
https://doc.rust-lang.org/std/fs/fn.canonicalize.html#errors
fs::canonicalize can fail in several common cases, such as when the path does not exist or a non-final component is not a directory. Propagating the error instead of using unwrap would allow us to inform the user about what went wrong.
| let canonical_workdir = args | ||
| .workdir | ||
| .as_ref() | ||
| .map(|w| fs::canonicalize(w).unwrap_or(w.clone())); |
There was a problem hiding this comment.
| if pattern_path.is_absolute() { | ||
| if let Ok(stripped) = pattern_path.strip_prefix(canonical_root) { | ||
| return Ok(Pattern::new(stripped.to_string_lossy().as_ref())?); | ||
| } | ||
| } |
There was a problem hiding this comment.
Is this a new feature? I believe this absolute path handling should not be part of this PR's scope. The previous implementation didn't have special handling for absolute paths.
If we want to add this behavior, it should be done in a separate PR with proper tests and documentation.
| patterns.push(Pattern::new(glob.to_string_lossy().as_ref())?); | ||
| } | ||
| } | ||
| } else if patterns.is_empty() && |
There was a problem hiding this comment.
I'm not sure if this patterns.is_empty() condition is necessary here.
Description
Extracts path resolution for
--workdirand--include-pathinto a singleresolve_include_pathsfunction that runs after repo discovery, where the repo root is available.Previously, the workdir block set
args.include_pathto a raw workdir path before repo discovery (absolute or cwd-relative), which never matched the repo-relative paths from git2's diff API. This broke--workdirin v2.11.0.The new function handles three cases:
--workdirset to a subdirectory adds a repo-relative glob; workdir at root means no filterAll paths (root, cwd, workdir) are canonicalized before comparison to handle symlinks.
Motivation and Context
Fixes #1369
As discussed in the issue, the path resolution logic was scattered across three locations in
lib.rs. @orhun asked for a proper refactor rather than a band-aid fix.How Has This Been Tested?
resolve_include_pathscovering: workdir at root, workdir subdirectory, workdir outside repo, implicit cwd scoping, no scoping at root, repository arg suppressing implicit scoping, config patterns combined with workdirnormalize_to_repo_relativetest-workdir-root(--workdir .with empty + file-changing commits)cargo clippy --tests -- -D warningscargo +nightly fmt --all -- --checkTypes of Changes
Checklist:
cargo +nightly fmt --allcargo clippy --tests --verbose -- -D warningscargo test