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

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

[Steffen Prohaska]

Here is the current version of the safecrlf patch.  It applies to
the post 1.5.4 master and has the following changes since v5:
 - Uses git config instead of git repo-config.
 - The tests use test_expect_success.

    Steffen

---- snip ---

CRLF conversion bears a slight chance of corrupting data.
autocrlf=true will convert CRLF to LF during commit and LF to
CRLF during checkout.  A file that contains a mixture of LF and
CRLF before the commit cannot be recreated by git.  For text
files this does not really matter because we do not care about
the line endings anyway; but for binary files that are
accidentally classified as text the conversion can corrupt data.

If you recognize such corruption early you can easily fix it by
setting the conversion type explicitly in .gitattributes.  Right
after committing you still have the original file in your work
tree and this file is not yet corrupted.  You can explicitly tell
git that this file is binary and git will handle the file
appropriately.

In mixed Windows/Unix environments text files quite easily can
end up containing a mixture of CRLF and LF line endings and git
should handle such situations gracefully.  For example, a user
could copy a CRLF file from Windows to Unix and mix it with an
existing LF file there.  The result would contain both types of
line endings.

Unfortunately, the desired effect of cleaning up text files with
mixed line endings and the undesired effect of corrupting binary
files can not be distinguished.  In both cases CRLF endings are
removed in an irreversible way.  For text files this is the right
thing to do, while for binary file it corrupts data.

In a sane environment, committing and checking out the same file
should not modify the original file in the work tree.  For
autocrlf=input the original file must not contain CRLF.  For
autocrlf=true the original file must not contain LF without
preceding CR.  Otherwise the conversion is irreversible.  Note,
git might be able to recreate the original file with different
autocrlf settings, but in the current environment checking out
will yield a file that differs from the file before the commit.

This patch adds a mechanism that can either warn the user about
an irreversible conversion or can even refuse to convert.  The
mechanism is controlled by the variable core.safecrlf, with the
following values
 - false: disable safecrlf mechanism
 - warn: warn about irreversible conversions
 - true: refuse irreversible conversions

The default is to warn.

The safecrlf mechanism's details depend on the git command.  If
safecrlf is active (not false), files in the work tree must not
be modified in an irreversible way without giving the user a
chance to backup the original file.  However, for read-only
operations that do not modify files in the work tree git should
not print annoying warnings.

This commit modifies git apply to fail even if safecrlf=warn,
because git apply writes its changes back to the work tree
immediately.  The user would not have a chance to backup the old
version of the file if only a warning was printed.  Hence, a
warning isn't sufficient and we should fail instead.

git diff, git add, and any other command that calls
index_fd(..., write_object=1) behave exactly as requested
by safecrlf.  The user still has the chance to save her data
before checkout or applying the diff.

git blame will never warn nor fail because it never writes to the
work tree.

The concept of a safety check was originally proposed in a similar
way by Linus Torvalds.  Thanks to Dimitry Potapov for insisting
on getting the naked LF/autocrlf=true case right.

Signed-off-by: Steffen Prohaska <prohaska@zib.de>
---
 Documentation/config.txt        |   22 +++++++++++++++
 Documentation/gitattributes.txt |    5 +++
 builtin-apply.c                 |    2 +-
 builtin-blame.c                 |    2 +-
 cache.h                         |   10 ++++++-
 config.c                        |    9 ++++++
 convert.c                       |   34 +++++++++++++++++++----
 diff.c                          |    2 +-
 environment.c                   |    1 +
 sha1_file.c                     |    2 +-
 t/t0020-crlf.sh                 |   58 +++++++++++++++++++++++++++++++++++++++
 11 files changed, 136 insertions(+), 11 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 4e222f1..b1b5c67 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -139,6 +139,28 @@ core.autocrlf::
 	"text" (i.e. be subjected to the autocrlf mechanism) is
 	decided purely based on the contents.
 
+core.safecrlf::
+	If true, makes git check if converting `CRLF` as controlled by
+	`core.autocrlf` is reversible.  Git will check if a command
+	modifies a file in the work tree either directly or indirectly.
+	For example, committing a file followed by checking out the
+	same file should yield the original file in the work tree.  If
+	this is not the case for the current setting of
+	`core.autocrlf`, git will reject the file.  The variable can
+	be set to "warn", in which case git will only warn about an
+	irreversible conversion but accept the file.
++
+Note, this safety check does not mean that a checkout will generate a
+file identical to the original file for different settings of
+`core.autocrlf`, but only for the current one.  For example, a text
+file with `LF` would be accepted with `core.autocrlf=input` and could
+later be checked out with `core.autocrlf=true`, in which case the
+resulting file would contain `CRLF`, although the original file
+contained `LF`.  However, in both work trees the line endings would be
+consistent, that is either all `LF` or all `CRLF`, but never mixed.  A
+file with mixed line endings would be reported by the `core.safecrlf`
+mechanism.
+
 core.symlinks::
 	If false, symbolic links are checked out as small plain files that
 	contain the link text. linkgit:git-update-index[1] and
diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt
index 35a29fd..2ffe1fb 100644
--- a/Documentation/gitattributes.txt
+++ b/Documentation/gitattributes.txt
@@ -133,6 +133,11 @@ When `core.autocrlf` is set to "input", line endings are
 converted to LF upon checkin, but there is no conversion done
 upon checkout.
 
+If `core.safecrlf` is set to "true" or "warn", git verifies if
+the conversion is reversible for the current setting of
+`core.autocrlf`.  For "true", git rejects irreversible
+conversions; for "warn", git only prints a warning but accepts
+an irreversible conversion.
 
 `ident`
 ^^^^^^^
diff --git a/builtin-apply.c b/builtin-apply.c
index 15432b6..e487936 100644
--- a/builtin-apply.c
+++ b/builtin-apply.c
@@ -1430,7 +1430,7 @@ static int read_old_data(struct stat *st, const char *path, struct strbuf *buf)
 	case S_IFREG:
 		if (strbuf_read_file(buf, path, st->st_size) != st->st_size)
 			return error("unable to open or read %s", path);
-		convert_to_git(path, buf->buf, buf->len, buf);
+		convert_to_git(path, buf->buf, buf->len, buf, safe_crlf ? SAFE_CRLF_FAIL : 0);
 		return 0;
 	default:
 		return -1;
diff --git a/builtin-blame.c b/builtin-blame.c
index 9b4c02e..c361ee1 100644
--- a/builtin-blame.c
+++ b/builtin-blame.c
@@ -2073,7 +2073,7 @@ static struct commit *fake_working_tree_commit(const char *path, const char *con
 		if (strbuf_read(&buf, 0, 0) < 0)
 			die("read error %s from stdin", strerror(errno));
 	}
-	convert_to_git(path, buf.buf, buf.len, &buf);
+	convert_to_git(path, buf.buf, buf.len, &buf, 0);
 	origin->file.ptr = buf.buf;
 	origin->file.size = buf.len;
 	pretend_sha1_file(buf.buf, buf.len, OBJ_BLOB, origin->blob_sha1);
diff --git a/cache.h b/cache.h
index 549f4bb..f8223a6 100644
--- a/cache.h
+++ b/cache.h
@@ -330,6 +330,14 @@ extern size_t packed_git_limit;
 extern size_t delta_base_cache_limit;
 extern int auto_crlf;
 
+enum safe_crlf {
+	SAFE_CRLF_FALSE = 0,
+	SAFE_CRLF_FAIL = 1,
+	SAFE_CRLF_WARN = 2,
+};
+
+extern enum safe_crlf safe_crlf;
+
 #define GIT_REPO_VERSION 0
 extern int repository_format_version;
 extern int check_repository_format(void);
@@ -633,7 +641,7 @@ extern void trace_argv_printf(const char **argv, const char *format, ...);
 
 /* convert.c */
 /* returns 1 if *dst was used */
-extern int convert_to_git(const char *path, const char *src, size_t len, struct strbuf *dst);
+extern int convert_to_git(const char *path, const char *src, size_t len, struct strbuf *dst, enum safe_crlf checksafe);
 extern int convert_to_working_tree(const char *path, const char *src, size_t len, struct strbuf *dst);
 
 /* add */
diff --git a/config.c b/config.c
index 526a3f4..5fe7cce 100644
--- a/config.c
+++ b/config.c
@@ -407,6 +407,15 @@ int git_default_config(const char *var, const char *value)
 		return 0;
 	}
 
+	if (!strcmp(var, "core.safecrlf")) {
+		if (value && !strcasecmp(value, "warn")) {
+			safe_crlf = SAFE_CRLF_WARN;
+			return 0;
+		}
+		safe_crlf = git_config_bool(var, value);
+		return 0;
+	}
+
 	if (!strcmp(var, "user.name")) {
 		strlcpy(git_default_name, value, sizeof(git_default_name));
 		return 0;
diff --git a/convert.c b/convert.c
index 80f114b..d580a62 100644
--- a/convert.c
+++ b/convert.c
@@ -86,7 +86,7 @@ static int is_binary(unsigned long size, struct text_stat *stats)
 }
 
 static int crlf_to_git(const char *path, const char *src, size_t len,
-                       struct strbuf *buf, int action)
+                       struct strbuf *buf, int action, enum safe_crlf checksafe)
 {
 	struct text_stat stats;
 	char *dst;
@@ -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;
 
 	if (action == CRLF_GUESS) {
 		/*
@@ -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 */
+			if (stats.crlf) {
+				if (checksafe == SAFE_CRLF_WARN)
+					warning("CRLF will be replaced by LF in %s.", path);
+				else
+					die("CRLF would be replaced by LF in %s.", path);
+			}
+		} else if (auto_crlf > 0) {
+			/* CRLFs would be added by checkout: check if we have "naked" LFs */
+			if (stats.lf != stats.crlf) {
+				if (checksafe == SAFE_CRLF_WARN)
+					warning("LFs will be replaced by CRLF in %s", path);
+				else
+					die("LF would be replaced by CRLF in %s", path);
+			}
+		}
+	}
+
+	/* Optimization: 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);
@@ -536,7 +557,8 @@ static int git_path_check_ident(const char *path, struct git_attr_check *check)
 	return !!ATTR_TRUE(value);
 }
 
-int convert_to_git(const char *path, const char *src, size_t len, struct strbuf *dst)
+int convert_to_git(const char *path, const char *src, size_t len,
+			struct strbuf *dst, enum safe_crlf checksafe)
 {
 	struct git_attr_check check[3];
 	int crlf = CRLF_GUESS;
@@ -558,7 +580,7 @@ int convert_to_git(const char *path, const char *src, size_t len, struct strbuf
 		src = dst->buf;
 		len = dst->len;
 	}
-	ret |= crlf_to_git(path, src, len, dst, crlf);
+	ret |= crlf_to_git(path, src, len, dst, crlf, checksafe);
 	if (ret) {
 		src = dst->buf;
 		len = dst->len;
diff --git a/diff.c b/diff.c
index 5b8afdc..562c20e 100644
--- a/diff.c
+++ b/diff.c
@@ -1624,7 +1624,7 @@ int diff_populate_filespec(struct diff_filespec *s, int size_only)
 		 * Convert from working tree format to canonical git format
 		 */
 		strbuf_init(&buf, 0);
