{"thread":{"id":"11679","subject":"[PATCH v5] safecrlf: Add mechanism to warn about irreversible crlf conversions","startedAt":"2008-01-19T17:52:31Z","lastAt":"2008-01-20T07:23:26Z","messageCount":2,"participants":["Steffen Prohaska","Dmitry Potapov"],"isPatch":true,"patchVersion":5,"patchTotal":null},"messages":[{"id":"65982","messageId":"12007651514111-git-send-email-prohaska@zib.de","threadId":"11679","inReplyTo":null,"subject":"[PATCH v5] safecrlf: Add mechanism to warn about irreversible crlf conversions","fromName":"Steffen Prohaska","fromEmail":"prohaska@zib.de","sentAt":"2008-01-19T17:52:31Z","receivedAt":"2008-01-19T17:52:31Z","isPatch":true,"sender":{"key":"prohaska@zib.de","avatar":"https://avatars.githubusercontent.com/u/217580?v=4"},"body":"Here is an improved version of the mechanism that lazily checks\nfor irreversible conversions, as announced in\n\n  http://mid.gmane.org/<7E3714CE-0073-4E42-A455-36D595946A6C@zib.de>\n\nI have no plans to continue working on the second alternative\n(is_worktree_safecrlf() check during startup).  I prefer to\nhave some feedback on this patch first.\n\nI believe it would be a good idea to gather some real world\nexperience on pu or next before merging to master.  As explained\nbelow quite a few commands may print warnings and this commit only\nhas tests for git add.\n\nTherefore, I think this is post 1.5.4.\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 containes 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 result in\ncorrupted data.\n\nIf you recognize such corruption early you can easily fix it by\nsetting the conversion type explicitly in .gitattributes.  For\nexample, right after committing you still have the original file\nin your work tree and this file is not yet corrupted.\n\nHowever, in mixed Windows/Unix environments text files quite\neasily can end up containing a mixture of CRLF and LF line\nendings and git should handle such situations gracefully.  For\nexample, a user could copy a CRLF file from Windows to Unix and\nmix it with an existing LF file there.  The result would contain\nboth types of line 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 means corrupting 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 save them.  However, if the file in the work tree will\nnever be modified we should not print annoying warnings.\n\ngit apply will fail even if safecrlf=warn, because git apply will\nwrite its changes back to the work tree immediately.  The user\nwould not have a chance to save the old version of the file after\na warning.  Hence, a warning isn't sufficient and we should fail\ninstead.\n\ngit diff, git add, and any other command that calls\nindex_fd(..., write_object=1) will 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 <prohaska@zib.de>\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 df091d1..50903ca 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 d57bb6e..847f70e 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 39331c2..943185b 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@@ -632,7 +640,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 857deb6..0a46046 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 4df7559..2700cde 100644\n--- a/convert.c\n+++ b/convert.c\n@@ -81,7 +81,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@@ -90,9 +90,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@@ -110,6 +107,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@@ -531,7 +552,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@@ -553,7 +575,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 b18c140..ad4e91e 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 6583797..09e5c8f 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -2342,7 +2342,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 89baebd..8e188b0 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_failure 'safecrlf: autocrlf=input, all CRLF' '\n+\n+\tgit repo-config core.autocrlf input &&\n+\tgit repo-config core.safecrlf true &&\n+\n+\tfor w in I am all CRLF; do echo $w; done | append_cr >allcrlf &&\n+\tgit add allcrlf\n+'\n+\n+test_expect_failure 'safecrlf: autocrlf=input, mixed LF/CRLF' '\n+\n+\tgit repo-config core.autocrlf input &&\n+\tgit repo-config core.safecrlf true &&\n+\n+\tfor w in Oh here is CRLFQ in text; do echo $w; done | q_to_cr >mixed &&\n+\tgit add mixed\n+'\n+\n+test_expect_failure 'safecrlf: autocrlf=true, all LF' '\n+\n+\tgit repo-config core.autocrlf true &&\n+\tgit repo-config core.safecrlf true &&\n+\n+\tfor w in I am all LF; do echo $w; done >alllf &&\n+\tgit add alllf\n+'\n+\n+test_expect_failure 'safecrlf: autocrlf=true mixed LF/CRLF' '\n+\n+\tgit repo-config core.autocrlf true &&\n+\tgit repo-config core.safecrlf true &&\n+\n+\tfor w in Oh here is CRLFQ in text; do echo $w; done | q_to_cr >mixed &&\n+\tgit add mixed\n+'\n+\n+test_expect_success 'safecrlf: print warning only once' '\n+\n+\tgit repo-config core.autocrlf input &&\n+\tgit repo-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 repo-config core.autocrlf false &&\n+\tgit repo-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.rc3.15.g8a53c\n"},{"id":"66019","messageId":"20080120072326.GN14871@dpotapov.dyndns.org","threadId":"11679","inReplyTo":"12007651514111-git-send-email-prohaska@zib.de","subject":"Re: [PATCH v5] safecrlf: Add mechanism to warn about irreversible crlf conversions","fromName":"Dmitry Potapov","fromEmail":"dpotapov@gmail.com","sentAt":"2008-01-20T07:23:26Z","receivedAt":"2008-01-20T07:23:26Z","isPatch":true,"sender":{"key":"dpotapov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6568595?v=4"},"body":"On Sat, Jan 19, 2008 at 06:52:31PM +0100, Steffen Prohaska wrote:\n>\n> +test_expect_failure 'safecrlf: autocrlf=input, all CRLF' '\n> +\n> +\tgit repo-config core.autocrlf input &&\n> +\tgit repo-config core.safecrlf true &&\n\nrepo-config is deprecated. Please, s/git repo-config/git config/\nhere and in other places.\n\nDmitry\n"}]}