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

Torsten Bögershausen [off-list ref] writes:

quoted

 * 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.