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

From: Phillip Wood <phillip.wood123@gmail.com>
Date: Nov 19, 2024, 16:49 UTC
Message-ID: <dc092d9e-d95c-4635-b4f9-85cf1802e571@gmail.com>
In-Reply-To: <xmqqbjyh5pa5.fsf@gitster.g>

On 15/11/2024 00:11, Junio C Hamano wrote:

Show 13 quoted lines

> 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