refactor(args): centralize include-path resolution after repo discovery by o1x3 · Pull Request #1379 · orhun/git-cliff

o1x3 · 2026-02-22T21:53:03Z

Description

Extracts path resolution for --workdir and --include-path into a single resolve_include_paths function that runs after repo discovery, where the repo root is available.

Previously, the workdir block set args.include_path to 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 --workdir in v2.11.0.

The new function handles three cases:

Config/CLI include paths are normalized to repo-relative (strips absolute repo root prefix)
--workdir set to a subdirectory adds a repo-relative glob; workdir at root means no filter
Implicit cwd scoping when running from a subdirectory (existing behavior, preserved)

All 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?

9 unit tests for resolve_include_paths covering: workdir at root, workdir subdirectory, workdir outside repo, implicit cwd scoping, no scoping at root, repository arg suppressing implicit scoping, config patterns combined with workdir
2 unit tests for normalize_to_repo_relative
New fixture test test-workdir-root (--workdir . with empty + file-changing commits)
Manual verification of all scenarios from the issue repro script
cargo clippy --tests -- -D warnings
cargo +nightly fmt --all -- --check

Types of Changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)
Documentation (no code change)
Refactor (refactoring production code)
Other

Checklist:

My code follows the code style of this project.
I have updated the documentation accordingly (if applicable).
I have formatted the code with rustfmt.
- cargo +nightly fmt --all
I checked the lints with clippy.
- cargo clippy --tests --verbose -- -D warnings
I have added tests to cover my changes.
All new and existing tests passed.
- cargo test

o1x3 · 2026-02-22T21:54:04Z

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-commenter · 2026-02-22T21:58:06Z

Codecov Report

❌ Patch coverage is 68.00000% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 47.66%. Comparing base (39cd76e) to head (eb1f825).

Files with missing lines	Patch %	Lines
git-cliff/src/lib.rs	68.00%	8 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1379      +/-   ##
==========================================
+ Coverage   47.35%   47.66%   +0.31%     
==========================================
  Files          24       24              
  Lines        2131     2153      +22     
==========================================
+ Hits         1009     1026      +17     
- Misses       1122     1127       +5

Flag	Coverage Δ
unit-tests	`47.66% <68.00%> (+0.31%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

ognis1205

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?

ognis1205 · 2026-03-03T07:50:18Z

+        let canonical_root = fs::canonicalize(&root).unwrap_or(root);
+        let canonical_cwd = fs::canonicalize(&cwd).unwrap_or(cwd);


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.

I removed the canonicalize based approach from this PR while narrowing the scope.

ognis1205 · 2026-03-03T07:50:36Z

+        let canonical_workdir = args
+            .workdir
+            .as_ref()
+            .map(|w| fs::canonicalize(w).unwrap_or(w.clone()));


Same as https://github.com/orhun/git-cliff/pull/1379/changes#r2876700091 .

I removed the canonicalize based approach from this PR while narrowing the scope.

ognis1205 · 2026-03-04T03:39:36Z

+    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())?);
+        }
+    }


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.

Removed from this PR. I agree that this was extra behavior and not needed for the --workdir fix itself.

ognis1205 · 2026-03-04T03:45:40Z

+                patterns.push(Pattern::new(glob.to_string_lossy().as_ref())?);
+            }
+        }
+    } else if patterns.is_empty() &&


I'm not sure if this patterns.is_empty() condition is necessary here.

Removed with the larger refactor. The current version keeps the existing structure and only changes the --workdir handling.

ognis1205 · 2026-03-16T17:45:48Z

Thanks again for working on this.

As mentioned in my review, I'm still a bit hesitant about the scope of the changes here. Given that this area has historically been quite fragile, I'd prefer to keep the existing implementation mostly intact for now and focus only on fixing the --workdir issue.

Would you be open to adjusting this PR to target the --workdir bug while preserving the current structure as much as possible?

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.

o1x3 · 2026-04-09T13:13:17Z

@ognis1205 @orhun sorry for the delay on my side. I had to take some time off for personal reasons, but I am back now and still available to take this forward.

I went back through the review feedback and reworked this into a smaller fix for the --workdir bug locally. I dropped the broader include path refactor and the extra path normalization changes from this PR.

What changed in the local rework:

stopped setting include_path early from --workdir
kept the current cwd scoping logic in place
only add a workdir include pattern later, after repo discovery, when the workdir is inside the repo and not the repo root
kept focused tests for the workdir cases
kept the root --workdir . fixture test

I also ran:

cargo test --package git-cliff
cargo clippy --tests --verbose --package git-cliff -- -D warnings
cargo +nightly fmt --all -- --check

I still need to push the narrowed version, but the broader refactor pieces are out now. Please take another look when I push the update.

Also happy to collaborate if that would make this easier to move forward.

orhun requested a review from ognis1205 February 23, 2026 11:11

o1x3 marked this pull request as ready for review February 24, 2026 06:47

o1x3 requested a review from orhun as a code owner February 24, 2026 06:47

ognis1205 suggested changes Mar 4, 2026

View reviewed changes

ognis1205 mentioned this pull request Mar 16, 2026

fix(args): handle --workdir include paths relative to repo root #1428

Open

15 tasks

o1x3 force-pushed the refactor/centralize-include-path-resolution branch from be7da48 to 6f387c8 Compare April 9, 2026 12:34

fix(args): narrow workdir include-path handling

eb1f825

		let canonical_root = fs::canonicalize(&root).unwrap_or(root);
		let canonical_cwd = fs::canonicalize(&cwd).unwrap_or(cwd);

Uh oh!

Conversation

o1x3 commented Feb 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Motivation and Context

How Has This Been Tested?

Types of Changes

Checklist:

Uh oh!

o1x3 commented Feb 22, 2026

Uh oh!

codecov-commenter commented Feb 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ognis1205 commented Mar 16, 2026

Uh oh!

o1x3 commented Apr 9, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

o1x3 commented Feb 22, 2026 •

edited

Loading

codecov-commenter commented Feb 22, 2026 •

edited

Loading

ognis1205 left a comment •

edited

Loading