Re: [PATCH v2] diff: update conflict handling for whitespace to issue a warning

[Phillip Wood <phillip.wood123@gmail.com>]

On 15/11/2024 00:11, Junio C Hamano wrote:
> Phillip Wood <phillip.wood123@gmail.com> writes:
> 
>> Usman - when you're writing a commit message it is important to
>> explain the reason for making the changes contained in the patch so
>> others can understand why it is a good idea. In this case the idea is
>> to avoid breaking "git diff" for everyone who clones a repository
>> containing a .gitattributes file with bad whitespace attributes
>> [1].
> 
> Hmph, it would certainly be a problem, but the right solution is not
> to butcher Git, but to make it easier for the participants of such a
> project to know what is broken *and* what needs to be updated, to let
> them move forward, no?

Arguably yes, but that's not the approach we take when the attributes 
file is too large, a line in the file is is too long or the file 
contains a negative filename pattern. For those cases we print a warning 
and continue. The recently merged e36f009e69b (merge: replace atoi() 
with strtol_i() for marker size validation, 2024-10-24) followed suit 
and warns rather than dies for an invalid marker size. It would be nice 
to be consistent in the way we treat invalid attributes. Consistently 
dying and telling the user how to fix the problem would be a reasonable 
approach on the client side but I wonder if it could cause problems for 
forges running "git diff" and "git merge-tree" on a server though.

> [...]
> If we were to fix anything, it is to make sure that we die() before
> producing a single line of output.

That would certainly be a good idea

Best Wishes

Phillip