Re: [PATCH v6] safecrlf: Add mechanism to warn about irreversible crlf conversions

From: Steffen Prohaska <prohaska@zib.de>
Date: Feb 3, 2008, 14:36 UTC
Message-ID: <4C024D67-9FA5-451D-BB91-CE9464C6F50D@zib.de>
In-Reply-To: <7vbq6yuxeh.fsf@gitster.siamese.dyndns.org>

On Feb 3, 2008, at 11:50 AM, Junio C Hamano wrote:

Show 9 quoted lines

> Steffen Prohaska <prohaska@zib.de> writes:
>
>> CRLF conversion bears a slight chance of corrupting data.
>> ...
>> thing to do, while for binary file it corrupts data.
>
> The above 25-line or so are well written and deserve to be in
> the end user documentation somewhere, I think, to explain why it
> is a good idea to have these warnings to them..

For now, I propose to add it to the documentation of core.safecrlf. In the future a section on cross-platform projects in the user manual would probably be the right place. But the manual does not yet have such a section.

Show 13 quoted lines

>> This commit modifies git apply to fail even if safecrlf=warn,
>> because git apply writes its changes back to the work tree
>> immediately.  The user would not have a chance to backup the old
>> version of the file if only a warning was printed.
>
> I do not get this logic at all.
>
> The whole point of git-apply is to apply the patch.  If you say
> --whitespace=fix and some contents (say one of the testsuite
> files in our t/ directory) needed to keep trailing newline, you
> obviously are left with a broken result, and you would recover
> by checking it out from index or HEAD and reapply.  Why
> shouldn't the same principle hold here?

You are right. All files should be committed before running git apply and therefore the original files can be recovered by git checkout. Hmm ... so apply should either just warn or be completely quiet as git blame is? I think it should warn.

	Steffen