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

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

tboegi@web.de writes:

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