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

[Junio C Hamano <gitster@pobox.com>]

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?

Thanks.