Re: [PATCH/RFC] autocrlf: Make it work also for un-normalized repositories

From: Dmitry Potapov <dpotapov@gmail.com>
Date: May 10, 2010, 19:43 UTC
Message-ID: <20100510194321.GG14069@dpotapov.dyndns.org>
In-Reply-To: <20100510171119.GA17875@pvv.org>

On Mon, May 10, 2010 at 07:11:19PM +0200, Finn Arne Gangstad wrote:

Show 16 quoted lines

> 
> Stealing the problem from Eyvind's initial mail (paraphrased and
> summarized a bit):
> 
> 1. Setting autocrlf globally is a pain since autocrlf does not work well
>    with CRLF in the repo
> 2. Setting it in individual repos is hard since you do it "too late"
>    (the clone will get it wrong)
> 3. If someone checks in a file with CRLF later, you get into problems again
> 4. If a repository once has contained CRLF, you can't tell autocrlf
>    at which commit everything is sane again
> 5. autocrlf does needless work if you know that all your users want
>    the same EOL style.
> 
> I belive that this patch makes autocrlf a safe (and good) default
> setting for Windows, and this solves problems 1-4.

It does not really solve #2, because you will have the wrong ending for files that must be LF, such as shell scripts, and then these scripts fail with some weird error...

> 
> I implemented it by looking for CR charactes in the index, and
> aborting any conversion attempt if this is found.

Does it have any measurable impact on the check-in when a lot of files are committed?

> 
> Note that ALL the tests still pass unmodified. This is a bit
> surprising perhaps, but think it is an indication that no one ever
> intented autocrlf to do what it does to files containing CRs.

Well, tests do not cover many corner cases... So, no surprise here...

Show 8 quoted lines

> @@ -147,6 +184,10 @@ static int crlf_to_git(const char *path, const char *src, size_t len,
>                        return 0;
>        }
>
> +       /* If the file in the index has any CR in it, do not convert. */
> +       if (has_cr_in_index(path))
> +               return 0;
> +

Why do you disable crlf conversion not only for "guess" case but also for those files that have the "crlf" attribute? Moreover, you do that silently without even a warning to the user. IMHO, it is incompatible change. In fact, it can seen as regression, because by specifying the correct attribute for that file, I could fix the ending of this file. Now, this is impossible.

Show 13 quoted lines

>  
>  	/* Optimization: No CR? Nothing to convert, regardless. */
> @@ -202,6 +243,10 @@ static int crlf_to_worktree(const char *path, const char *src, size_t len,
>  	if (stats.lf == stats.crlf)
>  		return 0;
>  
> +	/* Are there ANY lines at all with CRLF? If so, ignore */
> +	if (stats.crlf > 0)
> +		return 0;
> +
>  	if (action == CRLF_GUESS) {
>  		/* If we have any bare CR characters, we're not going to touch it */
>  		if (stats.cr != stats.crlf)

This chunk does not make sense. Can you explain what did you try to achieve here for guess and non-guess cases?

IMHO, we should conservative in our changes. So, to change behavior of autocrlf only where "crlf" is not set explicitly. Or, at least, produce some warning about discrepancy between what the user has _explicitly_ told to do and what git does. I really dislike that any tool silently ignores users explicit instructions, because it thinks it knows better than a human. It is plainly wrong.

Dmitry