diff mbox series

git crashes in `git commit --patch` with diff.suppressBlankEmpty = true

Message ID 9b31e86f-c408-4625-8d13-f48a209b541b@gmail.com (mailing list archive)
State New, archived
Headers show
Series git crashes in `git commit --patch` with diff.suppressBlankEmpty = true | expand

Commit Message

Ilya Tumaykin July 3, 2024, 2:41 p.m. UTC
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:
(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

Comments

Phillip Wood July 4, 2024, 1:14 p.m. UTC | #1
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
>
Junio C Hamano July 5, 2024, 4:39 p.m. UTC | #2
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 mbox series

Patch

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
+
+