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

[Junio C Hamano <gitster@pobox.com>]

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