Re: [PATCH 1/2] convert: add a helper to determine the correct EOL for a given path

From: Torsten Bögershausen <tboegi@web.de>
Date: Jan 23, 2016, 06:12 UTC
Message-ID: <56A319B9.50900@web.de>
In-Reply-To: <c4e6707b53d82cdeeb16c83bddf9c4870a53d9df.1453482052.git.johannes.schindelin@gmx.de>

On 2016-01-22 18.01, Johannes Schindelin wrote:

Show 20 quoted lines

> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  convert.c | 29 +++++++++++++++++++++++++++++
>  convert.h |  2 ++
>  2 files changed, 31 insertions(+)
> 
> diff --git a/convert.c b/convert.c
> index 814e814..b458734 100644
> --- a/convert.c
> +++ b/convert.c
> @@ -758,6 +758,35 @@ static void convert_attrs(struct conv_attrs *ca, const char *path)
>  	}
>  }
>  
> +enum eol eol_for_path(const char *path, const char *src, size_t len)
> +{
> +	struct conv_attrs ca;
> +	struct text_stat stats;
> +
> +	if (!path) {

I don't think this is ideal. When path is NULL, can we set it to "*" to catch .gitattributes like "* text=auto" ?

Show 9 quoted lines

> +		memset(&ca, 0, sizeof(ca));
> +		ca.crlf_action = CRLF_AUTO;
> +		ca.eol_attr = EOL_UNSET;
> +	} else {
> +		convert_attrs(&ca, path);
> +		if (ca.eol_attr == EOL_UNSET)
> +			ca.eol_attr = output_eol(ca.crlf_action);
> +		if (ca.eol_attr != EOL_UNSET)
> +			return ca.eol_attr;

This doesn't seem to be correct. When "File -text" is set, no CRLF needs to be added.

> +	}
> +	if (!len || (ca.crlf_action != CRLF_AUTO &&
> +				ca.crlf_action != CRLF_GUESS))

Currently core.autocrlf = true overrules "core.eol".

See the line in t0027: # core.eol core.autocrlf checkout_files lf true "" CRLF CRLF CRLF_mix_LF LF_mix_CR LF_nul

Letting core.autocrlf overrule core.eol is not documented very well, I have it on my todo-list. But I think it makes sense to keep the behavour.

I'm currently working on simplification of convert.c, so a funtion like this is really a step forward. Does it makes sense to add a comment, that some corner cases may need additional tweaking ?