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

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 25, 2016, 19:37 UTC
Message-ID: <xmqqtwipjx9f.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <1461603397-30873-1-git-send-email-tboegi@web.de>

tboegi@web.de writes:

Show 12 quoted lines

> 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
>
> Since the 'eol' attribute had higher priority than 'text=auto', this may
> corrupt binary files and is not what users expect to happen.

Is the last "Since ..." continuation of the above two, i.e. "with the current code, the first one (unfortunately) means the same as the second one, BECAUSE ..."? If so, s/Since/since/ (I needed to read it twice to guess that is probably what you meant).

The above is a very good justification of the entire series, illustrating why this is worth doing.

> Make the 'eol' attribute to obey 'text=auto', and now

s/to obey/obey/ (make X do Y, not make X to do Y); I'd probably further do 's/obey/honor/' or even 's/obey/work better with/'

Show 14 quoted lines

> $ echo "* text=auto" >.gitattributes
> $ echo "* eol=crlf" >>.gitattributes
> behaves the same as
> $ echo "* text=auto" >.gitattributes
> $ git config core.eol crlf
>
> In other words,
> $ 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

It is kind of disappointing that the none of the above block of configuration examples does not quite say what the reader really wants to know directly. I am guessing that the point is

    now text=auto does autodetection even when eol=crlf is
    specified, and files that are declared to be non-text will not
    get clobbered with eol=crlf conversion

right? If that is the case, replacing everything below "In other words" with these three lines would make the text much easier to grok, I would think.

Show 32 quoted lines

> Signed-off-by: Torsten Bögershausen <tboegi@web.de>
> ---
>  Documentation/config.txt        | 14 +++++++-------
>  Documentation/gitattributes.txt | 15 +++++++++------
>  convert.c                       | 42 +++++++++++++++++++++--------------------
>  convert.h                       |  3 ++-
>  t/t0025-crlf-auto.sh            |  4 ++--
>  t/t0027-auto-crlf.sh            | 32 +++++++++++++++----------------
>  t/t6038-merge-text-auto.sh      | 23 ++++++++++++++--------
>  7 files changed, 73 insertions(+), 60 deletions(-)
>
> 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?

Show 11 quoted lines

> 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?

Show 9 quoted lines

> 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?

Show 5 quoted lines

>  	case CRLF_AUTO_INPUT:
> +		return EOL_LF;
>  	case CRLF_TEXT:
>  	case CRLF_AUTO:
>  		/* fall through */