Re: [PATCH v2 4/7] convert.c: Use text_eol_is_crlf()

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 4, 2016, 20:13 UTC
Message-ID: <xmqqlh70groz.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <1454608194-5417-1-git-send-email-tboegi@web.de>

tboegi@web.de writes:

Show 61 quoted lines

> From: Torsten Bögershausen <tboegi@web.de>
>
> Add a helper function to find out, which line endings
> text files should get at checkout, depending on
> core.autocrlf and core.eol
>
> Signed-off-by: Torsten Bögershausen <tboegi@web.de>
> ---
>  convert.c | 25 +++++++++++++++++--------
>  1 file changed, 17 insertions(+), 8 deletions(-)
>
> diff --git a/convert.c b/convert.c
> index d0c8c66..9ffd043 100644
> --- a/convert.c
> +++ b/convert.c
> @@ -149,6 +149,19 @@ const char *get_wt_convert_stats_ascii(const char *path)
>  	return ret;
>  }
>  
> +static int text_eol_is_crlf(void)
> +{
> +	if (auto_crlf == AUTO_CRLF_TRUE)
> +		return 1;
> +	else if (auto_crlf == AUTO_CRLF_INPUT)
> +		return 0;
> +	if (core_eol == EOL_CRLF)
> +		return 1;
> +	if (core_eol == EOL_UNSET && EOL_NATIVE == EOL_CRLF)
> +		return 1;
> +	return 0;
> +}
> +
>  static enum eol output_eol(enum crlf_action crlf_action)
>  {
>  	switch (crlf_action) {
> @@ -164,12 +177,7 @@ static enum eol output_eol(enum crlf_action crlf_action)
>  		/* fall through */
>  	case CRLF_TEXT:
>  	case CRLF_AUTO:
> -		if (auto_crlf == AUTO_CRLF_TRUE)
> -			return EOL_CRLF;
> -		else if (auto_crlf == AUTO_CRLF_INPUT)
> -			return EOL_LF;
> -		else if (core_eol == EOL_UNSET)
> -			return EOL_NATIVE;
> +		return text_eol_is_crlf() ? EOL_CRLF : EOL_LF;
>  	}
>  	return core_eol;
>  }
> @@ -1378,8 +1386,9 @@ struct stream_filter *get_stream_filter(const char *path, const unsigned char *s
>  	    (crlf_action == CRLF_GUESS && auto_crlf == AUTO_CRLF_FALSE))
>  		filter = cascade_filter(filter, &null_filter_singleton);
>  
> -	else if (output_eol(crlf_action) == EOL_CRLF &&
> -		 !(crlf_action == CRLF_AUTO || crlf_action == CRLF_GUESS))
> +	else if ((crlf_action == CRLF_AUTO || crlf_action == CRLF_GUESS))
> +		;
> +	else if (output_eol(crlf_action) == EOL_CRLF)
>  		filter = cascade_filter(filter, lf_to_crlf_filter());
>  
>  	return filter;

I am a bit slow today so let me talk this change through aloud.

The condition under which we called cascade_filter() used to be that output_eol(crlf_action) yields EOL_CRLF and crlf_action is not set to one of these two values. Now, we say if crlf_action is one of these two values, we wouldn't call it, and then we check output_eol().

So they do the same thing, even though it smells that this change is outside the scope of "Add a helper and use it" theme of the patch.

While I do not think this new "split" conditions particularly easier to read compared to the previous one, if you plan to do something different in later patches when crlf_action is set to specific values, ignoring what output_eol() says, a patch to implement such a change would become easier to understand what is going on with this preparatory rewrite. So if such a preparatory rewrite is coming (I haven't checked 5-7/7 yet), perhaps have this hunk as a single patch that is separate from "add a helper text_eol_is_crlf()" patch.

By the way, your new 1/7 has s/: Remove/: remove/ applied to the subject, but not other ones like this one. Intended, or you forgot to do s/: Use/: use/ here?