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

From: Phillip Wood <phillip.wood123@gmail.com>
Date: Nov 14, 2024, 10:06 UTC
Message-ID: <29c81cbc-3678-4b70-9e0e-c500186d159f@gmail.com>
In-Reply-To: <xmqq4j4a8srw.fsf@gitster.g>

On 14/11/2024 02:15, Junio C Hamano wrote:

> "Usman Akinyemi via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
> [jc: As Phillip is blamed for suggesting this addition, I added him
> to the recipient of this message.]

Thanks

Show 6 quoted lines

>> From: Usman Akinyemi <usmanakinyemi202@gmail.com>
>>
>> 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

Show 19 quoted lines

>> Suggested-by: Phillip Wood <phillip.wood123@gmail.com>
>> Signed-off-by: Usman Akinyemi <usmanakinyemi202@gmail.com>
>> ---
> 
> 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.