Re: [PATCH v1 1/1] git diff --quiet exits with 1 on clean tree with CRLF conversions

From: Torsten Bögershausen <tboegi@web.de>
Date: Mar 4, 2017, 06:25 UTC
Message-ID: <9c9eeb35-e1c1-ec46-1d85-ef6a05886880@web.de>
In-Reply-To: <xmqq37eu2qxl.fsf@junio-linux.mtv.corp.google.com>

On 2017-03-03 18:47, Junio C Hamano wrote:

Show 24 quoted lines

> Torsten Bögershausen <tboegi@web.de> writes:
> 
>> Understood, thanks for the explanation.
>>
>> quiet is not quite any more..
>>
>> Does the following fix help ?
>>
>> --- a/diff.c
>> +++ b/diff.c
>> @@ -2826,6 +2826,8 @@ int diff_populate_filespec(struct diff_filespec *s,
>> unsigned int flags)
>>         enum safe_crlf crlf_warn = (safe_crlf == SAFE_CRLF_FAIL
>>                                     ? SAFE_CRLF_WARN
>>                                     : safe_crlf);
>> +       if (size_only)
>> +               crlf_warn = SAFE_CRLF_FALSE;
> 
> If you were to go this route, it may be sufficient to change its
> initialization from WARN to FALSE _unconditionally_, because this
> function uses the convert_to_git() only to _show_ the differences by
> computing canonical form out of working tree contents, and the
> conversion is not done to _write_ into object database to create a
> new object.

Hm, since when (is it not used) ?

I thought that it is needed to support the safecrlf handling introduced in
21e5ad50fc5e7277c74cfbb3cf6502468e840f86
Author: Steffen Prohaska <prohaska@zib.de>
Date:   Wed Feb 6 12:25:58 2008 +0100

    safecrlf: Add mechanism to warn about irreversible crlf conversions

-------------
The SAFE_CRLF_FAIL was converted into WARN here:
commit 5430bb283b478991a979437a79e10dcbb6f20e28
Author: Junio C Hamano <gitster@pobox.com>
Date:   Mon Jun 24 14:35:04 2013 -0700

    diff: demote core.safecrlf=true to core.safecrlf=warn

    Otherwise the user will not be able to start to guess where in the
    contents in the working tree the offending unsafe CR lies.
------------

My understanding is that we don't want to break the safecrlf feature, but after applying

diff --git a/diff.c b/diff.c
index a628ac3a95..a05d88dd9f 100644
--- a/diff.c
+++ b/diff.c
@@ -2820,12 +2820,10 @@ int diff_populate_filespec(struct diff_filespec *s,
unsigned int flags)
        int size_only = flags & CHECK_SIZE_ONLY;
        int err = 0;
        /*
-        * demote FAIL to WARN to allow inspecting the situation
-        * instead of refusing.
+        * Don't use FAIL or WARN as this code is not called when _writing_
+        * into object database to create a new object.
         */
-       enum safe_crlf crlf_warn = (safe_crlf == SAFE_CRLF_FAIL
-                                   ? SAFE_CRLF_WARN
-                                   : safe_crlf);
+       enum safe_crlf crlf_warn = SAFE_CRLF_FALSE;


None of the test cases in t0020--t0027 fails or complain about missing warnings.
Does this all means that, looking back,  5430bb283b478991 could have been more
aggressive and could have used SAFE_CRLF_FALSE ?
And we can do this change now?

(If the answer is yes, we don't need to deal with the problem below)
> Having size_only here is not a sign of getting --quiet passed from
> the command line, by the way.
>