| Message ID | 9b31e86f-c408-4625-8d13-f48a209b541b@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | git crashes in `git commit --patch` with diff.suppressBlankEmpty = true | expand |
Hi Ilya Thanks for reporting this and for the example reproduction. The problem is that the code that splits hunks expects context lines to begin with ' '. We could fix that fairly simply but I wonder if we should change 'diff-index' and 'diff-files' to ignore diff.suppressBlankEmpty instead. The plumbing diff commands already ignore most of the options that change diff output so I'm not quite sure why they respect this particular config setting. I've cc'd a few people to see what they think. Best Wishes Phillip On 03/07/2024 15:41, Ilya Tumaykin wrote: > Hello. > > `git commit --patch` crashes with diff.suppressBlankEmpty option enabled > under certain conditions. > > > Steps to reproduce: > 1. Prepare .gitconfig: > [user] > name = User > email = user@example.com > [diff] > suppressBlankEmpty = true > > 2. Initialize repo: > $ mkdir git_bug && cd git_bug > $ git init > $ echo -e 'test\n\n test \n\ntest' > test.txt > $ git add test.txt > $ git commit test.txt -m 'initial' > > 3. Make changes: > $ echo -e 'test\n\n test\ntest\n \n' > test.txt > > 4. Try to commit new changes > $ git commit --patch test.txt > > 5. Try to split the first hunk, press 's' in the git-commit interactive > interface. > > > Actual results: > diff --git a/test.txt b/test.txt > index 366cd4b..611ca9d 100644 > --- a/test.txt > +++ b/test.txt > @@ -1,5 +1,6 @@ > test > > - test > - > + test > test > + > + > (1/1) Stage this hunk [y,n,q,a,d,s,e,p,?]? s > BUG: add-patch.c:994: unhandled diff marker: ' > ' > Aborted (core dumped) > > > Expected results: > git-commit splits the hunk and continues. > > > Comment: > If I set diff.suppressBlankEmpty = false, then I get the expected behavior. > > git --version: 2.45.2 > OS: up-to-date Fedora 40 >
Phillip Wood <phillip.wood123@gmail.com> writes: > ... but I wonder if we > should change 'diff-index' and 'diff-files' to ignore > diff.suppressBlankEmpty instead. The plumbing diff commands already > ignore most of the options that change diff output so I'm not quite > sure why they respect this particular config setting. Very true. Even though POSIX adopted the text: It is implementation-defined whether an empty unaffected line is written as an empty line or a line containing a single <space> character. it merely allows the implementation to show an empty unaffected line as an empty line (where traditionally such an output was not allowed), and we probably want to give priority to consistent and stable output. If the addition of diff.suppressBlankEmpty were done in the usually recommended way to first add command line option to prove that the feature is useful and then add configuration variable to pretend as if it were passed, then it would have been perfect. We then could have made the plumbing to completely ignore the configuration to make the output more stable, while allowing script writers a choice to invoke plumbing commands with explicit comand line options. But that was not what happened, unfortunately. If we really wanted to force the world line to where we did what we should have done back then, I would say we need to do a two-step transition. - Add the --[no-]suppress-blank-empty option from the command line to all commands in the diff family. Plumbing diff trio will still pay attention to diff.suppressBlankEmpty but when they see it is set to any non-default value (i.e. true) without being set by the new command line option, we loudly warn that we will fix this historical mistake in Git 3.0 and encourage script writers to update their invocation of plumbing diff trio to use the command line to custimze. - At Git 3.0, plumbing diff trio stops paying attention to the diff.suppressBlankEmpty configuration. By this time, the warning we gave in earlier versions would have helped existing scripts to migrate away from relying on it and if they want they would instead explicitly pass the command line option, so we stop warning. As to the "commit -p" issue, I think the patch parser is in the wrong and needs to be corrected, period. As long as the patches given as input are well-formed, we should be prepared to grok them (we even allow manual editing of patches, right?). Thanks. https://pubs.opengroup.org/onlinepubs/9699919799/utilities/diff.html
diff --git a/test.txt b/test.txt index 366cd4b..611ca9d 100644 --- a/test.txt +++ b/test.txt @@ -1,5 +1,6 @@ test - test - + test test + +