Re: [PATCH v7 07/10] convert: unify the "auto" handling of CRLF

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

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

>>> diff --git a/convert.c b/convert.c
>>> index 24ab095..3782172 100644
>>> --- a/convert.c
>>> +++ b/convert.c
>>> @@ -227,7 +227,9 @@ static enum eol output_eol(enum crlf_action crlf_action)
>>>  		return EOL_LF;
>>>  	case CRLF_UNDEFINED:
>>>  	case CRLF_AUTO_CRLF:
>>> +		return EOL_CRLF;
>> 
>> Hmph, do we want UNDEFINED case to return EOL_CRLF here?
>> 
>>>  	case CRLF_AUTO_INPUT:
>>> +		return EOL_LF;
> One of the compilers claimed that UNDEFINED was not handled in switch-case.
> A Warning may be better ? 
>>>  	case CRLF_TEXT:
>>>  	case CRLF_AUTO:
>>>  		/* fall through */

The original made it fall-through; a very simple "a patch that makes
auto=crlf or auto=input do something different shouldn't have any
effect on the undefined case" was what triggered my reaction.

If returning EOL_CRLF does not make any sense for UNDEFINED case, it
shouldn't.  If the codeflow should not reach without crlf_action
computed to something other than UNDEFINED, then you should have a
die("BUG") there.

Looking at a larger context, you already have a warning there:

    static enum eol output_eol(enum crlf_action crlf_action)
    {
            switch (crlf_action) {
            case CRLF_BINARY:
                    return EOL_UNSET;
            ...
            case CRLF_AUTO:
                    /* fall through */
                    return text_eol_is_crlf() ? EOL_CRLF : EOL_LF;
            }
            warning("Illegal crlf_action %d\n", (int)crlf_action);
            return core_eol;
    }

which tells me that you shouldn't even have CRLF_UNDEFINED listed if
you have no intention to treat UNDEFINED as a valid input in this
codepath.  Instead just doing

	switch (crlf_action) {
        case ...: /* handle all valid inputs appropriately */
        	return ...;
	...
	default: /* the above should cover all valid cases */
		break;
	}
        warning(...);
        return ...;

and not listing CRLF_UNDEFINED in any of the case arms would make
your intention more clear.

>> 
> How about this as the commit message:
> --------------------------------------
> ...
> The 'eol' attribute had higher priority than 'text=auto'. Binary
> files may had been corrupted, and this is not what users expect to happen.
>
> Make the 'eol' attribute to work together with 'text=auto'.

The primary issue I had was that "work together" was a useful
explanation only to those who already understand what you are trying
to do with this patch.

Something like:

	Having an "eol" attribute is taken as a declaration that the
	path is text.  This may be logical (on a binary blob, you
	wouldn't be defining an "eol" attribute in the first place)
	but is not very useful if you wanted to say "I do not know
	if the path is text; inspect the contents to see if it is
	text, and apply this 'eol' conversion only if it is".

	"text=auto" attribute combined with an "eol" attribute ought
	to have meant that, but it didn't.  Make it so.

would be sufficient without any configuration example, I would think.

> With this change
>
> $ echo "* text=auto eol=crlf" >.gitattributes
> has the same effect as
> $ git config core.autocrlf true
>
> and
>
> $ echo "* text=auto eol=lf" >.gitattributes
> has the same effect as
> $ git config core.autocrlf input