{"thread":{"id":"23769","subject":"[PATCH/RFC] autocrlf: Make it work also for un-normalized repositories","startedAt":"2010-05-10T17:11:19Z","lastAt":"2010-05-11T22:52:03Z","messageCount":10,"participants":["Finn Arne Gangstad","Johannes Schindelin","Junio C Hamano","Eyvind Bernhardsen","Dmitry Potapov","Jakub Narebski"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"141412","messageId":"20100510171119.GA17875@pvv.org","threadId":"23769","inReplyTo":null,"subject":"[PATCH/RFC] autocrlf: Make it work also for un-normalized repositories","fromName":"Finn Arne Gangstad","fromEmail":"finnag@pvv.org","sentAt":"2010-05-10T17:11:19Z","receivedAt":"2010-05-10T17:11:19Z","isPatch":true,"sender":{"key":"finnag@pvv.org","avatar":"https://gravatar.com/avatar/b421ddd58c3f0f93aa473e17b98bb8d53c221fef741746bc8cb59fae4ec6d95e?d=mp&s=160"},"body":"Previously, autocrlf would only work well for normalized\nrepositories. Any text files that contained CRLF in the repository\nwould cause problems, and would be modified when handled with\ncore.autocrlf set.\n\nChange autocrlf to not do any conversions to files that in the\nrepository already contain a CR. git with autocrlf set will never\ncreate such a file, or change a LF only file to contain CRs, so the\n(new) assumption is that if a file contains a CR, it is intentional,\nand autocrlf should not change that.\n\nThe following sequence should now always be a NOP even with autocrlf\nset (assuming a clean working directory):\n\ngit checkout \ntouch *\ngit add -A . (will add nothing)\ngit comit (nothing to commit)\n\nPreviously this would break for any text file containing a CR\n\nSigned-off-by: Finn Arne Gangstad \n---\n\nSome of you may have been folowing Eyvind's excellent thread about\ntrying to make end-of-line translation in git a bit smoother.\n\nI decided to attack the problem from a different angle: Is it possible\nto make autocrlf behave non-destructively for all the previous problem cases?\n\nStealing the problem from Eyvind's initial mail (paraphrased and\nsummarized a bit):\n\n1. Setting autocrlf globally is a pain since autocrlf does not work well\n with CRLF in the repo\n2. Setting it in individual repos is hard since you do it \"too late\"\n (the clone will get it wrong)\n3. If someone checks in a file with CRLF later, you get into problems again\n4. If a repository once has contained CRLF, you can't tell autocrlf\n at which commit everything is sane again\n5. autocrlf does needless work if you know that all your users want\n the same EOL style.\n\nI belive that this patch makes autocrlf a safe (and good) default\nsetting for Windows, and this solves problems 1-4.\n\nI implemented it by looking for CR charactes in the index, and\naborting any conversion attempt if this is found. The code to read\nthe index contents was copied pretty verbatim from attr.c, and should\nprobably be made into a non-static function instead if there is no\nbetter way of doing this.\n\nNote that ALL the tests still pass unmodified. This is a bit\nsurprising perhaps, but think it is an indication that no one ever\nintented autocrlf to do what it does to files containing CRs.\n\n\n convert.c | 45 +++++++++++++++++++++++++++++++++++++++++++++\n 1 files changed, 45 insertions(+), 0 deletions(-)\n\ndiff --git a/convert.c b/convert.c\nindex 4f8fcb7..9d062c8 100644\n--- a/convert.c\n+++ b/convert.c\n@@ -120,6 +120,43 @@ static void check_safe_crlf(const char *path, int action,\n \t}\n }\n \n+static int has_cr_in_index(const char *path)\n+{\n+\tint pos, len;\n+\tunsigned long sz;\n+\tenum object_type type;\n+\tvoid *data;\n+\tint has_cr;\n+\tstruct index_state *istate = &the_index;\n+\n+\tlen = strlen(path);\n+\tpos = index_name_pos(istate, path, len);\n+\tif (pos < 0) {\n+\t\t/*\n+\t\t * We might be in the middle of a merge, in which\n+\t\t * case we would read stage #2 (ours).\n+\t\t */\n+\t\tint i;\n+\t\tfor (i = -pos - 1;\n+\t\t (pos < 0 && i < istate->cache_nr &&\n+\t\t !strcmp(istate->cache[i]->name, path));\n+\t\t i++)\n+\t\t\tif (ce_stage(istate->cache[i]) == 2)\n+\t\t\t\tpos = i;\n+\t}\n+\tif (pos < 0)\n+\t\treturn 0;\n+\tdata = read_sha1_file(istate->cache[pos]->sha1, &type, &sz);\n+\tif (!data || type != OBJ_BLOB) {\n+\t\tfree(data);\n+\t\treturn 0;\n+\t}\n+\n+\thas_cr = memchr(data, '\\r', sz) != NULL;\n+\tfree(data);\n+\treturn has_cr;\n+}\n+\n static int crlf_to_git(const char *path, const char *src, size_t len,\n struct strbuf *buf, int action, enum safe_crlf checksafe)\n {\n@@ -147,6 +184,10 @@ static int crlf_to_git(const char *path, const char *src, size_t len,\n \t\t\treturn 0;\n \t}\n \n+\t/* If the file in the index has any CR in it, do not convert. */\n+\tif (has_cr_in_index(path))\n+\t\treturn 0;\n+\n \tcheck_safe_crlf(path, action, &stats, checksafe);\n \n \t/* Optimization: No CR? Nothing to convert, regardless. */\n@@ -202,6 +243,10 @@ static int crlf_to_worktree(const char *path, const char *src, size_t len,\n \tif (stats.lf == stats.crlf)\n \t\treturn 0;\n \n+\t/* Are there ANY lines at all with CRLF? If so, ignore */\n+\tif (stats.crlf > 0)\n+\t\treturn 0;\n+\n \tif (action == CRLF_GUESS) {\n \t\t/* If we have any bare CR characters, we're not going to touch it */\n \t\tif (stats.cr != stats.crlf)\n-- \n1.7.1.1.g653e8\n"},{"id":"141423","messageId":"alpine.DEB.1.00.1005101921460.7651@pacific.mpi-cbg.de","threadId":"23769","inReplyTo":"20100510171119.GA17875@pvv.org","subject":"Re: [msysGit] [PATCH/RFC] autocrlf: Make it work also for un-normalized repositories","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2010-05-10T17:29:34Z","receivedAt":"2010-05-10T17:29:34Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Finn Arne,\n\nthis is great stuff!\n\nOn Mon, 10 May 2010, Finn Arne Gangstad wrote:\n\n> Previously, autocrlf would only work well for normalized\n> repositories. Any text files that contained CRLF in the repository\n> would cause problems, and would be modified when handled with\n> core.autocrlf set.\n> \n> Change autocrlf to not do any conversions to files that in the\n> repository already contain a CR. git with autocrlf set will never\n> create such a file, or change a LF only file to contain CRs, so the\n> (new) assumption is that if a file contains a CR, it is intentional,\n> and autocrlf should not change that.\n> \n> The following sequence should now always be a NOP even with autocrlf\n> set (assuming a clean working directory):\n> \n> git checkout \n> touch *\n> git add -A . (will add nothing)\n> git comit (nothing to commit)\n\ns/comit/commit/\n\n> Previously this would break for any text file containing a CR\n> \n> Signed-off-by: Finn Arne Gangstad \n> ---\n> \n> Some of you may have been folowing Eyvind's excellent thread about\n> trying to make end-of-line translation in git a bit smoother.\n> \n> I decided to attack the problem from a different angle: Is it possible\n> to make autocrlf behave non-destructively for all the previous problem cases?\n> \n> Stealing the problem from Eyvind's initial mail (paraphrased and\n> summarized a bit):\n> \n> 1. Setting autocrlf globally is a pain since autocrlf does not work well\n> with CRLF in the repo\n> 2. Setting it in individual repos is hard since you do it \"too late\"\n> (the clone will get it wrong)\n> 3. If someone checks in a file with CRLF later, you get into problems again\n> 4. If a repository once has contained CRLF, you can't tell autocrlf\n> at which commit everything is sane again\n> 5. autocrlf does needless work if you know that all your users want\n> the same EOL style.\n> \n> I belive that this patch makes autocrlf a safe (and good) default\n> setting for Windows, and this solves problems 1-4.\n> \n> I implemented it by looking for CR charactes in the index, and\n> aborting any conversion attempt if this is found. The code to read\n> the index contents was copied pretty verbatim from attr.c, and should\n> probably be made into a non-static function instead if there is no\n> better way of doing this.\n\nOne technical question, see below.\n\n> Note that ALL the tests still pass unmodified. This is a bit\n> surprising perhaps, but think it is an indication that no one ever\n> intented autocrlf to do what it does to files containing CRs.\n\nIndeed. But a test of its own would be nice, no? If you do not have time, \nI will come up with one.\n\nBTW all this technical description after the \"---\" should probably go into \nthe commit message.\n\n> diff --git a/convert.c b/convert.c\n> index 4f8fcb7..9d062c8 100644\n> --- a/convert.c\n> +++ b/convert.c\n> @@ -120,6 +120,43 @@ static void check_safe_crlf(const char *path, int action,\n> \t}\n> }\n> \n> +static int has_cr_in_index(const char *path)\n> +{\n> +\tint pos, len;\n> +\tunsigned long sz;\n> +\tenum object_type type;\n> +\tvoid *data;\n> +\tint has_cr;\n> +\tstruct index_state *istate = &the_index;\n> +\n> +\tlen = strlen(path);\n> +\tpos = index_name_pos(istate, path, len);\n> +\tif (pos < 0) {\n> +\t\t/*\n> +\t\t * We might be in the middle of a merge, in which\n> +\t\t * case we would read stage #2 (ours).\n> +\t\t */\n> +\t\tint i;\n> +\t\tfor (i = -pos - 1;\n> +\t\t (pos < 0 && i < istate->cache_nr &&\n> +\t\t !strcmp(istate->cache[i]->name, path));\n> +\t\t i++)\n> +\t\t\tif (ce_stage(istate->cache[i]) == 2)\n> +\t\t\t\tpos = i;\n> +\t}\n\nI think it makes sense to assume that \"ours\" determines whether we should \nassume that the index has a wrong format. But if there is also a \"base\" \nthat disagrees on CR-ness with \"ours\", should we not try to pick \"ours\"?\n\nCiao,\nJohannes\n"},{"id":"141429","messageId":"7v1vdj8ues.fsf@alter.siamese.dyndns.org","threadId":"23769","inReplyTo":"alpine.DEB.1.00.1005101921460.7651@pacific.mpi-cbg.de","subject":"Re: [msysGit] [PATCH/RFC] autocrlf: Make it work also for un-normalized repositories","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-05-10T18:48:43Z","receivedAt":"2010-05-10T18:48:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n>> Note that ALL the tests still pass unmodified. This is a bit\n>> surprising perhaps, but think it is an indication that no one ever\n>> intented autocrlf to do what it does to files containing CRs.\n>\n> Indeed. But a test of its own would be nice, no? If you do not have time, \n> I will come up with one.\n\nThanks for a review. This is a good stuff.\n"},{"id":"141430","messageId":"99999847-7CFD-4F44-94BE-35AAE8CC9835@gmail.com","threadId":"23769","inReplyTo":"20100510171119.GA17875@pvv.org","subject":"Re: [PATCH/RFC] autocrlf: Make it work also for un-normalized repositories","fromName":"Eyvind Bernhardsen","fromEmail":"eyvind.bernhardsen@gmail.com","sentAt":"2010-05-10T19:09:48Z","receivedAt":"2010-05-10T19:09:48Z","isPatch":true,"sender":{"key":"eyvind.bernhardsen@gmail.com","avatar":"https://avatars.githubusercontent.com/u/106762?v=4"},"body":"On 10. mai 2010, at 19.11, Finn Arne Gangstad wrote:\n\n> Previously, autocrlf would only work well for normalized\n> repositories. Any text files that contained CRLF in the repository\n> would cause problems, and would be modified when handled with\n> core.autocrlf set.\n> \n> Change autocrlf to not do any conversions to files that in the\n> repository already contain a CR. git with autocrlf set will never\n> create such a file, or change a LF only file to contain CRs, so the\n> (new) assumption is that if a file contains a CR, it is intentional,\n> and autocrlf should not change that.\n\nI'm of two minds about this: on the one hand, it appears to fix autocrlf's biggest problem (that it breaks down when the repository is not normalized), which was the main reason I started working on it in the first place. On the other hand, it does nothing for the user interface, which was (obviously :) another big motivator.\n\nI'll submit a cleaned-up series with optional extras in a couple of days.\n-- \nEyvind\n"},{"id":"141432","messageId":"20100510194321.GG14069@dpotapov.dyndns.org","threadId":"23769","inReplyTo":"20100510171119.GA17875@pvv.org","subject":"Re: [PATCH/RFC] autocrlf: Make it work also for un-normalized repositories","fromName":"Dmitry Potapov","fromEmail":"dpotapov@gmail.com","sentAt":"2010-05-10T19:43:21Z","receivedAt":"2010-05-10T19:43:21Z","isPatch":true,"sender":{"key":"dpotapov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6568595?v=4"},"body":"On Mon, May 10, 2010 at 07:11:19PM +0200, Finn Arne Gangstad wrote:\n\n> \n> Stealing the problem from Eyvind's initial mail (paraphrased and\n> summarized a bit):\n> \n> 1. Setting autocrlf globally is a pain since autocrlf does not work well\n> with CRLF in the repo\n> 2. Setting it in individual repos is hard since you do it \"too late\"\n> (the clone will get it wrong)\n> 3. If someone checks in a file with CRLF later, you get into problems again\n> 4. If a repository once has contained CRLF, you can't tell autocrlf\n> at which commit everything is sane again\n> 5. autocrlf does needless work if you know that all your users want\n> the same EOL style.\n> \n> I belive that this patch makes autocrlf a safe (and good) default\n> setting for Windows, and this solves problems 1-4.\n\nIt does not really solve #2, because you will have the wrong ending for\nfiles that must be LF, such as shell scripts, and then these scripts\nfail with some weird error...\n\n> \n> I implemented it by looking for CR charactes in the index, and\n> aborting any conversion attempt if this is found.\n\nDoes it have any measurable impact on the check-in when a lot of files\nare committed? \n\n> \n> Note that ALL the tests still pass unmodified. This is a bit\n> surprising perhaps, but think it is an indication that no one ever\n> intented autocrlf to do what it does to files containing CRs.\n\nWell, tests do not cover many corner cases... So, no surprise here...\n\n> @@ -147,6 +184,10 @@ static int crlf_to_git(const char *path, const char *src, size_t len,\n> return 0;\n> }\n>\n> + /* If the file in the index has any CR in it, do not convert. */\n> + if (has_cr_in_index(path))\n> + return 0;\n> +\n\nWhy do you disable crlf conversion not only for \"guess\" case but also\nfor those files that have the \"crlf\" attribute? Moreover, you do that\nsilently without even a warning to the user. IMHO, it is incompatible\nchange. In fact, it can seen as regression, because by specifying the\ncorrect attribute for that file, I could fix the ending of this file.\nNow, this is impossible.\n\n> \n> \t/* Optimization: No CR? Nothing to convert, regardless. */\n> @@ -202,6 +243,10 @@ static int crlf_to_worktree(const char *path, const char *src, size_t len,\n> \tif (stats.lf == stats.crlf)\n> \t\treturn 0;\n> \n> +\t/* Are there ANY lines at all with CRLF? If so, ignore */\n> +\tif (stats.crlf > 0)\n> +\t\treturn 0;\n> +\n> \tif (action == CRLF_GUESS) {\n> \t\t/* If we have any bare CR characters, we're not going to touch it */\n> \t\tif (stats.cr != stats.crlf)\n\nThis chunk does not make sense. Can you explain what did you try to\nachieve here for guess and non-guess cases?\n\nIMHO, we should conservative in our changes. So, to change behavior of\nautocrlf only where \"crlf\" is not set explicitly. Or, at least, produce\nsome warning about discrepancy between what the user has _explicitly_\ntold to do and what git does. I really dislike that any tool silently\nignores users explicit instructions, because it thinks it knows better\nthan a human. It is plainly wrong.\n\n\nDmitry\n"},{"id":"141434","messageId":"m37hnbec16.fsf@localhost.localdomain","threadId":"23769","inReplyTo":"20100510171119.GA17875@pvv.org","subject":"Re: [PATCH/RFC] autocrlf: Make it work also for un-normalized repositories","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2010-05-10T20:30:22Z","receivedAt":"2010-05-10T20:30:22Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Finn Arne Gangstad writes:\n\n> Previously, autocrlf would only work well for normalized\n> repositories. Any text files that contained CRLF in the repository\n> would cause problems, and would be modified when handled with\n> core.autocrlf set.\n> \n> Change autocrlf to not do any conversions to files that in the\n> repository already contain a CR. git with autocrlf set will never\n> create such a file, or change a LF only file to contain CRs, so the\n> (new) assumption is that if a file contains a CR, it is intentional,\n> and autocrlf should not change that.\n> \n> The following sequence should now always be a NOP even with autocrlf\n> set (assuming a clean working directory):\n> \n> git checkout \n> touch *\n> git add -A . (will add nothing)\n> git comit (nothing to commit)\n> \n> Previously this would break for any text file containing a CR\n\nHow this feature relates to `core.safecrfl'?\n\n-- \nJakub Narebski\nPoland\nShadeHawk on #git\n"},{"id":"141437","messageId":"20100510211739.GI14069@dpotapov.dyndns.org","threadId":"23769","inReplyTo":"m37hnbec16.fsf@localhost.localdomain","subject":"Re: [PATCH/RFC] autocrlf: Make it work also for un-normalized repositories","fromName":"Dmitry Potapov","fromEmail":"dpotapov@gmail.com","sentAt":"2010-05-10T21:17:39Z","receivedAt":"2010-05-10T21:17:39Z","isPatch":true,"sender":{"key":"dpotapov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6568595?v=4"},"body":"On Mon, May 10, 2010 at 01:30:22PM -0700, Jakub Narebski wrote:\n> Finn Arne Gangstad writes:\n> > \n> > The following sequence should now always be a NOP even with autocrlf\n> > set (assuming a clean working directory):\n> > \n> > git checkout \n> > touch *\n> > git add -A . (will add nothing)\n> > git comit (nothing to commit)\n> > \n> > Previously this would break for any text file containing a CR\n> \n> How this feature relates to `core.safecrfl'?\n\nsafecrlf is about making sure that you will get back exactly same file\non the next checkout as you have now in your working directory unless\nyou change your autocrlf value. So, the statement about breaking any\nfile with CR is certainly not correct.\n\nThis feature is about preserving CRLF in files inside of the repository\nthat were committed with CRLF despite that your current settings that\nsuggests that those files should be committed with LF. This makes sense\nfor the \"guess\" case, but it is clearly wrong when the user explicitly\nmandated CRLF conversion for that file through attributes.\n\n\nDmitry\n"},{"id":"141485","messageId":"13E8F544-9351-4B1F-9E4B-B625D20E2A8C@gmail.com","threadId":"23769","inReplyTo":"20100510194321.GG14069@dpotapov.dyndns.org","subject":"Re: [PATCH/RFC] autocrlf: Make it work also for un-normalized repositories","fromName":"Eyvind Bernhardsen","fromEmail":"eyvind.bernhardsen@gmail.com","sentAt":"2010-05-11T16:31:38Z","receivedAt":"2010-05-11T16:31:38Z","isPatch":true,"sender":{"key":"eyvind.bernhardsen@gmail.com","avatar":"https://avatars.githubusercontent.com/u/106762?v=4"},"body":"On 10. mai 2010, at 21.43, Dmitry Potapov wrote:\n\n> It does not really solve #2, because you will have the wrong ending for\n> files that must be LF, such as shell scripts, and then these scripts\n> fail with some weird error...\n\nThat's okay, because I'll solve that in my next iteration with \"crlf=crlf\" and \"crlf=lf\" (yes, I know). Finn Arne's patch is about fixing the current broken behaviour when autocrlf is enabled in a repository containing CRLFs, while mine's the one you need if you want normalized text files in your repository.\n-- \nEyvind\n"},{"id":"141491","messageId":"20100511222802.GA16974@pvv.org","threadId":"23769","inReplyTo":"alpine.DEB.1.00.1005101921460.7651@pacific.mpi-cbg.de","subject":"Re: [msysGit] [PATCH/RFC] autocrlf: Make it work also for un-normalized repositories","fromName":"Finn Arne Gangstad","fromEmail":"finnag@pvv.org","sentAt":"2010-05-11T22:28:02Z","receivedAt":"2010-05-11T22:28:02Z","isPatch":true,"sender":{"key":"finnag@pvv.org","avatar":"https://gravatar.com/avatar/b421ddd58c3f0f93aa473e17b98bb8d53c221fef741746bc8cb59fae4ec6d95e?d=mp&s=160"},"body":"On Mon, May 10, 2010 at 07:29:34PM +0200, Johannes Schindelin wrote:\n\n> > +\t\t\tif (ce_stage(istate->cache[i]) == 2)\n> > +\t\t\t\tpos = i;\n> > +\t}\n> \n> I think it makes sense to assume that \"ours\" determines whether we should \n> assume that the index has a wrong format. But if there is also a \"base\" \n> that disagrees on CR-ness with \"ours\", should we not try to pick \"ours\"?\n\nDid you make a typo there, or did I misunderstand something? As far as\nI can tell we do pick \"ours\" in this case, and I think that may be the\nbest choice overall (it is easiest for you to fix \"ours\" if the\nresult is wrong).\n\n- Finn Arne\n"},{"id":"141493","messageId":"20100511225202.GC16974@pvv.org","threadId":"23769","inReplyTo":"m37hnbec16.fsf@localhost.localdomain","subject":"Re: [PATCH/RFC] autocrlf: Make it work also for un-normalized repositories","fromName":"Finn Arne Gangstad","fromEmail":"finnag@pvv.org","sentAt":"2010-05-11T22:52:03Z","receivedAt":"2010-05-11T22:52:03Z","isPatch":true,"sender":{"key":"finnag@pvv.org","avatar":"https://gravatar.com/avatar/b421ddd58c3f0f93aa473e17b98bb8d53c221fef741746bc8cb59fae4ec6d95e?d=mp&s=160"},"body":"On Mon, May 10, 2010 at 01:30:22PM -0700, Jakub Narebski wrote:\n> Finn Arne Gangstad writes:\n> \n> How this feature relates to `core.safecrfl'?\n\nsafecrlf is about checking what you add, it does not concern itself\nabout what is already in the repository as far as I can tell.\n\nThe safecrlf check will only be done if autocrlf converts a file. For\nnew files autocrlf is unchanged, and safecrlf will complain or die as\nbefore. For existing files, you will only be able to get autocrlf to\ndo anything (and thus do the safecrlf check) if the files are\nnormalized to LF only in the repo, or if you have set the crlf\nattribute on them.\n\n- Finn Arne\n"}]}