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

[Dmitry Potapov <dpotapov@gmail.com>]

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

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

> @@ -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.

>  
>  	/* 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