{"thread":{"id":"11608","subject":"[PATCH v3] safecrlf: Add mechanism to warn about irreversible crlf conversions","startedAt":"2008-01-13T16:30:47Z","lastAt":"2008-01-15T23:23:21Z","messageCount":15,"participants":["Steffen Prohaska","Dmitry Potapov","Junio C Hamano"],"isPatch":true,"patchVersion":3,"patchTotal":null},"messages":[{"id":"65256","messageId":"1200241847776-git-send-email-prohaska@zib.de","threadId":"11608","inReplyTo":null,"subject":"[PATCH v3] safecrlf: Add mechanism to warn about irreversible crlf conversions","fromName":"Steffen Prohaska","fromEmail":"prohaska@zib.de","sentAt":"2008-01-13T16:30:47Z","receivedAt":"2008-01-13T16:30:47Z","isPatch":true,"sender":{"key":"prohaska@zib.de","avatar":"https://avatars.githubusercontent.com/u/217580?v=4"},"body":"This one could be applied.\n\nI mentioned earlier that crlf_to_git() would be called twice.  Unfortunately,\nI can't reproduce this behaviour and are not even sure if it ever happend.\nMaybe it was only an illusion in my terminal?\n\n    Steffen\n\n---- snip snap ---\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 during commit you can easily fix\nit by setting the conversion type explicitly in .gitattributes.\nRight after committing you still have the original file in your\nwork 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\nwith mixed lineendings and undesired effect of corrupting binary\nfiles can not be distinguished.  In both cases CRLF are removed\nin an irreversible way.  For text files this is the right thing\nto do, while for binary file its corrupting data.\n\nIn a sane environment committing and checking out the same file\nshould not modify the origin 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 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        |   21 ++++++++++++++++++\n Documentation/gitattributes.txt |    5 ++++\n cache.h                         |    8 +++++++\n config.c                        |    9 +++++++\n convert.c                       |   28 +++++++++++++++++++++--\n environment.c                   |    1 +\n t/t0020-crlf.sh                 |   45 +++++++++++++++++++++++++++++++++++++++\n 7 files changed, 114 insertions(+), 3 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex df091d1..d2a9fdd 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -139,6 +139,27 @@ 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 committing a\n+\tfile followed by checking out the same file will yield the\n+\toriginal file in the work tree.  If this is not the case for\n+\tthe current setting of `core.autocrlf`, git will reject the\n+\tfile.  The variable can be set to \"warn\", in which case git\n+\twill only warn about an irreversible conversion but accept the\n+\tfile.\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/cache.h b/cache.h\nindex 39331c2..4e03e3d 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);\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..c9678ee 100644\n--- a/convert.c\n+++ b/convert.c\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,20 @@ static int crlf_to_git(const char *path, const char *src, size_t len,\n \t\t\treturn 0;\n \t}\n \n+\tif (safe_crlf && auto_crlf > 0 && action != CRLF_INPUT) {\n+\t\t/* CRLFs would be added by checkout: check if we have \"naked\" LFs */\n+\t\tif (stats.lf != stats.crlf) {\n+\t\t\tif (safe_crlf == SAFE_CRLF_WARN)\n+\t\t\t\twarning(\"Checkout will replace LFs with CRLF in %s\", path);\n+\t\t\telse\n+\t\t\t\tdie(\"Checkout would replace LFs with CRLF in %s\", path);\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@@ -132,6 +143,17 @@ static int crlf_to_git(const char *path, const char *src, size_t len,\n \t\t\t\t*dst++ = c;\n \t\t} while (--len);\n \t}\n+\n+\tif (safe_crlf && (action == CRLF_INPUT || auto_crlf <= 0)) {\n+\t\t/* CRLFs would not be restored by checkout: check if we removed CRLFs */\n+\t\tif (buf->len != dst - buf->buf) {\n+\t\t\tif (safe_crlf == SAFE_CRLF_WARN)\n+\t\t\t\twarning(\"Stripped CRLF from %s.\", path);\n+\t\t\telse\n+\t\t\t\tdie(\"Refusing to strip CRLF from %s.\", path);\n+\t\t}\n+\t}\n+\n \tstrbuf_setlen(buf, dst - buf->buf);\n \treturn 1;\n }\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/t/t0020-crlf.sh b/t/t0020-crlf.sh\nindex 89baebd..e2e0f7b 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,47 @@ 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 'switch off autocrlf, safecrlf' '\n+\tgit repo-config core.autocrlf false &&\n+\tgit repo-config core.safecrlf false\n+'\n+\n test_expect_success 'update with autocrlf=input' '\n \n \trm -f tmp one dir/two three &&\n-- \n1.5.4.rc2.60.g46ee\n"},{"id":"65276","messageId":"20080113220227.GR2963@dpotapov.dyndns.org","threadId":"11608","inReplyTo":"1200241847776-git-send-email-prohaska@zib.de","subject":"Re: [PATCH v3] safecrlf: Add mechanism to warn about irreversible crlf conversions","fromName":"Dmitry Potapov","fromEmail":"dpotapov@gmail.com","sentAt":"2008-01-13T22:02:27Z","receivedAt":"2008-01-13T22:02:27Z","isPatch":true,"sender":{"key":"dpotapov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6568595?v=4"},"body":"Hi Steffan,\n\nThe patch looks good to me. So:\n\nAcked-by: Dmitry Potapov <dpotapov@gmail.com>\n\nDmitry\n"},{"id":"65275","messageId":"20080113221304.GS2963@dpotapov.dyndns.org","threadId":"11608","inReplyTo":"1200241847776-git-send-email-prohaska@zib.de","subject":"Re: [PATCH v3] safecrlf: Add mechanism to warn about irreversible crlf conversions","fromName":"Dmitry Potapov","fromEmail":"dpotapov@gmail.com","sentAt":"2008-01-13T22:13:04Z","receivedAt":"2008-01-13T22:13:04Z","isPatch":true,"sender":{"key":"dpotapov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6568595?v=4"},"body":"Hi Steffan,\n\nIt looks like I was to quick to ack your patch\n\nOn Sun, Jan 13, 2008 at 05:30:47PM +0100, Steffen Prohaska wrote:\n> \n> I mentioned earlier that crlf_to_git() would be called twice.  Unfortunately,\n> I can't reproduce this behaviour and are not even sure if it ever happend.\n\nthat happened to me right now. My setting is autocrlf = input.\nand file that I am adding has mixing line-ending:\n===\nfoo\nbar\\r\n===\n\nDmitry\n"},{"id":"65284","messageId":"1200267979-17683-1-git-send-email-dpotapov@gmail.com","threadId":"11608","inReplyTo":"1200241847776-git-send-email-prohaska@zib.de","subject":"[PATCH] Don't display crlf warning twice","fromName":"Dmitry Potapov","fromEmail":"dpotapov@gmail.com","sentAt":"2008-01-13T23:46:19Z","receivedAt":"2008-01-13T23:46:19Z","isPatch":true,"sender":{"key":"dpotapov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6568595?v=4"},"body":"'git add' could call crlf_to_git() twice, and this caused that the same\ncrlf warning being display twice. The first time crlf_to_git() is called\nwhen a file is added to index, and it could be called the second time\nduring writing the index.\n\nThis patches sets safe_crlf to false before the second call.\n\nSigned-off-by: Dmitry Potapov <dpotapov@gmail.com>\n---\n\nOn Sun, Jan 13, 2008 at 05:30:47PM +0100, Steffen Prohaska wrote:\n>_\n> I mentioned earlier that crlf_to_git() would be called twice.  Unfortunately,\n> I can't reproduce this behaviour and are not even sure if it ever happend.\n\nI think I have found the cause. It can be seen from the following trace:\n\n==============\n#0  crlf_to_git (path=0x814b37e \"a\", src=0xb7fb4000 \"Hello\\r\\nHello\\r\\nHello\\n\", len=20, buf=0xbfad504c, action=-1) at convert.c:89\n#1  0x080e0454 in convert_to_git (path=0x814b37e \"a\", src=0xb7fb4000 \"Hello\\r\\nHello\\r\\nHello\\n\", len=20, dst=0xbfad504c) at convert.c:578\n#2  0x080b7194 in index_fd (sha1=0xbfad508c \"\", fd=7, st=0xbfad50d8, write_object=0, type=OBJ_BLOB, path=0x814b37e \"a\") at sha1_file.c:2345\n#3  0x080a7dd4 in ce_compare_data (ce=0x814b340, st=0xbfad50d8) at read-cache.c:56\n#4  0x080a8045 in ce_modified_check_fs (ce=0x814b340, st=0xbfad50d8) at read-cache.c:111\n#5  0x080aa66d in ce_smudge_racily_clean_entry (ce=0x814b340) at read-cache.c:1121\n#6  0x080aa79d in write_index (istate=0x814a3e0, newfd=6) at read-cache.c:1177\n#7  0x0804c66a in cmd_add (argc=1, argv=0xbfad6408, prefix=0x0) at builtin-add.c:261\n==============\n#0  crlf_to_git (path=0x814b094 \"a\", src=0xb7fb4000 \"Hello\\r\\nHello\\r\\nHello\\n\", len=20, buf=0xbfad50ec, action=-1) at convert.c:89\n#1  0x080e0454 in convert_to_git (path=0x814b094 \"a\", src=0xb7fb4000 \"Hello\\r\\nHello\\r\\nHello\\n\", len=20, dst=0xbfad50ec) at convert.c:578\n#2  0x080b7194 in index_fd (sha1=0x814b368 \"\", fd=7, st=0xbfad5174, write_object=1, type=OBJ_BLOB, path=0x814b094 \"a\") at sha1_file.c:2345\n#3  0x080b731f in index_path (sha1=0x814b368 \"\", path=0x814b094 \"a\", st=0xbfad5174, write_object=1) at sha1_file.c:2377\n#4  0x080a8c00 in add_file_to_index (istate=0x814a3e0, path=0x814b094 \"a\", verbose=0) at read-cache.c:433\n#5  0x0804c640 in cmd_add (argc=1, argv=0xbfad6408, prefix=0x0) at builtin-add.c:257\n==============\n\nThis patch works for me but it certainly needs better testing.\n\n builtin-add.c |    3 +++\n 1 files changed, 3 insertions(+), 0 deletions(-)\n\ndiff --git a/builtin-add.c b/builtin-add.c\nindex 5c29cc2..f113fc1 100644\n--- a/builtin-add.c\n+++ b/builtin-add.c\n@@ -258,9 +258,12 @@ int cmd_add(int argc, const char **argv, const char *prefix)\n \n  finish:\n \tif (active_cache_changed) {\n+\t\tenum safe_crlf old = safe_crlf;\n+\t\tsafe_crlf = SAFE_CRLF_FALSE;\n \t\tif (write_cache(newfd, active_cache, active_nr) ||\n \t\t    close(newfd) || commit_locked_index(&lock_file))\n \t\t\tdie(\"Unable to write new index file\");\n+\t\tsafe_crlf = old;\n \t}\n \n \treturn 0;\n-- \n1.5.3.5\n"},{"id":"65299","messageId":"B4297A2E-9F8F-4167-AB48-10AC1FB4E1D5@zib.de","threadId":"11608","inReplyTo":"1200267979-17683-1-git-send-email-dpotapov@gmail.com","subject":"Re: [PATCH] Don't display crlf warning twice","fromName":"Steffen Prohaska","fromEmail":"prohaska@zib.de","sentAt":"2008-01-14T06:17:26Z","receivedAt":"2008-01-14T06:17:26Z","isPatch":true,"sender":{"key":"prohaska@zib.de","avatar":"https://avatars.githubusercontent.com/u/217580?v=4"},"body":"Dimitry,\nThanks for finding the code path that leads to double printing\nthe warning.\n\nYour traces reveal that it is a racy condition that can trigger\nthe double warnings.  In my test cases this apparently does not\nhappen, at least not on my machine.\n\nDo you have a test case that reliably triggers the second call to\nconvert_to_git()?\n\n\n\nOn Jan 14, 2008, at 12:46 AM, Dmitry Potapov wrote:\n\n> 'git add' could call crlf_to_git() twice, and this caused that the  \n> same\n> crlf warning being display twice. The first time crlf_to_git() is  \n> called\n> when a file is added to index, and it could be called the second time\n> during writing the index.\n>\n> This patches sets safe_crlf to false before the second call.\n\nThis would certainly work.\n\nHowever I do not think it is the right place to fix ...\n\n\n> Signed-off-by: Dmitry Potapov <dpotapov@gmail.com>\n> ---\n>\n> On Sun, Jan 13, 2008 at 05:30:47PM +0100, Steffen Prohaska wrote:\n>> _\n>> I mentioned earlier that crlf_to_git() would be called twice.   \n>> Unfortunately,\n>> I can't reproduce this behaviour and are not even sure if it ever  \n>> happend.\n>\n> I think I have found the cause. It can be seen from the following  \n> trace:\n>\n> ==============\n> #0  crlf_to_git (path=0x814b37e \"a\", src=0xb7fb4000 \"Hello\\r\\nHello \n> \\r\\nHello\\n\", len=20, buf=0xbfad504c, action=-1) at convert.c:89\n> #1  0x080e0454 in convert_to_git (path=0x814b37e \"a\",  \n> src=0xb7fb4000 \"Hello\\r\\nHello\\r\\nHello\\n\", len=20, dst=0xbfad504c)  \n> at convert.c:578\n> #2  0x080b7194 in index_fd (sha1=0xbfad508c \"\", fd=7,  \n> st=0xbfad50d8, write_object=0, type=OBJ_BLOB, path=0x814b37e \"a\")  \n> at sha1_file.c:2345\n\nHere, index_fd() is called with write_object=0 ...\n\n\n> #3  0x080a7dd4 in ce_compare_data (ce=0x814b340, st=0xbfad50d8) at  \n> read-cache.c:56\n> #4  0x080a8045 in ce_modified_check_fs (ce=0x814b340,  \n> st=0xbfad50d8) at read-cache.c:111\n> #5  0x080aa66d in ce_smudge_racily_clean_entry (ce=0x814b340) at  \n> read-cache.c:1121\n> #6  0x080aa79d in write_index (istate=0x814a3e0, newfd=6) at read- \n> cache.c:1177\n> #7  0x0804c66a in cmd_add (argc=1, argv=0xbfad6408, prefix=0x0) at  \n> builtin-add.c:261\n> ==============\n> #0  crlf_to_git (path=0x814b094 \"a\", src=0xb7fb4000 \"Hello\\r\\nHello \n> \\r\\nHello\\n\", len=20, buf=0xbfad50ec, action=-1) at convert.c:89\n> #1  0x080e0454 in convert_to_git (path=0x814b094 \"a\",  \n> src=0xb7fb4000 \"Hello\\r\\nHello\\r\\nHello\\n\", len=20, dst=0xbfad50ec)  \n> at convert.c:578\n> #2  0x080b7194 in index_fd (sha1=0x814b368 \"\", fd=7, st=0xbfad5174,  \n> write_object=1, type=OBJ_BLOB, path=0x814b094 \"a\") at sha1_file.c:2345\n\nwhile here index_fd() is called with write_object=1.\n\n\n> #3  0x080b731f in index_path (sha1=0x814b368 \"\", path=0x814b094  \n> \"a\", st=0xbfad5174, write_object=1) at sha1_file.c:2377\n> #4  0x080a8c00 in add_file_to_index (istate=0x814a3e0,  \n> path=0x814b094 \"a\", verbose=0) at read-cache.c:433\n> #5  0x0804c640 in cmd_add (argc=1, argv=0xbfad6408, prefix=0x0) at  \n> builtin-add.c:257\n> ==============\n\n\nWithout digging deeply through the code, this looks to me as if\nwe could base our decision wether to print a warning or not on\nthis difference.\n\nMy first take is: we should print a warning if write_object=1\nand should be quiet if write_object=0.  The flag can easily be\npassed to convert_to_git().\n\nMy assumption is that users would only be interested in the\nwarning if data is really written to the repository (and that\nthis is the meaning of write_object=1).  But maybe there are\ncases where a warning is also appropriate even if the data\nis not written?  I don't now.\n\nAnother question is if we should die() even if write_object=0;\nor only if write_object=1?\n\n\tSteffen\n"},{"id":"65300","messageId":"20080114064036.GX2963@dpotapov.dyndns.org","threadId":"11608","inReplyTo":"B4297A2E-9F8F-4167-AB48-10AC1FB4E1D5@zib.de","subject":"Re: [PATCH] Don't display crlf warning twice","fromName":"Dmitry Potapov","fromEmail":"dpotapov@gmail.com","sentAt":"2008-01-14T06:40:36Z","receivedAt":"2008-01-14T06:40:36Z","isPatch":true,"sender":{"key":"dpotapov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6568595?v=4"},"body":"On Mon, Jan 14, 2008 at 07:17:26AM +0100, Steffen Prohaska wrote:\n> \n> Your traces reveal that it is a racy condition that can trigger\n> the double warnings.\n\nIt is not a racy condition. It is just another branch in the\ncode, which happens when an existing file is modified. Your\ntests always added a new file, so they could not reproduce\nthe problem.\n\n> \n> Do you have a test case that reliably triggers the second call to\n> convert_to_git()?\n\n===========================================\ngit config core.autocrlf input\n\necho Hello > foo\ngit add foo\ngit commit -m 'add foo'\n\necho Hello >> foo\nunix2dos foo\n# This should trigger double crlf warning\ngit add foo\n===========================================\n\nDmitry\n"},{"id":"65303","messageId":"004D2FB5-2CEA-4DB1-AF8D-D5ADEB0F0508@zib.de","threadId":"11608","inReplyTo":"20080114064036.GX2963@dpotapov.dyndns.org","subject":"Re: [PATCH] Don't display crlf warning twice","fromName":"Steffen Prohaska","fromEmail":"prohaska@zib.de","sentAt":"2008-01-14T06:53:13Z","receivedAt":"2008-01-14T06:53:13Z","isPatch":true,"sender":{"key":"prohaska@zib.de","avatar":"https://avatars.githubusercontent.com/u/217580?v=4"},"body":"\nOn Jan 14, 2008, at 7:40 AM, Dmitry Potapov wrote:\n\n> On Mon, Jan 14, 2008 at 07:17:26AM +0100, Steffen Prohaska wrote:\n>>\n>> Your traces reveal that it is a racy condition that can trigger\n>> the double warnings.\n>\n> It is not a racy condition. It is just another branch in the\n> code, which happens when an existing file is modified. Your\n> tests always added a new file, so they could not reproduce\n> the problem.\n>\n>>\n>> Do you have a test case that reliably triggers the second call to\n>> convert_to_git()?\n>\n> ===========================================\n> git config core.autocrlf input\n>\n> echo Hello > foo\n> git add foo\n> git commit -m 'add foo'\n>\n> echo Hello >> foo\n> unix2dos foo\n> # This should trigger double crlf warning\n> git add foo\n> ===========================================\n\nYes, this reproduces the problem.\n\nThanks,\nSteffen\n"},{"id":"65345","messageId":"12003528401309-git-send-email-prohaska@zib.de","threadId":"11608","inReplyTo":"004D2FB5-2CEA-4DB1-AF8D-D5ADEB0F0508@zib.de","subject":"[PATCH] safecrlf: Add flag to convert_to_git() to disable safecrlf check","fromName":"Steffen Prohaska","fromEmail":"prohaska@zib.de","sentAt":"2008-01-14T23:20:40Z","receivedAt":"2008-01-14T23:20:40Z","isPatch":true,"sender":{"key":"prohaska@zib.de","avatar":"https://avatars.githubusercontent.com/u/217580?v=4"},"body":"This commit goes on top of\n\"[PATCH v3] safecrlf: Add mechanism to warn about irreversible crlf conversions\"\nI send it as a separate patch to make reviewing easier.\nEventually it should be squashed.\n\nI looked briefly at the various places where convert_to_git() is\ncalled.  I think that only the one code path through index_fd()\nactually writes data to the repsitory.  Maybe someone else with\na better understanding of git's internals should confirm this.\n\n    Steffen\n\n---- snip ---\n\nWe want to verify if an autocrlf conversion is reversible only if\nthe converted data is actually written to the repository.  Only\nin this case the file would be modified during the next checkout.\nBut convert_to_git() is used for some other purposes.\n\nThis commit adds a flag to convert_to_git() that controls if the\nsafecrlf check is enabled.  The check is only enabled if\nindex_fd() writes the object converted.  In all other cases the\ncheck is disabled.\n\nSigned-off-by: Steffen Prohaska <prohaska@zib.de>\n---\n builtin-apply.c |    2 +-\n builtin-blame.c |    2 +-\n cache.h         |    2 +-\n convert.c       |   10 +++++-----\n diff.c          |    2 +-\n sha1_file.c     |    2 +-\n 6 files changed, 10 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin-apply.c b/builtin-apply.c\nindex d57bb6e..9fb6fe3 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, 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 4e03e3d..43e7d0e 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -640,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, int 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/convert.c b/convert.c\nindex c9678ee..5adef4f 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, int checksafe)\n {\n \tstruct text_stat stats;\n \tchar *dst;\n@@ -107,7 +107,7 @@ static int crlf_to_git(const char *path, const char *src, size_t len,\n \t\t\treturn 0;\n \t}\n \n-\tif (safe_crlf && auto_crlf > 0 && action != CRLF_INPUT) {\n+\tif (safe_crlf && checksafe && auto_crlf > 0 && action != CRLF_INPUT) {\n \t\t/* CRLFs would be added by checkout: check if we have \"naked\" LFs */\n \t\tif (stats.lf != stats.crlf) {\n \t\t\tif (safe_crlf == SAFE_CRLF_WARN)\n@@ -144,7 +144,7 @@ static int crlf_to_git(const char *path, const char *src, size_t len,\n \t\t} while (--len);\n \t}\n \n-\tif (safe_crlf && (action == CRLF_INPUT || auto_crlf <= 0)) {\n+\tif (safe_crlf && checksafe && (action == CRLF_INPUT || auto_crlf <= 0)) {\n \t\t/* CRLFs would not be restored by checkout: check if we removed CRLFs */\n \t\tif (buf->len != dst - buf->buf) {\n \t\t\tif (safe_crlf == SAFE_CRLF_WARN)\n@@ -553,7 +553,7 @@ 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, struct strbuf *dst, int checksafe)\n {\n \tstruct git_attr_check check[3];\n \tint crlf = CRLF_GUESS;\n@@ -575,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..b5ac4f9 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, 0)) {\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/sha1_file.c b/sha1_file.c\nindex 6583797..3b5413d 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)) {\n \t\t\tmunmap(buf, size);\n \t\t\tbuf = strbuf_detach(&nbuf, &size);\n \t\t\tre_allocated = 1;\n-- \n1.5.4.rc2.60.g46ee\n"},{"id":"65351","messageId":"7vk5mchsct.fsf@gitster.siamese.dyndns.org","threadId":"11608","inReplyTo":"12003528401309-git-send-email-prohaska@zib.de","subject":"Re: [PATCH] safecrlf: Add flag to convert_to_git() to disable safecrlf check","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-01-14T23:58:42Z","receivedAt":"2008-01-14T23:58:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Steffen Prohaska <prohaska@zib.de> writes:\n\n> We want to verify if an autocrlf conversion is reversible only if\n> the converted data is actually written to the repository.  Only\n> in this case the file would be modified during the next checkout.\n> But convert_to_git() is used for some other purposes.\n> This commit adds a flag to convert_to_git() that controls if the\n> safecrlf check is enabled...\n\nAt first this felt dirty to me as convert_to_git() is not\nlimited to crlf, but about external vs canonical representation.\nThe variable name being \"checksafe\" however makes it much more\npalatable.  It is clear that it is talking about irreversible\nconversion.\n\nWhen running diff with a work tree file and the index (or a\nnamed tree), we read the work tree file and run convert_to_git()\non it before comparing it with what we have in the object store\n(either index or a named tree).  When running apply without\ntouching the index, we also use convert_to_git() on the work\ntree file.  The patch file is supposed to record the data in\ncanonical format, I think.\n\nOf course, \"git add\" on the path will warn or fail with your\npatch, but we may somehow want to be warned about the breakage\nbefore \"git add\" on that path triggers it.  Perhaps we can have\na separate \"check-work-tree\" command that iterates over locally\nmodified work tree files and runs convert_to_git() with checking\nenabled.\n"},{"id":"65370","messageId":"20080115102626.GE2963@dpotapov.dyndns.org","threadId":"11608","inReplyTo":"12003528401309-git-send-email-prohaska@zib.de","subject":"Re: [PATCH] safecrlf: Add flag to convert_to_git() to disable safecrlf check","fromName":"Dmitry Potapov","fromEmail":"dpotapov@gmail.com","sentAt":"2008-01-15T10:26:26Z","receivedAt":"2008-01-15T10:26:26Z","isPatch":true,"sender":{"key":"dpotapov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6568595?v=4"},"body":"On Tue, Jan 15, 2008 at 12:20:40AM +0100, Steffen Prohaska wrote:\n> \n> I looked briefly at the various places where convert_to_git() is\n> called.  I think that only the one code path through index_fd()\n> actually writes data to the repsitory.  Maybe someone else with\n> a better understanding of git's internals should confirm this.\n\nYour patch is certainly better than my quick hack for git-add,\nand perhaps you are right that the check should be done only\nwhen data are written, but it also means that there is no longer\nany warning when you are running git diff with the work tree,\nwhich would be useful, because it is what most users do before\nadding anything.\n\nHowever, my real concern is that it seems we have two different\nheuristics for binary -- one that is used inside of convert.c\nand the other one is buffer_is_binary() in xdiff-interface.c.\n\nSo, I am running 'git diff' for some test file, and it says:\n\ndiff --git a/foo b/foo\nindex e965047..c102bdc 100644\nBinary files a/foo and b/foo differ\n\nokay, now I want to add this *binary* file, so I run 'git add':\n\nwarning: Stripped CRLF from foo.\n\nI imagine a user saying: \"What the hell! Why did this stupid Git\nstrip CRLF from my _binary_ file?\"\n\nAnd the current version of Git, which does not print CRLF warning,\nseems to be dangerous, because when 'git diff' told me that it is\na _binary_ file, I expect that Git will put it as *binary*. So,\nfrom the user's point of view, it looks like a bug.\n\nSo, I suppose that at least we should make is_binary heuristic in\nconvert.c more strict than those that is used by diff. Namely, if\nthere is at least one NUL byte in the buffer, it should be treated\nas binary.\n\n\nDmitry\n"},{"id":"65415","messageId":"53A4FB98-25B5-4598-BED8-7D96AF5527F4@zib.de","threadId":"11608","inReplyTo":"20080115102626.GE2963@dpotapov.dyndns.org","subject":"Re: [PATCH] safecrlf: Add flag to convert_to_git() to disable safecrlf check","fromName":"Steffen Prohaska","fromEmail":"prohaska@zib.de","sentAt":"2008-01-15T20:39:00Z","receivedAt":"2008-01-15T20:39:00Z","isPatch":true,"sender":{"key":"prohaska@zib.de","avatar":"https://avatars.githubusercontent.com/u/217580?v=4"},"body":"\nOn Jan 15, 2008, at 11:26 AM, Dmitry Potapov wrote:\n\n> On Tue, Jan 15, 2008 at 12:20:40AM +0100, Steffen Prohaska wrote:\n>>\n>> I looked briefly at the various places where convert_to_git() is\n>> called.  I think that only the one code path through index_fd()\n>> actually writes data to the repsitory.  Maybe someone else with\n>> a better understanding of git's internals should confirm this.\n>\n> Your patch is certainly better than my quick hack for git-add,\n> and perhaps you are right that the check should be done only\n> when data are written, but it also means that there is no longer\n> any warning when you are running git diff with the work tree,\n> which would be useful, because it is what most users do before\n> adding anything.\n\nMy first concern is to avoid data corruption.  But we should also\navoid to unnecessarily bother users with annoying warnings.  Thus\nI thought that guarding only the code paths that modify data in\nan irreversible way is sufficient and hence I only guarded the\ncode path that writes to the repository.  The underlying\nassumption is that git checkout is the only way of destroying the\noriginal data.  Unless you check out you still have the original\ndata and can copy them to a safe place.\n\nHowever, there might be other ways by which git will modify the\nfiles in the work tree.  And even commands that never touch the\nlocal file system could be dangerous.  For example a user could\nrun git diff to create a patch, which is sent to someone else and\napplied there.  The change is committed in the remote repository,\npublished, and pulled back to the local repository and eventually\nthis might result in the corruption we originally tried to avoid.\n\nSo perhaps we should guard _every_ code path that calls\nconvert_to_git() in an irreversible way; and only avoid printing\nthe same warning twice for a single run of git add.  This is\nexactly what your \"quick hack\" does.\n\nMore in my reply to Junios mail ...\n\n\n> However, my real concern is that it seems we have two different\n> heuristics for binary -- one that is used inside of convert.c\n> and the other one is buffer_is_binary() in xdiff-interface.c.\n>\n> So, I am running 'git diff' for some test file, and it says:\n>\n> diff --git a/foo b/foo\n> index e965047..c102bdc 100644\n> Binary files a/foo and b/foo differ\n>\n> okay, now I want to add this *binary* file, so I run 'git add':\n>\n> warning: Stripped CRLF from foo.\n>\n> I imagine a user saying: \"What the hell! Why did this stupid Git\n> strip CRLF from my _binary_ file?\"\n>\n> And the current version of Git, which does not print CRLF warning,\n> seems to be dangerous, because when 'git diff' told me that it is\n> ca _binary_ file, I expect that Git will put it as *binary*. So,\n> from the user's point of view, it looks like a bug.\n>\n> So, I suppose that at least we should make is_binary heuristic in\n> convert.c more strict than those that is used by diff. Namely, if\n> there is at least one NUL byte in the buffer, it should be treated\n> as binary.\n\nI think we should do this.\n\nPerhaps we should also place a hint for future developers in\nxdiff-interface.c, like the patch below.\n\n\tSteffen\n\n\ndiff --git a/xdiff-interface.c b/xdiff-interface.c\nindex 4b8e5cc..baa3664 100644\n--- a/xdiff-interface.c\n+++ b/xdiff-interface.c\n@@ -160,6 +160,10 @@ int read_mmfile(mmfile_t *ptr, const char  \n*filename)\n         return 0;\n  }\n\n+/* If you ever modify buffer_is_binary() you should check that  \nis_binary()\n+   in convert.c always uses a stricter heuristic.  That is all files  \nthat are\n+   classified as binary here should also be classified as binary in  \nconvert.c.\n+ */\n  #define FIRST_FEW_BYTES 8000\n  int buffer_is_binary(const char *ptr, unsigned long size)\n  {\n"},{"id":"65417","messageId":"F825ADAF-036C-46FE-8E3D-540B9AA092A8@zib.de","threadId":"11608","inReplyTo":"7vk5mchsct.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] safecrlf: Add flag to convert_to_git() to disable safecrlf check","fromName":"Steffen Prohaska","fromEmail":"prohaska@zib.de","sentAt":"2008-01-15T20:52:53Z","receivedAt":"2008-01-15T20:52:53Z","isPatch":true,"sender":{"key":"prohaska@zib.de","avatar":"https://avatars.githubusercontent.com/u/217580?v=4"},"body":"\nOn Jan 15, 2008, at 12:58 AM, Junio C Hamano wrote:\n\n> Steffen Prohaska <prohaska@zib.de> writes:\n>\n>> We want to verify if an autocrlf conversion is reversible only if\n>> the converted data is actually written to the repository.  Only\n>> in this case the file would be modified during the next checkout.\n>> But convert_to_git() is used for some other purposes.\n>> This commit adds a flag to convert_to_git() that controls if the\n>> safecrlf check is enabled...\n>\n> At first this felt dirty to me as convert_to_git() is not\n> limited to crlf, but about external vs canonical representation.\n> The variable name being \"checksafe\" however makes it much more\n> palatable.  It is clear that it is talking about irreversible\n> conversion.\n>\n> When running diff with a work tree file and the index (or a\n> named tree), we read the work tree file and run convert_to_git()\n> on it before comparing it with what we have in the object store\n> (either index or a named tree).  When running apply without\n> touching the index, we also use convert_to_git() on the work\n> tree file.  The patch file is supposed to record the data in\n> canonical format, I think.\n>\n> Of course, \"git add\" on the path will warn or fail with your\n> patch, but we may somehow want to be warned about the breakage\n> before \"git add\" on that path triggers it.  Perhaps we can have\n> a separate \"check-work-tree\" command that iterates over locally\n> modified work tree files and runs convert_to_git() with checking\n> enabled.\n\nWe could certainly have such a command, yet the question remains\nwhen to call it.  Do you have in mind calling it when we enter\nthe work tree, such that all files in the work tree will always\nbe verified?  Running the check once during start up should be\nsufficient and we could switch it off for the remainder of the\nexecution.\n\nWe would certainly print all paths with an irreversible conversion\nand only die() afterwards if requested by core.safecrlf=true.\nAll information would be printed at once in an ordered way.  This\ncould be more user friendly than the current approach.\n\nI'll work on this.\n\n\tSteffen\n"},{"id":"65424","messageId":"FF86F119-5BF4-4ED3-B6AD-BADFDC91301D@zib.de","threadId":"11608","inReplyTo":"F825ADAF-036C-46FE-8E3D-540B9AA092A8@zib.de","subject":"Re: [PATCH] safecrlf: Add flag to convert_to_git() to disable safecrlf check","fromName":"Steffen Prohaska","fromEmail":"prohaska@zib.de","sentAt":"2008-01-15T21:41:53Z","receivedAt":"2008-01-15T21:41:53Z","isPatch":true,"sender":{"key":"prohaska@zib.de","avatar":"https://avatars.githubusercontent.com/u/217580?v=4"},"body":"\nOn Jan 15, 2008, at 9:52 PM, Steffen Prohaska wrote:\n\n>> Of course, \"git add\" on the path will warn or fail with your\n>> patch, but we may somehow want to be warned about the breakage\n>> before \"git add\" on that path triggers it.  Perhaps we can have\n>> a separate \"check-work-tree\" command that iterates over locally\n>> modified work tree files and runs convert_to_git() with checking\n>> enabled.\n>\n> We could certainly have such a command, yet the question remains\n> when to call it.  Do you have in mind calling it when we enter\n> the work tree, such that all files in the work tree will always\n> be verified?  Running the check once during start up should be\n> sufficient and we could switch it off for the remainder of the\n> execution.\n>\n> We would certainly print all paths with an irreversible conversion\n> and only die() afterwards if requested by core.safecrlf=true.\n> All information would be printed at once in an ordered way.  This\n> could be more user friendly than the current approach.\n>\n> I'll work on this.\n\n\nWhat is the right way to iterate over the changed files?\nShould I copy and adapt the following from wt-status.c?\n\nstatic void wt_status_print_changed(struct wt_status *s)\n{\n     struct rev_info rev;\n     init_revisions(&rev, \"\");\n     setup_revisions(0, NULL, &rev, NULL);\n     rev.diffopt.output_format |= DIFF_FORMAT_CALLBACK;\n     rev.diffopt.format_callback = wt_status_print_changed_cb;\n     rev.diffopt.format_callback_data = s;\n     wt_read_cache(s);\n     run_diff_files(&rev, 0);\n}\n\n\tSteffen\n"},{"id":"65431","messageId":"20080115230312.GB13984@dpotapov.dyndns.org","threadId":"11608","inReplyTo":"53A4FB98-25B5-4598-BED8-7D96AF5527F4@zib.de","subject":"Re: [PATCH] safecrlf: Add flag to convert_to_git() to disable safecrlf check","fromName":"Dmitry Potapov","fromEmail":"dpotapov@gmail.com","sentAt":"2008-01-15T23:03:12Z","receivedAt":"2008-01-15T23:03:12Z","isPatch":true,"sender":{"key":"dpotapov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6568595?v=4"},"body":"On Tue, Jan 15, 2008 at 09:39:00PM +0100, Steffen Prohaska wrote:\n> \n> On Jan 15, 2008, at 11:26 AM, Dmitry Potapov wrote:\n> \n> >but it also means that there is no longer\n> >any warning when you are running git diff with the work tree,\n> >which would be useful, because it is what most users do before\n> >adding anything.\n> \n> My first concern is to avoid data corruption.  But we should also\n> avoid to unnecessarily bother users with annoying warnings. \n\nI don't think we bother users with unnecessary warnings. If there\nis a problem then it is better to report it earlier than later,\nand we *really* want that the user took some action, i.e. either\nto mark this file as binary using .gitattributes or corrected its\nendings. However, reporting the same problem twice during running\none command does not look nice...\n\n> Thus\n> I thought that guarding only the code paths that modify data in\n> an irreversible way is sufficient and hence I only guarded the\n> code path that writes to the repository.\n\nThis policy makes sense to me, it is just I would prefer if we\ncould find a way to warn a user a bit earlier...\n\n> The underlying\n> assumption is that git checkout is the only way of destroying the\n> original data.  Unless you check out you still have the original\n> data and can copy them to a safe place.\n\nIt is obvious to you but it may be not so obvious to a new user.\nRunning 'git diff' before check in is a common practice, so if\nthe warning was reported at that moment then he or she would not\ncheck in.\n\nAnyway, I like your solution for being simple and guarding the\nmost important pass reliably.\n\nDmitry\n"},{"id":"65436","messageId":"7vodbmd66u.fsf@gitster.siamese.dyndns.org","threadId":"11608","inReplyTo":"FF86F119-5BF4-4ED3-B6AD-BADFDC91301D@zib.de","subject":"Re: [PATCH] safecrlf: Add flag to convert_to_git() to disable safecrlf check","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-01-15T23:23:21Z","receivedAt":"2008-01-15T23:23:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Steffen Prohaska <prohaska@zib.de> writes:\n\n> What is the right way to iterate over the changed files?\n\nI think something like\n\n\tread_cache();\n        for (i = 0; i < active_nr; i++) {\n\t\tstruct cache_entry *ce = active_cache[i];\n\t\tstruct stat st;\n\t\tif (!lstat(ce->name, &st) &&\n\t\t\tce_match_stat(ce, &st, 0)) {\n\t\t\t/* do your thing */\n\t\t}\n\t}\n\nwould do.\n"}]}