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

[Usman Akinyemi <usmanakinyemi202@gmail.com>]

On Fri, Nov 15, 2024 at 12:11 AM Junio C Hamano <gitster@pobox.com> 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?
>
> > 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().
>
> I doubt that such a complexity is warranted.
>
> It depends on the size of diff you are showing, but if it is large,
> then giving a small warning that gets buried in the large diff is a
> conter-productive way to encourage users to correct such broken
> setting.  If it is small, then the damage may not be too bad, but
> still, we are showing what the user did not really request.
>
> If we were to fix anything, it is to make sure that we die() before
> producing a single line of output.  If you have a change to a path
> whose "type" is without such a misconfigured attribute, that sorts
> lexicographically earlier than another path with a change, with a
> conflicting whitespace attribute, I suspect that with the way the
> code is structured currently, we show the diff for the first path,
> before realizing that the second path has an issue and then die.
>
> If we fix it, and then make sure that the die() message shows
> clearly what attribute setting we did not like, that would be
> sufficient to help users to locate the problem, fix it, and quickly
> move on, no?
Hi Junio,

Thanks for the review. From what I understand from your comment,
we should leave it the way it was which was die right ?

Thanks.
Usman.
>
> Thanks.