Fix diff parsing to support mnemonicPrefix configuration#2092
Conversation
Fixes gitpython-developers#2013 When diff.mnemonicPrefix=true is set in git config, git uses different prefixes for diff paths instead of the standard a/ and b/: - c/ for commit - w/ for worktree - i/ for index - o/ for object - h/ for HEAD Previously, the diff regex and decode_path() function only accepted a/ 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.
Pull request overview
This PR fixes diff parsing to support Git's diff.mnemonicPrefix configuration option, which was causing parse failures when users enabled this feature. When diff.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) |
There was a problem hiding this comment.
The comment is missing the h/ 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 |
There was a problem hiding this comment.
The docstring is missing the h/ 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/ for object | |
| - o/ for object | |
| - h/ for HEAD |
test/test_diff.py
Outdated
| def test_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") |
There was a problem hiding this comment.
The test only covers c/ and w/ 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 the decode_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.
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.
… values Changes 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 correctly with 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