Re: [PATCH v2 7/7] convert.c: simplify text_stat

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

tboegi@web.de writes:

Show 5 quoted lines

>  static int convert_is_binary(unsigned long size, const struct text_stat *stats)
>  {
> -	if (stats->cr != stats->crlf)
> +	if (stats->lonecr)
>  		return 1;

This is an equivalent conversion, but...

Show 9 quoted lines

> @@ -266,8 +267,8 @@ static int crlf_to_git(const char *path, const char *src, size_t len,
>  
>  	check_safe_crlf(path, crlf_action, &stats, checksafe);
>  
> -	/* Optimization: No CR? Nothing to convert, regardless. */
> -	if (!stats.cr)
> +	/* Optimization: No CRLF? Nothing to convert, regardless. */
> +	if (!stats.crlf)
>  		return 0;

... this is not. In fact this is even better, as we used to try the remainder of the function when we saw a lone CR, but we no longer do. I am of course assuming that the original turned into a no-op if we had a lone CR in the input--otherwise this patch changes the behaviour.

However, I see this comment after the function returns with the above optimization:

       /*
        * If we guessed, we already know we rejected a file with
        * lone CR, and we can strip a CR without looking at what
        * follow it.
        */

So the code around there used to have a reason to worry about a lone CR (i.e. it didn't want to lose them). With the change in this hunk for the "optimization", it no longer is necessary to do so, i.e. we know we do not have a lone CR so every CR can be stripped because it must be followed by LF, isn't it?

But I do not see a matching change to simplify that here. Am I following the current code incorrectly or something?

Puzzled...

Show 11 quoted lines

>  	/*
> @@ -315,21 +316,16 @@ static int crlf_to_worktree(const char *path, const char *src, size_t len,
>  	gather_stats(src, len, &stats);
>  
>  	/* No LF? Nothing to convert, regardless. */
> -	if (!stats.lf)
> -		return 0;
> -
> -	/* Was it already in CRLF format? */
> -	if (stats.lf == stats.crlf)
> +	if (!stats.lonelf)

Doesn't the comment above need adjustment?

>  		return 0;

Otherwise, the output side looks correct to me.