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

[Dmitry Potapov <dpotapov@gmail.com>]

On Sat, Jan 12, 2008 at 06:54:13PM +0100, Steffen Prohaska wrote:
> diff --git a/convert.c b/convert.c
> index 4df7559..598cf0b 100644
> --- a/convert.c
> +++ b/convert.c
> @@ -132,6 +132,27 @@ static int crlf_to_git(const char *path, const char *src, size_t len,
>  				*dst++ = c;
>  		} while (--len);
>  	}
> +	if (safe_crlf) {
> +		if ((action == CRLF_INPUT) || auto_crlf <= 0) {
> +			/* autocrlf=input: check if we removed CRLFs */
> +			if (buf->len != dst - buf->buf) {
> +				if (safe_crlf == SAFE_CRLF_WARN)
> +					warning("Stripped CRLF from %s.", path);
> +				else
> +					die("Refusing to strip CRLF from %s.", path);
> +			}

This check is okay, however

> +		} else {
> +			/* autocrlf=true: check if we had LFs (without CR) */
> +			if (stats.lf != stats.crlf) {
> +				if (safe_crlf == SAFE_CRLF_WARN)
> +					warning(
> +					  "Checkout will replace LFs with CRLF in %s", path);
> +				else
> +					die("Checkout would replace LFs with CRLF in %s", path);
> +			}
> +		}

this is not, because if you really want to be sure that file will not be mangled
by checkout, you should not allow a text file with naked LF when autocrlf=true.
And the following lines after gather_stats() can cause:

		/* No CR? Nothing to convert, regardless. */
		if (!stats.cr)
			return 0;

So, I propose a slightly different patch for convert.c:

diff --git a/convert.c b/convert.c
index 4df7559..9fd88d9 100644
--- a/convert.c
+++ b/convert.c
@@ -90,9 +90,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;
 
 	if (action == CRLF_GUESS) {
 		/*
@@ -108,8 +105,23 @@ static int crlf_to_git(const char *path, const char *src, size_t len,
 		 */
 		if (is_binary(len, &stats))
 			return 0;
+
+		if (safe_crlf) {
+			/* check if we have "naked" LFs */
+			if (stats.lf != stats.crlf) {
+				if (safe_crlf == SAFE_CRLF_WARN)
+					warning(
+					  "Checkout will replace LFs with CRLF in %s", path);
+				else
+					die("Checkout would replace LFs with CRLF in %s", path);
+			}
+		}
 	}
 
+	/* No CR? Nothing to convert, regardless. */
+	if (!stats.cr)
+		return 0;
+
 	/* only grow if not in place */
 	if (strbuf_avail(buf) + buf->len < len)
 		strbuf_grow(buf, len - buf->len);
@@ -131,6 +143,16 @@ static int crlf_to_git(const char *path, const char *src, size_t len,
 			if (! (c == '\r' && (1 < len && *src == '\n')))
 				*dst++ = c;
 		} while (--len);
+
+		if (safe_crlf && (action == CRLF_INPUT || auto_crlf <= 0)) {
+			/* autocrlf=input: check if we removed CRLFs */
+			if (buf->len != dst - buf->buf) {
+				if (safe_crlf == SAFE_CRLF_WARN)
+					warning("Stripped CRLF from %s.", path);
+				else
+					die("Refusing to strip CRLF from %s.", path);
+			}
+		}
 	}
 	strbuf_setlen(buf, dst - buf->buf);
 	return 1;


Dmitry