-		if (convert_to_git(s->path, s->data, s->size, &buf)) {
+		if (convert_to_git(s->path, s->data, s->size, &buf, safe_crlf)) {
 			size_t size = 0;
 			munmap(s->data, s->size);
 			s->should_munmap = 0;
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;
 unsigned whitespace_rule_cfg = WS_DEFAULT_RULE;
 
 /* This is set by setup_git_dir_gently() and/or git_default_config() */
diff --git a/sha1_file.c b/sha1_file.c
index 66a4e00..1cf8337 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -2358,7 +2358,7 @@ int index_fd(unsigned char *sha1, int fd, struct stat *st, int write_object,
 	if ((type == OBJ_BLOB) && S_ISREG(st->st_mode)) {
 		struct strbuf nbuf;
 		strbuf_init(&nbuf, 0);
-		if (convert_to_git(path, buf, size, &nbuf)) {
+		if (convert_to_git(path, buf, size, &nbuf, write_object ? safe_crlf : 0)) {
 			munmap(buf, size);
 			buf = strbuf_detach(&nbuf, &size);
 			re_allocated = 1;
diff --git a/t/t0020-crlf.sh b/t/t0020-crlf.sh
index 8b27aa8..90ea081 100755
--- a/t/t0020-crlf.sh
+++ b/t/t0020-crlf.sh
@@ -8,6 +8,10 @@ q_to_nul () {
 	tr Q '\000'
 }
 
+q_to_cr () {
+	tr Q '\015'
+}
+
 append_cr () {
 	sed -e 's/$/Q/' | tr Q '\015'
 }
@@ -42,6 +46,60 @@ test_expect_success setup '
 	echo happy.
 '
 
+test_expect_success 'safecrlf: autocrlf=input, all CRLF' '
+
+	git config core.autocrlf input &&
+	git config core.safecrlf true &&
+
+	for w in I am all CRLF; do echo $w; done | append_cr >allcrlf &&
+	! git add allcrlf
+'
+
+test_expect_success 'safecrlf: autocrlf=input, mixed LF/CRLF' '
+
+	git config core.autocrlf input &&
+	git config core.safecrlf true &&
+
+	for w in Oh here is CRLFQ in text; do echo $w; done | q_to_cr >mixed &&
+	! git add mixed
+'
+
+test_expect_success 'safecrlf: autocrlf=true, all LF' '
+
+	git config core.autocrlf true &&
+	git config core.safecrlf true &&
+
+	for w in I am all LF; do echo $w; done >alllf &&
+	! git add alllf
+'
+
+test_expect_success 'safecrlf: autocrlf=true mixed LF/CRLF' '
+
+	git config core.autocrlf true &&
+	git config core.safecrlf true &&
+
+	for w in Oh here is CRLFQ in text; do echo $w; done | q_to_cr >mixed &&
+	! git add mixed
+'
+
+test_expect_success 'safecrlf: print warning only once' '
+
+	git config core.autocrlf input &&
+	git config core.safecrlf warn &&
+
+	for w in I am all LF; do echo $w; done >doublewarn &&
+	git add doublewarn &&
+	git commit -m "nowarn" &&
+	for w in Oh here is CRLFQ in text; do echo $w; done | q_to_cr >doublewarn &&
+	test $(git add doublewarn 2>&1 | grep "CRLF will be replaced by LF" | wc -l) = 1
+'
+
+test_expect_success 'switch off autocrlf, safecrlf, reset HEAD' '
+	git config core.autocrlf false &&
+	git config core.safecrlf false &&
+	git reset --hard HEAD^
+'
+
 test_expect_success 'update with autocrlf=input' '
 
 	rm -f tmp one dir/two three &&
-- 
1.5.4.rc5.33.gc24ec

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

[Junio C Hamano]

Steffen Prohaska <prohaska@zib.de> writes:

> CRLF conversion bears a slight chance of corrupting data.
> ...
> thing to do, while for binary file it corrupts data.

The above 25-line or so are well written and deserve to be in
the end user documentation somewhere, I think, to explain why it
is a good idea to have these warnings to them..

> This commit modifies git apply to fail even if safecrlf=warn,
> because git apply writes its changes back to the work tree
> immediately.  The user would not have a chance to backup the old
> version of the file if only a warning was printed.

I do not get this logic at all.

The whole point of git-apply is to apply the patch.  If you say
--whitespace=fix and some contents (say one of the testsuite
files in our t/ directory) needed to keep trailing newline, you
obviously are left with a broken result, and you would recover
by checking it out from index or HEAD and reapply.  Why
shouldn't the same principle hold here?

I haven't looked at the code of this round yet, but I promise I
will.

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

[Steffen Prohaska]

On Feb 3, 2008, at 11:50 AM, Junio C Hamano wrote:

> Steffen Prohaska <prohaska@zib.de> writes:
>
>> CRLF conversion bears a slight chance of corrupting data.
>> ...
>> thing to do, while for binary file it corrupts data.
>
> The above 25-line or so are well written and deserve to be in
> the end user documentation somewhere, I think, to explain why it
> is a good idea to have these warnings to them..

For now, I propose to add it to the documentation of
core.safecrlf.  In the future a section on cross-platform
projects in the user manual would probably be the right place.
But the manual does not yet have such a section.


>> This commit modifies git apply to fail even if safecrlf=warn,
>> because git apply writes its changes back to the work tree
>> immediately.  The user would not have a chance to backup the old
>> version of the file if only a warning was printed.
>
> I do not get this logic at all.
>
> The whole point of git-apply is to apply the patch.  If you say
> --whitespace=fix and some contents (say one of the testsuite
> files in our t/ directory) needed to keep trailing newline, you
> obviously are left with a broken result, and you would recover
> by checking it out from index or HEAD and reapply.  Why
> shouldn't the same principle hold here?

You are right.  All files should be committed before running
git apply and therefore the original files can be recovered by
git checkout.  Hmm ... so apply should either just warn or be
completely quiet as git blame is?  I think it should warn.

	Steffen

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

[Steffen Prohaska]

This repaces v6 of the patch and includes the following changes
 - Reason behind safecrlf documented in config.txt
 - git apply exactly behaves as configured in core.safecrlf

    Steffen

---- snip ---

CRLF conversion bears a slight chance of corrupting data.
autocrlf=true will convert CRLF to LF during commit and LF to
CRLF during checkout.  A file that contains a mixture of LF and
CRLF before the commit cannot be recreated by git.  For text
files this does not really matter because we do not care about
the line endings anyway; but for binary files that are
accidentally classified as text the conversion can corrupt data.

If you recognize such corruption early you can easily fix it by
setting the conversion type explicitly in .gitattributes.  Right
after committing you still have the original file in your work
tree and this file is not yet corrupted.  You can explicitly tell
git that this file is binary and git will handle the file
appropriately.

In mixed Windows/Unix environments text files quite easily can
end up containing a mixture of CRLF and LF line endings and git
should handle such situations gracefully.  For example, a user
could copy a CRLF file from Windows to Unix and mix it with an
existing LF file there.  The result would contain both types of
line endings.

Unfortunately, the desired effect of cleaning up text files with
mixed line endings and the undesired effect of corrupting binary
files cannot be distinguished.  In both cases CRLF endings are
removed in an irreversible way.  For text files this is the right
thing to do, while for binary file it corrupts data.

In a sane environment, committing and checking out the same file
should not modify the original file in the work tree.  For
autocrlf=input the original file must not contain CRLF.  For
autocrlf=true the original file must not contain LF without
preceding CR.  Otherwise the conversion is irreversible.  Note,
git might be able to recreate the original file with different
autocrlf settings, but in the current environment checking out
will yield a file that differs from the file before the commit.

This patch adds a mechanism that can either warn the user about
an irreversible conversion or can even refuse to convert.  The
mechanism is controlled by the variable core.safecrlf, with the
following values
 - false: disable safecrlf mechanism
 - warn: warn about irreversible conversions
 - true: refuse irreversible conversions

The default is to warn.

The safecrlf mechanism's details depend on the git command.  If
safecrlf is active (not false), files in the work tree must not
be modified in an irreversible way without giving the user a
chance to backup the original file.  However, for read-only
operations that do not modify files in the work tree git should
not print annoying warnings.

git apply behave as requested by by safecrlf.  Even though
git apply writes changes directly back to the work tree, the
user still can get the original files back by checking out from
the index or HEAD.  git apply should only be run on a clean
work tree.

git diff, git add, and any other command that calls
index_fd(..., write_object=1) also behave as requested by
safecrlf.  The user still has the chance to save her data before
checkout or applying the diff.

git blame will never warn nor fail because it never writes to the
work tree.

The concept of a safety check was originally proposed in a similar
way by Linus Torvalds.  Thanks to Dimitry Potapov for insisting
on getting the naked LF/autocrlf=true case right.

Signed-off-by: Steffen Prohaska <prohaska@zib.de>
---
 Documentation/config.txt        |   50 +++++++++++++++++++++++++++++++++
 Documentation/gitattributes.txt |    5 +++
 builtin-apply.c                 |    2 +-
 builtin-blame.c                 |    2 +-
 cache.h                         |   10 ++++++-
 config.c                        |    9 ++++++
 convert.c                       |   34 +++++++++++++++++++----
 diff.c                          |    2 +-
 environment.c                   |    1 +
 sha1_file.c                     |    2 +-
 t/t0020-crlf.sh                 |   58 +++++++++++++++++++++++++++++++++++++++
 11 files changed, 164 insertions(+), 11 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 4e222f1..7ebee51 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -139,6 +139,56 @@ core.autocrlf::
 	"text" (i.e. be subjected to the autocrlf mechanism) is
 	decided purely based on the contents.
 
+core.safecrlf::
+	If true, makes git check if converting `CRLF` as controlled by
+	`core.autocrlf` is reversible.  Git will verify if a command
+	modifies a file in the work tree either directly or indirectly.
+	For example, committing a file followed by checking out the
+	same file should yield the original file in the work tree.  If
+	this is not the case for the current setting of
+	`core.autocrlf`, git will reject the file.  The variable can
+	be set to "warn", in which case git will only warn about an
+	irreversible conversion but continue the operation.
++
+CRLF conversion bears a slight chance of corrupting data.
+autocrlf=true will convert CRLF to LF during commit and LF to
+CRLF during checkout.  A file that contains a mixture of LF and
+CRLF before the commit cannot be recreated by git.  For text
+files this does not really matter because we do not care about
+the line endings anyway; but for binary files that are
+accidentally classified as text the conversion can corrupt data.
++
+If you recognize such corruption early you can easily fix it by
+setting the conversion type explicitly in .gitattributes.  Right
+after committing you still have the original file in your work
+tree and this file is not yet corrupted.  You can explicitly tell
+git that this file is binary and git will handle the file
+appropriately.
++
+In mixed Windows/Unix environments text files quite easily can
+end up containing a mixture of CRLF and LF line endings and git
+should handle such situations gracefully.  For example, a user
+could copy a CRLF file from Windows to Unix and mix it with an
+existing LF file there.  The result would contain both types of
+line endings.
++
+Unfortunately, the desired effect of cleaning up text files with
+mixed line endings and the undesired effect of corrupting binary
+files cannot be distinguished.  In both cases CRLF endings are
+removed in an irreversible way.  For text files this is the right
+thing to do, while for binary file it corrupts data.
++
+Note, this safety check does not mean that a checkout will generate a
+file identical to the original file for a different setting of
+`core.autocrlf`, but only for the current one.  For example, a text
+file with `LF` would be accepted with `core.autocrlf=input` and could
+later be checked out with `core.autocrlf=true`, in which case the
+resulting file would contain `CRLF`, although the original file
+contained `LF`.  However, in both work trees the line endings would be
+consistent, that is either all `LF` or all `CRLF`, but never mixed.  A
+file with mixed line endings would be reported by the `core.safecrlf`
+mechanism.
+
 core.symlinks::
 	If false, symbolic links are checked out as small plain files that
 	contain the link text. linkgit:git-update-index[1] and
diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt
index 35a29fd..2ffe1fb 100644
--- a/Documentation/gitattributes.txt
+++ b/Documentation/gitattributes.txt
@@ -133,6 +133,11 @@ When `core.autocrlf` is set to "input", line endings are
 converted to LF upon checkin, but there is no conversion done
 upon checkout.
 
+If `core.safecrlf` is set to "true" or "warn", git verifies if
+the conversion is reversible for the current setting of
+`core.autocrlf`.  For "true", git rejects irreversible
+conversions; for "warn", git only prints a warning but accepts
+an irreversible conversion.
 
 `ident`
 ^^^^^^^
diff --git a/builtin-apply.c b/builtin-apply.c
index 15432b6..d0b3586 100644
--- a/builtin-apply.c
+++ b/builtin-apply.c
@@ -1430,7 +1430,7 @@ static int read_old_data(struct stat *st, const char *path, struct strbuf *buf)
 	case S_IFREG:
 		if (strbuf_read_file(buf, path, st->st_size) != st->st_size)
 			return error("unable to open or read %s", path);
-		convert_to_git(path, buf->buf, buf->len, buf);
+		convert_to_git(path, buf->buf, buf->len, buf, safe_crlf);
 		return 0;
 	default:
 		return -1;
diff --git a/builtin-blame.c b/builtin-blame.c
index 9b4c02e..c361ee1 100644
--- a/builtin-blame.c
+++ b/builtin-blame.c
@@ -2073,7 +2073,7 @@ static struct commit *fake_working_tree_commit(const char *path, const char *con
 		if (strbuf_read(&buf, 0, 0) < 0)
 			die("read error %s from stdin", strerror(errno));
 	}
-	convert_to_git(path, buf.buf, buf.len, &buf);
+	convert_to_git(path, buf.buf, buf.len, &buf, 0);
 	origin->file.ptr = buf.buf;
 	origin->file.size = buf.len;
 	pretend_sha1_file(buf.buf, buf.len, OBJ_BLOB, origin->blob_sha1);
diff --git a/cache.h b/cache.h
index 549f4bb..f8223a6 100644
--- a/cache.h
+++ b/cache.h
@@ -330,6 +330,14 @@ extern size_t packed_git_limit;
 extern size_t delta_base_cache_limit;
 extern int auto_crlf;
 
+enum safe_crlf {
+	SAFE_CRLF_FALSE = 0,
+	SAFE_CRLF_FAIL = 1,
+	SAFE_CRLF_WARN = 2,
+};
+
+extern enum safe_crlf safe_crlf;
+
 #define GIT_REPO_VERSION 0
 extern int repository_format_version;
 extern int check_repository_format(void);
@@ -633,7 +641,7 @@ extern void trace_argv_printf(const char **argv, const char *format, ...);
 
 /* convert.c */
 /* returns 1 if *dst was used */
-extern int convert_to_git(const char *path, const char *src, size_t len, struct strbuf *dst);
+extern int convert_to_git(const char *path, const char *src, size_t len, struct strbuf *dst, enum safe_crlf checksafe);
 extern int convert_to_working_tree(const char *path, const char *src, size_t len, struct strbuf *dst);
 
 /* add */
diff --git a/config.c b/config.c
index 526a3f4..5fe7cce 100644
--- a/config.c
+++ b/config.c
@@ -407,6 +407,15 @@ int git_default_config(const char *var, const char *value)
 		return 0;
 	}
 
+	if (!strcmp(var, "core.safecrlf")) {
+		if (value && !strcasecmp(value, "warn")) {
+			safe_crlf = SAFE_CRLF_WARN;
+			return 0;
+		}
+		safe_crlf = git_config_bool(var, value);
+		return 0;
+	}
+
 	if (!strcmp(var, "user.name")) {
 		strlcpy(git_default_name, value, sizeof(git_default_name));
 		return 0;
diff --git a/convert.c b/convert.c
index 80f114b..d580a62 100644
--- a/convert.c
+++ b/convert.c
@@ -86,7 +86,7 @@ static int is_binary(unsigned long size, struct text_stat *stats)
 }
 
 static int crlf_to_git(const char *path, const char *src, size_t len,
-                       struct strbuf *buf, int action)
+                       struct strbuf *buf, int action, enum safe_crlf checksafe)
 {
 	struct text_stat stats;
 	char *dst;
@@ -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;
 
 	if (action == CRLF_GUESS) {
 		/*
@@ -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 */
+			if (stats.crlf) {
+				if (checksafe == SAFE_CRLF_WARN)
+					warning("CRLF will be replaced by LF in %s.", path);
+				else
+					die("CRLF would be replaced by LF in %s.", path);
+			}
+		} else if (auto_crlf > 0) {
+			/* CRLFs would be added by checkout: check if we have "naked" LFs */
+			if (stats.lf != stats.crlf) {
+				if (checksafe == SAFE_CRLF_WARN)
+					warning("LFs will be replaced by CRLF in %s", path);
+				else
+					die("LF would be replaced by CRLF in %s", path);
+			}
+		}
+	}
+
+	/* Optimization: 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);
@@ -536,7 +557,8 @@ static int git_path_check_ident(const char *path, struct git_attr_check *check)
 	return !!ATTR_TRUE(value);
 }
 
-int convert_to_git(const char *path, const char *src, size_t len, struct strbuf *dst)
+int convert_to_git(const char *path, const char *src, size_t len,
+			struct strbuf *dst, enum safe_crlf checksafe)
 {
 	struct git_attr_check check[3];
 	int crlf = CRLF_GUESS;
@@ -558,7 +580,7 @@ int convert_to_git(const char *path, const char *src, size_t len, struct strbuf
 		src = dst->buf;
 		len = dst->len;
 	}
-	ret |= crlf_to_git(path, src, len, dst, crlf);
+	ret |= crlf_to_git(path, src, len, dst, crlf, checksafe);
 	if (ret) {
 		src = dst->buf;
 		len = dst->len;
diff --git a/diff.c b/diff.c
index 5b8afdc..562c20e 100644
--- a/diff.c
+++ b/diff.c
@@ -1624,7 +1624,7 @@ int diff_populate_filespec(struct diff_filespec *s, int size_only)
 		 * Convert from working tree format to canonical git format
 		 */
 		strbuf_init(&buf, 0);
-		if (convert_to_git(s->path, s->data, s->size, &buf)) {
+		if (convert_to_git(s->path, s->data, s->size, &buf, safe_crlf)) {
 			size_t size = 0;
 			munmap(s->data, s->size);
 			s->should_munmap = 0;
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;
 unsigned whitespace_rule_cfg = WS_DEFAULT_RULE;
 
 /* This is set by setup_git_dir_gently() and/or git_default_config() */
diff --git a/sha1_file.c b/sha1_file.c
index 66a4e00..1cf8337 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -2358,7 +2358,7 @@ int index_fd(unsigned char *sha1, int fd, struct stat *st, int write_object,
 	if ((type == OBJ_BLOB) && S_ISREG(st->st_mode)) {
 		struct strbuf nbuf;
 		strbuf_init(&nbuf, 0);
-		if (convert_to_git(path, buf, size, &nbuf)) {
+		if (convert_to_git(path, buf, size, &nbuf, write_object ? safe_crlf : 0)) {
 			munmap(buf, size);
 			buf = strbuf_detach(&nbuf, &size);
 			re_allocated = 1;
diff --git a/t/t0020-crlf.sh b/t/t0020-crlf.sh
index 8b27aa8..90ea081 100755
--- a/t/t0020-crlf.sh
+++ b/t/t0020-crlf.sh
@@ -8,6 +8,10 @@ q_to_nul () {
 	tr Q '\000'
 }
 
+q_to_cr () {
+	tr Q '\015'
+}
+
 append_cr () {
 	sed -e 's/$/Q/' | tr Q '\015'
 }
@@ -42,6 +46,60 @@ test_expect_success setup '
 	echo happy.
 '
 
+test_expect_success 'safecrlf: autocrlf=input, all CRLF' '
+
+	git config core.autocrlf input &&
+	git config core.safecrlf true &&
+
+	for w in I am all CRLF; do echo $w; done | append_cr >allcrlf &&
+	! git add allcrlf
+'
+
+test_expect_success 'safecrlf: autocrlf=input, mixed LF/CRLF' '
+
+	git config core.autocrlf input &&
+	git config core.safecrlf true &&
+
+	for w in Oh here is CRLFQ in text; do echo $w; done | q_to_cr >mixed &&
+	! git add mixed
+'
+
+test_expect_success 'safecrlf: autocrlf=true, all LF' '
+
+	git config core.autocrlf true &&
+	git config core.safecrlf true &&
+
+	for w in I am all LF; do echo $w; done >alllf &&
+	! git add alllf
+'
+
+test_expect_success 'safecrlf: autocrlf=true mixed LF/CRLF' '
+
+	git config core.autocrlf true &&
+	git config core.safecrlf true &&
+
+	for w in Oh here is CRLFQ in text; do echo $w; done | q_to_cr >mixed &&
+	! git add mixed
+'
+
+test_expect_success 'safecrlf: print warning only once' '
+
+	git config core.autocrlf input &&
+	git config core.safecrlf warn &&
+
+	for w in I am all LF; do echo $w; done >doublewarn &&
+	git add doublewarn &&
+	git commit -m "nowarn" &&
+	for w in Oh here is CRLFQ in text; do echo $w; done | q_to_cr >doublewarn &&
+	test $(git add doublewarn 2>&1 | grep "CRLF will be replaced by LF" | wc -l) = 1
+'
+
+test_expect_success 'switch off autocrlf, safecrlf, reset HEAD' '
+	git config core.autocrlf false &&
+	git config core.safecrlf false &&
+	git reset --hard HEAD^
+'
+
 test_expect_success 'update with autocrlf=input' '
 
 	rm -f tmp one dir/two three &&
-- 
1.5.4.19.g5f7bf

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

[Johannes Schindelin]

Hi,

On Sun, 3 Feb 2008, Steffen Prohaska wrote:

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

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

> 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

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

[Junio C Hamano]

Steffen Prohaska <prohaska@zib.de> writes:

>> The whole point of git-apply is to apply the patch.  If you say
>> --whitespace=fix and some contents (say one of the testsuite
>> files in our t/ directory) needed to keep trailing newline, you
>> obviously are left with a broken result, and you would recover
>> by checking it out from index or HEAD and reapply.  Why
>> shouldn't the same principle hold here?
>
> You are right.  All files should be committed before running
> git apply and therefore the original files can be recovered by
> git checkout.  Hmm ... so apply should either just warn or be
> completely quiet as git blame is?  I think it should warn.

I agree that warning is needed if the setting says "warn".

Of course, it could become part of an irreversible action if you
did this:

	$ git commit ;# or whatever.  Now there is no local mods
        $ git apply <patch-1
	$ edit ;# to fix things up
        $ git apply <patch-2

and if you get the warning from patch-2.

But the safecrlf warning is primarily about a misdetection of
binaryness, so I do not think this sequence is not something we
would even want to worry about.

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

[Steffen Prohaska]

On Feb 3, 2008, at 11:29 PM, Johannes Schindelin wrote:

> On Sun, 3 Feb 2008, Steffen Prohaska wrote:
>
>> 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.

Strictly speaking we would not need it at all, because it's an
optimization.  By moving the optimization a bit down in the
code we don't loose much because all the code that is executed
between the old location and the new one only compares a few
numbers.

The expensive part (scanning the buffer) is still avoided.

So I don't think it would be "more correct" to duplicate the
optimization to safe a few cycles if checksafe == false.

Or do you mean that your proposed "!checksafe &&" would be
easier to understand by a human?


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

I refactored the code.


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

I add a comment to the commit message.

However, I don't fully agree with your comment.  If your Unix
environment is as sane as you assume and you never exchange any
data with the "cursed" people, you can safely set
core.autocrlf=input and core.safecrlf=warn and still should
never see any warning.  Well you'd spent some CPU cycles on
verifying that your assumptions hold.  But those cycles would be
well spent if you accidentally received data from the underworld
that contained the insane line endings; because in this case git
would immediately warn you and would keep your repository sane.
If you had not activated autocrlf/safecrlf as proposed, the
insane line endings would pollute your repository for all eternity.
Even if you recognized them later, they would most likely already
be part of your history.

That said, the next logical step is to set core.autocrlf=input
as the default on Unix.  But this can be discussed later, after
we have the core.safecrlf mechanism in place.

	Steffen

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

[Steffen Prohaska]

This repaces v7 of the patch and includes the following changes
 - Commit message defends safecrlf=warn default.
 - refactored to lines max 84 chars long.

    Steffen

---- snip ---

CRLF conversion bears a slight chance of corrupting data.
autocrlf=true will convert CRLF to LF during commit and LF to
CRLF during checkout.  A file that contains a mixture of LF and
CRLF before the commit cannot be recreated by git.  For text
files this does not really matter because we do not care about
the line endings anyway; but for binary files that are
accidentally classified as text the conversion can corrupt data.

If you recognize such corruption early you can easily fix it by
setting the conversion type explicitly in .gitattributes.  Right
after committing you still have the original file in your work
tree and this file is not yet corrupted.  You can explicitly tell
git that this file is binary and git will handle the file
appropriately.

In mixed Windows/Unix environments text files quite easily can
end up containing a mixture of CRLF and LF line endings and git
should handle such situations gracefully.  For example, a user
could copy a CRLF file from Windows to Unix and mix it with an
existing LF file there.  The result would contain both types of
line endings.

Unfortunately, the desired effect of cleaning up text files with
mixed line endings and the undesired effect of corrupting binary
files cannot be distinguished.  In both cases CRLF endings are
removed in an irreversible way.  For text files this is the right
thing to do, while for binary file it corrupts data.

In a sane environment, committing and checking out the same file
should not modify the original file in the work tree.  For
autocrlf=input the original file must not contain CRLF.  For
autocrlf=true the original file must not contain LF without
preceding CR.  Otherwise the conversion is irreversible.  Note,
git might be able to recreate the original file with different
autocrlf settings, but in the current environment checking out
will yield a file that differs from the file before the commit.

This patch adds a mechanism that can either warn the user about
an irreversible conversion or can even refuse to convert.  The
mechanism is controlled by the variable core.safecrlf, with the
following values
 - false: disable safecrlf mechanism
 - warn: warn about irreversible conversions
 - true: refuse irreversible conversions

The default is to warn.  Users are only affected by this default
if core.autocrlf is set.  But the current default of git is to
leave core.autocrlf unset, so users will not see warnings unless
they deliberately chose to activate the autocrlf mechanism.

The safecrlf mechanism's details depend on the git command.  If
safecrlf is active (not false), files in the work tree must not
be modified in an irreversible way without giving the user a
chance to backup the original file.  However, for read-only
operations that do not modify files in the work tree git should
not print annoying warnings.

git apply behave as requested by by safecrlf.  Even though
git apply writes changes directly back to the work tree, the
user still can get the original files back by checking out from
the index or HEAD.  git apply should only be run on a clean
work tree.

git diff, git add, and any other command that calls
index_fd(..., write_object=1) also behave as requested by
safecrlf.  The user still has the chance to save her data before
checkout or applying the diff.

git blame will never warn nor fail because it never writes to the
work tree.

The concept of a safety check was originally proposed in a similar
way by Linus Torvalds.  Thanks to Dimitry Potapov for insisting
on getting the naked LF/autocrlf=true case right.

Signed-off-by: Steffen Prohaska <prohaska@zib.de>
---
 Documentation/config.txt        |   50 +++++++++++++++++++++++++++++++++
 Documentation/gitattributes.txt |    5 +++
 builtin-apply.c                 |    2 +-
 builtin-blame.c                 |    2 +-
 cache.h                         |   11 +++++++-
 config.c                        |    9 ++++++
 convert.c                       |   45 ++++++++++++++++++++++++++----
 diff.c                          |    2 +-
 environment.c                   |    1 +
 sha1_file.c                     |    3 +-
 t/t0020-crlf.sh                 |   58 +++++++++++++++++++++++++++++++++++++++
 11 files changed, 177 insertions(+), 11 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 4e222f1..7ebee51 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -139,6 +139,56 @@ core.autocrlf::
 	"text" (i.e. be subjected to the autocrlf mechanism) is
 	decided purely based on the contents.
 
+core.safecrlf::
+	If true, makes git check if converting `CRLF` as controlled by
+	`core.autocrlf` is reversible.  Git will verify if a command
+	modifies a file in the work tree either directly or indirectly.
+	For example, committing a file followed by checking out the
+	same file should yield the original file in the work tree.  If
+	this is not the case for the current setting of
+	`core.autocrlf`, git will reject the file.  The variable can
+	be set to "warn", in which case git will only warn about an
+	irreversible conversion but continue the operation.
++
+CRLF conversion bears a slight chance of corrupting data.
+autocrlf=true will convert CRLF to LF during commit and LF to
+CRLF during checkout.  A file that contains a mixture of LF and
+CRLF before the commit cannot be recreated by git.  For text
+files this does not really matter because we do not care about
+the line endings anyway; but for binary files that are
+accidentally classified as text the conversion can corrupt data.
++
+If you recognize such corruption early you can easily fix it by
+setting the conversion type explicitly in .gitattributes.  Right
+after committing you still have the original file in your work
+tree and this file is not yet corrupted.  You can explicitly tell
+git that this file is binary and git will handle the file
+appropriately.
++
+In mixed Windows/Unix environments text files quite easily can
+end up containing a mixture of CRLF and LF line endings and git
+should handle such situations gracefully.  For example, a user
+could copy a CRLF file from Windows to Unix and mix it with an
+existing LF file there.  The result would contain both types of
+line endings.
++
+Unfortunately, the desired effect of cleaning up text files with
+mixed line endings and the undesired effect of corrupting binary
+files cannot be distinguished.  In both cases CRLF endings are
+removed in an irreversible way.  For text files this is the right
+thing to do, while for binary file it corrupts data.
++
+Note, this safety check does not mean that a checkout will generate a
+file identical to the original file for a different setting of
+`core.autocrlf`, but only for the current one.  For example, a text
+file with `LF` would be accepted with `core.autocrlf=input` and could
+later be checked out with `core.autocrlf=true`, in which case the
+resulting file would contain `CRLF`, although the original file
+contained `LF`.  However, in both work trees the line endings would be
+consistent, that is either all `LF` or all `CRLF`, but never mixed.  A
+file with mixed line endings would be reported by the `core.safecrlf`
+mechanism.
+
 core.symlinks::
 	If false, symbolic links are checked out as small plain files that
 	contain the link text. linkgit:git-update-index[1] and
diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt
index 35a29fd..2ffe1fb 100644
--- a/Documentation/gitattributes.txt
+++ b/Documentation/gitattributes.txt
@@ -133,6 +133,11 @@ When `core.autocrlf` is set to "input", line endings are
 converted to LF upon checkin, but there is no conversion done
 upon checkout.
 
+If `core.safecrlf` is set to "true" or "warn", git verifies if
+the conversion is reversible for the current setting of
+`core.autocrlf`.  For "true", git rejects irreversible
+conversions; for "warn", git only prints a warning but accepts
+an irreversible conversion.
 
 `ident`
 ^^^^^^^
diff --git a/builtin-apply.c b/builtin-apply.c
index 15432b6..d0b3586 100644
--- a/builtin-apply.c
+++ b/builtin-apply.c
@@ -1430,7 +1430,7 @@ static int read_old_data(struct stat *st, const char *path, struct strbuf *buf)
 	case S_IFREG:
 		if (strbuf_read_file(buf, path, st->st_size) != st->st_size)
 			return error("unable to open or read %s", path);
-		convert_to_git(path, buf->buf, buf->len, buf);
+		convert_to_git(path, buf->buf, buf->len, buf, safe_crlf);
 		return 0;
 	default:
 		return -1;
diff --git a/builtin-blame.c b/builtin-blame.c
index 9b4c02e..c361ee1 100644
--- a/builtin-blame.c
+++ b/builtin-blame.c
@@ -2073,7 +2073,7 @@ static struct commit *fake_working_tree_commit(const char *path, const char *con
 		if (strbuf_read(&buf, 0, 0) < 0)
 			die("read error %s from stdin", strerror(errno));
 	}
-	convert_to_git(path, buf.buf, buf.len, &buf);
+	convert_to_git(path, buf.buf, buf.len, &buf, 0);
 	origin->file.ptr = buf.buf;
 	origin->file.size = buf.len;
 	pretend_sha1_file(buf.buf, buf.len, OBJ_BLOB, origin->blob_sha1);
diff --git a/cache.h b/cache.h
index 549f4bb..6003c83 100644
--- a/cache.h
+++ b/cache.h
@@ -330,6 +330,14 @@ extern size_t packed_git_limit;
 extern size_t delta_base_cache_limit;
 extern int auto_crlf;
 
+enum safe_crlf {
+	SAFE_CRLF_FALSE = 0,
+	SAFE_CRLF_FAIL = 1,
+	SAFE_CRLF_WARN = 2,
+};
+
+extern enum safe_crlf safe_crlf;
+
 #define GIT_REPO_VERSION 0
 extern int repository_format_version;
 extern int check_repository_format(void);
@@ -633,7 +641,8 @@ extern void trace_argv_printf(const char **argv, const char *format, ...);
 
 /* convert.c */
 /* returns 1 if *dst was used */
-extern int convert_to_git(const char *path, const char *src, size_t len, struct strbuf *dst);
+extern int convert_to_git(const char *path, const char *src, size_t len,
+                          struct strbuf *dst, enum safe_crlf checksafe);
 extern int convert_to_working_tree(const char *path, const char *src, size_t len, struct strbuf *dst);
 
 /* add */
diff --git a/config.c b/config.c
index 526a3f4..5fe7cce 100644
--- a/config.c
+++ b/config.c
@@ -407,6 +407,15 @@ int git_default_config(const char *var, const char *value)
 		return 0;
 	}
 
+	if (!strcmp(var, "core.safecrlf")) {
+		if (value && !strcasecmp(value, "warn")) {
+			safe_crlf = SAFE_CRLF_WARN;
+			return 0;
+		}
+		safe_crlf = git_config_bool(var, value);
+		return 0;
+	}
+
 	if (!strcmp(var, "user.name")) {
 		strlcpy(git_default_name, value, sizeof(git_default_name));
 		return 0;
diff --git a/convert.c b/convert.c
index 80f114b..94c70f4 100644
--- a/convert.c
+++ b/convert.c
@@ -85,8 +85,36 @@ static int is_binary(unsigned long size, struct text_stat *stats)
 	return 0;
 }
 
+static void check_safe_crlf(const char* path, int action,
+                            struct text_stat* stats, enum safe_crlf checksafe)
+{
+	if (action == CRLF_INPUT || auto_crlf <= 0) {
+		/*
+		 * CRLFs would not be restored by checkout:
+		 * check if we'd remove CRLFs
+		 */
+		if (stats->crlf) {
+			if (checksafe == SAFE_CRLF_WARN)
+				warning("CRLF will be replaced by LF in %s.", path);
+			else
+				die("CRLF would be replaced by LF in %s.", path);
+		}
+	} else if (auto_crlf > 0) {
+		/*
+		 * CRLFs would be added by checkout:
+		 * check if we have "naked" LFs
+		 */
+		if (stats->lf != stats->crlf) {
+			if (checksafe == SAFE_CRLF_WARN)
+				warning("LF will be replaced by CRLF in %s", path);
+			else
+				die("LF would be replaced by CRLF in %s", path);
+		}
+	}
+}
+
 static int crlf_to_git(const char *path, const char *src, size_t len,
-                       struct strbuf *buf, int action)
+                       struct strbuf *buf, int action, enum safe_crlf checksafe)
 {
 	struct text_stat stats;
 	char *dst;
@@ -95,9 +123,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) {
 		/*
@@ -115,6 +140,13 @@ static int crlf_to_git(const char *path, const char *src, size_t len,
 			return 0;
 	}
 
+	if (checksafe)
+		check_safe_crlf(path, action, &stats, checksafe);
+
+	/* Optimization: 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);
@@ -536,7 +568,8 @@ static int git_path_check_ident(const char *path, struct git_attr_check *check)
 	return !!ATTR_TRUE(value);
 }
 
-int convert_to_git(const char *path, const char *src, size_t len, struct strbuf *dst)
+int convert_to_git(const char *path, const char *src, size_t len,
+                   struct strbuf *dst, enum safe_crlf checksafe)
 {
 	struct git_attr_check check[3];
 	int crlf = CRLF_GUESS;
@@ -558,7 +591,7 @@ int convert_to_git(const char *path, const char *src, size_t len, struct strbuf
 		src = dst->buf;
 		len = dst->len;
 	}
-	ret |= crlf_to_git(path, src, len, dst, crlf);
+	ret |= crlf_to_git(path, src, len, dst, crlf, checksafe);
 	if (ret) {
 		src = dst->buf;
 		len = dst->len;
diff --git a/diff.c b/diff.c
index 5b8afdc..562c20e 100644
--- a/diff.c
+++ b/diff.c
@@ -1624,7 +1624,7 @@ int diff_populate_filespec(struct diff_filespec *s, int size_only)
 		 * Convert from working tree format to canonical git format
 		 */
 		strbuf_init(&buf, 0);
-		if (convert_to_git(s->path, s->data, s->size, &buf)) {
+		if (convert_to_git(s->path, s->data, s->size, &buf, safe_crlf)) {
 			size_t size = 0;
 			munmap(s->data, s->size);
 			s->should_munmap = 0;
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;
 unsigned whitespace_rule_cfg = WS_DEFAULT_RULE;
 
 /* This is set by setup_git_dir_gently() and/or git_default_config() */
diff --git a/sha1_file.c b/sha1_file.c
index 66a4e00..4179949 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -2358,7 +2358,8 @@ int index_fd(unsigned char *sha1, int fd, struct stat *st, int write_object,
 	if ((type == OBJ_BLOB) && S_ISREG(st->st_mode)) {
 		struct strbuf nbuf;
 		strbuf_init(&nbuf, 0);
-		if (convert_to_git(path, buf, size, &nbuf)) {
+		if (convert_to_git(path, buf, size, &nbuf,
+		                   write_object ? safe_crlf : 0)) {
 			munmap(buf, size);
 			buf = strbuf_detach(&nbuf, &size);
 			re_allocated = 1;
diff --git a/t/t0020-crlf.sh b/t/t0020-crlf.sh
index 8b27aa8..90ea081 100755
--- a/t/t0020-crlf.sh
+++ b/t/t0020-crlf.sh
@@ -8,6 +8,10 @@ q_to_nul () {
 	tr Q '\000'
 }
 
+q_to_cr () {
+	tr Q '\015'
+}
+
 append_cr () {
 	sed -e 's/$/Q/' | tr Q '\015'
 }
@@ -42,6 +46,60 @@ test_expect_success setup '
 	echo happy.
 '
 
+test_expect_success 'safecrlf: autocrlf=input, all CRLF' '
+
+	git config core.autocrlf input &&
+	git config core.safecrlf true &&
+
+	for w in I am all CRLF; do echo $w; done | append_cr >allcrlf &&
+	! git add allcrlf
+'
+
+test_expect_success 'safecrlf: autocrlf=input, mixed LF/CRLF' '
+
+	git config core.autocrlf input &&
+	git config core.safecrlf true &&
+
+	for w in Oh here is CRLFQ in text; do echo $w; done | q_to_cr >mixed &&
+	! git add mixed
+'
+
+test_expect_success 'safecrlf: autocrlf=true, all LF' '
+
+	git config core.autocrlf true &&
+	git config core.safecrlf true &&
+
+	for w in I am all LF; do echo $w; done >alllf &&
+	! git add alllf
+'
+
+test_expect_success 'safecrlf: autocrlf=true mixed LF/CRLF' '
+
+	git config core.autocrlf true &&
+	git config core.safecrlf true &&
+
+	for w in Oh here is CRLFQ in text; do echo $w; done | q_to_cr >mixed &&
+	! git add mixed
+'
+
+test_expect_success 'safecrlf: print warning only once' '
+
+	git config core.autocrlf input &&
+	git config core.safecrlf warn &&
+
+	for w in I am all LF; do echo $w; done >doublewarn &&
+	git add doublewarn &&
+	git commit -m "nowarn" &&
+	for w in Oh here is CRLFQ in text; do echo $w; done | q_to_cr >doublewarn &&
+	test $(git add doublewarn 2>&1 | grep "CRLF will be replaced by LF" | wc -l) = 1
+'
+
+test_expect_success 'switch off autocrlf, safecrlf, reset HEAD' '
+	git config core.autocrlf false &&
+	git config core.safecrlf false &&
+	git reset --hard HEAD^
+'
+
 test_expect_success 'update with autocrlf=input' '
 
 	rm -f tmp one dir/two three &&
-- 
1.5.4.24.g6d45

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

[Johannes Schindelin]

Hi,

On Mon, 4 Feb 2008, Steffen Prohaska wrote:

> On Feb 3, 2008, at 11:29 PM, Johannes Schindelin wrote:
> 
> > 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.
> 
> I add a comment to the commit message.
> 
> However, I don't fully agree with your comment.  If your Unix 
> environment is as sane as you assume and you never exchange any data 
> with the "cursed" people, you can safely set core.autocrlf=input and 
> core.safecrlf=warn and still should never see any warning.  Well you'd 
> spent some CPU cycles on verifying that your assumptions hold.

It is not only about spend CPU cycles.  It is about content-tracking.  
Personally, I have no single repository which munges data when putting it 
into the index.  That is what the crlf handling is: munging.  I want the 
data verbatim, and if I decide to check in a file with carriage return 
before line feed, then so be it.

Of course, the code paths in your patch would be less exercised, and 
bugs/interactions could be conveniently hidden for all those who decide 
not to activate safecrlf like me, but I do not see why Linux/Unix people 
should be punished for a shortcoming that affects only projects which 
partly work on Windows, and then only if non-POSIX tools are used.

Ciao,
Dscho

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

[Johannes Schindelin]

Hi,

On Mon, 4 Feb 2008, Steffen Prohaska wrote:

>  - refactored to lines max 84 chars long.

Why 84?  I think the standard is still 80.

Ciao,
Dscho

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

[Steffen Prohaska]

On Feb 4, 2008, at 4:02 PM, Johannes Schindelin wrote:

> Hi,
>
> On Mon, 4 Feb 2008, Steffen Prohaska wrote:
>
>>  - refactored to lines max 84 chars long.
>
> Why 84?  I think the standard is still 80.

Because the surrounding code already had lines with
more than 80 characters and the most natural refactoring
without too much wrapping effort resulted in 84.

Is 80 a strict limit?

	Steffen

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

[Johannes Schindelin]

Hi,

On Mon, 4 Feb 2008, Steffen Prohaska wrote:

> On Feb 4, 2008, at 4:02 PM, Johannes Schindelin wrote:
> 
> > On Mon, 4 Feb 2008, Steffen Prohaska wrote:
> > 
> > >  - refactored to lines max 84 chars long.
> > 
> > Why 84?  I think the standard is still 80.
> 
> Because the surrounding code already had lines with more than 80 
> characters and the most natural refactoring without too much wrapping 
> effort resulted in 84.
> 
> Is 80 a strict limit?

It is not a _strict_ limit.  But it is a limit [*1*].  84 is not.

Ciao,
Dscho

*1*: Linux' CodingStyle says/said this:

Statements longer than 80 columns will be broken into sensible chunks. 
Descendants are always substantially shorter than the parent and are 
placed substantially to the right. The same applies to function headers 
with a long argument list. Long strings are as well broken into shorter 
strings. The only exception to this is where exceeding 80 columns 
significantly increases readability and does not hide information.

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

[Junio C Hamano]

Steffen Prohaska <prohaska@zib.de> writes:

> This repaces v7 of the patch and includes the following changes
>  - Commit message defends safecrlf=warn default.
>  - refactored to lines max 84 chars long.
>
>     Steffen
>
> ---- snip ---
>
> CRLF conversion bears a slight chance of corrupting data.
> ...

Is this meant for application?

If so please do not do "^---- snip ---".  More than three dashes
at the beginning of line discards everything after that from the
commit log message so "git am -i" becomes very cumbersome.  And
that is the reason you often see "-- >8 --" scissors mark from
me (notice two dashes, not three).

> autocrlf=true will convert CRLF to LF during commit and LF to
> CRLF during checkout.  A file that contains a mixture of LF and
> CRLF before the commit cannot be recreated by git.  For text
> files this does not really matter because we do not care about
> the line endings anyway; ...

That is a wrong explanation.  It is not that we do not _care_.

Far from it.  With autocrlf, we _deeply_ care about the line
endings of text files to be canonical with LF in the repository
data, and canonical with platform convention in the work tree
data.  A file with mixed LF and CRLF have _wrong_ line endings
in the work tree data, hence it actively gets _corrected_.

So it is not _anyway_.  It is what we _want to happen_.

> ... but for binary files that are
> accidentally classified as text the conversion can corrupt data.

Correct.

> In mixed Windows/Unix environments text files quite easily can
> end up containing a mixture of CRLF and LF line endings and git
> should handle such situations gracefully.  For example, a user
> could copy a CRLF file from Windows to Unix and mix it with an
> existing LF file there.  The result would contain both types of
> line endings.

Correct, but I do not think this is necessary for this commit,
as you have established already that canonicalization is what we
want for text and what we want to avoid for binary.  Just skip
these 6 lines and the thought will still flow naturally.

> Unfortunately, the desired effect of cleaning up text files with
> mixed line endings and the undesired effect of corrupting binary
> files cannot be distinguished.  In both cases CRLF endings are
> removed in an irreversible way.  For text files this is the right
> thing to do, while for binary file it corrupts data.

"CRLF" are not even "endings" in a binary file.  I'd suggest
rewriting the above paragraph as "... distinguished.  In both
cases, CRLF pairs are removed in an irreversible way."  The rest
of the paragraph is redundant --- you've said that twice already
(and I suggested to remove the second one earlier).

> In a sane environment, committing and checking out the same file
> should not modify the original file in the work tree.

Is "In a sane environment, " needed?  In what environment is it
permissible that checkout followed by commit gives a different
result?

> ... For
> autocrlf=input the original file must not contain CRLF.  For
> autocrlf=true the original file must not contain LF without
> preceding CR.  Otherwise the conversion is irreversible.  Note,
> git might be able to recreate the original file with different
> autocrlf settings, but in the current environment checking out
> will yield a file that differs from the file before the commit.

I suspect this also may be redundant.  You've already explained
that existing autocrlf conversion are lossy with respect to
mixed LF and CRLF.  I think it is easier to read if we drop
everything from "In a sane environment, " up to here.

> This patch adds a mechanism that can either warn the user about
> an irreversible conversion or can even refuse to convert.  The
> mechanism is controlled by the variable core.safecrlf, with the
> following values
>  - false: disable safecrlf mechanism
>  - warn: warn about irreversible conversions
>  - true: refuse irreversible conversions

Please end the line "following values" with a colon and have a
blan line after that.

> The default is to warn.  Users are only affected by this default
> if core.autocrlf is set.  But the current default of git is to
> leave core.autocrlf unset, so users will not see warnings unless
> they deliberately chose to activate the autocrlf mechanism.

Very sensible.

> The safecrlf mechanism's details depend on the git command.  If
> safecrlf is active (not false), files in the work tree must not
> be modified in an irreversible way without giving the user a
> chance to backup the original file.  However, for read-only
> operations that do not modify files in the work tree git should
> not print annoying warnings.
>
> git apply behave as requested by by safecrlf.  Even though
> git apply writes changes directly back to the work tree, the
> user still can get the original files back by checking out from
> the index or HEAD.  git apply should only be run on a clean
> work tree.

I think the conclusion "should" in the last sentence is wrong.
Running "apply" to get effects from more than one patches is a
norm, and you are telling the readers they are doing a wrong
thing if the did so.

With "apply", we are talking about "patches", which means we are
talking about "text", which by definition is outside of what
"safecrlf" cares about.  That is why apply does not need to be
molested.

	Side note.  Maybe when we apply a --binary patch, we
	should read the target image from the work tree without
	"convert-to-git" regardless of what the autocrlf nor the
	gitattributes say.  I did not check if we already do
	that, though.

The rest of the commit log message looked fine.

> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 4e222f1..7ebee51 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -139,6 +139,56 @@ core.autocrlf::
>  	"text" (i.e. be subjected to the autocrlf mechanism) is
>  	decided purely based on the contents.
>  
> +core.safecrlf::
> +	If true, makes git check if converting `CRLF` as controlled by
> +	`core.autocrlf` is reversible.  Git will verify if a command
> +	modifies a file in the work tree either directly or indirectly.
> +	For example, committing a file followed by checking out the
> +	same file should yield the original file in the work tree.  If
> +	this is not the case for the current setting of
> +	`core.autocrlf`, git will reject the file.  The variable can
> +	be set to "warn", in which case git will only warn about an
> +	irreversible conversion but continue the operation.
> ++
> +CRLF conversion bears a slight chance of corrupting data.
> +...
> +mechanism.

The same comment about redundancy applies here.

> diff --git a/convert.c b/convert.c
> index 80f114b..94c70f4 100644
> --- a/convert.c
> +++ b/convert.c
> @@ -85,8 +85,36 @@ static int is_binary(unsigned long size, struct text_stat *stats)
>  	return 0;
>  }
>  
> +static void check_safe_crlf(const char* path, int action,
> +                            struct text_stat* stats, enum safe_crlf checksafe)

Asterisk comes near the variable name not type: "const char *path".

> +{
> +	if (action == CRLF_INPUT || auto_crlf <= 0) {
> +		/*
> +		 * CRLFs would not be restored by checkout:
> +		 * check if we'd remove CRLFs
> +		 */
> +		if (stats->crlf) {
> +			if (checksafe == SAFE_CRLF_WARN)
> +				warning("CRLF will be replaced by LF in %s.", path);
> +			else
> +				die("CRLF would be replaced by LF in %s.", path);

What happens when checksafe == SAFE_CRLF_FALSE, I had to wonder,
until looking at the caller.

			else /* i.e. SAFE_CRLF_FAIL */

would have been easier to follow.

> diff --git a/sha1_file.c b/sha1_file.c
> index 66a4e00..4179949 100644
> --- a/sha1_file.c
> +++ b/sha1_file.c
> @@ -2358,7 +2358,8 @@ int index_fd(unsigned char *sha1, int fd, struct stat *st, int write_object,
>  	if ((type == OBJ_BLOB) && S_ISREG(st->st_mode)) {
>  		struct strbuf nbuf;
>  		strbuf_init(&nbuf, 0);
> -		if (convert_to_git(path, buf, size, &nbuf)) {
> +		if (convert_to_git(path, buf, size, &nbuf,
> +		                   write_object ? safe_crlf : 0)) {
>  			munmap(buf, size);
>  			buf = strbuf_detach(&nbuf, &size);
>  			re_allocated = 1;

Very nicely done ;-).

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

[Steffen Prohaska]

On Feb 6, 2008, at 9:33 AM, Junio C Hamano wrote:

> Steffen Prohaska <prohaska@zib.de> writes:
>
>> This repaces v7 of the patch and includes the following changes
>>  - Commit message defends safecrlf=warn default.
>>  - refactored to lines max 84 chars long.
>>
>>     Steffen
>>
>> ---- snip ---
>>
>> CRLF conversion bears a slight chance of corrupting data.
>> ...
>
> Is this meant for application?

Yes, this is the best I have and I personally test it in practice.
It works for me, although I recognized that I tend to miss warnings
sometimes, for example during "git commit -a".  The reason is that
the warnings are printed, but the editor for the commit message
pops up right afterwards.  With "git add" this is not a problem.

More feedback by others would be helpful.  So yes, the patch
should be applied.

But you have quite a few comments below; so I'll send an update.



> If so please do not do "^---- snip ---".  More than three dashes
> at the beginning of line discards everything after that from the
> commit log message so "git am -i" becomes very cumbersome.  And
> that is the reason you often see "-- >8 --" scissors mark from
> me (notice two dashes, not three).

Ahh.. ok, I thought any number of dashes unequal 3 would work.
I'll use your mark in the future.


>
>> autocrlf=true will convert CRLF to LF during commit and LF to
>> CRLF during checkout.  A file that contains a mixture of LF and
>> CRLF before the commit cannot be recreated by git.  For text
>> files this does not really matter because we do not care about
>> the line endings anyway; ...
>
> That is a wrong explanation.  It is not that we do not _care_.
>
> Far from it.  With autocrlf, we _deeply_ care about the line
> endings of text files to be canonical with LF in the repository
> data, and canonical with platform convention in the work tree
> data.  A file with mixed LF and CRLF have _wrong_ line endings
> in the work tree data, hence it actively gets _corrected_.
>
> So it is not _anyway_.  It is what we _want to happen_.

Hmm, yes, I agree with your reasoning.  We do care and we only
want LF.  Maybe I had Dscho's arguments in mind when I wrote
this paragraph.  He often claims that git users on Unix do care
about every single bit of the files, which means including the
"wrong" line endings.  But I don't think this is true. I think
we don't mind that autocrlf=true modifies the line endings in
a way that they cannot be exactly restored.  We don't mind
because we do not care about the exact line endings.  We only
need to know that the line ends, not if it ends in CRLF or LF.

[ comments about commit message that I'll include in the update. ]


>
>> diff --git a/convert.c b/convert.c
>> index 80f114b..94c70f4 100644
>> --- a/convert.c
>> +++ b/convert.c
>> @@ -85,8 +85,36 @@ static int is_binary(unsigned long size, struct  
>> text_stat *stats)
>>  	return 0;
>>  }
>>
>> +static void check_safe_crlf(const char* path, int action,
>> +                            struct text_stat* stats, enum  
>> safe_crlf checksafe)
>
> Asterisk comes near the variable name not type: "const char *path".

I know that but I tend to forget because I follow a different rule
all day ;)  Will fix.


>> +{
>> +	if (action == CRLF_INPUT || auto_crlf <= 0) {
>> +		/*
>> +		 * CRLFs would not be restored by checkout:
>> +		 * check if we'd remove CRLFs
>> +		 */
>> +		if (stats->crlf) {
>> +			if (checksafe == SAFE_CRLF_WARN)
>> +				warning("CRLF will be replaced by LF in %s.", path);
>> +			else
>> +				die("CRLF would be replaced by LF in %s.", path);
>
> What happens when checksafe == SAFE_CRLF_FALSE, I had to wonder,
> until looking at the caller.
>
> 			else /* i.e. SAFE_CRLF_FAIL */
>
> would have been easier to follow.

I'll add a comment and will move the "if (!checksafe)" from the  
caller to
the beginning of check_safe_crlf().


>> diff --git a/sha1_file.c b/sha1_file.c
>> index 66a4e00..4179949 100644
>> --- a/sha1_file.c
>> +++ b/sha1_file.c
>> @@ -2358,7 +2358,8 @@ int index_fd(unsigned char *sha1, int fd,  
>> struct stat *st, int write_object,
>>  	if ((type == OBJ_BLOB) && S_ISREG(st->st_mode)) {
>>  		struct strbuf nbuf;
>>  		strbuf_init(&nbuf, 0);
>> -		if (convert_to_git(path, buf, size, &nbuf)) {
>> +		if (convert_to_git(path, buf, size, &nbuf,
>> +		                   write_object ? safe_crlf : 0)) {
>>  			munmap(buf, size);
>>  			buf = strbuf_detach(&nbuf, &size);
>>  			re_allocated = 1;
>
> Very nicely done ;-).

thanks.

	Steffen

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

[Steffen Prohaska]

This repaces v9 of the patch and should be applied.
It contains modifications as suggested by Junio in
<7vbq6u32mq.fsf@gitster.siamese.dyndns.org>

    Steffen

-- >8 --

CRLF conversion bears a slight chance of corrupting data.
autocrlf=true will convert CRLF to LF during commit and LF to
CRLF during checkout.  A file that contains a mixture of LF and
CRLF before the commit cannot be recreated by git.  For text
files this is the right thing to do: it corrects line endings
such that we have only LF line endings in the repository.
But for binary files that are accidentally classified as text the
conversion can corrupt data.

If you recognize such corruption early you can easily fix it by
setting the conversion type explicitly in .gitattributes.  Right
after committing you still have the original file in your work
tree and this file is not yet corrupted.  You can explicitly tell
git that this file is binary and git will handle the file
appropriately.

Unfortunately, the desired effect of cleaning up text files with
mixed line endings and the undesired effect of corrupting binary
files cannot be distinguished.  In both cases CRLFs are removed
in an irreversible way.  For text files this is the right thing
to do because CRLFs are line endings, while for binary files
converting CRLFs corrupts data.

This patch adds a mechanism that can either warn the user about
an irreversible conversion or can even refuse to convert.  The
mechanism is controlled by the variable core.safecrlf, with the
following values:

 - false: disable safecrlf mechanism
 - warn: warn about irreversible conversions
 - true: refuse irreversible conversions

The default is to warn.  Users are only affected by this default
if core.autocrlf is set.  But the current default of git is to
leave core.autocrlf unset, so users will not see warnings unless
they deliberately chose to activate the autocrlf mechanism.

The safecrlf mechanism's details depend on the git command.  If
safecrlf is active (not false), files in the work tree must not
be modified in an irreversible way without giving the user a
chance to backup the original file.  However, for read-only
operations that do not modify files in the work tree git should
not print annoying warnings.

git apply behaves as requested by by safecrlf.  Even though git
apply writes changes directly back to the work tree, we assume
that the user still can get the original files back by checking
out from the index or HEAD.  git apply is about applying patches,
which means we are talking about "text" and therefore it is
safe to write the results directly back to the work tree even
if CRLFs were converted to LFs.

git diff, git add, and any other command that calls
index_fd(..., write_object=1) also behave as requested by
safecrlf.  The user still has the chance to save her data before
checkout or applying the diff.

git blame will never warn nor fail because it never writes to the
work tree.

The concept of a safety check was originally proposed in a similar
way by Linus Torvalds.  Thanks to Dimitry Potapov for insisting
on getting the naked LF/autocrlf=true case right.

Signed-off-by: Steffen Prohaska <prohaska@zib.de>
---
 Documentation/config.txt        |   45 ++++++++++++++++++++++++++++++
 Documentation/gitattributes.txt |    5 +++
 builtin-apply.c                 |    2 +-
 builtin-blame.c                 |    2 +-
 cache.h                         |   11 +++++++-
 config.c                        |    9 ++++++
 convert.c                       |   47 +++++++++++++++++++++++++++----
 diff.c                          |    2 +-
 environment.c                   |    1 +
 sha1_file.c                     |    3 +-
 t/t0020-crlf.sh                 |   58 +++++++++++++++++++++++++++++++++++++++
 11 files changed, 174 insertions(+), 11 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 4e222f1..7b8cae1 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -139,6 +139,51 @@ core.autocrlf::
 	"text" (i.e. be subjected to the autocrlf mechanism) is
 	decided purely based on the contents.
 
+core.safecrlf::
+	If true, makes git check if converting `CRLF` as controlled by
+	`core.autocrlf` is reversible.  Git will verify if a command
+	modifies a file in the work tree either directly or indirectly.
+	For example, committing a file followed by checking out the
+	same file should yield the original file in the work tree.  If
+	this is not the case for the current setting of
+	`core.autocrlf`, git will reject the file.  The variable can
+	be set to "warn", in which case git will only warn about an
+	irreversible conversion but continue the operation.
++
+CRLF conversion bears a slight chance of corrupting data.
+autocrlf=true will convert CRLF to LF during commit and LF to
+CRLF during checkout.  A file that contains a mixture of LF and
+CRLF before the commit cannot be recreated by git.  For text
+files this is the right thing to do: it corrects line endings
+such that we have only LF line endings in the repository.
+But for binary files that are accidentally classified as text the
+conversion can corrupt data.
++
+If you recognize such corruption early you can easily fix it by
+setting the conversion type explicitly in .gitattributes.  Right
+after committing you still have the original file in your work
+tree and this file is not yet corrupted.  You can explicitly tell
+git that this file is binary and git will handle the file
+appropriately.
++
+Unfortunately, the desired effect of cleaning up text files with
+mixed line endings and the undesired effect of corrupting binary
+files cannot be distinguished.  In both cases CRLFs are removed
+in an irreversible way.  For text files this is the right thing
+to do because CRLFs are line endings, while for binary files
+converting CRLFs corrupts data.
++
+Note, this safety check does not mean that a checkout will generate a
+file identical to the original file for a different setting of
+`core.autocrlf`, but only for the current one.  For example, a text
+file with `LF` would be accepted with `core.autocrlf=input` and could
+later be checked out with `core.autocrlf=true`, in which case the
+resulting file would contain `CRLF`, although the original file
+contained `LF`.  However, in both work trees the line endings would be
+consistent, that is either all `LF` or all `CRLF`, but never mixed.  A
+file with mixed line endings would be reported by the `core.safecrlf`
+mechanism.
+
 core.symlinks::
 	If false, symbolic links are checked out as small plain files that
 	contain the link text. linkgit:git-update-index[1] and
diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt
index 35a29fd..2ffe1fb 100644
--- a/Documentation/gitattributes.txt
+++ b/Documentation/gitattributes.txt
@@ -133,6 +133,11 @@ When `core.autocrlf` is set to "input", line endings are
 converted to LF upon checkin, but there is no conversion done
 upon checkout.
 
+If `core.safecrlf` is set to "true" or "warn", git verifies if
+the conversion is reversible for the current setting of
+`core.autocrlf`.  For "true", git rejects irreversible
+conversions; for "warn", git only prints a warning but accepts
+an irreversible conversion.
 
 `ident`
 ^^^^^^^
diff --git a/builtin-apply.c b/builtin-apply.c
index 15432b6..d0b3586 100644
--- a/builtin-apply.c
+++ b/builtin-apply.c
@@ -1430,7 +1430,7 @@ static int read_old_data(struct stat *st, const char *path, struct strbuf *buf)
 	case S_IFREG:
 		if (strbuf_read_file(buf, path, st->st_size) != st->st_size)
 			return error("unable to open or read %s", path);
-		convert_to_git(path, buf->buf, buf->len, buf);
+		convert_to_git(path, buf->buf, buf->len, buf, safe_crlf);
 		return 0;
 	default:
 		return -1;
diff --git a/builtin-blame.c b/builtin-blame.c
index 9b4c02e..c361ee1 100644
--- a/builtin-blame.c
+++ b/builtin-blame.c
@@ -2073,7 +2073,7 @@ static struct commit *fake_working_tree_commit(const char *path, const char *con
 		if (strbuf_read(&buf, 0, 0) < 0)
 			die("read error %s from stdin", strerror(errno));
 	}
-	convert_to_git(path, buf.buf, buf.len, &buf);
+	convert_to_git(path, buf.buf, buf.len, &buf, 0);
 	origin->file.ptr = buf.buf;
 	origin->file.size = buf.len;
 	pretend_sha1_file(buf.buf, buf.len, OBJ_BLOB, origin->blob_sha1);
diff --git a/cache.h b/cache.h
index 549f4bb..6003c83 100644
--- a/cache.h
+++ b/cache.h
@@ -330,6 +330,14 @@ extern size_t packed_git_limit;
 extern size_t delta_base_cache_limit;
 extern int auto_crlf;
 
+enum safe_crlf {
+	SAFE_CRLF_FALSE = 0,
+	SAFE_CRLF_FAIL = 1,
+	SAFE_CRLF_WARN = 2,
+};
+
+extern enum safe_crlf safe_crlf;
+
 #define GIT_REPO_VERSION 0
 extern int repository_format_version;
 extern int check_repository_format(void);
@@ -633,7 +641,8 @@ extern void trace_argv_printf(const char **argv, const char *format, ...);
 
 /* convert.c */
 /* returns 1 if *dst was used */
-extern int convert_to_git(const char *path, const char *src, size_t len, struct strbuf *dst);
+extern int convert_to_git(const char *path, const char *src, size_t len,
+                          struct strbuf *dst, enum safe_crlf checksafe);
 extern int convert_to_working_tree(const char *path, const char *src, size_t len, struct strbuf *dst);
 
 /* add */
diff --git a/config.c b/config.c
index 498259e..3256c99 100644
--- a/config.c
+++ b/config.c
@@ -407,6 +407,15 @@ int git_default_config(const char *var, const char *value)
 		return 0;
 	}
 
+	if (!strcmp(var, "core.safecrlf")) {
+		if (value && !strcasecmp(value, "warn")) {
+			safe_crlf = SAFE_CRLF_WARN;
+			return 0;
+		}
+		safe_crlf = git_config_bool(var, value);
+		return 0;
+	}
+
 	if (!strcmp(var, "user.name")) {
 		strlcpy(git_default_name, value, sizeof(git_default_name));
 		return 0;
diff --git a/convert.c b/convert.c
index 80f114b..3bbf0c5 100644
--- a/convert.c
+++ b/convert.c
@@ -85,8 +85,39 @@ static int is_binary(unsigned long size, struct text_stat *stats)
 	return 0;
 }
 
+static void check_safe_crlf(const char *path, int action,
+                            struct text_stat *stats, enum safe_crlf checksafe)
+{
+	if (!checksafe)
+		return;
+
+	if (action == CRLF_INPUT || auto_crlf <= 0) {
+		/*
+		 * CRLFs would not be restored by checkout:
+		 * check if we'd remove CRLFs
+		 */
+		if (stats->crlf) {
+			if (checksafe == SAFE_CRLF_WARN)
+				warning("CRLF will be replaced by LF in %s.", path);
+			else /* i.e. SAFE_CRLF_FAIL */
+				die("CRLF would be replaced by LF in %s.", path);
+		}
+	} else if (auto_crlf > 0) {
+		/*
+		 * CRLFs would be added by checkout:
+		 * check if we have "naked" LFs
+		 */
+		if (stats->lf != stats->crlf) {
+			if (checksafe == SAFE_CRLF_WARN)
+				warning("LF will be replaced by CRLF in %s", path);
+			else /* i.e. SAFE_CRLF_FAIL */
+				die("LF would be replaced by CRLF in %s", path);
+		}
+	}
+}
+
 static int crlf_to_git(const char *path, const char *src, size_t len,
-                       struct strbuf *buf, int action)
+                       struct strbuf *buf, int action, enum safe_crlf checksafe)
 {
 	struct text_stat stats;
 	char *dst;
@@ -95,9 +126,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) {
 		/*
@@ -115,6 +143,12 @@ static int crlf_to_git(const char *path, const char *src, size_t len,
 			return 0;
 	}
 
+	check_safe_crlf(path, action, &stats, checksafe);
+
+	/* Optimization: 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);
@@ -536,7 +570,8 @@ static int git_path_check_ident(const char *path, struct git_attr_check *check)
 	return !!ATTR_TRUE(value);
 }
 
-int convert_to_git(const char *path, const char *src, size_t len, struct strbuf *dst)
+int convert_to_git(const char *path, const char *src, size_t len,
+                   struct strbuf *dst, enum safe_crlf checksafe)
 {
 	struct git_attr_check check[3];
 	int crlf = CRLF_GUESS;
@@ -558,7 +593,7 @@ int convert_to_git(const char *path, const char *src, size_t len, struct strbuf
 		src = dst->buf;
 		len = dst->len;
 	}
-	ret |= crlf_to_git(path, src, len, dst, crlf);
+	ret |= crlf_to_git(path, src, len, dst, crlf, checksafe);
 	if (ret) {
 		src = dst->buf;
 		len = dst->len;
diff --git a/diff.c b/diff.c
index 5b8afdc..562c20e 100644
--- a/diff.c
+++ b/diff.c
@@ -1624,7 +1624,7 @@ int diff_populate_filespec(struct diff_filespec *s, int size_only)
 		 * Convert from working tree format to canonical git format
 		 */
 		strbuf_init(&buf, 0);
-		if (convert_to_git(s->path, s->data, s->size, &buf)) {
+		if (convert_to_git(s->path, s->data, s->size, &buf, safe_crlf)) {
 			size_t size = 0;
 			munmap(s->data, s->size);
 			s->should_munmap = 0;
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;
 unsigned whitespace_rule_cfg = WS_DEFAULT_RULE;
 
 /* This is set by setup_git_dir_gently() and/or git_default_config() */
diff --git a/sha1_file.c b/sha1_file.c
index 66a4e00..4179949 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -2358,7 +2358,8 @@ int index_fd(unsigned char *sha1, int fd, struct stat *st, int write_object,
 	if ((type == OBJ_BLOB) && S_ISREG(st->st_mode)) {
 		struct strbuf nbuf;
 		strbuf_init(&nbuf, 0);
-		if (convert_to_git(path, buf, size, &nbuf)) {
+		if (convert_to_git(path, buf, size, &nbuf,
+		                   write_object ? safe_crlf : 0)) {
 			munmap(buf, size);
 			buf = strbuf_detach(&nbuf, &size);
 			re_allocated = 1;
diff --git a/t/t0020-crlf.sh b/t/t0020-crlf.sh
index 8b27aa8..90ea081 100755
--- a/t/t0020-crlf.sh
+++ b/t/t0020-crlf.sh
@@ -8,6 +8,10 @@ q_to_nul () {
 	tr Q '\000'
 }
 
+q_to_cr () {
+	tr Q '\015'
+}
+
 append_cr () {
 	sed -e 's/$/Q/' | tr Q '\015'
 }
@@ -42,6 +46,60 @@ test_expect_success setup '
 	echo happy.
 '
 
+test_expect_success 'safecrlf: autocrlf=input, all CRLF' '
+
+	git config core.autocrlf input &&
+	git config core.safecrlf true &&
+
+	for w in I am all CRLF; do echo $w; done | append_cr >allcrlf &&
+	! git add allcrlf
+'
+
+test_expect_success 'safecrlf: autocrlf=input, mixed LF/CRLF' '
+
+	git config core.autocrlf input &&
+	git config core.safecrlf true &&
+
+	for w in Oh here is CRLFQ in text; do echo $w; done | q_to_cr >mixed &&
+	! git add mixed
+'
+
+test_expect_success 'safecrlf: autocrlf=true, all LF' '
+
+	git config core.autocrlf true &&
+	git config core.safecrlf true &&
+
+	for w in I am all LF; do echo $w; done >alllf &&
+	! git add alllf
+'
+
+test_expect_success 'safecrlf: autocrlf=true mixed LF/CRLF' '
+
+	git config core.autocrlf true &&
+	git config core.safecrlf true &&
+
+	for w in Oh here is CRLFQ in text; do echo $w; done | q_to_cr >mixed &&
+	! git add mixed
+'
+
+test_expect_success 'safecrlf: print warning only once' '
+
+	git config core.autocrlf input &&
+	git config core.safecrlf warn &&
+
+	for w in I am all LF; do echo $w; done >doublewarn &&
+	git add doublewarn &&
+	git commit -m "nowarn" &&
+	for w in Oh here is CRLFQ in text; do echo $w; done | q_to_cr >doublewarn &&
+	test $(git add doublewarn 2>&1 | grep "CRLF will be replaced by LF" | wc -l) = 1
+'
+
+test_expect_success 'switch off autocrlf, safecrlf, reset HEAD' '
+	git config core.autocrlf false &&
+	git config core.safecrlf false &&
+	git reset --hard HEAD^
+'
+
 test_expect_success 'update with autocrlf=input' '
 
 	rm -f tmp one dir/two three &&
-- 
1.5.4.40.g4a680

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

[Junio C Hamano]

Steffen Prohaska <prohaska@zib.de> writes:

> This repaces v9 of the patch and should be applied.
> It contains modifications as suggested by Junio in
> <7vbq6u32mq.fsf@gitster.siamese.dyndns.org>
>
>     Steffen
>
> -- >8 --

This v9 replaces itself, ah it repaces ;-)

> git diff, git add, and any other command that calls
> index_fd(..., write_object=1) also behave as requested by
> safecrlf.  The user still has the chance to save her data before
> checkout or applying the diff.

"git diff" writes by calling index_fd(..., write_object=1)???

I think warning in "git diff" is probably a good thing to do but
that is not because it writes.  The warning would trigger only
when you are comparing what you have in the work tree with
something else, and if you get such a warning, it means what you
can potentially commit (i.e. what you have in the work tree) is
not "autocrlf" safe.  You would get the same warning when you
later runn "git add" on such a file anyway, but it is nicer to
catch the potential problem earlier.

I do not know if the above "why this command should behave this
way with respect to safecrlf" needs to be in the commit log
message.  But I think that information should be in the
documentation to help the end users (the end users typically do
not have access to the commit log of git.git).

The rest of the patch looked sane.  Thanks.

So I'd suggest the following change to the commit log message,
and then another patch at the end squashed into the
Documentation/ part.

If you agree with these, you can just say "Ok, amend it like
so".  Or you can say "Here is an even better replacement."

----------------------------------------------------------------
[proposed change to the commit log message]

--- /var/tmp/1	2008-02-06 11:36:28.000000000 -0800
+++ /var/tmp/2	2008-02-06 11:36:19.000000000 -0800
@@ -49,28 +49,29 @@
     leave core.autocrlf unset, so users will not see warnings unless
     they deliberately chose to activate the autocrlf mechanism.
     
-    The safecrlf mechanism's details depend on the git command.  If
-    safecrlf is active (not false), files in the work tree must not
-    be modified in an irreversible way without giving the user a
-    chance to backup the original file.  However, for read-only
-    operations that do not modify files in the work tree git should
-    not print annoying warnings.
-    
-    git apply behaves as requested by by safecrlf.  Even though git
-    apply writes changes directly back to the work tree, we assume
-    that the user still can get the original files back by checking
-    out from the index or HEAD.  git apply is about applying patches,
-    which means we are talking about "text" and therefore it is
-    safe to write the results directly back to the work tree even
-    if CRLFs were converted to LFs.
-    
-    git diff, git add, and any other command that calls
-    index_fd(..., write_object=1) also behave as requested by
-    safecrlf.  The user still has the chance to save her data before
-    checkout or applying the diff.
+    The safecrlf mechanism's details depend on the git command.  The
+    general principles when safecrlf is active (not false) are:
     
-    git blame will never warn nor fail because it never writes to the
-    work tree.
+     - we warn/error out if files in the work tree can modified in an
+       irreversible way without giving the user a chance to backup the
+       original file.
+    
+     - for read-only operations that do not modify files in the work tree
+       we do not not print annoying warnings.
+    
+    There are exceptions.  Even though...
+    
+     - "git add" itself does not touch the files in the work tree, the
+       next checkout would, so the safety triggers;
+    
+     - "git apply" to update a text file with a patch does touch the files
+       in the work tree, but the operation is about text files and CRLF
+       conversion is about fixing the line ending inconsistencies, so the
+       safety does not trigger;
+    
+     - "git diff" itself does not touch the files in the work tree, it is
+       often run to inspect the changes you intend to next "git add".  To
+       catch potential problems early, safety triggers.
     
     The concept of a safety check was originally proposed in a similar
     way by Linus Torvalds.  Thanks to Dimitry Potapov for insisting

----------------------------------------------------------------
[adding the general rule and per-command behaviour to the doc]

diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt
index 2ffe1fb..84ec962 100644
--- a/Documentation/gitattributes.txt
+++ b/Documentation/gitattributes.txt
@@ -137,7 +137,22 @@ If `core.safecrlf` is set to "true" or "warn", git verifies if
 the conversion is reversible for the current setting of
 `core.autocrlf`.  For "true", git rejects irreversible
 conversions; for "warn", git only prints a warning but accepts
-an irreversible conversion.
+an irreversible conversion.  The safety triggers to prevent such
+a conversion done to the files in the work tree, but there are a
+few exceptions.  Even though...
+
+- "git add" itself does not touch the files in the work tree, the
+  next checkout would, so the safety triggers;
+
+- "git apply" to update a text file with a patch does touch the files
+  in the work tree, but the operation is about text files and CRLF
+  conversion is about fixing the line ending inconsistencies, so the
+  safety does not trigger;
+
+- "git diff" itself does not touch the files in the work tree, it is
+  often run to inspect the changes you intend to next "git add".  To
+  catch potential problems early, safety triggers.
+
 
 `ident`
 ^^^^^^^

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

[Steffen Prohaska]

On Feb 6, 2008, at 8:42 PM, Junio C Hamano wrote:

> Steffen Prohaska <prohaska@zib.de> writes:
>
>> This repaces v9 of the patch and should be applied.
>> It contains modifications as suggested by Junio in
>> <7vbq6u32mq.fsf@gitster.siamese.dyndns.org>
>>
>>     Steffen
>>
>> -- >8 --
>
> This v9 replaces itself, ah it repaces ;-)

Yeah, I recognized this, too; but was to lazy to send another
correction ;)


>
>> git diff, git add, and any other command that calls
>> index_fd(..., write_object=1) also behave as requested by
>> safecrlf.  The user still has the chance to save her data before
>> checkout or applying the diff.
>
> "git diff" writes by calling index_fd(..., write_object=1)???
>
> I think warning in "git diff" is probably a good thing to do but
> that is not because it writes.  The warning would trigger only
> when you are comparing what you have in the work tree with
> something else, and if you get such a warning, it means what you
> can potentially commit (i.e. what you have in the work tree) is
> not "autocrlf" safe.  You would get the same warning when you
> later runn "git add" on such a file anyway, but it is nicer to
> catch the potential problem earlier.
>
> I do not know if the above "why this command should behave this
> way with respect to safecrlf" needs to be in the commit log
> message.  But I think that information should be in the
> documentation to help the end users (the end users typically do
> not have access to the commit log of git.git).
>
> The rest of the patch looked sane.  Thanks.
>
> So I'd suggest the following change to the commit log message,
> and then another patch at the end squashed into the
> Documentation/ part.
>
> If you agree with these, you can just say "Ok, amend it like
> so".  Or you can say "Here is an even better replacement."

Ok, amend it as you proposed but also squash in the following ...

-- >8 --
diff --git a/builtin-apply.c b/builtin-apply.c
index d0b3586..3b5618d 100644
--- a/builtin-apply.c
+++ b/builtin-apply.c
@@ -1430,7 +1430,7 @@ static int read_old_data(struct stat *st, const  
char *path, struct strbuf *buf)
  	case S_IFREG:
  		if (strbuf_read_file(buf, path, st->st_size) != st->st_size)
  			return error("unable to open or read %s", path);
-		convert_to_git(path, buf->buf, buf->len, buf, safe_crlf);
+		convert_to_git(path, buf->buf, buf->len, buf, 0);
  		return 0;
  	default:
  		return -1;
-- >8 --

... because ...

[...]

> +
> +- "git apply" to update a text file with a patch does touch the files
> +  in the work tree, but the operation is about text files and CRLF
> +  conversion is about fixing the line ending inconsistencies, so the
> +  safety does not trigger;

... now after I read this I fully understand what you meant before.
But my v9 patch does not match your description without the change
to builtin-apply.c from above.  In v9 the mechanism was still activated
in "git apply".

	Steffen