There was a problem hiding this comment.
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:
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| 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.
Sorry, something went wrong.
|
|
||
| # 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) |
Sorry, something went wrong.
| - 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 |
Sorry, something went wrong.
| 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.
Sorry, something went wrong.
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.
Sorry, something went wrong.
Summary
Fixes #2013
When diff.mnemonicPrefix=true is set in git config, git uses different prefixes for diff paths instead of the standard a/ and b/:
Previously, the diff regex and decode_path() function only accepted a/ and b/ prefixes, causing create_patch=True diffs to fail parsing when users had this config enabled.
Reproduction
As described in #2013:
Changes
Testing
References