Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork964
Fix diff parsing to support mnemonicPrefix configuration#2092
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Conversation
Fixesgitpython-developers#2013When diff.mnemonicPrefix=true is set in git config, git uses differentprefixes for diff paths instead of the standard a/ and b/:- c/ for commit- w/ for worktree- i/ for index- o/ for object- h/ for HEADPreviously, the diff regex and decode_path() function only accepteda/ and b/ prefixes, causing create_patch=True diffs to fail parsing.Changes:- Update re_header regex to accept [abciwoh]/ prefixes- Update decode_path() assertion to accept all valid mnemonic prefixes- Add test case for mnemonicPrefix-style diffs
- Remove whitespace from blank lines in docstrings (ruff W293)- Use !r format specifier for bytes in f-string (mypy str-bytes-safe)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Pull request overview
This PR fixes diff parsing to support Git'sdiff.mnemonicPrefix configuration option, which was causing parse failures when users enabled this feature. Whendiff.mnemonicPrefix=true is set, Git uses alternative path prefixes (c/, w/, i/, o/, h/) instead of the standard a/ and b/ prefixes in diff output.
Key changes:
- Updated the
re_headerregex pattern to accept all valid mnemonic prefixes in addition to standard a/b prefixes - Modified
decode_path()function to validate and strip all supported prefix types - Added test coverage for mnemonicPrefix-style diffs
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| git/diff.py | Updated regex pattern and decode_path() function to support c/, w/, i/, o/, and h/ prefixes alongside standard a/ and b/ prefixes |
| test/test_diff.py | Added test case verifying diff parsing works correctly with mnemonicPrefix-style c/ and w/ prefixes |
💡Add Copilot custom instructions for smarter, more guided reviews.Learn how to get started.
git/diff.py Outdated
| # Precompiled regex. | ||
| # Note: The path prefixes support both default (a/b) and mnemonicPrefix mode | ||
| # which can use prefixes like c/ (commit), w/ (worktree), i/ (index), o/ (object) |
CopilotAIDec 8, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The comment is missing theh/ prefix for HEAD. According to the PR description and the Git documentation,h/ is also a valid mnemonicPrefix. The comment should list all supported prefixes: c/ (commit), w/ (worktree), i/ (index), o/ (object), and h/ (HEAD).
| # which can use prefixes like c/ (commit), w/ (worktree), i/ (index), o/ (object) | |
| # which can use prefixes like c/ (commit), w/ (worktree), i/ (index), o/ (object), and h/ (HEAD) |
| - c/ for commit | ||
| - w/ for worktree | ||
| - i/ for index | ||
| - o/ for object |
CopilotAIDec 8, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The docstring is missing theh/ prefix for HEAD. According to the PR description and Git documentation,h/ is also a valid mnemonicPrefix. The comment should list all supported prefixes: c/ (commit), w/ (worktree), i/ (index), o/ (object), and h/ (HEAD).
| -o/forobject | |
| -o/forobject | |
| -h/forHEAD |
test/test_diff.py Outdated
| deftest_diff_mnemonic_prefix(self): | ||
| """Test that diff parsing works with mnemonicPrefix enabled. | ||
| When diff.mnemonicPrefix=true is set in git config, git uses different | ||
| prefixes for diff paths: | ||
| - c/ for commit | ||
| - w/ for worktree | ||
| - i/ for index | ||
| - o/ for object | ||
| This addresses issue #2013 where the regex only matched [ab]/ prefixes. | ||
| """ | ||
| # Create a diff with mnemonicPrefix-style c/ and w/ prefixes | ||
| # Using valid 40-char hex SHAs | ||
| diff_mnemonic=b"""diff --git c/.vscode/launch.json w/.vscode/launch.json | ||
| index 1234567890abcdef1234567890abcdef12345678..abcdef1234567890abcdef1234567890abcdef12 100644 | ||
| --- c/.vscode/launch.json | ||
| +++ w/.vscode/launch.json | ||
| @@ -1,3 +1,3 @@ | ||
| -old content | ||
| +new content | ||
| """ | ||
| diff_proc=StringProcessAdapter(diff_mnemonic) | ||
| diffs=Diff._index_from_patch_format(self.rorepo,diff_proc) | ||
| # Should parse successfully (previously would fail or return empty) | ||
| self.assertEqual(len(diffs),1) | ||
| diff=diffs[0] | ||
| # The path should be extracted correctly (without the c/ or w/ prefix) | ||
| self.assertEqual(diff.a_path,".vscode/launch.json") | ||
| self.assertEqual(diff.b_path,".vscode/launch.json") |
CopilotAIDec 8, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The test only coversc/ andw/ prefixes but doesn't test the other valid mnemonicPrefix options (i/,o/,h/). Consider adding test cases for these prefixes as well to ensure the regex pattern[abciwoh] and thedecode_path() function work correctly with all supported prefixes. This could be done either by adding more test data in this test or creating a parameterized test.
Byron left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Thanks a lot for contributing! I particularly like the exhaustive documentation around the matter, and what I see looks good!
The test is also something I'd love to see improved, and I side with Copilot here.
… valuesChanges per reviewer (Byron) and Copilot suggestions:- Add h/ (HEAD) to the comment listing supported prefixes- Update test docstring to include h/ prefix- Expand test to cover all prefix combinations using subTest: - c/ (commit) vs w/ (worktree) - c/ (commit) vs i/ (index) - i/ (index) vs w/ (worktree) - o/ (object) vs w/ (worktree) - h/ (HEAD) vs i/ (index) - h/ (HEAD) vs w/ (worktree)This ensures the regex pattern [abciwoh] and decode_path() work correctlywith all supported mnemonicPrefix values.
Summary
Fixes#2013
When
diff.mnemonicPrefix=trueis set in git config, git uses different prefixes for diff paths instead of the standarda/andb/:c/for commitw/for worktreei/for indexo/for objecth/for HEADPreviously, the diff regex and
decode_path()function only accepteda/andb/prefixes, causingcreate_patch=Truediffs to fail parsing when users had this config enabled.Reproduction
As described in#2013:
Changes
re_headerregex to accept[abciwoh]/prefixes instead of just[ab]/decode_path()assertion to accept all valid mnemonic prefixesTesting
test_diff_mnemonic_prefixverifies parsing works withc/andw/prefixesReferences