From: Phillip Wood Date: Thu, 14 Nov 2024 10:06:12 GMT Subject: Re: [PATCH v2] diff: update conflict handling for whitespace to issue a warning Message-ID: <29c81cbc-3678-4b70-9e0e-c500186d159f@gmail.com> In-Reply-To: On 14/11/2024 02:15, Junio C Hamano wrote: > "Usman Akinyemi via GitGitGadget" writes: > > [jc: As Phillip is blamed for suggesting this addition, I added him > to the recipient of this message.] Thanks >> From: Usman Akinyemi >> >> Modify the conflict resolution between tab-in-indent and >> indent-with-non-tab to issue a warning instead of terminating >> the operation with `die()`. Update the `git diff --check` test to >> capture and verify the warning message output. 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]. As I mentioned in [2] I think we only want to change the behavior when parsing whitespace attributes - we still want the other callers of parse_whitespace_rule() to die() so the user can fix their config or commandline. We can do that by adding a boolean parameter called "gentle" that determines whether we call warning() or die(). Best Wishes Phillip [1] https://lore.kernel.org/git/e4a70501-af2d-450a-a232-4c7952196a74@gmail.com [2] https://lore.kernel.org/git/3c081d3c-3f6f-45ff-b254-09f1cd6b7de5@gmail.com >> Suggested-by: Phillip Wood >> Signed-off-by: Usman Akinyemi >> --- > > If the settings requires an impossible way to use whitespaces, the > settings is buggy, and it generally would be better to correct the > setting before moving on. > > I am curious to know in what situations this new behaviour can be > seen as an improvement. It may allow you to go on _without_ fixing > such a broken setting, but how would it help the end user? If the > user set both of these mutually-incompatible options A and B by > mistake, but what the user really wanted to check for was A, picking > just one of A or B arbitrarily and disabling it would not help, and > disabling both would not help, either. But wouldn't the real source > of the problem be that we are trying to demote die() to force the > user to correct contradictiong setting into warning()? > > Thanks.