Skip to content

feat(cli): blame --ignore-rev/--ignore-revs-file (#2064) #2125

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

ali90h
Copy link

@ali90h ali90h commented Aug 19, 2025

What

Add CLI support in gix blame:

  • --ignore-rev <REV> (repeatable)
  • --ignore-revs-file <PATH> (multiple, unioned; comments/blank lines/BOM/CRLF handled)

Why

Parity with git blame --ignore-rev, enabling users to exclude formatting/refactor commits when attributing lines. Tracks #2064.

How

  • Parse flags into a HashSet<ObjectId> (de-duplicated).
  • Resolve revs via repo (accepts full SHA and tags pointing to commits).
  • Repo-root semantics for relative files: paths are resolved from the repository root, not CWD.
  • Errors: invalid revs, non-commit objects, unreadable files, and ambiguous short SHAs.

Verification

Manual matrix (all green):

  • Help shows both flags.
  • Baseline re-attribution B→A matches git blame --ignore-rev.
  • Files: comments/blank/BOM/CRLF accepted; duplicates de-duplicated; absolute + subdir paths OK.
  • Tags that point to the ignored commit work.
  • Errors produce actionable messages and non-zero exit codes.

Repro script excerpt:

cargo build -p gitoxide
GIX="$(pwd)/target/debug/gix"
"$GIX" blame -h | grep -- "--ignore-rev" && grep -- "--ignore-revs-file"

# Tiny repo: A -> B(reformat) -> C
# … create commits …
"$GIX" blame test.txt > /tmp/before
"$GIX" blame --ignore-rev "$B" test.txt > /tmp/after
diff -u /tmp/before /tmp/after   # first line re-attributed B→A

# Files, BOM/CRLF, union, repo-root semantics, tags, and error cases tested as documented.

Compatibility

AI Disclosure

AI-Assisted: Claude Code
Agent Mode: No (no autonomous actions)
Multi-line Completions: Yes — some suggestions were pasted, then edited and reviewed by me

all code human-edited and verified with clippy & the test suite

ali90h added 7 commits August 19, 2025 18:58
AI-Assisted: Claude Code
AI-Agent-Mode: no
AI-MultiLine-Completions: yes (pasted then edited/reviewed)
human-reviewed; clippy+tests green:
)

- Parse multiple --ignore-rev and --ignore-revs-file
- Repo-root semantics for relative paths
- Clear errors for non-commit/invalid/ambiguous revs
- Integrates with gix_blame::Options::with_ignored_revisions()

AI-Assisted: tool=Claude Code; agent-mode=no; multiline-completions=yes
@ali90h ali90h force-pushed the feat/blame-ignore-rev-cli branch from d2e7baa to f261c92 Compare August 19, 2025 15:58
@ali90h
Copy link
Author

ali90h commented Aug 20, 2025

CI note: The errors that appeared to have failed are due to: gix-url::issue2064_scp_username::*

that test file isn’t present on current upstream/main I’ve rebased onto the latest main and retriggered CI.
@Byron 🙏

@EliahKagan
Copy link
Member

that test file isn’t present on current upstream/main I’ve rebased onto the latest main and retriggered CI.

I wouldn't expect rebasing to affect the failures--and it doesn't seem to have affected them--because this PR itself introduces that test file in ali90h@c8bdddd.

@ali90h
Copy link
Author

ali90h commented Aug 20, 2025

I wouldn't expect rebasing to affect the failures--and it doesn't seem to have affected them--because this PR itself introduces that test file in ali90h/gitoxide@c8bdddd.

Sorry, I'll check it out.

@ali90h ali90h marked this pull request as ready for review August 20, 2025 06:10
@Byron Byron mentioned this pull request Aug 20, 2025
1 task
@cruessler
Copy link
Contributor

Just to set expectations: since this seems partly/mostly LLM-generated, this makes the PR harder to review, so it is probably going to take longer than if the code had been written by a human.

@ali90h
Copy link
Author

ali90h commented Aug 20, 2025

Just to set expectations: since this seems partly/mostly LLM-generated, this makes the PR harder to review, so it is probably going to take longer than if the code had been written by a human.

“Acknowledged. I’ve disclosed AI assistance and add commit trailers. Review focus and a repro script are in the PR body. I’m responsive to changes and happy to adjust.”

@EliahKagan EliahKagan force-pushed the feat/blame-ignore-rev-cli branch from 4ee51e3 to c6ca617 Compare August 20, 2025 15:32
@EliahKagan
Copy link
Member

EliahKagan commented Aug 20, 2025

I hope it's okay that I've rebased to drop the second-to-last commit, f261c92. This was an intentionally empty commit, which had been pushed as a workaround to force CI to rerun (since GitHub Actions doesn't allow users without write access to rerun CI jobs whose workflows are triggered by the pull_request event). My understanding is that this extra empty commit is not needed anymore, especially since another commit has been added after it.

If for any reason this change has to be undone, that can be achieved by force-pushing the branch back to 4ee51e3, where it was before.

I've noticed that gix-blame::ignore_revisions_perf zero_cost_abstraction_verification failed. But this is a performance test, and I suspect this is an intermittent failure due to random fluctuations in how long things take, rather than a new problem introduced by removing the empty commit. Maybe the test should be adjusted, or the performance of the code it seeks to test optimized further. For now, I'll try to rerun the failing test to see if it passes next time.

@Byron
Copy link
Member

Byron commented Aug 21, 2025

I feel like this PR has taken enough maintenance time as is and would appreciate if no more lines would be added to it.As initial verdict: it seems bloated for what it tries to accomplish and the original author of gix-blame is already looking into how Git is doing it. Based on that outcome we will know if the approach taken here is correct to begin with.
Thanks for your understanding.

@ali90h
Copy link
Author

ali90h commented Aug 21, 2025

Thanks @EliahKagan for cleaning up the empty commit and reruns.

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.

4 participants