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

["Torsten Bögershausen" <tboegi@web.de>]

On 25.04.16 21:37, Junio C Hamano wrote:
> tboegi@web.de writes:
Thanks for review.
Some comments are inline, and a suggestion to a new commit message at the end.
[]
>> diff --git a/Documentation/config.txt b/Documentation/config.txt
>> index 4a27ad4..9caf6ae 100644
>> --- a/Documentation/config.txt
>> +++ b/Documentation/config.txt
>> @@ -389,13 +389,13 @@ file with mixed line endings would be reported by the `core.safecrlf`
>>  mechanism.
>>  
>>  core.autocrlf::
>> -	Setting this variable to "true" is almost the same as setting
>> -	the `text` attribute to "auto" on all files except that text
>> -	files are not guaranteed to be normalized: files that contain
>> -	`CRLF` in the repository will not be touched.  Use this
>> -	setting if you want to have `CRLF` line endings in your
>> -	working directory even though the repository does not have
>> -	normalized line endings.  This variable can be set to 'input',
>> +	Setting this variable to "true" is the same as setting
>> +	the `text` attribute to "auto" on all files and core.eol to "crlf".
>> +	Set to true if you want to have `CRLF` line endings in your
>> +	working directory and the repository has LF line endings.
>> +	Text files are guaranteed not to be normalized: files that contain
>> +	`CRLF` in the repository will not be touched.
> 
> This is not a new problem but the last sentence and a half look
> bad.  Telling readers "X is not guaranteed to happen" is not all
> that useful--telling them what happens is.  Also the use of colon
> there is probably ungrammatical.

> 
> 	Set to true if you want to have CRLF line endings in your
> 	working directory and LF line endings in the repository.
> 	Text files that contain CRLF in the repository will not get
> 	their end-of-line converted.
> 
> perhaps?
OK, but may be 
s/end-of-line/line endings/
----------
Set to true if you want to have CRLF line endings in your
working directory and LF line endings in the repository.
Text files that contain CRLF in the repository will not get
their line endings converted.


> 
>> diff --git a/convert.h b/convert.h
>> index ccf436b..81b6cdf 100644
>> --- a/convert.h
>> +++ b/convert.h
>> @@ -7,7 +7,8 @@
>>  enum safe_crlf {
>>  	SAFE_CRLF_FALSE = 0,
>>  	SAFE_CRLF_FAIL = 1,
>> -	SAFE_CRLF_WARN = 2
>> +	SAFE_CRLF_WARN = 2,
>> +	SAFE_CRLF_RENORMALIZE = 4
> 
> Are these bits in a word?  If not where did 3 go?
We use an enum, sometimes mixed with an int, so my brain automatically
changed it into a bit field.
"3" is probably better.
> 
>> 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 */
> 
How about this as the commit message:
--------------------------------------

convert: unify the "auto" handling of CRLF

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

Before this change,
$ echo "* text=auto" >.gitattributes
$ echo "* eol=crlf" >>.gitattributes
would have the same effect as
$ echo "* text" >.gitattributes
$ git config core.eol crlf

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

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