{"thread":{"id":"11841","subject":"[PATCH v6] safecrlf: Add mechanism to warn about irreversible crlf conversions","startedAt":"2008-02-02T16:08:28Z","lastAt":"2008-02-06T20:35:11Z","messageCount":17,"participants":["Steffen Prohaska","Junio C Hamano","Johannes Schindelin"],"isPatch":true,"patchVersion":6,"patchTotal":null},"messages":[{"id":"67142","messageId":"12019685081997-git-send-email-prohaska@zib.de","threadId":"11841","inReplyTo":null,"subject":"[PATCH v6] safecrlf: Add mechanism to warn about irreversible crlf conversions","fromName":"Steffen Prohaska","fromEmail":"prohaska@zib.de","sentAt":"2008-02-02T16:08:28Z","receivedAt":"2008-02-02T16:08:28Z","isPatch":true,"sender":{"key":"prohaska@zib.de","avatar":"https://avatars.githubusercontent.com/u/217580?v=4"},"body":"Here is the current version of the safecrlf patch. It applies to\nthe post 1.5.4 master and has the following changes since v5:\n - Uses git config instead of git repo-config.\n - The tests use test_expect_success.\n\n Steffen\n\n---- snip ---\n\nCRLF conversion bears a slight chance of corrupting data.\nautocrlf=true will convert CRLF to LF during commit and LF to\nCRLF during checkout. A file that contains a mixture of LF and\nCRLF before the commit cannot be recreated by git. For text\nfiles this does not really matter because we do not care about\nthe line endings anyway; but for binary files that are\naccidentally classified as text the conversion can corrupt data.\n\nIf you recognize such corruption early you can easily fix it by\nsetting the conversion type explicitly in .gitattributes. Right\nafter committing you still have the original file in your work\ntree and this file is not yet corrupted. You can explicitly tell\ngit that this file is binary and git will handle the file\nappropriately.\n\nIn mixed Windows/Unix environments text files quite easily can\nend up containing a mixture of CRLF and LF line endings and git\nshould handle such situations gracefully. For example, a user\ncould copy a CRLF file from Windows to Unix and mix it with an\nexisting LF file there. The result would contain both types of\nline endings.\n\nUnfortunately, the desired effect of cleaning up text files with\nmixed line endings and the undesired effect of corrupting binary\nfiles can not be distinguished. In both cases CRLF endings are\nremoved in an irreversible way. For text files this is the right\nthing to do, while for binary file it corrupts data.\n\nIn a sane environment, committing and checking out the same file\nshould not modify the original file in the work tree. For\nautocrlf=input the original file must not contain CRLF. For\nautocrlf=true the original file must not contain LF without\npreceding CR. Otherwise the conversion is irreversible. Note,\ngit might be able to recreate the original file with different\nautocrlf settings, but in the current environment checking out\nwill yield a file that differs from the file before the commit.\n\nThis patch adds a mechanism that can either warn the user about\nan irreversible conversion or can even refuse to convert. The\nmechanism is controlled by the variable core.safecrlf, with the\nfollowing values\n - false: disable safecrlf mechanism\n - warn: warn about irreversible conversions\n - true: refuse irreversible conversions\n\nThe default is to warn.\n\nThe safecrlf mechanism's details depend on the git command. If\nsafecrlf is active (not false), files in the work tree must not\nbe modified in an irreversible way without giving the user a\nchance to backup the original file. However, for read-only\noperations that do not modify files in the work tree git should\nnot print annoying warnings.\n\nThis commit modifies git apply to fail even if safecrlf=warn,\nbecause git apply writes its changes back to the work tree\nimmediately. The user would not have a chance to backup the old\nversion of the file if only a warning was printed. Hence, a\nwarning isn't sufficient and we should fail instead.\n\ngit diff, git add, and any other command that calls\nindex_fd(..., write_object=1) behave exactly as requested\nby safecrlf. The user still has the chance to save her data\nbefore checkout or applying the diff.\n\ngit blame will never warn nor fail because it never writes to the\nwork tree.\n\nThe concept of a safety check was originally proposed in a similar\nway by Linus Torvalds. Thanks to Dimitry Potapov for insisting\non getting the naked LF/autocrlf=true case right.\n\nSigned-off-by: Steffen Prohaska \n---\n Documentation/config.txt | 22 +++++++++++++++\n Documentation/gitattributes.txt | 5 +++\n builtin-apply.c | 2 +-\n builtin-blame.c | 2 +-\n cache.h | 10 ++++++-\n config.c | 9 ++++++\n convert.c | 34 +++++++++++++++++++----\n diff.c | 2 +-\n environment.c | 1 +\n sha1_file.c | 2 +-\n t/t0020-crlf.sh | 58 +++++++++++++++++++++++++++++++++++++++\n 11 files changed, 136 insertions(+), 11 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 4e222f1..b1b5c67 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -139,6 +139,28 @@ core.autocrlf::\n \t\"text\" (i.e. be subjected to the autocrlf mechanism) is\n \tdecided purely based on the contents.\n \n+core.safecrlf::\n+\tIf true, makes git check if converting `CRLF` as controlled by\n+\t`core.autocrlf` is reversible. Git will check if a command\n+\tmodifies a file in the work tree either directly or indirectly.\n+\tFor example, committing a file followed by checking out the\n+\tsame file should yield the original file in the work tree. If\n+\tthis is not the case for the current setting of\n+\t`core.autocrlf`, git will reject the file. The variable can\n+\tbe set to \"warn\", in which case git will only warn about an\n+\tirreversible conversion but accept the file.\n++\n+Note, this safety check does not mean that a checkout will generate a\n+file identical to the original file for different settings of\n+`core.autocrlf`, but only for the current one. For example, a text\n+file with `LF` would be accepted with `core.autocrlf=input` and could\n+later be checked out with `core.autocrlf=true`, in which case the\n+resulting file would contain `CRLF`, although the original file\n+contained `LF`. However, in both work trees the line endings would be\n+consistent, that is either all `LF` or all `CRLF`, but never mixed. A\n+file with mixed line endings would be reported by the `core.safecrlf`\n+mechanism.\n+\n core.symlinks::\n \tIf false, symbolic links are checked out as small plain files that\n \tcontain the link text. linkgit:git-update-index[1] and\ndiff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt\nindex 35a29fd..2ffe1fb 100644\n--- a/Documentation/gitattributes.txt\n+++ b/Documentation/gitattributes.txt\n@@ -133,6 +133,11 @@ When `core.autocrlf` is set to \"input\", line endings are\n converted to LF upon checkin, but there is no conversion done\n upon checkout.\n \n+If `core.safecrlf` is set to \"true\" or \"warn\", git verifies if\n+the conversion is reversible for the current setting of\n+`core.autocrlf`. For \"true\", git rejects irreversible\n+conversions; for \"warn\", git only prints a warning but accepts\n+an irreversible conversion.\n \n `ident`\n ^^^^^^^\ndiff --git a/builtin-apply.c b/builtin-apply.c\nindex 15432b6..e487936 100644\n--- a/builtin-apply.c\n+++ b/builtin-apply.c\n@@ -1430,7 +1430,7 @@ static int read_old_data(struct stat *st, const char *path, struct strbuf *buf)\n \tcase S_IFREG:\n \t\tif (strbuf_read_file(buf, path, st->st_size) != st->st_size)\n \t\t\treturn error(\"unable to open or read %s\", path);\n-\t\tconvert_to_git(path, buf->buf, buf->len, buf);\n+\t\tconvert_to_git(path, buf->buf, buf->len, buf, safe_crlf ? SAFE_CRLF_FAIL : 0);\n \t\treturn 0;\n \tdefault:\n \t\treturn -1;\ndiff --git a/builtin-blame.c b/builtin-blame.c\nindex 9b4c02e..c361ee1 100644\n--- a/builtin-blame.c\n+++ b/builtin-blame.c\n@@ -2073,7 +2073,7 @@ static struct commit *fake_working_tree_commit(const char *path, const char *con\n \t\tif (strbuf_read(&buf, 0, 0) < 0)\n \t\t\tdie(\"read error %s from stdin\", strerror(errno));\n \t}\n-\tconvert_to_git(path, buf.buf, buf.len, &buf);\n+\tconvert_to_git(path, buf.buf, buf.len, &buf, 0);\n \torigin->file.ptr = buf.buf;\n \torigin->file.size = buf.len;\n \tpretend_sha1_file(buf.buf, buf.len, OBJ_BLOB, origin->blob_sha1);\ndiff --git a/cache.h b/cache.h\nindex 549f4bb..f8223a6 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -330,6 +330,14 @@ extern size_t packed_git_limit;\n extern size_t delta_base_cache_limit;\n extern int auto_crlf;\n \n+enum safe_crlf {\n+\tSAFE_CRLF_FALSE = 0,\n+\tSAFE_CRLF_FAIL = 1,\n+\tSAFE_CRLF_WARN = 2,\n+};\n+\n+extern enum safe_crlf safe_crlf;\n+\n #define GIT_REPO_VERSION 0\n extern int repository_format_version;\n extern int check_repository_format(void);\n@@ -633,7 +641,7 @@ extern void trace_argv_printf(const char **argv, const char *format, ...);\n \n /* convert.c */\n /* returns 1 if *dst was used */\n-extern int convert_to_git(const char *path, const char *src, size_t len, struct strbuf *dst);\n+extern int convert_to_git(const char *path, const char *src, size_t len, struct strbuf *dst, enum safe_crlf checksafe);\n extern int convert_to_working_tree(const char *path, const char *src, size_t len, struct strbuf *dst);\n \n /* add */\ndiff --git a/config.c b/config.c\nindex 526a3f4..5fe7cce 100644\n--- a/config.c\n+++ b/config.c\n@@ -407,6 +407,15 @@ int git_default_config(const char *var, const char *value)\n \t\treturn 0;\n \t}\n \n+\tif (!strcmp(var, \"core.safecrlf\")) {\n+\t\tif (value && !strcasecmp(value, \"warn\")) {\n+\t\t\tsafe_crlf = SAFE_CRLF_WARN;\n+\t\t\treturn 0;\n+\t\t}\n+\t\tsafe_crlf = git_config_bool(var, value);\n+\t\treturn 0;\n+\t}\n+\n \tif (!strcmp(var, \"user.name\")) {\n \t\tstrlcpy(git_default_name, value, sizeof(git_default_name));\n \t\treturn 0;\ndiff --git a/convert.c b/convert.c\nindex 80f114b..d580a62 100644\n--- a/convert.c\n+++ b/convert.c\n@@ -86,7 +86,7 @@ static int is_binary(unsigned long size, struct text_stat *stats)\n }\n \n static int crlf_to_git(const char *path, const char *src, size_t len,\n- struct strbuf *buf, int action)\n+ struct strbuf *buf, int action, enum safe_crlf checksafe)\n {\n \tstruct text_stat stats;\n \tchar *dst;\n@@ -95,9 +95,6 @@ static int crlf_to_git(const char *path, const char *src, size_t len,\n \t\treturn 0;\n \n \tgather_stats(src, len, &stats);\n-\t/* No CR? Nothing to convert, regardless. */\n-\tif (!stats.cr)\n-\t\treturn 0;\n \n \tif (action == CRLF_GUESS) {\n \t\t/*\n@@ -115,6 +112,30 @@ static int crlf_to_git(const char *path, const char *src, size_t len,\n \t\t\treturn 0;\n \t}\n \n+\tif (checksafe) {\n+\t\tif (action == CRLF_INPUT || auto_crlf <= 0) {\n+\t\t\t/* CRLFs would not be restored by checkout: check if we'd remove CRLFs */\n+\t\t\tif (stats.crlf) {\n+\t\t\t\tif (checksafe == SAFE_CRLF_WARN)\n+\t\t\t\t\twarning(\"CRLF will be replaced by LF in %s.\", path);\n+\t\t\t\telse\n+\t\t\t\t\tdie(\"CRLF would be replaced by LF in %s.\", path);\n+\t\t\t}\n+\t\t} else if (auto_crlf > 0) {\n+\t\t\t/* CRLFs would be added by checkout: check if we have \"naked\" LFs */\n+\t\t\tif (stats.lf != stats.crlf) {\n+\t\t\t\tif (checksafe == SAFE_CRLF_WARN)\n+\t\t\t\t\twarning(\"LFs will be replaced by CRLF in %s\", path);\n+\t\t\t\telse\n+\t\t\t\t\tdie(\"LF would be replaced by CRLF in %s\", path);\n+\t\t\t}\n+\t\t}\n+\t}\n+\n+\t/* Optimization: No CR? Nothing to convert, regardless. */\n+\tif (!stats.cr)\n+\t\treturn 0;\n+\n \t/* only grow if not in place */\n \tif (strbuf_avail(buf) + buf->len < len)\n \t\tstrbuf_grow(buf, len - buf->len);\n@@ -536,7 +557,8 @@ static int git_path_check_ident(const char *path, struct git_attr_check *check)\n \treturn !!ATTR_TRUE(value);\n }\n \n-int convert_to_git(const char *path, const char *src, size_t len, struct strbuf *dst)\n+int convert_to_git(const char *path, const char *src, size_t len,\n+\t\t\tstruct strbuf *dst, enum safe_crlf checksafe)\n {\n \tstruct git_attr_check check[3];\n \tint crlf = CRLF_GUESS;\n@@ -558,7 +580,7 @@ int convert_to_git(const char *path, const char *src, size_t len, struct strbuf\n \t\tsrc = dst->buf;\n \t\tlen = dst->len;\n \t}\n-\tret |= crlf_to_git(path, src, len, dst, crlf);\n+\tret |= crlf_to_git(path, src, len, dst, crlf, checksafe);\n \tif (ret) {\n \t\tsrc = dst->buf;\n \t\tlen = dst->len;\ndiff --git a/diff.c b/diff.c\nindex 5b8afdc..562c20e 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -1624,7 +1624,7 @@ int diff_populate_filespec(struct diff_filespec *s, int size_only)\n \t\t * Convert from working tree format to canonical git format\n \t\t */\n \t\tstrbuf_init(&buf, 0);\n-\t\tif (convert_to_git(s->path, s->data, s->size, &buf)) {\n+\t\tif (convert_to_git(s->path, s->data, s->size, &buf, safe_crlf)) {\n \t\t\tsize_t size = 0;\n \t\t\tmunmap(s->data, s->size);\n \t\t\ts->should_munmap = 0;\ndiff --git a/environment.c b/environment.c\nindex 18a1c4e..e351e99 100644\n--- a/environment.c\n+++ b/environment.c\n@@ -35,6 +35,7 @@ int pager_use_color = 1;\n char *editor_program;\n char *excludes_file;\n int auto_crlf = 0;\t/* 1: both ways, -1: only when adding git objects */\n+enum safe_crlf safe_crlf = SAFE_CRLF_WARN;\n unsigned whitespace_rule_cfg = WS_DEFAULT_RULE;\n \n /* This is set by setup_git_dir_gently() and/or git_default_config() */\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 66a4e00..1cf8337 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -2358,7 +2358,7 @@ int index_fd(unsigned char *sha1, int fd, struct stat *st, int write_object,\n \tif ((type == OBJ_BLOB) && S_ISREG(st->st_mode)) {\n \t\tstruct strbuf nbuf;\n \t\tstrbuf_init(&nbuf, 0);\n-\t\tif (convert_to_git(path, buf, size, &nbuf)) {\n+\t\tif (convert_to_git(path, buf, size, &nbuf, write_object ? safe_crlf : 0)) {\n \t\t\tmunmap(buf, size);\n \t\t\tbuf = strbuf_detach(&nbuf, &size);\n \t\t\tre_allocated = 1;\ndiff --git a/t/t0020-crlf.sh b/t/t0020-crlf.sh\nindex 8b27aa8..90ea081 100755\n--- a/t/t0020-crlf.sh\n+++ b/t/t0020-crlf.sh\n@@ -8,6 +8,10 @@ q_to_nul () {\n \ttr Q '\\000'\n }\n \n+q_to_cr () {\n+\ttr Q '\\015'\n+}\n+\n append_cr () {\n \tsed -e 's/$/Q/' | tr Q '\\015'\n }\n@@ -42,6 +46,60 @@ test_expect_success setup '\n \techo happy.\n '\n \n+test_expect_success 'safecrlf: autocrlf=input, all CRLF' '\n+\n+\tgit config core.autocrlf input &&\n+\tgit config core.safecrlf true &&\n+\n+\tfor w in I am all CRLF; do echo $w; done | append_cr >allcrlf &&\n+\t! git add allcrlf\n+'\n+\n+test_expect_success 'safecrlf: autocrlf=input, mixed LF/CRLF' '\n+\n+\tgit config core.autocrlf input &&\n+\tgit config core.safecrlf true &&\n+\n+\tfor w in Oh here is CRLFQ in text; do echo $w; done | q_to_cr >mixed &&\n+\t! git add mixed\n+'\n+\n+test_expect_success 'safecrlf: autocrlf=true, all LF' '\n+\n+\tgit config core.autocrlf true &&\n+\tgit config core.safecrlf true &&\n+\n+\tfor w in I am all LF; do echo $w; done >alllf &&\n+\t! git add alllf\n+'\n+\n+test_expect_success 'safecrlf: autocrlf=true mixed LF/CRLF' '\n+\n+\tgit config core.autocrlf true &&\n+\tgit config core.safecrlf true &&\n+\n+\tfor w in Oh here is CRLFQ in text; do echo $w; done | q_to_cr >mixed &&\n+\t! git add mixed\n+'\n+\n+test_expect_success 'safecrlf: print warning only once' '\n+\n+\tgit config core.autocrlf input &&\n+\tgit config core.safecrlf warn &&\n+\n+\tfor w in I am all LF; do echo $w; done >doublewarn &&\n+\tgit add doublewarn &&\n+\tgit commit -m \"nowarn\" &&\n+\tfor w in Oh here is CRLFQ in text; do echo $w; done | q_to_cr >doublewarn &&\n+\ttest $(git add doublewarn 2>&1 | grep \"CRLF will be replaced by LF\" | wc -l) = 1\n+'\n+\n+test_expect_success 'switch off autocrlf, safecrlf, reset HEAD' '\n+\tgit config core.autocrlf false &&\n+\tgit config core.safecrlf false &&\n+\tgit reset --hard HEAD^\n+'\n+\n test_expect_success 'update with autocrlf=input' '\n \n \trm -f tmp one dir/two three &&\n-- \n1.5.4.rc5.33.gc24ec\n"},{"id":"67206","messageId":"7vbq6yuxeh.fsf@gitster.siamese.dyndns.org","threadId":"11841","inReplyTo":"12019685081997-git-send-email-prohaska@zib.de","subject":"Re: [PATCH v6] safecrlf: Add mechanism to warn about irreversible crlf conversions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-02-03T10:50:14Z","receivedAt":"2008-02-03T10:50:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Steffen Prohaska writes:\n\n> CRLF conversion bears a slight chance of corrupting data.\n> ...\n> thing to do, while for binary file it corrupts data.\n\nThe above 25-line or so are well written and deserve to be in\nthe end user documentation somewhere, I think, to explain why it\nis a good idea to have these warnings to them..\n\n> This commit modifies git apply to fail even if safecrlf=warn,\n> because git apply writes its changes back to the work tree\n> immediately. The user would not have a chance to backup the old\n> version of the file if only a warning was printed.\n\nI do not get this logic at all.\n\nThe whole point of git-apply is to apply the patch. If you say\n--whitespace=fix and some contents (say one of the testsuite\nfiles in our t/ directory) needed to keep trailing newline, you\nobviously are left with a broken result, and you would recover\nby checking it out from index or HEAD and reapply. Why\nshouldn't the same principle hold here?\n\nI haven't looked at the code of this round yet, but I promise I\nwill.\n"},{"id":"67214","messageId":"4C024D67-9FA5-451D-BB91-CE9464C6F50D@zib.de","threadId":"11841","inReplyTo":"7vbq6yuxeh.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH v6] safecrlf: Add mechanism to warn about irreversible crlf conversions","fromName":"Steffen Prohaska","fromEmail":"prohaska@zib.de","sentAt":"2008-02-03T14:36:22Z","receivedAt":"2008-02-03T14:36:22Z","isPatch":true,"sender":{"key":"prohaska@zib.de","avatar":"https://avatars.githubusercontent.com/u/217580?v=4"},"body":"\nOn Feb 3, 2008, at 11:50 AM, Junio C Hamano wrote:\n\n> Steffen Prohaska writes:\n>\n>> CRLF conversion bears a slight chance of corrupting data.\n>> ...\n>> thing to do, while for binary file it corrupts data.\n>\n> The above 25-line or so are well written and deserve to be in\n> the end user documentation somewhere, I think, to explain why it\n> is a good idea to have these warnings to them..\n\nFor now, I propose to add it to the documentation of\ncore.safecrlf. In the future a section on cross-platform\nprojects in the user manual would probably be the right place.\nBut the manual does not yet have such a section.\n\n\n>> This commit modifies git apply to fail even if safecrlf=warn,\n>> because git apply writes its changes back to the work tree\n>> immediately. The user would not have a chance to backup the old\n>> version of the file if only a warning was printed.\n>\n> I do not get this logic at all.\n>\n> The whole point of git-apply is to apply the patch. If you say\n> --whitespace=fix and some contents (say one of the testsuite\n> files in our t/ directory) needed to keep trailing newline, you\n> obviously are left with a broken result, and you would recover\n> by checking it out from index or HEAD and reapply. Why\n> shouldn't the same principle hold here?\n\nYou are right. All files should be committed before running\ngit apply and therefore the original files can be recovered by\ngit checkout. Hmm ... so apply should either just warn or be\ncompletely quiet as git blame is? I think it should warn.\n\n\tSteffen\n"},{"id":"67222","messageId":"1202053379677-git-send-email-prohaska@zib.de","threadId":"11841","inReplyTo":"4C024D67-9FA5-451D-BB91-CE9464C6F50D@zib.de","subject":"[PATCH v7] safecrlf: Add mechanism to warn about irreversible crlf conversions","fromName":"Steffen Prohaska","fromEmail":"prohaska@zib.de","sentAt":"2008-02-03T15:42:59Z","receivedAt":"2008-02-03T15:42:59Z","isPatch":true,"sender":{"key":"prohaska@zib.de","avatar":"https://avatars.githubusercontent.com/u/217580?v=4"},"body":"This repaces v6 of the patch and includes the following changes\n - Reason behind safecrlf documented in config.txt\n - git apply exactly behaves as configured in core.safecrlf\n\n Steffen\n\n---- snip ---\n\nCRLF conversion bears a slight chance of corrupting data.\nautocrlf=true will convert CRLF to LF during commit and LF to\nCRLF during checkout. A file that contains a mixture of LF and\nCRLF before the commit cannot be recreated by git. For text\nfiles this does not really matter because we do not care about\nthe line endings anyway; but for binary files that are\naccidentally classified as text the conversion can corrupt data.\n\nIf you recognize such corruption early you can easily fix it by\nsetting the conversion type explicitly in .gitattributes. Right\nafter committing you still have the original file in your work\ntree and this file is not yet corrupted. You can explicitly tell\ngit that this file is binary and git will handle the file\nappropriately.\n\nIn mixed Windows/Unix environments text files quite easily can\nend up containing a mixture of CRLF and LF line endings and git\nshould handle such situations gracefully. For example, a user\ncould copy a CRLF file from Windows to Unix and mix it with an\nexisting LF file there. The result would contain both types of\nline endings.\n\nUnfortunately, the desired effect of cleaning up text files with\nmixed line endings and the undesired effect of corrupting binary\nfiles cannot be distinguished. In both cases CRLF endings are\nremoved in an irreversible way. For text files this is the right\nthing to do, while for binary file it corrupts data.\n\nIn a sane environment, committing and checking out the same file\nshould not modify the original file in the work tree. For\nautocrlf=input the original file must not contain CRLF. For\nautocrlf=true the original file must not contain LF without\npreceding CR. Otherwise the conversion is irreversible. Note,\ngit might be able to recreate the original file with different\nautocrlf settings, but in the current environment checking out\nwill yield a file that differs from the file before the commit.\n\nThis patch adds a mechanism that can either warn the user about\nan irreversible conversion or can even refuse to convert. The\nmechanism is controlled by the variable core.safecrlf, with the\nfollowing values\n - false: disable safecrlf mechanism\n - warn: warn about irreversible conversions\n - true: refuse irreversible conversions\n\nThe default is to warn.\n\nThe safecrlf mechanism's details depend on the git command. If\nsafecrlf is active (not false), files in the work tree must not\nbe modified in an irreversible way without giving the user a\nchance to backup the original file. However, for read-only\noperations that do not modify files in the work tree git should\nnot print annoying warnings.\n\ngit apply behave as requested by by safecrlf. Even though\ngit apply writes changes directly back to the work tree, the\nuser still can get the original files back by checking out from\nthe index or HEAD. git apply should only be run on a clean\nwork tree.\n\ngit diff, git add, and any other command that calls\nindex_fd(..., write_object=1) also behave as requested by\nsafecrlf. The user still has the chance to save her data before\ncheckout or applying the diff.\n\ngit blame will never warn nor fail because it never writes to the\nwork tree.\n\nThe concept of a safety check was originally proposed in a similar\nway by Linus Torvalds. Thanks to Dimitry Potapov for insisting\non getting the naked LF/autocrlf=true case right.\n\nSigned-off-by: Steffen Prohaska \n---\n Documentation/config.txt | 50 +++++++++++++++++++++++++++++++++\n Documentation/gitattributes.txt | 5 +++\n builtin-apply.c | 2 +-\n builtin-blame.c | 2 +-\n cache.h | 10 ++++++-\n config.c | 9 ++++++\n convert.c | 34 +++++++++++++++++++----\n diff.c | 2 +-\n environment.c | 1 +\n sha1_file.c | 2 +-\n t/t0020-crlf.sh | 58 +++++++++++++++++++++++++++++++++++++++\n 11 files changed, 164 insertions(+), 11 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 4e222f1..7ebee51 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -139,6 +139,56 @@ core.autocrlf::\n \t\"text\" (i.e. be subjected to the autocrlf mechanism) is\n \tdecided purely based on the contents.\n \n+core.safecrlf::\n+\tIf true, makes git check if converting `CRLF` as controlled by\n+\t`core.autocrlf` is reversible. Git will verify if a command\n+\tmodifies a file in the work tree either directly or indirectly.\n+\tFor example, committing a file followed by checking out the\n+\tsame file should yield the original file in the work tree. If\n+\tthis is not the case for the current setting of\n+\t`core.autocrlf`, git will reject the file. The variable can\n+\tbe set to \"warn\", in which case git will only warn about an\n+\tirreversible conversion but continue the operation.\n++\n+CRLF conversion bears a slight chance of corrupting data.\n+autocrlf=true will convert CRLF to LF during commit and LF to\n+CRLF during checkout. A file that contains a mixture of LF and\n+CRLF before the commit cannot be recreated by git. For text\n+files this does not really matter because we do not care about\n+the line endings anyway; but for binary files that are\n+accidentally classified as text the conversion can corrupt data.\n++\n+If you recognize such corruption early you can easily fix it by\n+setting the conversion type explicitly in .gitattributes. Right\n+after committing you still have the original file in your work\n+tree and this file is not yet corrupted. You can explicitly tell\n+git that this file is binary and git will handle the file\n+appropriately.\n++\n+In mixed Windows/Unix environments text files quite easily can\n+end up containing a mixture of CRLF and LF line endings and git\n+should handle such situations gracefully. For example, a user\n+could copy a CRLF file from Windows to Unix and mix it with an\n+existing LF file there. The result would contain both types of\n+line endings.\n++\n+Unfortunately, the desired effect of cleaning up text files with\n+mixed line endings and the undesired effect of corrupting binary\n+files cannot be distinguished. In both cases CRLF endings are\n+removed in an irreversible way. For text files this is the right\n+thing to do, while for binary file it corrupts data.\n++\n+Note, this safety check does not mean that a checkout will generate a\n+file identical to the original file for a different setting of\n+`core.autocrlf`, but only for the current one. For example, a text\n+file with `LF` would be accepted with `core.autocrlf=input` and could\n+later be checked out with `core.autocrlf=true`, in which case the\n+resulting file would contain `CRLF`, although the original file\n+contained `LF`. However, in both work trees the line endings would be\n+consistent, that is either all `LF` or all `CRLF`, but never mixed. A\n+file with mixed line endings would be reported by the `core.safecrlf`\n+mechanism.\n+\n core.symlinks::\n \tIf false, symbolic links are checked out as small plain files that\n \tcontain the link text. linkgit:git-update-index[1] and\ndiff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt\nindex 35a29fd..2ffe1fb 100644\n--- a/Documentation/gitattributes.txt\n+++ b/Documentation/gitattributes.txt\n@@ -133,6 +133,11 @@ When `core.autocrlf` is set to \"input\", line endings are\n converted to LF upon checkin, but there is no conversion done\n upon checkout.\n \n+If `core.safecrlf` is set to \"true\" or \"warn\", git verifies if\n+the conversion is reversible for the current setting of\n+`core.autocrlf`. For \"true\", git rejects irreversible\n+conversions; for \"warn\", git only prints a warning but accepts\n+an irreversible conversion.\n \n `ident`\n ^^^^^^^\ndiff --git a/builtin-apply.c b/builtin-apply.c\nindex 15432b6..d0b3586 100644\n--- a/builtin-apply.c\n+++ b/builtin-apply.c\n@@ -1430,7 +1430,7 @@ static int read_old_data(struct stat *st, const char *path, struct strbuf *buf)\n \tcase S_IFREG:\n \t\tif (strbuf_read_file(buf, path, st->st_size) != st->st_size)\n \t\t\treturn error(\"unable to open or read %s\", path);\n-\t\tconvert_to_git(path, buf->buf, buf->len, buf);\n+\t\tconvert_to_git(path, buf->buf, buf->len, buf, safe_crlf);\n \t\treturn 0;\n \tdefault:\n \t\treturn -1;\ndiff --git a/builtin-blame.c b/builtin-blame.c\nindex 9b4c02e..c361ee1 100644\n--- a/builtin-blame.c\n+++ b/builtin-blame.c\n@@ -2073,7 +2073,7 @@ static struct commit *fake_working_tree_commit(const char *path, const char *con\n \t\tif (strbuf_read(&buf, 0, 0) < 0)\n \t\t\tdie(\"read error %s from stdin\", strerror(errno));\n \t}\n-\tconvert_to_git(path, buf.buf, buf.len, &buf);\n+\tconvert_to_git(path, buf.buf, buf.len, &buf, 0);\n \torigin->file.ptr = buf.buf;\n \torigin->file.size = buf.len;\n \tpretend_sha1_file(buf.buf, buf.len, OBJ_BLOB, origin->blob_sha1);\ndiff --git a/cache.h b/cache.h\nindex 549f4bb..f8223a6 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -330,6 +330,14 @@ extern size_t packed_git_limit;\n extern size_t delta_base_cache_limit;\n extern int auto_crlf;\n \n+enum safe_crlf {\n+\tSAFE_CRLF_FALSE = 0,\n+\tSAFE_CRLF_FAIL = 1,\n+\tSAFE_CRLF_WARN = 2,\n+};\n+\n+extern enum safe_crlf safe_crlf;\n+\n #define GIT_REPO_VERSION 0\n extern int repository_format_version;\n extern int check_repository_format(void);\n@@ -633,7 +641,7 @@ extern void trace_argv_printf(const char **argv, const char *format, ...);\n \n /* convert.c */\n /* returns 1 if *dst was used */\n-extern int convert_to_git(const char *path, const char *src, size_t len, struct strbuf *dst);\n+extern int convert_to_git(const char *path, const char *src, size_t len, struct strbuf *dst, enum safe_crlf checksafe);\n extern int convert_to_working_tree(const char *path, const char *src, size_t len, struct strbuf *dst);\n \n /* add */\ndiff --git a/config.c b/config.c\nindex 526a3f4..5fe7cce 100644\n--- a/config.c\n+++ b/config.c\n@@ -407,6 +407,15 @@ int git_default_config(const char *var, const char *value)\n \t\treturn 0;\n \t}\n \n+\tif (!strcmp(var, \"core.safecrlf\")) {\n+\t\tif (value && !strcasecmp(value, \"warn\")) {\n+\t\t\tsafe_crlf = SAFE_CRLF_WARN;\n+\t\t\treturn 0;\n+\t\t}\n+\t\tsafe_crlf = git_config_bool(var, value);\n+\t\treturn 0;\n+\t}\n+\n \tif (!strcmp(var, \"user.name\")) {\n \t\tstrlcpy(git_default_name, value, sizeof(git_default_name));\n \t\treturn 0;\ndiff --git a/convert.c b/convert.c\nindex 80f114b..d580a62 100644\n--- a/convert.c\n+++ b/convert.c\n@@ -86,7 +86,7 @@ static int is_binary(unsigned long size, struct text_stat *stats)\n }\n \n static int crlf_to_git(const char *path, const char *src, size_t len,\n- struct strbuf *buf, int action)\n+ struct strbuf *buf, int action, enum safe_crlf checksafe)\n {\n \tstruct text_stat stats;\n \tchar *dst;\n@@ -95,9 +95,6 @@ static int crlf_to_git(const char *path, const char *src, size_t len,\n \t\treturn 0;\n \n \tgather_stats(src, len, &stats);\n-\t/* No CR? Nothing to convert, regardless. */\n-\tif (!stats.cr)\n-\t\treturn 0;\n \n \tif (action == CRLF_GUESS) {\n \t\t/*\n@@ -115,6 +112,30 @@ static int crlf_to_git(const char *path, const char *src, size_t len,\n \t\t\treturn 0;\n \t}\n \n+\tif (checksafe) {\n+\t\tif (action == CRLF_INPUT || auto_crlf <= 0) {\n+\t\t\t/* CRLFs would not be restored by checkout: check if we'd remove CRLFs */\n+\t\t\tif (stats.crlf) {\n+\t\t\t\tif (checksafe == SAFE_CRLF_WARN)\n+\t\t\t\t\twarning(\"CRLF will be replaced by LF in %s.\", path);\n+\t\t\t\telse\n+\t\t\t\t\tdie(\"CRLF would be replaced by LF in %s.\", path);\n+\t\t\t}\n+\t\t} else if (auto_crlf > 0) {\n+\t\t\t/* CRLFs would be added by checkout: check if we have \"naked\" LFs */\n+\t\t\tif (stats.lf != stats.crlf) {\n+\t\t\t\tif (checksafe == SAFE_CRLF_WARN)\n+\t\t\t\t\twarning(\"LFs will be replaced by CRLF in %s\", path);\n+\t\t\t\telse\n+\t\t\t\t\tdie(\"LF would be replaced by CRLF in %s\", path);\n+\t\t\t}\n+\t\t}\n+\t}\n+\n+\t/* Optimization: No CR? Nothing to convert, regardless. */\n+\tif (!stats.cr)\n+\t\treturn 0;\n+\n \t/* only grow if not in place */\n \tif (strbuf_avail(buf) + buf->len < len)\n \t\tstrbuf_grow(buf, len - buf->len);\n@@ -536,7 +557,8 @@ static int git_path_check_ident(const char *path, struct git_attr_check *check)\n \treturn !!ATTR_TRUE(value);\n }\n \n-int convert_to_git(const char *path, const char *src, size_t len, struct strbuf *dst)\n+int convert_to_git(const char *path, const char *src, size_t len,\n+\t\t\tstruct strbuf *dst, enum safe_crlf checksafe)\n {\n \tstruct git_attr_check check[3];\n \tint crlf = CRLF_GUESS;\n@@ -558,7 +580,7 @@ int convert_to_git(const char *path, const char *src, size_t len, struct strbuf\n \t\tsrc = dst->buf;\n \t\tlen = dst->len;\n \t}\n-\tret |= crlf_to_git(path, src, len, dst, crlf);\n+\tret |= crlf_to_git(path, src, len, dst, crlf, checksafe);\n \tif (ret) {\n \t\tsrc = dst->buf;\n \t\tlen = dst->len;\ndiff --git a/diff.c b/diff.c\nindex 5b8afdc..562c20e 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -1624,7 +1624,7 @@ int diff_populate_filespec(struct diff_filespec *s, int size_only)\n \t\t * Convert from working tree format to canonical git format\n \t\t */\n \t\tstrbuf_init(&buf, 0);\n-\t\tif (convert_to_git(s->path, s->data, s->size, &buf)) {\n+\t\tif (convert_to_git(s->path, s->data, s->size, &buf, safe_crlf)) {\n \t\t\tsize_t size = 0;\n \t\t\tmunmap(s->data, s->size);\n \t\t\ts->should_munmap = 0;\ndiff --git a/environment.c b/environment.c\nindex 18a1c4e..e351e99 100644\n--- a/environment.c\n+++ b/environment.c\n@@ -35,6 +35,7 @@ int pager_use_color = 1;\n char *editor_program;\n char *excludes_file;\n int auto_crlf = 0;\t/* 1: both ways, -1: only when adding git objects */\n+enum safe_crlf safe_crlf = SAFE_CRLF_WARN;\n unsigned whitespace_rule_cfg = WS_DEFAULT_RULE;\n \n /* This is set by setup_git_dir_gently() and/or git_default_config() */\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 66a4e00..1cf8337 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -2358,7 +2358,7 @@ int index_fd(unsigned char *sha1, int fd, struct stat *st, int write_object,\n \tif ((type == OBJ_BLOB) && S_ISREG(st->st_mode)) {\n \t\tstruct strbuf nbuf;\n \t\tstrbuf_init(&nbuf, 0);\n-\t\tif (convert_to_git(path, buf, size, &nbuf)) {\n+\t\tif (convert_to_git(path, buf, size, &nbuf, write_object ? safe_crlf : 0)) {\n \t\t\tmunmap(buf, size);\n \t\t\tbuf = strbuf_detach(&nbuf, &size);\n \t\t\tre_allocated = 1;\ndiff --git a/t/t0020-crlf.sh b/t/t0020-crlf.sh\nindex 8b27aa8..90ea081 100755\n--- a/t/t0020-crlf.sh\n+++ b/t/t0020-crlf.sh\n@@ -8,6 +8,10 @@ q_to_nul () {\n \ttr Q '\\000'\n }\n \n+q_to_cr () {\n+\ttr Q '\\015'\n+}\n+\n append_cr () {\n \tsed -e 's/$/Q/' | tr Q '\\015'\n }\n@@ -42,6 +46,60 @@ test_expect_success setup '\n \techo happy.\n '\n \n+test_expect_success 'safecrlf: autocrlf=input, all CRLF' '\n+\n+\tgit config core.autocrlf input &&\n+\tgit config core.safecrlf true &&\n+\n+\tfor w in I am all CRLF; do echo $w; done | append_cr >allcrlf &&\n+\t! git add allcrlf\n+'\n+\n+test_expect_success 'safecrlf: autocrlf=input, mixed LF/CRLF' '\n+\n+\tgit config core.autocrlf input &&\n+\tgit config core.safecrlf true &&\n+\n+\tfor w in Oh here is CRLFQ in text; do echo $w; done | q_to_cr >mixed &&\n+\t! git add mixed\n+'\n+\n+test_expect_success 'safecrlf: autocrlf=true, all LF' '\n+\n+\tgit config core.autocrlf true &&\n+\tgit config core.safecrlf true &&\n+\n+\tfor w in I am all LF; do echo $w; done >alllf &&\n+\t! git add alllf\n+'\n+\n+test_expect_success 'safecrlf: autocrlf=true mixed LF/CRLF' '\n+\n+\tgit config core.autocrlf true &&\n+\tgit config core.safecrlf true &&\n+\n+\tfor w in Oh here is CRLFQ in text; do echo $w; done | q_to_cr >mixed &&\n+\t! git add mixed\n+'\n+\n+test_expect_success 'safecrlf: print warning only once' '\n+\n+\tgit config core.autocrlf input &&\n+\tgit config core.safecrlf warn &&\n+\n+\tfor w in I am all LF; do echo $w; done >doublewarn &&\n+\tgit add doublewarn &&\n+\tgit commit -m \"nowarn\" &&\n+\tfor w in Oh here is CRLFQ in text; do echo $w; done | q_to_cr >doublewarn &&\n+\ttest $(git add doublewarn 2>&1 | grep \"CRLF will be replaced by LF\" | wc -l) = 1\n+'\n+\n+test_expect_success 'switch off autocrlf, safecrlf, reset HEAD' '\n+\tgit config core.autocrlf false &&\n+\tgit config core.safecrlf false &&\n+\tgit reset --hard HEAD^\n+'\n+\n test_expect_success 'update with autocrlf=input' '\n \n \trm -f tmp one dir/two three &&\n-- \n1.5.4.19.g5f7bf\n"},{"id":"67270","messageId":"alpine.LSU.1.00.0802032222380.7372@racer.site","threadId":"11841","inReplyTo":"1202053379677-git-send-email-prohaska@zib.de","subject":"Re: [PATCH v7] safecrlf: Add mechanism to warn about irreversible crlf conversions","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-02-03T22:29:54Z","receivedAt":"2008-02-03T22:29:54Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Sun, 3 Feb 2008, Steffen Prohaska wrote:\n\n> diff --git a/convert.c b/convert.c\n> index 80f114b..d580a62 100644\n> --- a/convert.c\n> +++ b/convert.c\n> @@ -95,9 +95,6 @@ static int crlf_to_git(const char *path, const char *src, size_t len,\n> \t\treturn 0;\n> \n> \tgather_stats(src, len, &stats);\n> -\t/* No CR? Nothing to convert, regardless. */\n> -\tif (!stats.cr)\n> -\t\treturn 0;\n\nYes, you add it later, but would this not be \"more correct\" if you \nprepended a \"!checksafe &&\" and kept the if here? Of course, you'd _also_ \nneed it later, but then _inside_ the \"if (checksafe)\" clause.\n\n> @@ -115,6 +112,30 @@ static int crlf_to_git(const char *path, const char *src, size_t len,\n> \t\t\treturn 0;\n> \t}\n> \n> +\tif (checksafe) {\n> +\t\tif (action == CRLF_INPUT || auto_crlf <= 0) {\n> +\t\t\t/* CRLFs would not be restored by checkout: check if we'd remove CRLFs */\n\nThere are lines in your patch that are substantially longer than 80 \ncharacters / line. Please fix.\n\n> diff --git a/environment.c b/environment.c\n> index 18a1c4e..e351e99 100644\n> --- a/environment.c\n> +++ b/environment.c\n> @@ -35,6 +35,7 @@ int pager_use_color = 1;\n> char *editor_program;\n> char *excludes_file;\n> int auto_crlf = 0;\t/* 1: both ways, -1: only when adding git objects */\n> +enum safe_crlf safe_crlf = SAFE_CRLF_WARN;\n\nI think this choice needs some defending. At first I thought: \"no way \nthat this will be the default; it affects too many users it should _not_ \naffect\". The thing is, Unix users should not need to suffer, only because \nother users are cursed by insane choices of their OS developers.\n\nHowever, safe_crlf != SAFE_CRLF_FALSE does not affect people who did not \nset core.crlf = input or core.crlf = true. And for those who set \ncore.crlf, the default makes sense, absolutely.\n\nSo I think this patch should go in, with shortened lines (and possibly the \nchanges to the \"if (!stats.cr)\" if you agree with me).\n\nCiao,\nDscho\n"},{"id":"67272","messageId":"7vejbtsld4.fsf@gitster.siamese.dyndns.org","threadId":"11841","inReplyTo":"4C024D67-9FA5-451D-BB91-CE9464C6F50D@zib.de","subject":"Re: [PATCH v6] safecrlf: Add mechanism to warn about irreversible crlf conversions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-02-03T22:53:11Z","receivedAt":"2008-02-03T22:53:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Steffen Prohaska writes:\n\n>> The whole point of git-apply is to apply the patch. If you say\n>> --whitespace=fix and some contents (say one of the testsuite\n>> files in our t/ directory) needed to keep trailing newline, you\n>> obviously are left with a broken result, and you would recover\n>> by checking it out from index or HEAD and reapply. Why\n>> shouldn't the same principle hold here?\n>\n> You are right. All files should be committed before running\n> git apply and therefore the original files can be recovered by\n> git checkout. Hmm ... so apply should either just warn or be\n> completely quiet as git blame is? I think it should warn.\n\nI agree that warning is needed if the setting says \"warn\".\n\nOf course, it could become part of an irreversible action if you\ndid this:\n\n\t$ git commit ;# or whatever. Now there is no local mods\n $ git apply On Sun, 3 Feb 2008, Steffen Prohaska wrote:\n>\n>> diff --git a/convert.c b/convert.c\n>> index 80f114b..d580a62 100644\n>> --- a/convert.c\n>> +++ b/convert.c\n>> @@ -95,9 +95,6 @@ static int crlf_to_git(const char *path, const \n>> char *src, size_t len,\n>> \t\treturn 0;\n>>\n>> \tgather_stats(src, len, &stats);\n>> -\t/* No CR? Nothing to convert, regardless. */\n>> -\tif (!stats.cr)\n>> -\t\treturn 0;\n>\n> Yes, you add it later, but would this not be \"more correct\" if you\n> prepended a \"!checksafe &&\" and kept the if here? Of course, you'd \n> _also_\n> need it later, but then _inside_ the \"if (checksafe)\" clause.\n\nStrictly speaking we would not need it at all, because it's an\noptimization. By moving the optimization a bit down in the\ncode we don't loose much because all the code that is executed\nbetween the old location and the new one only compares a few\nnumbers.\n\nThe expensive part (scanning the buffer) is still avoided.\n\nSo I don't think it would be \"more correct\" to duplicate the\noptimization to safe a few cycles if checksafe == false.\n\nOr do you mean that your proposed \"!checksafe &&\" would be\neasier to understand by a human?\n\n\n>> @@ -115,6 +112,30 @@ static int crlf_to_git(const char *path, \n>> const char *src, size_t len,\n>> \t\t\treturn 0;\n>> \t}\n>>\n>> +\tif (checksafe) {\n>> +\t\tif (action == CRLF_INPUT || auto_crlf <= 0) {\n>> +\t\t\t/* CRLFs would not be restored by checkout: check if we'd \n>> remove CRLFs */\n>\n> There are lines in your patch that are substantially longer than 80\n> characters / line. Please fix.\n\nI refactored the code.\n\n\n>> diff --git a/environment.c b/environment.c\n>> index 18a1c4e..e351e99 100644\n>> --- a/environment.c\n>> +++ b/environment.c\n>> @@ -35,6 +35,7 @@ int pager_use_color = 1;\n>> char *editor_program;\n>> char *excludes_file;\n>> int auto_crlf = 0;\t/* 1: both ways, -1: only when adding git \n>> objects */\n>> +enum safe_crlf safe_crlf = SAFE_CRLF_WARN;\n>\n> I think this choice needs some defending. At first I thought: \"no way\n> that this will be the default; it affects too many users it should \n> _not_\n> affect\". The thing is, Unix users should not need to suffer, only \n> because\n> other users are cursed by insane choices of their OS developers.\n>\n> However, safe_crlf != SAFE_CRLF_FALSE does not affect people who \n> did not\n> set core.crlf = input or core.crlf = true. And for those who set\n> core.crlf, the default makes sense, absolutely.\n\nI add a comment to the commit message.\n\nHowever, I don't fully agree with your comment. If your Unix\nenvironment is as sane as you assume and you never exchange any\ndata with the \"cursed\" people, you can safely set\ncore.autocrlf=input and core.safecrlf=warn and still should\nnever see any warning. Well you'd spent some CPU cycles on\nverifying that your assumptions hold. But those cycles would be\nwell spent if you accidentally received data from the underworld\nthat contained the insane line endings; because in this case git\nwould immediately warn you and would keep your repository sane.\nIf you had not activated autocrlf/safecrlf as proposed, the\ninsane line endings would pollute your repository for all eternity.\nEven if you recognized them later, they would most likely already\nbe part of your history.\n\nThat said, the next logical step is to set core.autocrlf=input\nas the default on Unix. But this can be discussed later, after\nwe have the core.safecrlf mechanism in place.\n\n\tSteffen\n"},{"id":"67317","messageId":"12021001351967-git-send-email-prohaska@zib.de","threadId":"11841","inReplyTo":"86F2B863-6FE8-407E-B535-3F49782DA5F9@zib.de","subject":"[PATCH] [v8] safecrlf: Add mechanism to warn about irreversible crlf conversions","fromName":"Steffen Prohaska","fromEmail":"prohaska@zib.de","sentAt":"2008-02-04T04:42:15Z","receivedAt":"2008-02-04T04:42:15Z","isPatch":true,"sender":{"key":"prohaska@zib.de","avatar":"https://avatars.githubusercontent.com/u/217580?v=4"},"body":"This repaces v7 of the patch and includes the following changes\n - Commit message defends safecrlf=warn default.\n - refactored to lines max 84 chars long.\n\n Steffen\n\n---- snip ---\n\nCRLF conversion bears a slight chance of corrupting data.\nautocrlf=true will convert CRLF to LF during commit and LF to\nCRLF during checkout. A file that contains a mixture of LF and\nCRLF before the commit cannot be recreated by git. For text\nfiles this does not really matter because we do not care about\nthe line endings anyway; but for binary files that are\naccidentally classified as text the conversion can corrupt data.\n\nIf you recognize such corruption early you can easily fix it by\nsetting the conversion type explicitly in .gitattributes. Right\nafter committing you still have the original file in your work\ntree and this file is not yet corrupted. You can explicitly tell\ngit that this file is binary and git will handle the file\nappropriately.\n\nIn mixed Windows/Unix environments text files quite easily can\nend up containing a mixture of CRLF and LF line endings and git\nshould handle such situations gracefully. For example, a user\ncould copy a CRLF file from Windows to Unix and mix it with an\nexisting LF file there. The result would contain both types of\nline endings.\n\nUnfortunately, the desired effect of cleaning up text files with\nmixed line endings and the undesired effect of corrupting binary\nfiles cannot be distinguished. In both cases CRLF endings are\nremoved in an irreversible way. For text files this is the right\nthing to do, while for binary file it corrupts data.\n\nIn a sane environment, committing and checking out the same file\nshould not modify the original file in the work tree. For\nautocrlf=input the original file must not contain CRLF. For\nautocrlf=true the original file must not contain LF without\npreceding CR. Otherwise the conversion is irreversible. Note,\ngit might be able to recreate the original file with different\nautocrlf settings, but in the current environment checking out\nwill yield a file that differs from the file before the commit.\n\nThis patch adds a mechanism that can either warn the user about\nan irreversible conversion or can even refuse to convert. The\nmechanism is controlled by the variable core.safecrlf, with the\nfollowing values\n - false: disable safecrlf mechanism\n - warn: warn about irreversible conversions\n - true: refuse irreversible conversions\n\nThe default is to warn. Users are only affected by this default\nif core.autocrlf is set. But the current default of git is to\nleave core.autocrlf unset, so users will not see warnings unless\nthey deliberately chose to activate the autocrlf mechanism.\n\nThe safecrlf mechanism's details depend on the git command. If\nsafecrlf is active (not false), files in the work tree must not\nbe modified in an irreversible way without giving the user a\nchance to backup the original file. However, for read-only\noperations that do not modify files in the work tree git should\nnot print annoying warnings.\n\ngit apply behave as requested by by safecrlf. Even though\ngit apply writes changes directly back to the work tree, the\nuser still can get the original files back by checking out from\nthe index or HEAD. git apply should only be run on a clean\nwork tree.\n\ngit diff, git add, and any other command that calls\nindex_fd(..., write_object=1) also behave as requested by\nsafecrlf. The user still has the chance to save her data before\ncheckout or applying the diff.\n\ngit blame will never warn nor fail because it never writes to the\nwork tree.\n\nThe concept of a safety check was originally proposed in a similar\nway by Linus Torvalds. Thanks to Dimitry Potapov for insisting\non getting the naked LF/autocrlf=true case right.\n\nSigned-off-by: Steffen Prohaska \n---\n Documentation/config.txt | 50 +++++++++++++++++++++++++++++++++\n Documentation/gitattributes.txt | 5 +++\n builtin-apply.c | 2 +-\n builtin-blame.c | 2 +-\n cache.h | 11 +++++++-\n config.c | 9 ++++++\n convert.c | 45 ++++++++++++++++++++++++++----\n diff.c | 2 +-\n environment.c | 1 +\n sha1_file.c | 3 +-\n t/t0020-crlf.sh | 58 +++++++++++++++++++++++++++++++++++++++\n 11 files changed, 177 insertions(+), 11 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 4e222f1..7ebee51 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -139,6 +139,56 @@ core.autocrlf::\n \t\"text\" (i.e. be subjected to the autocrlf mechanism) is\n \tdecided purely based on the contents.\n \n+core.safecrlf::\n+\tIf true, makes git check if converting `CRLF` as controlled by\n+\t`core.autocrlf` is reversible. Git will verify if a command\n+\tmodifies a file in the work tree either directly or indirectly.\n+\tFor example, committing a file followed by checking out the\n+\tsame file should yield the original file in the work tree. If\n+\tthis is not the case for the current setting of\n+\t`core.autocrlf`, git will reject the file. The variable can\n+\tbe set to \"warn\", in which case git will only warn about an\n+\tirreversible conversion but continue the operation.\n++\n+CRLF conversion bears a slight chance of corrupting data.\n+autocrlf=true will convert CRLF to LF during commit and LF to\n+CRLF during checkout. A file that contains a mixture of LF and\n+CRLF before the commit cannot be recreated by git. For text\n+files this does not really matter because we do not care about\n+the line endings anyway; but for binary files that are\n+accidentally classified as text the conversion can corrupt data.\n++\n+If you recognize such corruption early you can easily fix it by\n+setting the conversion type explicitly in .gitattributes. Right\n+after committing you still have the original file in your work\n+tree and this file is not yet corrupted. You can explicitly tell\n+git that this file is binary and git will handle the file\n+appropriately.\n++\n+In mixed Windows/Unix environments text files quite easily can\n+end up containing a mixture of CRLF and LF line endings and git\n+should handle such situations gracefully. For example, a user\n+could copy a CRLF file from Windows to Unix and mix it with an\n+existing LF file there. The result would contain both types of\n+line endings.\n++\n+Unfortunately, the desired effect of cleaning up text files with\n+mixed line endings and the undesired effect of corrupting binary\n+files cannot be distinguished. In both cases CRLF endings are\n+removed in an irreversible way. For text files this is the right\n+thing to do, while for binary file it corrupts data.\n++\n+Note, this safety check does not mean that a checkout will generate a\n+file identical to the original file for a different setting of\n+`core.autocrlf`, but only for the current one. For example, a text\n+file with `LF` would be accepted with `core.autocrlf=input` and could\n+later be checked out with `core.autocrlf=true`, in which case the\n+resulting file would contain `CRLF`, although the original file\n+contained `LF`. However, in both work trees the line endings would be\n+consistent, that is either all `LF` or all `CRLF`, but never mixed. A\n+file with mixed line endings would be reported by the `core.safecrlf`\n+mechanism.\n+\n core.symlinks::\n \tIf false, symbolic links are checked out as small plain files that\n \tcontain the link text. linkgit:git-update-index[1] and\ndiff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt\nindex 35a29fd..2ffe1fb 100644\n--- a/Documentation/gitattributes.txt\n+++ b/Documentation/gitattributes.txt\n@@ -133,6 +133,11 @@ When `core.autocrlf` is set to \"input\", line endings are\n converted to LF upon checkin, but there is no conversion done\n upon checkout.\n \n+If `core.safecrlf` is set to \"true\" or \"warn\", git verifies if\n+the conversion is reversible for the current setting of\n+`core.autocrlf`. For \"true\", git rejects irreversible\n+conversions; for \"warn\", git only prints a warning but accepts\n+an irreversible conversion.\n \n `ident`\n ^^^^^^^\ndiff --git a/builtin-apply.c b/builtin-apply.c\nindex 15432b6..d0b3586 100644\n--- a/builtin-apply.c\n+++ b/builtin-apply.c\n@@ -1430,7 +1430,7 @@ static int read_old_data(struct stat *st, const char *path, struct strbuf *buf)\n \tcase S_IFREG:\n \t\tif (strbuf_read_file(buf, path, st->st_size) != st->st_size)\n \t\t\treturn error(\"unable to open or read %s\", path);\n-\t\tconvert_to_git(path, buf->buf, buf->len, buf);\n+\t\tconvert_to_git(path, buf->buf, buf->len, buf, safe_crlf);\n \t\treturn 0;\n \tdefault:\n \t\treturn -1;\ndiff --git a/builtin-blame.c b/builtin-blame.c\nindex 9b4c02e..c361ee1 100644\n--- a/builtin-blame.c\n+++ b/builtin-blame.c\n@@ -2073,7 +2073,7 @@ static struct commit *fake_working_tree_commit(const char *path, const char *con\n \t\tif (strbuf_read(&buf, 0, 0) < 0)\n \t\t\tdie(\"read error %s from stdin\", strerror(errno));\n \t}\n-\tconvert_to_git(path, buf.buf, buf.len, &buf);\n+\tconvert_to_git(path, buf.buf, buf.len, &buf, 0);\n \torigin->file.ptr = buf.buf;\n \torigin->file.size = buf.len;\n \tpretend_sha1_file(buf.buf, buf.len, OBJ_BLOB, origin->blob_sha1);\ndiff --git a/cache.h b/cache.h\nindex 549f4bb..6003c83 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -330,6 +330,14 @@ extern size_t packed_git_limit;\n extern size_t delta_base_cache_limit;\n extern int auto_crlf;\n \n+enum safe_crlf {\n+\tSAFE_CRLF_FALSE = 0,\n+\tSAFE_CRLF_FAIL = 1,\n+\tSAFE_CRLF_WARN = 2,\n+};\n+\n+extern enum safe_crlf safe_crlf;\n+\n #define GIT_REPO_VERSION 0\n extern int repository_format_version;\n extern int check_repository_format(void);\n@@ -633,7 +641,8 @@ extern void trace_argv_printf(const char **argv, const char *format, ...);\n \n /* convert.c */\n /* returns 1 if *dst was used */\n-extern int convert_to_git(const char *path, const char *src, size_t len, struct strbuf *dst);\n+extern int convert_to_git(const char *path, const char *src, size_t len,\n+ struct strbuf *dst, enum safe_crlf checksafe);\n extern int convert_to_working_tree(const char *path, const char *src, size_t len, struct strbuf *dst);\n \n /* add */\ndiff --git a/config.c b/config.c\nindex 526a3f4..5fe7cce 100644\n--- a/config.c\n+++ b/config.c\n@@ -407,6 +407,15 @@ int git_default_config(const char *var, const char *value)\n \t\treturn 0;\n \t}\n \n+\tif (!strcmp(var, \"core.safecrlf\")) {\n+\t\tif (value && !strcasecmp(value, \"warn\")) {\n+\t\t\tsafe_crlf = SAFE_CRLF_WARN;\n+\t\t\treturn 0;\n+\t\t}\n+\t\tsafe_crlf = git_config_bool(var, value);\n+\t\treturn 0;\n+\t}\n+\n \tif (!strcmp(var, \"user.name\")) {\n \t\tstrlcpy(git_default_name, value, sizeof(git_default_name));\n \t\treturn 0;\ndiff --git a/convert.c b/convert.c\nindex 80f114b..94c70f4 100644\n--- a/convert.c\n+++ b/convert.c\n@@ -85,8 +85,36 @@ static int is_binary(unsigned long size, struct text_stat *stats)\n \treturn 0;\n }\n \n+static void check_safe_crlf(const char* path, int action,\n+ struct text_stat* stats, enum safe_crlf checksafe)\n+{\n+\tif (action == CRLF_INPUT || auto_crlf <= 0) {\n+\t\t/*\n+\t\t * CRLFs would not be restored by checkout:\n+\t\t * check if we'd remove CRLFs\n+\t\t */\n+\t\tif (stats->crlf) {\n+\t\t\tif (checksafe == SAFE_CRLF_WARN)\n+\t\t\t\twarning(\"CRLF will be replaced by LF in %s.\", path);\n+\t\t\telse\n+\t\t\t\tdie(\"CRLF would be replaced by LF in %s.\", path);\n+\t\t}\n+\t} else if (auto_crlf > 0) {\n+\t\t/*\n+\t\t * CRLFs would be added by checkout:\n+\t\t * check if we have \"naked\" LFs\n+\t\t */\n+\t\tif (stats->lf != stats->crlf) {\n+\t\t\tif (checksafe == SAFE_CRLF_WARN)\n+\t\t\t\twarning(\"LF will be replaced by CRLF in %s\", path);\n+\t\t\telse\n+\t\t\t\tdie(\"LF would be replaced by CRLF in %s\", path);\n+\t\t}\n+\t}\n+}\n+\n static int crlf_to_git(const char *path, const char *src, size_t len,\n- struct strbuf *buf, int action)\n+ struct strbuf *buf, int action, enum safe_crlf checksafe)\n {\n \tstruct text_stat stats;\n \tchar *dst;\n@@ -95,9 +123,6 @@ static int crlf_to_git(const char *path, const char *src, size_t len,\n \t\treturn 0;\n \n \tgather_stats(src, len, &stats);\n-\t/* No CR? Nothing to convert, regardless. */\n-\tif (!stats.cr)\n-\t\treturn 0;\n \n \tif (action == CRLF_GUESS) {\n \t\t/*\n@@ -115,6 +140,13 @@ static int crlf_to_git(const char *path, const char *src, size_t len,\n \t\t\treturn 0;\n \t}\n \n+\tif (checksafe)\n+\t\tcheck_safe_crlf(path, action, &stats, checksafe);\n+\n+\t/* Optimization: No CR? Nothing to convert, regardless. */\n+\tif (!stats.cr)\n+\t\treturn 0;\n+\n \t/* only grow if not in place */\n \tif (strbuf_avail(buf) + buf->len < len)\n \t\tstrbuf_grow(buf, len - buf->len);\n@@ -536,7 +568,8 @@ static int git_path_check_ident(const char *path, struct git_attr_check *check)\n \treturn !!ATTR_TRUE(value);\n }\n \n-int convert_to_git(const char *path, const char *src, size_t len, struct strbuf *dst)\n+int convert_to_git(const char *path, const char *src, size_t len,\n+ struct strbuf *dst, enum safe_crlf checksafe)\n {\n \tstruct git_attr_check check[3];\n \tint crlf = CRLF_GUESS;\n@@ -558,7 +591,7 @@ int convert_to_git(const char *path, const char *src, size_t len, struct strbuf\n \t\tsrc = dst->buf;\n \t\tlen = dst->len;\n \t}\n-\tret |= crlf_to_git(path, src, len, dst, crlf);\n+\tret |= crlf_to_git(path, src, len, dst, crlf, checksafe);\n \tif (ret) {\n \t\tsrc = dst->buf;\n \t\tlen = dst->len;\ndiff --git a/diff.c b/diff.c\nindex 5b8afdc..562c20e 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -1624,7 +1624,7 @@ int diff_populate_filespec(struct diff_filespec *s, int size_only)\n \t\t * Convert from working tree format to canonical git format\n \t\t */\n \t\tstrbuf_init(&buf, 0);\n-\t\tif (convert_to_git(s->path, s->data, s->size, &buf)) {\n+\t\tif (convert_to_git(s->path, s->data, s->size, &buf, safe_crlf)) {\n \t\t\tsize_t size = 0;\n \t\t\tmunmap(s->data, s->size);\n \t\t\ts->should_munmap = 0;\ndiff --git a/environment.c b/environment.c\nindex 18a1c4e..e351e99 100644\n--- a/environment.c\n+++ b/environment.c\n@@ -35,6 +35,7 @@ int pager_use_color = 1;\n char *editor_program;\n char *excludes_file;\n int auto_crlf = 0;\t/* 1: both ways, -1: only when adding git objects */\n+enum safe_crlf safe_crlf = SAFE_CRLF_WARN;\n unsigned whitespace_rule_cfg = WS_DEFAULT_RULE;\n \n /* This is set by setup_git_dir_gently() and/or git_default_config() */\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 66a4e00..4179949 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -2358,7 +2358,8 @@ int index_fd(unsigned char *sha1, int fd, struct stat *st, int write_object,\n \tif ((type == OBJ_BLOB) && S_ISREG(st->st_mode)) {\n \t\tstruct strbuf nbuf;\n \t\tstrbuf_init(&nbuf, 0);\n-\t\tif (convert_to_git(path, buf, size, &nbuf)) {\n+\t\tif (convert_to_git(path, buf, size, &nbuf,\n+\t\t write_object ? safe_crlf : 0)) {\n \t\t\tmunmap(buf, size);\n \t\t\tbuf = strbuf_detach(&nbuf, &size);\n \t\t\tre_allocated = 1;\ndiff --git a/t/t0020-crlf.sh b/t/t0020-crlf.sh\nindex 8b27aa8..90ea081 100755\n--- a/t/t0020-crlf.sh\n+++ b/t/t0020-crlf.sh\n@@ -8,6 +8,10 @@ q_to_nul () {\n \ttr Q '\\000'\n }\n \n+q_to_cr () {\n+\ttr Q '\\015'\n+}\n+\n append_cr () {\n \tsed -e 's/$/Q/' | tr Q '\\015'\n }\n@@ -42,6 +46,60 @@ test_expect_success setup '\n \techo happy.\n '\n \n+test_expect_success 'safecrlf: autocrlf=input, all CRLF' '\n+\n+\tgit config core.autocrlf input &&\n+\tgit config core.safecrlf true &&\n+\n+\tfor w in I am all CRLF; do echo $w; done | append_cr >allcrlf &&\n+\t! git add allcrlf\n+'\n+\n+test_expect_success 'safecrlf: autocrlf=input, mixed LF/CRLF' '\n+\n+\tgit config core.autocrlf input &&\n+\tgit config core.safecrlf true &&\n+\n+\tfor w in Oh here is CRLFQ in text; do echo $w; done | q_to_cr >mixed &&\n+\t! git add mixed\n+'\n+\n+test_expect_success 'safecrlf: autocrlf=true, all LF' '\n+\n+\tgit config core.autocrlf true &&\n+\tgit config core.safecrlf true &&\n+\n+\tfor w in I am all LF; do echo $w; done >alllf &&\n+\t! git add alllf\n+'\n+\n+test_expect_success 'safecrlf: autocrlf=true mixed LF/CRLF' '\n+\n+\tgit config core.autocrlf true &&\n+\tgit config core.safecrlf true &&\n+\n+\tfor w in Oh here is CRLFQ in text; do echo $w; done | q_to_cr >mixed &&\n+\t! git add mixed\n+'\n+\n+test_expect_success 'safecrlf: print warning only once' '\n+\n+\tgit config core.autocrlf input &&\n+\tgit config core.safecrlf warn &&\n+\n+\tfor w in I am all LF; do echo $w; done >doublewarn &&\n+\tgit add doublewarn &&\n+\tgit commit -m \"nowarn\" &&\n+\tfor w in Oh here is CRLFQ in text; do echo $w; done | q_to_cr >doublewarn &&\n+\ttest $(git add doublewarn 2>&1 | grep \"CRLF will be replaced by LF\" | wc -l) = 1\n+'\n+\n+test_expect_success 'switch off autocrlf, safecrlf, reset HEAD' '\n+\tgit config core.autocrlf false &&\n+\tgit config core.safecrlf false &&\n+\tgit reset --hard HEAD^\n+'\n+\n test_expect_success 'update with autocrlf=input' '\n \n \trm -f tmp one dir/two three &&\n-- \n1.5.4.24.g6d45\n"},{"id":"67369","messageId":"alpine.LSU.1.00.0802041457190.7372@racer.site","threadId":"11841","inReplyTo":"86F2B863-6FE8-407E-B535-3F49782DA5F9@zib.de","subject":"Re: [PATCH v7] safecrlf: Add mechanism to warn about irreversible crlf conversions","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-02-04T15:01:52Z","receivedAt":"2008-02-04T15:01:52Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Mon, 4 Feb 2008, Steffen Prohaska wrote:\n\n> On Feb 3, 2008, at 11:29 PM, Johannes Schindelin wrote:\n> \n> > However, safe_crlf != SAFE_CRLF_FALSE does not affect people who did \n> > not set core.crlf = input or core.crlf = true. And for those who set \n> > core.crlf, the default makes sense, absolutely.\n> \n> I add a comment to the commit message.\n> \n> However, I don't fully agree with your comment. If your Unix \n> environment is as sane as you assume and you never exchange any data \n> with the \"cursed\" people, you can safely set core.autocrlf=input and \n> core.safecrlf=warn and still should never see any warning. Well you'd \n> spent some CPU cycles on verifying that your assumptions hold.\n\nIt is not only about spend CPU cycles. It is about content-tracking. \nPersonally, I have no single repository which munges data when putting it \ninto the index. That is what the crlf handling is: munging. I want the \ndata verbatim, and if I decide to check in a file with carriage return \nbefore line feed, then so be it.\n\nOf course, the code paths in your patch would be less exercised, and \nbugs/interactions could be conveniently hidden for all those who decide \nnot to activate safecrlf like me, but I do not see why Linux/Unix people \nshould be punished for a shortcoming that affects only projects which \npartly work on Windows, and then only if non-POSIX tools are used.\n\nCiao,\nDscho\n"},{"id":"67370","messageId":"alpine.LSU.1.00.0802041502290.7372@racer.site","threadId":"11841","inReplyTo":"12021001351967-git-send-email-prohaska@zib.de","subject":"Re: [PATCH] [v8] safecrlf: Add mechanism to warn about irreversible crlf conversions","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-02-04T15:02:55Z","receivedAt":"2008-02-04T15:02:55Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Mon, 4 Feb 2008, Steffen Prohaska wrote:\n\n> - refactored to lines max 84 chars long.\n\nWhy 84? I think the standard is still 80.\n\nCiao,\nDscho\n"},{"id":"67381","messageId":"459F2F36-7110-42A0-BDC1-FED70BC499BD@zib.de","threadId":"11841","inReplyTo":"alpine.LSU.1.00.0802041502290.7372@racer.site","subject":"Re: [PATCH] [v8] safecrlf: Add mechanism to warn about irreversible crlf conversions","fromName":"Steffen Prohaska","fromEmail":"prohaska@zib.de","sentAt":"2008-02-04T16:43:52Z","receivedAt":"2008-02-04T16:43:52Z","isPatch":true,"sender":{"key":"prohaska@zib.de","avatar":"https://avatars.githubusercontent.com/u/217580?v=4"},"body":"\nOn Feb 4, 2008, at 4:02 PM, Johannes Schindelin wrote:\n\n> Hi,\n>\n> On Mon, 4 Feb 2008, Steffen Prohaska wrote:\n>\n>> - refactored to lines max 84 chars long.\n>\n> Why 84? I think the standard is still 80.\n\nBecause the surrounding code already had lines with\nmore than 80 characters and the most natural refactoring\nwithout too much wrapping effort resulted in 84.\n\nIs 80 a strict limit?\n\n\tSteffen\n"},{"id":"67387","messageId":"alpine.LSU.1.00.0802041722260.7372@racer.site","threadId":"11841","inReplyTo":"459F2F36-7110-42A0-BDC1-FED70BC499BD@zib.de","subject":"Re: [PATCH] [v8] safecrlf: Add mechanism to warn about irreversible crlf conversions","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-02-04T17:27:57Z","receivedAt":"2008-02-04T17:27:57Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Mon, 4 Feb 2008, Steffen Prohaska wrote:\n\n> On Feb 4, 2008, at 4:02 PM, Johannes Schindelin wrote:\n> \n> > On Mon, 4 Feb 2008, Steffen Prohaska wrote:\n> > \n> > > - refactored to lines max 84 chars long.\n> > \n> > Why 84? I think the standard is still 80.\n> \n> Because the surrounding code already had lines with more than 80 \n> characters and the most natural refactoring without too much wrapping \n> effort resulted in 84.\n> \n> Is 80 a strict limit?\n\nIt is not a _strict_ limit. But it is a limit [*1*]. 84 is not.\n\nCiao,\nDscho\n\n*1*: Linux' CodingStyle says/said this:\n\nStatements longer than 80 columns will be broken into sensible chunks. \nDescendants are always substantially shorter than the parent and are \nplaced substantially to the right. The same applies to function headers \nwith a long argument list. Long strings are as well broken into shorter \nstrings. The only exception to this is where exceeding 80 columns \nsignificantly increases readability and does not hide information.\n"},{"id":"67627","messageId":"7vbq6u32mq.fsf@gitster.siamese.dyndns.org","threadId":"11841","inReplyTo":"12021001351967-git-send-email-prohaska@zib.de","subject":"Re: [PATCH] [v8] safecrlf: Add mechanism to warn about irreversible crlf conversions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-02-06T08:33:49Z","receivedAt":"2008-02-06T08:33:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Steffen Prohaska writes:\n\n> This repaces v7 of the patch and includes the following changes\n> - Commit message defends safecrlf=warn default.\n> - refactored to lines max 84 chars long.\n>\n> Steffen\n>\n> ---- snip ---\n>\n> CRLF conversion bears a slight chance of corrupting data.\n> ...\n\nIs this meant for application?\n\nIf so please do not do \"^---- snip ---\". More than three dashes\nat the beginning of line discards everything after that from the\ncommit log message so \"git am -i\" becomes very cumbersome. And\nthat is the reason you often see \"-- >8 --\" scissors mark from\nme (notice two dashes, not three).\n\n> autocrlf=true will convert CRLF to LF during commit and LF to\n> CRLF during checkout. A file that contains a mixture of LF and\n> CRLF before the commit cannot be recreated by git. For text\n> files this does not really matter because we do not care about\n> the line endings anyway; ...\n\nThat is a wrong explanation. It is not that we do not _care_.\n\nFar from it. With autocrlf, we _deeply_ care about the line\nendings of text files to be canonical with LF in the repository\ndata, and canonical with platform convention in the work tree\ndata. A file with mixed LF and CRLF have _wrong_ line endings\nin the work tree data, hence it actively gets _corrected_.\n\nSo it is not _anyway_. It is what we _want to happen_.\n\n> ... but for binary files that are\n> accidentally classified as text the conversion can corrupt data.\n\nCorrect.\n\n> In mixed Windows/Unix environments text files quite easily can\n> end up containing a mixture of CRLF and LF line endings and git\n> should handle such situations gracefully. For example, a user\n> could copy a CRLF file from Windows to Unix and mix it with an\n> existing LF file there. The result would contain both types of\n> line endings.\n\nCorrect, but I do not think this is necessary for this commit,\nas you have established already that canonicalization is what we\nwant for text and what we want to avoid for binary. Just skip\nthese 6 lines and the thought will still flow naturally.\n\n> Unfortunately, the desired effect of cleaning up text files with\n> mixed line endings and the undesired effect of corrupting binary\n> files cannot be distinguished. In both cases CRLF endings are\n> removed in an irreversible way. For text files this is the right\n> thing to do, while for binary file it corrupts data.\n\n\"CRLF\" are not even \"endings\" in a binary file. I'd suggest\nrewriting the above paragraph as \"... distinguished. In both\ncases, CRLF pairs are removed in an irreversible way.\" The rest\nof the paragraph is redundant --- you've said that twice already\n(and I suggested to remove the second one earlier).\n\n> In a sane environment, committing and checking out the same file\n> should not modify the original file in the work tree.\n\nIs \"In a sane environment, \" needed? In what environment is it\npermissible that checkout followed by commit gives a different\nresult?\n\n> ... For\n> autocrlf=input the original file must not contain CRLF. For\n> autocrlf=true the original file must not contain LF without\n> preceding CR. Otherwise the conversion is irreversible. Note,\n> git might be able to recreate the original file with different\n> autocrlf settings, but in the current environment checking out\n> will yield a file that differs from the file before the commit.\n\nI suspect this also may be redundant. You've already explained\nthat existing autocrlf conversion are lossy with respect to\nmixed LF and CRLF. I think it is easier to read if we drop\neverything from \"In a sane environment, \" up to here.\n\n> This patch adds a mechanism that can either warn the user about\n> an irreversible conversion or can even refuse to convert. The\n> mechanism is controlled by the variable core.safecrlf, with the\n> following values\n> - false: disable safecrlf mechanism\n> - warn: warn about irreversible conversions\n> - true: refuse irreversible conversions\n\nPlease end the line \"following values\" with a colon and have a\nblan line after that.\n\n> The default is to warn. Users are only affected by this default\n> if core.autocrlf is set. But the current default of git is to\n> leave core.autocrlf unset, so users will not see warnings unless\n> they deliberately chose to activate the autocrlf mechanism.\n\nVery sensible.\n\n> The safecrlf mechanism's details depend on the git command. If\n> safecrlf is active (not false), files in the work tree must not\n> be modified in an irreversible way without giving the user a\n> chance to backup the original file. However, for read-only\n> operations that do not modify files in the work tree git should\n> not print annoying warnings.\n>\n> git apply behave as requested by by safecrlf. Even though\n> git apply writes changes directly back to the work tree, the\n> user still can get the original files back by checking out from\n> the index or HEAD. git apply should only be run on a clean\n> work tree.\n\nI think the conclusion \"should\" in the last sentence is wrong.\nRunning \"apply\" to get effects from more than one patches is a\nnorm, and you are telling the readers they are doing a wrong\nthing if the did so.\n\nWith \"apply\", we are talking about \"patches\", which means we are\ntalking about \"text\", which by definition is outside of what\n\"safecrlf\" cares about. That is why apply does not need to be\nmolested.\n\n\tSide note. Maybe when we apply a --binary patch, we\n\tshould read the target image from the work tree without\n\t\"convert-to-git\" regardless of what the autocrlf nor the\n\tgitattributes say. I did not check if we already do\n\tthat, though.\n\nThe rest of the commit log message looked fine.\n\n> diff --git a/Documentation/config.txt b/Documentation/config.txt\n> index 4e222f1..7ebee51 100644\n> --- a/Documentation/config.txt\n> +++ b/Documentation/config.txt\n> @@ -139,6 +139,56 @@ core.autocrlf::\n> \t\"text\" (i.e. be subjected to the autocrlf mechanism) is\n> \tdecided purely based on the contents.\n> \n> +core.safecrlf::\n> +\tIf true, makes git check if converting `CRLF` as controlled by\n> +\t`core.autocrlf` is reversible. Git will verify if a command\n> +\tmodifies a file in the work tree either directly or indirectly.\n> +\tFor example, committing a file followed by checking out the\n> +\tsame file should yield the original file in the work tree. If\n> +\tthis is not the case for the current setting of\n> +\t`core.autocrlf`, git will reject the file. The variable can\n> +\tbe set to \"warn\", in which case git will only warn about an\n> +\tirreversible conversion but continue the operation.\n> ++\n> +CRLF conversion bears a slight chance of corrupting data.\n> +...\n> +mechanism.\n\nThe same comment about redundancy applies here.\n\n> diff --git a/convert.c b/convert.c\n> index 80f114b..94c70f4 100644\n> --- a/convert.c\n> +++ b/convert.c\n> @@ -85,8 +85,36 @@ static int is_binary(unsigned long size, struct text_stat *stats)\n> \treturn 0;\n> }\n> \n> +static void check_safe_crlf(const char* path, int action,\n> + struct text_stat* stats, enum safe_crlf checksafe)\n\nAsterisk comes near the variable name not type: \"const char *path\".\n\n> +{\n> +\tif (action == CRLF_INPUT || auto_crlf <= 0) {\n> +\t\t/*\n> +\t\t * CRLFs would not be restored by checkout:\n> +\t\t * check if we'd remove CRLFs\n> +\t\t */\n> +\t\tif (stats->crlf) {\n> +\t\t\tif (checksafe == SAFE_CRLF_WARN)\n> +\t\t\t\twarning(\"CRLF will be replaced by LF in %s.\", path);\n> +\t\t\telse\n> +\t\t\t\tdie(\"CRLF would be replaced by LF in %s.\", path);\n\nWhat happens when checksafe == SAFE_CRLF_FALSE, I had to wonder,\nuntil looking at the caller.\n\n\t\t\telse /* i.e. SAFE_CRLF_FAIL */\n\nwould have been easier to follow.\n\n> diff --git a/sha1_file.c b/sha1_file.c\n> index 66a4e00..4179949 100644\n> --- a/sha1_file.c\n> +++ b/sha1_file.c\n> @@ -2358,7 +2358,8 @@ int index_fd(unsigned char *sha1, int fd, struct stat *st, int write_object,\n> \tif ((type == OBJ_BLOB) && S_ISREG(st->st_mode)) {\n> \t\tstruct strbuf nbuf;\n> \t\tstrbuf_init(&nbuf, 0);\n> -\t\tif (convert_to_git(path, buf, size, &nbuf)) {\n> +\t\tif (convert_to_git(path, buf, size, &nbuf,\n> +\t\t write_object ? safe_crlf : 0)) {\n> \t\t\tmunmap(buf, size);\n> \t\t\tbuf = strbuf_detach(&nbuf, &size);\n> \t\t\tre_allocated = 1;\n\nVery nicely done ;-).\n"},{"id":"67657","messageId":"885376AD-510D-4E8E-8DCF-87073AC0F88F@zib.de","threadId":"11841","inReplyTo":"7vbq6u32mq.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] [v8] safecrlf: Add mechanism to warn about irreversible crlf conversions","fromName":"Steffen Prohaska","fromEmail":"prohaska@zib.de","sentAt":"2008-02-06T11:23:42Z","receivedAt":"2008-02-06T11:23:42Z","isPatch":true,"sender":{"key":"prohaska@zib.de","avatar":"https://avatars.githubusercontent.com/u/217580?v=4"},"body":"\nOn Feb 6, 2008, at 9:33 AM, Junio C Hamano wrote:\n\n> Steffen Prohaska writes:\n>\n>> This repaces v7 of the patch and includes the following changes\n>> - Commit message defends safecrlf=warn default.\n>> - refactored to lines max 84 chars long.\n>>\n>> Steffen\n>>\n>> ---- snip ---\n>>\n>> CRLF conversion bears a slight chance of corrupting data.\n>> ...\n>\n> Is this meant for application?\n\nYes, this is the best I have and I personally test it in practice.\nIt works for me, although I recognized that I tend to miss warnings\nsometimes, for example during \"git commit -a\". The reason is that\nthe warnings are printed, but the editor for the commit message\npops up right afterwards. With \"git add\" this is not a problem.\n\nMore feedback by others would be helpful. So yes, the patch\nshould be applied.\n\nBut you have quite a few comments below; so I'll send an update.\n\n\n\n> If so please do not do \"^---- snip ---\". More than three dashes\n> at the beginning of line discards everything after that from the\n> commit log message so \"git am -i\" becomes very cumbersome. And\n> that is the reason you often see \"-- >8 --\" scissors mark from\n> me (notice two dashes, not three).\n\nAhh.. ok, I thought any number of dashes unequal 3 would work.\nI'll use your mark in the future.\n\n\n>\n>> autocrlf=true will convert CRLF to LF during commit and LF to\n>> CRLF during checkout. A file that contains a mixture of LF and\n>> CRLF before the commit cannot be recreated by git. For text\n>> files this does not really matter because we do not care about\n>> the line endings anyway; ...\n>\n> That is a wrong explanation. It is not that we do not _care_.\n>\n> Far from it. With autocrlf, we _deeply_ care about the line\n> endings of text files to be canonical with LF in the repository\n> data, and canonical with platform convention in the work tree\n> data. A file with mixed LF and CRLF have _wrong_ line endings\n> in the work tree data, hence it actively gets _corrected_.\n>\n> So it is not _anyway_. It is what we _want to happen_.\n\nHmm, yes, I agree with your reasoning. We do care and we only\nwant LF. Maybe I had Dscho's arguments in mind when I wrote\nthis paragraph. He often claims that git users on Unix do care\nabout every single bit of the files, which means including the\n\"wrong\" line endings. But I don't think this is true. I think\nwe don't mind that autocrlf=true modifies the line endings in\na way that they cannot be exactly restored. We don't mind\nbecause we do not care about the exact line endings. We only\nneed to know that the line ends, not if it ends in CRLF or LF.\n\n[ comments about commit message that I'll include in the update. ]\n\n\n>\n>> diff --git a/convert.c b/convert.c\n>> index 80f114b..94c70f4 100644\n>> --- a/convert.c\n>> +++ b/convert.c\n>> @@ -85,8 +85,36 @@ static int is_binary(unsigned long size, struct \n>> text_stat *stats)\n>> \treturn 0;\n>> }\n>>\n>> +static void check_safe_crlf(const char* path, int action,\n>> + struct text_stat* stats, enum \n>> safe_crlf checksafe)\n>\n> Asterisk comes near the variable name not type: \"const char *path\".\n\nI know that but I tend to forget because I follow a different rule\nall day ;) Will fix.\n\n\n>> +{\n>> +\tif (action == CRLF_INPUT || auto_crlf <= 0) {\n>> +\t\t/*\n>> +\t\t * CRLFs would not be restored by checkout:\n>> +\t\t * check if we'd remove CRLFs\n>> +\t\t */\n>> +\t\tif (stats->crlf) {\n>> +\t\t\tif (checksafe == SAFE_CRLF_WARN)\n>> +\t\t\t\twarning(\"CRLF will be replaced by LF in %s.\", path);\n>> +\t\t\telse\n>> +\t\t\t\tdie(\"CRLF would be replaced by LF in %s.\", path);\n>\n> What happens when checksafe == SAFE_CRLF_FALSE, I had to wonder,\n> until looking at the caller.\n>\n> \t\t\telse /* i.e. SAFE_CRLF_FAIL */\n>\n> would have been easier to follow.\n\nI'll add a comment and will move the \"if (!checksafe)\" from the \ncaller to\nthe beginning of check_safe_crlf().\n\n\n>> diff --git a/sha1_file.c b/sha1_file.c\n>> index 66a4e00..4179949 100644\n>> --- a/sha1_file.c\n>> +++ b/sha1_file.c\n>> @@ -2358,7 +2358,8 @@ int index_fd(unsigned char *sha1, int fd, \n>> struct stat *st, int write_object,\n>> \tif ((type == OBJ_BLOB) && S_ISREG(st->st_mode)) {\n>> \t\tstruct strbuf nbuf;\n>> \t\tstrbuf_init(&nbuf, 0);\n>> -\t\tif (convert_to_git(path, buf, size, &nbuf)) {\n>> +\t\tif (convert_to_git(path, buf, size, &nbuf,\n>> +\t\t write_object ? safe_crlf : 0)) {\n>> \t\t\tmunmap(buf, size);\n>> \t\t\tbuf = strbuf_detach(&nbuf, &size);\n>> \t\t\tre_allocated = 1;\n>\n> Very nicely done ;-).\n\nthanks.\n\n\tSteffen\n"},{"id":"67658","messageId":"12022971582185-git-send-email-prohaska@zib.de","threadId":"11841","inReplyTo":"7vbq6u32mq.fsf@gitster.siamese.dyndns.org","subject":"[PATCH] [v9] safecrlf: Add mechanism to warn about irreversible crlf conversions","fromName":"Steffen Prohaska","fromEmail":"prohaska@zib.de","sentAt":"2008-02-06T11:25:58Z","receivedAt":"2008-02-06T11:25:58Z","isPatch":true,"sender":{"key":"prohaska@zib.de","avatar":"https://avatars.githubusercontent.com/u/217580?v=4"},"body":"This repaces v9 of the patch and should be applied.\nIt contains modifications as suggested by Junio in\n<7vbq6u32mq.fsf@gitster.siamese.dyndns.org>\n\n Steffen\n\n-- >8 --\n\nCRLF conversion bears a slight chance of corrupting data.\nautocrlf=true will convert CRLF to LF during commit and LF to\nCRLF during checkout. A file that contains a mixture of LF and\nCRLF before the commit cannot be recreated by git. For text\nfiles this is the right thing to do: it corrects line endings\nsuch that we have only LF line endings in the repository.\nBut for binary files that are accidentally classified as text the\nconversion can corrupt data.\n\nIf you recognize such corruption early you can easily fix it by\nsetting the conversion type explicitly in .gitattributes. Right\nafter committing you still have the original file in your work\ntree and this file is not yet corrupted. You can explicitly tell\ngit that this file is binary and git will handle the file\nappropriately.\n\nUnfortunately, the desired effect of cleaning up text files with\nmixed line endings and the undesired effect of corrupting binary\nfiles cannot be distinguished. In both cases CRLFs are removed\nin an irreversible way. For text files this is the right thing\nto do because CRLFs are line endings, while for binary files\nconverting CRLFs corrupts data.\n\nThis patch adds a mechanism that can either warn the user about\nan irreversible conversion or can even refuse to convert. The\nmechanism is controlled by the variable core.safecrlf, with the\nfollowing values:\n\n - false: disable safecrlf mechanism\n - warn: warn about irreversible conversions\n - true: refuse irreversible conversions\n\nThe default is to warn. Users are only affected by this default\nif core.autocrlf is set. But the current default of git is to\nleave core.autocrlf unset, so users will not see warnings unless\nthey deliberately chose to activate the autocrlf mechanism.\n\nThe safecrlf mechanism's details depend on the git command. If\nsafecrlf is active (not false), files in the work tree must not\nbe modified in an irreversible way without giving the user a\nchance to backup the original file. However, for read-only\noperations that do not modify files in the work tree git should\nnot print annoying warnings.\n\ngit apply behaves as requested by by safecrlf. Even though git\napply writes changes directly back to the work tree, we assume\nthat the user still can get the original files back by checking\nout from the index or HEAD. git apply is about applying patches,\nwhich means we are talking about \"text\" and therefore it is\nsafe to write the results directly back to the work tree even\nif CRLFs were converted to LFs.\n\ngit diff, git add, and any other command that calls\nindex_fd(..., write_object=1) also behave as requested by\nsafecrlf. The user still has the chance to save her data before\ncheckout or applying the diff.\n\ngit blame will never warn nor fail because it never writes to the\nwork tree.\n\nThe concept of a safety check was originally proposed in a similar\nway by Linus Torvalds. Thanks to Dimitry Potapov for insisting\non getting the naked LF/autocrlf=true case right.\n\nSigned-off-by: Steffen Prohaska \n---\n Documentation/config.txt | 45 ++++++++++++++++++++++++++++++\n Documentation/gitattributes.txt | 5 +++\n builtin-apply.c | 2 +-\n builtin-blame.c | 2 +-\n cache.h | 11 +++++++-\n config.c | 9 ++++++\n convert.c | 47 +++++++++++++++++++++++++++----\n diff.c | 2 +-\n environment.c | 1 +\n sha1_file.c | 3 +-\n t/t0020-crlf.sh | 58 +++++++++++++++++++++++++++++++++++++++\n 11 files changed, 174 insertions(+), 11 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 4e222f1..7b8cae1 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -139,6 +139,51 @@ core.autocrlf::\n \t\"text\" (i.e. be subjected to the autocrlf mechanism) is\n \tdecided purely based on the contents.\n \n+core.safecrlf::\n+\tIf true, makes git check if converting `CRLF` as controlled by\n+\t`core.autocrlf` is reversible. Git will verify if a command\n+\tmodifies a file in the work tree either directly or indirectly.\n+\tFor example, committing a file followed by checking out the\n+\tsame file should yield the original file in the work tree. If\n+\tthis is not the case for the current setting of\n+\t`core.autocrlf`, git will reject the file. The variable can\n+\tbe set to \"warn\", in which case git will only warn about an\n+\tirreversible conversion but continue the operation.\n++\n+CRLF conversion bears a slight chance of corrupting data.\n+autocrlf=true will convert CRLF to LF during commit and LF to\n+CRLF during checkout. A file that contains a mixture of LF and\n+CRLF before the commit cannot be recreated by git. For text\n+files this is the right thing to do: it corrects line endings\n+such that we have only LF line endings in the repository.\n+But for binary files that are accidentally classified as text the\n+conversion can corrupt data.\n++\n+If you recognize such corruption early you can easily fix it by\n+setting the conversion type explicitly in .gitattributes. Right\n+after committing you still have the original file in your work\n+tree and this file is not yet corrupted. You can explicitly tell\n+git that this file is binary and git will handle the file\n+appropriately.\n++\n+Unfortunately, the desired effect of cleaning up text files with\n+mixed line endings and the undesired effect of corrupting binary\n+files cannot be distinguished. In both cases CRLFs are removed\n+in an irreversible way. For text files this is the right thing\n+to do because CRLFs are line endings, while for binary files\n+converting CRLFs corrupts data.\n++\n+Note, this safety check does not mean that a checkout will generate a\n+file identical to the original file for a different setting of\n+`core.autocrlf`, but only for the current one. For example, a text\n+file with `LF` would be accepted with `core.autocrlf=input` and could\n+later be checked out with `core.autocrlf=true`, in which case the\n+resulting file would contain `CRLF`, although the original file\n+contained `LF`. However, in both work trees the line endings would be\n+consistent, that is either all `LF` or all `CRLF`, but never mixed. A\n+file with mixed line endings would be reported by the `core.safecrlf`\n+mechanism.\n+\n core.symlinks::\n \tIf false, symbolic links are checked out as small plain files that\n \tcontain the link text. linkgit:git-update-index[1] and\ndiff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt\nindex 35a29fd..2ffe1fb 100644\n--- a/Documentation/gitattributes.txt\n+++ b/Documentation/gitattributes.txt\n@@ -133,6 +133,11 @@ When `core.autocrlf` is set to \"input\", line endings are\n converted to LF upon checkin, but there is no conversion done\n upon checkout.\n \n+If `core.safecrlf` is set to \"true\" or \"warn\", git verifies if\n+the conversion is reversible for the current setting of\n+`core.autocrlf`. For \"true\", git rejects irreversible\n+conversions; for \"warn\", git only prints a warning but accepts\n+an irreversible conversion.\n \n `ident`\n ^^^^^^^\ndiff --git a/builtin-apply.c b/builtin-apply.c\nindex 15432b6..d0b3586 100644\n--- a/builtin-apply.c\n+++ b/builtin-apply.c\n@@ -1430,7 +1430,7 @@ static int read_old_data(struct stat *st, const char *path, struct strbuf *buf)\n \tcase S_IFREG:\n \t\tif (strbuf_read_file(buf, path, st->st_size) != st->st_size)\n \t\t\treturn error(\"unable to open or read %s\", path);\n-\t\tconvert_to_git(path, buf->buf, buf->len, buf);\n+\t\tconvert_to_git(path, buf->buf, buf->len, buf, safe_crlf);\n \t\treturn 0;\n \tdefault:\n \t\treturn -1;\ndiff --git a/builtin-blame.c b/builtin-blame.c\nindex 9b4c02e..c361ee1 100644\n--- a/builtin-blame.c\n+++ b/builtin-blame.c\n@@ -2073,7 +2073,7 @@ static struct commit *fake_working_tree_commit(const char *path, const char *con\n \t\tif (strbuf_read(&buf, 0, 0) < 0)\n \t\t\tdie(\"read error %s from stdin\", strerror(errno));\n \t}\n-\tconvert_to_git(path, buf.buf, buf.len, &buf);\n+\tconvert_to_git(path, buf.buf, buf.len, &buf, 0);\n \torigin->file.ptr = buf.buf;\n \torigin->file.size = buf.len;\n \tpretend_sha1_file(buf.buf, buf.len, OBJ_BLOB, origin->blob_sha1);\ndiff --git a/cache.h b/cache.h\nindex 549f4bb..6003c83 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -330,6 +330,14 @@ extern size_t packed_git_limit;\n extern size_t delta_base_cache_limit;\n extern int auto_crlf;\n \n+enum safe_crlf {\n+\tSAFE_CRLF_FALSE = 0,\n+\tSAFE_CRLF_FAIL = 1,\n+\tSAFE_CRLF_WARN = 2,\n+};\n+\n+extern enum safe_crlf safe_crlf;\n+\n #define GIT_REPO_VERSION 0\n extern int repository_format_version;\n extern int check_repository_format(void);\n@@ -633,7 +641,8 @@ extern void trace_argv_printf(const char **argv, const char *format, ...);\n \n /* convert.c */\n /* returns 1 if *dst was used */\n-extern int convert_to_git(const char *path, const char *src, size_t len, struct strbuf *dst);\n+extern int convert_to_git(const char *path, const char *src, size_t len,\n+ struct strbuf *dst, enum safe_crlf checksafe);\n extern int convert_to_working_tree(const char *path, const char *src, size_t len, struct strbuf *dst);\n \n /* add */\ndiff --git a/config.c b/config.c\nindex 498259e..3256c99 100644\n--- a/config.c\n+++ b/config.c\n@@ -407,6 +407,15 @@ int git_default_config(const char *var, const char *value)\n \t\treturn 0;\n \t}\n \n+\tif (!strcmp(var, \"core.safecrlf\")) {\n+\t\tif (value && !strcasecmp(value, \"warn\")) {\n+\t\t\tsafe_crlf = SAFE_CRLF_WARN;\n+\t\t\treturn 0;\n+\t\t}\n+\t\tsafe_crlf = git_config_bool(var, value);\n+\t\treturn 0;\n+\t}\n+\n \tif (!strcmp(var, \"user.name\")) {\n \t\tstrlcpy(git_default_name, value, sizeof(git_default_name));\n \t\treturn 0;\ndiff --git a/convert.c b/convert.c\nindex 80f114b..3bbf0c5 100644\n--- a/convert.c\n+++ b/convert.c\n@@ -85,8 +85,39 @@ static int is_binary(unsigned long size, struct text_stat *stats)\n \treturn 0;\n }\n \n+static void check_safe_crlf(const char *path, int action,\n+ struct text_stat *stats, enum safe_crlf checksafe)\n+{\n+\tif (!checksafe)\n+\t\treturn;\n+\n+\tif (action == CRLF_INPUT || auto_crlf <= 0) {\n+\t\t/*\n+\t\t * CRLFs would not be restored by checkout:\n+\t\t * check if we'd remove CRLFs\n+\t\t */\n+\t\tif (stats->crlf) {\n+\t\t\tif (checksafe == SAFE_CRLF_WARN)\n+\t\t\t\twarning(\"CRLF will be replaced by LF in %s.\", path);\n+\t\t\telse /* i.e. SAFE_CRLF_FAIL */\n+\t\t\t\tdie(\"CRLF would be replaced by LF in %s.\", path);\n+\t\t}\n+\t} else if (auto_crlf > 0) {\n+\t\t/*\n+\t\t * CRLFs would be added by checkout:\n+\t\t * check if we have \"naked\" LFs\n+\t\t */\n+\t\tif (stats->lf != stats->crlf) {\n+\t\t\tif (checksafe == SAFE_CRLF_WARN)\n+\t\t\t\twarning(\"LF will be replaced by CRLF in %s\", path);\n+\t\t\telse /* i.e. SAFE_CRLF_FAIL */\n+\t\t\t\tdie(\"LF would be replaced by CRLF in %s\", path);\n+\t\t}\n+\t}\n+}\n+\n static int crlf_to_git(const char *path, const char *src, size_t len,\n- struct strbuf *buf, int action)\n+ struct strbuf *buf, int action, enum safe_crlf checksafe)\n {\n \tstruct text_stat stats;\n \tchar *dst;\n@@ -95,9 +126,6 @@ static int crlf_to_git(const char *path, const char *src, size_t len,\n \t\treturn 0;\n \n \tgather_stats(src, len, &stats);\n-\t/* No CR? Nothing to convert, regardless. */\n-\tif (!stats.cr)\n-\t\treturn 0;\n \n \tif (action == CRLF_GUESS) {\n \t\t/*\n@@ -115,6 +143,12 @@ static int crlf_to_git(const char *path, const char *src, size_t len,\n \t\t\treturn 0;\n \t}\n \n+\tcheck_safe_crlf(path, action, &stats, checksafe);\n+\n+\t/* Optimization: No CR? Nothing to convert, regardless. */\n+\tif (!stats.cr)\n+\t\treturn 0;\n+\n \t/* only grow if not in place */\n \tif (strbuf_avail(buf) + buf->len < len)\n \t\tstrbuf_grow(buf, len - buf->len);\n@@ -536,7 +570,8 @@ static int git_path_check_ident(const char *path, struct git_attr_check *check)\n \treturn !!ATTR_TRUE(value);\n }\n \n-int convert_to_git(const char *path, const char *src, size_t len, struct strbuf *dst)\n+int convert_to_git(const char *path, const char *src, size_t len,\n+ struct strbuf *dst, enum safe_crlf checksafe)\n {\n \tstruct git_attr_check check[3];\n \tint crlf = CRLF_GUESS;\n@@ -558,7 +593,7 @@ int convert_to_git(const char *path, const char *src, size_t len, struct strbuf\n \t\tsrc = dst->buf;\n \t\tlen = dst->len;\n \t}\n-\tret |= crlf_to_git(path, src, len, dst, crlf);\n+\tret |= crlf_to_git(path, src, len, dst, crlf, checksafe);\n \tif (ret) {\n \t\tsrc = dst->buf;\n \t\tlen = dst->len;\ndiff --git a/diff.c b/diff.c\nindex 5b8afdc..562c20e 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -1624,7 +1624,7 @@ int diff_populate_filespec(struct diff_filespec *s, int size_only)\n \t\t * Convert from working tree format to canonical git format\n \t\t */\n \t\tstrbuf_init(&buf, 0);\n-\t\tif (convert_to_git(s->path, s->data, s->size, &buf)) {\n+\t\tif (convert_to_git(s->path, s->data, s->size, &buf, safe_crlf)) {\n \t\t\tsize_t size = 0;\n \t\t\tmunmap(s->data, s->size);\n \t\t\ts->should_munmap = 0;\ndiff --git a/environment.c b/environment.c\nindex 18a1c4e..e351e99 100644\n--- a/environment.c\n+++ b/environment.c\n@@ -35,6 +35,7 @@ int pager_use_color = 1;\n char *editor_program;\n char *excludes_file;\n int auto_crlf = 0;\t/* 1: both ways, -1: only when adding git objects */\n+enum safe_crlf safe_crlf = SAFE_CRLF_WARN;\n unsigned whitespace_rule_cfg = WS_DEFAULT_RULE;\n \n /* This is set by setup_git_dir_gently() and/or git_default_config() */\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 66a4e00..4179949 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -2358,7 +2358,8 @@ int index_fd(unsigned char *sha1, int fd, struct stat *st, int write_object,\n \tif ((type == OBJ_BLOB) && S_ISREG(st->st_mode)) {\n \t\tstruct strbuf nbuf;\n \t\tstrbuf_init(&nbuf, 0);\n-\t\tif (convert_to_git(path, buf, size, &nbuf)) {\n+\t\tif (convert_to_git(path, buf, size, &nbuf,\n+\t\t write_object ? safe_crlf : 0)) {\n \t\t\tmunmap(buf, size);\n \t\t\tbuf = strbuf_detach(&nbuf, &size);\n \t\t\tre_allocated = 1;\ndiff --git a/t/t0020-crlf.sh b/t/t0020-crlf.sh\nindex 8b27aa8..90ea081 100755\n--- a/t/t0020-crlf.sh\n+++ b/t/t0020-crlf.sh\n@@ -8,6 +8,10 @@ q_to_nul () {\n \ttr Q '\\000'\n }\n \n+q_to_cr () {\n+\ttr Q '\\015'\n+}\n+\n append_cr () {\n \tsed -e 's/$/Q/' | tr Q '\\015'\n }\n@@ -42,6 +46,60 @@ test_expect_success setup '\n \techo happy.\n '\n \n+test_expect_success 'safecrlf: autocrlf=input, all CRLF' '\n+\n+\tgit config core.autocrlf input &&\n+\tgit config core.safecrlf true &&\n+\n+\tfor w in I am all CRLF; do echo $w; done | append_cr >allcrlf &&\n+\t! git add allcrlf\n+'\n+\n+test_expect_success 'safecrlf: autocrlf=input, mixed LF/CRLF' '\n+\n+\tgit config core.autocrlf input &&\n+\tgit config core.safecrlf true &&\n+\n+\tfor w in Oh here is CRLFQ in text; do echo $w; done | q_to_cr >mixed &&\n+\t! git add mixed\n+'\n+\n+test_expect_success 'safecrlf: autocrlf=true, all LF' '\n+\n+\tgit config core.autocrlf true &&\n+\tgit config core.safecrlf true &&\n+\n+\tfor w in I am all LF; do echo $w; done >alllf &&\n+\t! git add alllf\n+'\n+\n+test_expect_success 'safecrlf: autocrlf=true mixed LF/CRLF' '\n+\n+\tgit config core.autocrlf true &&\n+\tgit config core.safecrlf true &&\n+\n+\tfor w in Oh here is CRLFQ in text; do echo $w; done | q_to_cr >mixed &&\n+\t! git add mixed\n+'\n+\n+test_expect_success 'safecrlf: print warning only once' '\n+\n+\tgit config core.autocrlf input &&\n+\tgit config core.safecrlf warn &&\n+\n+\tfor w in I am all LF; do echo $w; done >doublewarn &&\n+\tgit add doublewarn &&\n+\tgit commit -m \"nowarn\" &&\n+\tfor w in Oh here is CRLFQ in text; do echo $w; done | q_to_cr >doublewarn &&\n+\ttest $(git add doublewarn 2>&1 | grep \"CRLF will be replaced by LF\" | wc -l) = 1\n+'\n+\n+test_expect_success 'switch off autocrlf, safecrlf, reset HEAD' '\n+\tgit config core.autocrlf false &&\n+\tgit config core.safecrlf false &&\n+\tgit reset --hard HEAD^\n+'\n+\n test_expect_success 'update with autocrlf=input' '\n \n \trm -f tmp one dir/two three &&\n-- \n1.5.4.40.g4a680\n"},{"id":"67690","messageId":"7vk5lhyir6.fsf@gitster.siamese.dyndns.org","threadId":"11841","inReplyTo":"12022971582185-git-send-email-prohaska@zib.de","subject":"Re: [PATCH] [v9] safecrlf: Add mechanism to warn about irreversible crlf conversions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-02-06T19:42:05Z","receivedAt":"2008-02-06T19:42:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Steffen Prohaska writes:\n\n> This repaces v9 of the patch and should be applied.\n> It contains modifications as suggested by Junio in\n> <7vbq6u32mq.fsf@gitster.siamese.dyndns.org>\n>\n> Steffen\n>\n> -- >8 --\n\nThis v9 replaces itself, ah it repaces ;-)\n\n> git diff, git add, and any other command that calls\n> index_fd(..., write_object=1) also behave as requested by\n> safecrlf. The user still has the chance to save her data before\n> checkout or applying the diff.\n\n\"git diff\" writes by calling index_fd(..., write_object=1)???\n\nI think warning in \"git diff\" is probably a good thing to do but\nthat is not because it writes. The warning would trigger only\nwhen you are comparing what you have in the work tree with\nsomething else, and if you get such a warning, it means what you\ncan potentially commit (i.e. what you have in the work tree) is\nnot \"autocrlf\" safe. You would get the same warning when you\nlater runn \"git add\" on such a file anyway, but it is nicer to\ncatch the potential problem earlier.\n\nI do not know if the above \"why this command should behave this\nway with respect to safecrlf\" needs to be in the commit log\nmessage. But I think that information should be in the\ndocumentation to help the end users (the end users typically do\nnot have access to the commit log of git.git).\n\nThe rest of the patch looked sane. Thanks.\n\nSo I'd suggest the following change to the commit log message,\nand then another patch at the end squashed into the\nDocumentation/ part.\n\nIf you agree with these, you can just say \"Ok, amend it like\nso\". Or you can say \"Here is an even better replacement.\"\n\n----------------------------------------------------------------\n[proposed change to the commit log message]\n\n--- /var/tmp/1\t2008-02-06 11:36:28.000000000 -0800\n+++ /var/tmp/2\t2008-02-06 11:36:19.000000000 -0800\n@@ -49,28 +49,29 @@\n leave core.autocrlf unset, so users will not see warnings unless\n they deliberately chose to activate the autocrlf mechanism.\n \n- The safecrlf mechanism's details depend on the git command. If\n- safecrlf is active (not false), files in the work tree must not\n- be modified in an irreversible way without giving the user a\n- chance to backup the original file. However, for read-only\n- operations that do not modify files in the work tree git should\n- not print annoying warnings.\n- \n- git apply behaves as requested by by safecrlf. Even though git\n- apply writes changes directly back to the work tree, we assume\n- that the user still can get the original files back by checking\n- out from the index or HEAD. git apply is about applying patches,\n- which means we are talking about \"text\" and therefore it is\n- safe to write the results directly back to the work tree even\n- if CRLFs were converted to LFs.\n- \n- git diff, git add, and any other command that calls\n- index_fd(..., write_object=1) also behave as requested by\n- safecrlf. The user still has the chance to save her data before\n- checkout or applying the diff.\n+ The safecrlf mechanism's details depend on the git command. The\n+ general principles when safecrlf is active (not false) are:\n \n- git blame will never warn nor fail because it never writes to the\n- work tree.\n+ - we warn/error out if files in the work tree can modified in an\n+ irreversible way without giving the user a chance to backup the\n+ original file.\n+ \n+ - for read-only operations that do not modify files in the work tree\n+ we do not not print annoying warnings.\n+ \n+ There are exceptions. Even though...\n+ \n+ - \"git add\" itself does not touch the files in the work tree, the\n+ next checkout would, so the safety triggers;\n+ \n+ - \"git apply\" to update a text file with a patch does touch the files\n+ in the work tree, but the operation is about text files and CRLF\n+ conversion is about fixing the line ending inconsistencies, so the\n+ safety does not trigger;\n+ \n+ - \"git diff\" itself does not touch the files in the work tree, it is\n+ often run to inspect the changes you intend to next \"git add\". To\n+ catch potential problems early, safety triggers.\n \n The concept of a safety check was originally proposed in a similar\n way by Linus Torvalds. Thanks to Dimitry Potapov for insisting\n\n----------------------------------------------------------------\n[adding the general rule and per-command behaviour to the doc]\n\ndiff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt\nindex 2ffe1fb..84ec962 100644\n--- a/Documentation/gitattributes.txt\n+++ b/Documentation/gitattributes.txt\n@@ -137,7 +137,22 @@ If `core.safecrlf` is set to \"true\" or \"warn\", git verifies if\n the conversion is reversible for the current setting of\n `core.autocrlf`. For \"true\", git rejects irreversible\n conversions; for \"warn\", git only prints a warning but accepts\n-an irreversible conversion.\n+an irreversible conversion. The safety triggers to prevent such\n+a conversion done to the files in the work tree, but there are a\n+few exceptions. Even though...\n+\n+- \"git add\" itself does not touch the files in the work tree, the\n+ next checkout would, so the safety triggers;\n+\n+- \"git apply\" to update a text file with a patch does touch the files\n+ in the work tree, but the operation is about text files and CRLF\n+ conversion is about fixing the line ending inconsistencies, so the\n+ safety does not trigger;\n+\n+- \"git diff\" itself does not touch the files in the work tree, it is\n+ often run to inspect the changes you intend to next \"git add\". To\n+ catch potential problems early, safety triggers.\n+\n \n `ident`\n ^^^^^^^\n"},{"id":"67709","messageId":"E2F063EE-06C5-4B85-8511-5FD4EB6FFEC9@zib.de","threadId":"11841","inReplyTo":"7vk5lhyir6.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] [v9] safecrlf: Add mechanism to warn about irreversible crlf conversions","fromName":"Steffen Prohaska","fromEmail":"prohaska@zib.de","sentAt":"2008-02-06T20:35:11Z","receivedAt":"2008-02-06T20:35:11Z","isPatch":true,"sender":{"key":"prohaska@zib.de","avatar":"https://avatars.githubusercontent.com/u/217580?v=4"},"body":"\nOn Feb 6, 2008, at 8:42 PM, Junio C Hamano wrote:\n\n> Steffen Prohaska writes:\n>\n>> This repaces v9 of the patch and should be applied.\n>> It contains modifications as suggested by Junio in\n>> <7vbq6u32mq.fsf@gitster.siamese.dyndns.org>\n>>\n>> Steffen\n>>\n>> -- >8 --\n>\n> This v9 replaces itself, ah it repaces ;-)\n\nYeah, I recognized this, too; but was to lazy to send another\ncorrection ;)\n\n\n>\n>> git diff, git add, and any other command that calls\n>> index_fd(..., write_object=1) also behave as requested by\n>> safecrlf. The user still has the chance to save her data before\n>> checkout or applying the diff.\n>\n> \"git diff\" writes by calling index_fd(..., write_object=1)???\n>\n> I think warning in \"git diff\" is probably a good thing to do but\n> that is not because it writes. The warning would trigger only\n> when you are comparing what you have in the work tree with\n> something else, and if you get such a warning, it means what you\n> can potentially commit (i.e. what you have in the work tree) is\n> not \"autocrlf\" safe. You would get the same warning when you\n> later runn \"git add\" on such a file anyway, but it is nicer to\n> catch the potential problem earlier.\n>\n> I do not know if the above \"why this command should behave this\n> way with respect to safecrlf\" needs to be in the commit log\n> message. But I think that information should be in the\n> documentation to help the end users (the end users typically do\n> not have access to the commit log of git.git).\n>\n> The rest of the patch looked sane. Thanks.\n>\n> So I'd suggest the following change to the commit log message,\n> and then another patch at the end squashed into the\n> Documentation/ part.\n>\n> If you agree with these, you can just say \"Ok, amend it like\n> so\". Or you can say \"Here is an even better replacement.\"\n\nOk, amend it as you proposed but also squash in the following ...\n\n-- >8 --\ndiff --git a/builtin-apply.c b/builtin-apply.c\nindex d0b3586..3b5618d 100644\n--- a/builtin-apply.c\n+++ b/builtin-apply.c\n@@ -1430,7 +1430,7 @@ static int read_old_data(struct stat *st, const \nchar *path, struct strbuf *buf)\n \tcase S_IFREG:\n \t\tif (strbuf_read_file(buf, path, st->st_size) != st->st_size)\n \t\t\treturn error(\"unable to open or read %s\", path);\n-\t\tconvert_to_git(path, buf->buf, buf->len, buf, safe_crlf);\n+\t\tconvert_to_git(path, buf->buf, buf->len, buf, 0);\n \t\treturn 0;\n \tdefault:\n \t\treturn -1;\n-- >8 --\n\n... because ...\n\n[...]\n\n> +\n> +- \"git apply\" to update a text file with a patch does touch the files\n> + in the work tree, but the operation is about text files and CRLF\n> + conversion is about fixing the line ending inconsistencies, so the\n> + safety does not trigger;\n\n... now after I read this I fully understand what you meant before.\nBut my v9 patch does not match your description without the change\nto builtin-apply.c from above. In v9 the mechanism was still activated\nin \"git apply\".\n\n\tSteffen\n"}]}