Re: [PATCH v7] safecrlf: Add mechanism to warn about irreversible crlf conversions

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Feb 3, 2008, 22:29 UTC
Message-ID: <alpine.LSU.1.00.0802032222380.7372@racer.site>
In-Reply-To: <1202053379677-git-send-email-prohaska@zib.de>

Hi,

On Sun, 3 Feb 2008, Steffen Prohaska wrote:

Show 11 quoted lines

> diff --git a/convert.c b/convert.c
> index 80f114b..d580a62 100644
> --- a/convert.c
> +++ b/convert.c
> @@ -95,9 +95,6 @@ static int crlf_to_git(const char *path, const char *src, size_t len,
>  		return 0;
>  
>  	gather_stats(src, len, &stats);
> -	/* No CR? Nothing to convert, regardless. */
> -	if (!stats.cr)
> -		return 0;

Yes, you add it later, but would this not be "more correct" if you prepended a "!checksafe &&" and kept the if here? Of course, you'd _also_ need it later, but then _inside_ the "if (checksafe)" clause.

Show 7 quoted lines

> @@ -115,6 +112,30 @@ static int crlf_to_git(const char *path, const char *src, size_t len,
>  			return 0;
>  	}
>  
> +	if (checksafe) {
> +		if (action == CRLF_INPUT || auto_crlf <= 0) {
> +			/* CRLFs would not be restored by checkout: check if we'd remove CRLFs */

There are lines in your patch that are substantially longer than 80 characters / line. Please fix.

Show 9 quoted lines

> diff --git a/environment.c b/environment.c
> index 18a1c4e..e351e99 100644
> --- a/environment.c
> +++ b/environment.c
> @@ -35,6 +35,7 @@ int pager_use_color = 1;
>  char *editor_program;
>  char *excludes_file;
>  int auto_crlf = 0;	/* 1: both ways, -1: only when adding git objects */
> +enum safe_crlf safe_crlf = SAFE_CRLF_WARN;

I think this choice needs some defending. At first I thought: "no way that this will be the default; it affects too many users it should _not_ affect". The thing is, Unix users should not need to suffer, only because other users are cursed by insane choices of their OS developers.

However, safe_crlf != SAFE_CRLF_FALSE does not affect people who did not set core.crlf = input or core.crlf = true. And for those who set core.crlf, the default makes sense, absolutely.

So I think this patch should go in, with shortened lines (and possibly the changes to the "if (!stats.cr)" if you agree with me).

Ciao, Dscho