Skip to content

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

Open
SergioChan wants to merge 2 commits intoorhun:mainfrom
SergioChan:cron/workdir-1369
Open

fix(args): handle --workdir include paths relative to repo root#1428
SergioChan wants to merge 2 commits intoorhun:mainfrom
SergioChan:cron/workdir-1369

Conversation

@SergioChan
Copy link
Copy Markdown

Description

Fixes --workdir path handling so repository-root workdirs no longer force an include filter that drops commits.

Motivation and Context

Fixes #1369.

Previously, --workdir always injected an include glob from the raw workdir path. When workdir pointed to the repository root (or a parent directory in combinations like --repository ... --workdir ./), the generated include pattern did not match commit file paths, producing an empty changelog.

This change:

  • Stops injecting args.include_path early from --workdir.
  • Resolves --workdir against the discovered repo root later (in process_repository).
  • Only applies a derived include pattern when workdir is inside the repo and not equal to repo root.

That preserves subdirectory scoping while avoiding false-empty output for root workdirs.

How Has This Been Tested?

Local Rust toolchain is unavailable in this environment (cargo not installed), so I could not run cargo test / clippy here.

Validation performed:

  • Reproduced and inspected the failing argument/path flow in run_with_changelog_modifier + process_repository.
  • Verified the updated logic path-by-path against the issue repro scenarios:
    • git-cliff --workdir . from repo root: no include filter forced.
    • git-cliff --workdir <repo-dir> from parent: no include filter forced.
    • git-cliff --repository <repo-dir> --workdir ./ from parent: no include filter forced.
    • git-cliff --workdir <subdir>: include filtering still applied for subdirectory scope.

Screenshots / Logs (if applicable)

N/A

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

@SergioChan SergioChan requested a review from orhun as a code owner March 10, 2026 03:48
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Mar 10, 2026

Codecov Report

❌ Patch coverage is 0% with 11 lines in your changes missing coverage. Please review.
✅ Project coverage is 47.37%. Comparing base (fb906bc) to head (20fff96).
⚠️ Report is 13 commits behind head on main.

Files with missing lines Patch % Lines
git-cliff/src/lib.rs 0.00% 11 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1428      +/-   ##
==========================================
- Coverage   47.57%   47.37%   -0.20%     
==========================================
  Files          24       24              
  Lines        2119     2128       +9     
==========================================
  Hits         1008     1008              
- Misses       1111     1120       +9     
Flag Coverage Δ
unit-tests 47.37% <0.00%> (-0.20%) ⬇️

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.

@orhun
Copy link
Copy Markdown
Owner

orhun commented Mar 13, 2026

Hey, thanks for the PR. This probably fixes the issue but I don't think we agreed to go with a fix like this, see #1369 (comment)

It further complicates the logic and we probably want to attack this on a wider scale (by refactoring some parts and adding some test fixtures first.)

cc @ognis1205

@ognis1205
Copy link
Copy Markdown
Contributor

Thanks for the PR and for looking into this issue!

Just to give some context. Another contributor is already working on the same problem in a different PR:

#1379

For now I'm trying to contact the author of that PR since they opened it earlier, and I'd like to prioritize based on the timeline. Depending on how things evolve, we might still move forward with this PR or potentially collaborate on it. Also, sorry that the issue wasn't explicitly assigned to anyone.

On my side, I'm planning to prepare some test fixtures soon to make it easier to work on this area:

#1324

If you're interested in helping with that as well, feel free to take it.

@SergioChan
Copy link
Copy Markdown
Author

Thanks for the context and the update.

Makes sense to prioritize #1379 since it came first. I’m happy to defer to that track to avoid duplicate work.
If it helps, I can also contribute on the fixtures/testing work in #1324.

@ognis1205
Copy link
Copy Markdown
Contributor

Thanks for the context and the update.

Makes sense to prioritize #1379 since it came first. I’m happy to defer to that track to avoid duplicate work. If it helps, I can also contribute on the fixtures/testing work in #1324.

Thanks, that sounds great!

I'll assign you to #1324 for the fixtures work 👍
Let me know if you need any context or help getting started.

@ognis1205
Copy link
Copy Markdown
Contributor

@SergioChan

Could you leave a quick comment on #1324 so I can assign you to it?

@SergioChan
Copy link
Copy Markdown
Author

Done — I just commented on #1324 and I’m ready to help with the fixtures/test coverage there.

@SergioChan
Copy link
Copy Markdown
Author

Pushed follow-up commit 20fff96 to address the failing Formatting check.

What changed

  • Applied the rustfmt-expected line wrap in git-cliff/src/lib.rs at the resolved_workdir assignment inside process_repository.
  • No logic changes; formatting-only adjustment.

Validation

  • git diff --check (clean)
  • I couldn't run cargo +nightly fmt --check locally because the Rust toolchain isn't available in this environment, so CI should confirm the formatter job is now green.

@ognis1205
Copy link
Copy Markdown
Contributor

Hi @SergioChan cc: @o1x3

Sorry for the slow progress. The author of the earlier PR addressing the same issue hasn't responded yet. If you still intend to move forward with this PR, would you like to continue handling it?

@o1x3
Copy link
Copy Markdown

o1x3 commented Apr 9, 2026

@ognis1205 @SergioChan sorry for the delay on my side. I had to take some time off for personal reasons, but I am back now.

Thanks Sergio for carrying the issue while I was inactive. I reworked #1379 locally into the smaller --workdir fix that was requested in review, so I think it still makes sense to keep the implementation on that PR and avoid having two overlapping fixes open.

It also still makes sense to me for fixture work to stay on #1324, unless there is a specific test case we should coordinate on.

Happy to collaborate too if that would help.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Setting --workdir results in an empty output

5 participants