Re: [PATCH v3 5/7] convert: auto_crlf=false and no attributes set: same as binary

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 9, 2016, 18:06 UTC
Message-ID: <xmqqh9hh4v2v.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <56B9F90E.2030304@web.de>

Torsten Bögershausen <tboegi@web.de> writes:

Show 12 quoted lines

>>  * convert_attrs() has "If BINARY don't do anything and return".
>>    Will the patch change behaviour for the "not-autocrlf,
>>    CRLF_GUESS" case in this codepath?  I think ca->crlf_action used
>>    to be left as CRLF_GUESS here before the patch, and now by the
>>    time the control flow reaches here it is already CRLF_BINARY.
>>    Would it affect the callers, and if so how?
> Not sure if I fully understand the question:
>
> The old CRLF_GUESS could mean (a) core.autocrlf=true,
> (b) core.autocrlf=input or (c) core.autocrlf=false.
> The callers had to look at the core.autocrlf them self.
> This patch removes (c), the next (or over next) (a) and (b)

That part I understand and fully agree.

If a function that appears much later in the control flow depends on seeing CRLF_GUESS to do certain things, and showing CRLF_BINARY to the code would make it behave differently, that would make this patch incorrect.

That is, if some function that you did not touch (hence did not appear in the context of this patch) did this:

	if (CRLF_GUESS)
        	do the "guess thing"
	else if (CRLF_BINARY)
        	do the "binary thing"
	else
        	do other things

then depending on how 'guess thing' and 'binary thing' work differently, this patch would change the outcome. If the "guess thing" had

		if (!AUTOCRLF) goto "binary thing"

upfront, then it obviously is OK, though ;-)

If the _ONLY_ way all the code that use CRLF_GUESS is

	if ((CRLF_GUESS && !AUTOCRLF) || CRLF_BINARY)
        	do the "binary thing"

then the conversion done by the patch is perfectly fine. It was unclear from the patch text and the proposed log message if you made sure that is the case. And with your response, it still isn't clear to me.