{"thread":{"id":"47023","subject":"Consequences of CRLF in index?","startedAt":"2017-10-24T17:48:51Z","lastAt":"2017-11-15T04:28:39Z","messageCount":44,"participants":["Lars Schneider","Jonathan Nieder","Torsten Bögershausen","Johannes Sixt","Junio C Hamano","Johannes Schindelin","Stefan Beller","Ross Kabus","Jeff King"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"330929","messageId":"D0A67AD8-2D63-4683-9F2A-20B0E8E65D4B@gmail.com","threadId":"47023","inReplyTo":null,"subject":"Consequences of CRLF in index?","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2017-10-24T17:48:42Z","receivedAt":"2017-10-24T17:48:51Z","isPatch":false,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"Hi,\n\nI've migrated a large repo (110k+ files) with a lot of history (177k commits) \nand a lot of users (200+) to Git. Unfortunately, all text files in the index\nof the repo have CRLF line endings. In general this seems not to be a problem \nas the project is developed exclusively on Windows.\n\nHowever, I wonder if there are any \"hidden consequences\" of this setup?\nIf there are consequences, then I see two options. Either I rebase the repo \nand fix the line endings for all commits or I add a single commit that fixes \nthe line endings for all files. Both actions require coordination with the \nusers to avoid repo trouble/merge conflicts. The \"single fixup commit\" options \nwould also make diffs into the past look bad. Would a single large commit have\nany impact on the performance of standard Git operations?\n\nThanks,\nLars\n\n\n"},{"id":"330930","messageId":"20171024181415.3tvmc36aqi335v66@aiede.mtv.corp.google.com","threadId":"47023","inReplyTo":"D0A67AD8-2D63-4683-9F2A-20B0E8E65D4B@gmail.com","subject":"Re: Consequences of CRLF in index?","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-10-24T18:14:15Z","receivedAt":"2017-10-24T18:14:25Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nLars Schneider wrote:\n\n> I've migrated a large repo (110k+ files) with a lot of history (177k commits)\n> and a lot of users (200+) to Git. Unfortunately, all text files in the index\n> of the repo have CRLF line endings. In general this seems not to be a problem\n> as the project is developed exclusively on Windows.\n\nSounds good.\n\n> However, I wonder if there are any \"hidden consequences\" of this setup?\n> If there are consequences, then I see two options. Either I rebase the repo\n> and fix the line endings for all commits or I add a single commit that fixes\n> the line endings for all files. Both actions require coordination with the\n> users to avoid repo trouble/merge conflicts. The \"single fixup commit\" options\n> would also make diffs into the past look bad. Would a single large commit have\n> any impact on the performance of standard Git operations?\n\nThere are no hidden consequences that I'm aware of. If you later\ndecide that you want to become a cross-platform project, then you may\nwant to switch to LF endings, in which case I suggest the \"single\nfixup commit\" strategy.\n\nIn any event, you also probably want to declare what you're doing\nusing .gitattributes. By checking in the files as CRLF, you are\ndeclaring that you do *not* want Git to treat them as text files\n(i.e., you do not want Git to change the line endings), so something\nas simple as\n\n\t* -text\n\nshould do the trick. See gitattributes(5) for details.\n\nI'd be interested to hear what happens when diff-ing across a line\nending fixup commit. Is this an area where Git needs some\nimprovement? \"git merge\" knows an -Xrenormalize option to deal with a\nrelated problem --- it's possible that \"git diff\" needs to learn a\nsimilar trick.\n\nThanks and hope that helps,\nJonathan\n"},{"id":"330954","messageId":"20171024190234.GA21077@tor.lan","threadId":"47023","inReplyTo":"20171024181415.3tvmc36aqi335v66@aiede.mtv.corp.google.com","subject":"Re: Consequences of CRLF in index?","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2017-10-24T19:02:34Z","receivedAt":"2017-10-24T19:02:49Z","isPatch":false,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On Tue, Oct 24, 2017 at 11:14:15AM -0700, Jonathan Nieder wrote:\n> Hi,\n> \n> Lars Schneider wrote:\n> \n> > I've migrated a large repo (110k+ files) with a lot of history (177k commits)\n> > and a lot of users (200+) to Git. Unfortunately, all text files in the index\n> > of the repo have CRLF line endings. In general this seems not to be a problem\n> > as the project is developed exclusively on Windows.\n> \n> Sounds good.\n> \n> > However, I wonder if there are any \"hidden consequences\" of this setup?\n> > If there are consequences, then I see two options. Either I rebase the repo\n> > and fix the line endings for all commits or I add a single commit that fixes\n> > the line endings for all files. Both actions require coordination with the\n> > users to avoid repo trouble/merge conflicts. The \"single fixup commit\" options\n> > would also make diffs into the past look bad. Would a single large commit have\n> > any impact on the performance of standard Git operations?\n> \n> There are no hidden consequences that I'm aware of. If you later\n> decide that you want to become a cross-platform project, then you may\n> want to switch to LF endings, in which case I suggest the \"single\n> fixup commit\" strategy.\n> \n> In any event, you also probably want to declare what you're doing\n> using .gitattributes. By checking in the files as CRLF, you are\n> declaring that you do *not* want Git to treat them as text files\n> (i.e., you do not want Git to change the line endings), so something\n> as simple as\n> \n> \t* -text\n> \n> should do the trick. See gitattributes(5) for details.\n\nExactly.\nThe \"hidden consequence\" may be the other way around:\nIf you don't specify .gitattributes, then all people who have core.autocrlf=true\nwill suffer from a runtime penalty.\ncore.autocrlf=true means \"auto\".\nAt each checkout Git needs to figure out that the file has CRLF in the repo,\nso that there is no conversion done.\nThe same happens on \"git add\" or \"git commit\" (and sometimes on \"git status\").\n\nThe penalty may be low, but Dscho once reported that it is measurable & painful\non a \"big repo\".\n\nThe other advantage of \"* -text\" in .gitattributes is that new files are handled\nconsistantly accross developers.\n\nThose who have \"core.autocrlf=false\" would produce commits with CRLF for new\nfiles, and those developpers who have core.autocrlf=true would produce files\nwith LF in the index and CRLF in the worktree.\nThis may (most probably will) cause confusion later, when things are pushed and\npulled.\n\n> \n> I'd be interested to hear what happens when diff-ing across a line\n> ending fixup commit. Is this an area where Git needs some\n> improvement? \"git merge\" knows an -Xrenormalize option to deal with a\n> related problem --- it's possible that \"git diff\" needs to learn a\n> similar trick.\n\nThat is a tricky thing.\nSometimes you want to see the CLRF - LF as a diff, (represented as \"^M\"),\nand sometimes not.\n\nAll in all \"* -text\" is a good choice for Windows-only projects.\n\n> \n> Thanks and hope that helps,\n> Jonathan\n"},{"id":"330968","messageId":"eda48d5e-a52c-9a39-8e55-a6828efdb3ef@kdbg.org","threadId":"47023","inReplyTo":"D0A67AD8-2D63-4683-9F2A-20B0E8E65D4B@gmail.com","subject":"Re: Consequences of CRLF in index?","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2017-10-24T21:04:07Z","receivedAt":"2017-10-24T21:04:15Z","isPatch":false,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 24.10.2017 um 19:48 schrieb Lars Schneider:\n> I've migrated a large repo (110k+ files) with a lot of history (177k commits)\n> and a lot of users (200+) to Git. Unfortunately, all text files in the index\n> of the repo have CRLF line endings. In general this seems not to be a problem\n> as the project is developed exclusively on Windows.\n> \n> However, I wonder if there are any \"hidden consequences\" of this setup?\n\nI've been working on a project with CRLF in every source file for a \ndecade now. It's C++ source, and it isn't even Windows-only: when \nchecked out on Linux, there are CRs in the files, with no bad \nconsequences so far. GCC is happy with them.\n\n-- Hannes\n"},{"id":"330980","messageId":"xmqq4lqoj8pe.fsf@gitster.mtv.corp.google.com","threadId":"47023","inReplyTo":"20171024181415.3tvmc36aqi335v66@aiede.mtv.corp.google.com","subject":"Re: Consequences of CRLF in index?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-10-25T01:51:41Z","receivedAt":"2017-10-25T01:51:48Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> I'd be interested to hear what happens when diff-ing across a line\n> ending fixup commit. Is this an area where Git needs some\n> improvement? \"git merge\" knows an -Xrenormalize option to deal with a\n> related problem --- it's possible that \"git diff\" needs to learn a\n> similar trick.\n\nMy knee-jerk reaction is that (1) the end user definitely wants to\nsee preimage and postimage lines are different in such a commit by\ndefault, one side has and the other side lacks ^M at the end., and\n(2) one of the \"whitespace ignoring\" options (namely, \"ignore space\nat eol\") may suffice, but if not, it should be easy to invent a new\none \"ignore CR at eol\".\n"},{"id":"330989","messageId":"xmqqshe7j0af.fsf@gitster.mtv.corp.google.com","threadId":"47023","inReplyTo":"xmqq4lqoj8pe.fsf@gitster.mtv.corp.google.com","subject":"Re: Consequences of CRLF in index?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-10-25T04:53:28Z","receivedAt":"2017-10-25T04:53:36Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Jonathan Nieder writes:\n>\n>> I'd be interested to hear what happens when diff-ing across a line\n>> ending fixup commit. Is this an area where Git needs some\n>> improvement? \"git merge\" knows an -Xrenormalize option to deal with a\n>> related problem --- it's possible that \"git diff\" needs to learn a\n>> similar trick.\n>\n> My knee-jerk reaction is that (1) the end user definitely wants to\n> see preimage and postimage lines are different in such a commit by\n> default, one side has and the other side lacks ^M at the end., and\n> (2) one of the \"whitespace ignoring\" options (namely, \"ignore space\n> at eol\") may suffice, but if not, it should be easy to invent a new\n> one \"ignore CR at eol\".\n\nHere is a lunch-time hack to add that option. As you can see from\nthe lack of docs, tests and a proper log message, I haven't played\nwith it long enough to know how buggy it is, though. I am not all\nthat interested in polishing it to completion myself and prefer to\nleave it as #leftoverbits ;-)\n\nAlso I didn't bother teaching this to Stefan's color-moved thing, as\nthe line comparator it uses will hopefully be unified with the one I\nam touching in xdiff/ with this patch.\n\nHave fun.\n\n diff.c | 5 ++++-\n merge-recursive.c | 2 ++\n xdiff/xdiff.h | 3 ++-\n xdiff/xutils.c | 34 ++++++++++++++++++++++++++++++++--\n 4 files changed, 40 insertions(+), 4 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 6fd288420b..eeca0fd3b8 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -4202,7 +4202,8 @@ void diff_setup_done(struct diff_options *options)\n \n \tif (DIFF_XDL_TST(options, IGNORE_WHITESPACE) ||\n \t DIFF_XDL_TST(options, IGNORE_WHITESPACE_CHANGE) ||\n-\t DIFF_XDL_TST(options, IGNORE_WHITESPACE_AT_EOL))\n+\t DIFF_XDL_TST(options, IGNORE_WHITESPACE_AT_EOL) ||\n+\t DIFF_XDL_TST(options, IGNORE_CR_AT_EOL))\n \t\tDIFF_OPT_SET(options, DIFF_FROM_CONTENTS);\n \telse\n \t\tDIFF_OPT_CLR(options, DIFF_FROM_CONTENTS);\n@@ -4659,6 +4660,8 @@ int diff_opt_parse(struct diff_options *options,\n \t\tDIFF_XDL_SET(options, IGNORE_WHITESPACE_CHANGE);\n \telse if (!strcmp(arg, \"--ignore-space-at-eol\"))\n \t\tDIFF_XDL_SET(options, IGNORE_WHITESPACE_AT_EOL);\n+\telse if (!strcmp(arg, \"--ignore-cr-at-eol\"))\n+\t\tDIFF_XDL_SET(options, IGNORE_CR_AT_EOL);\n \telse if (!strcmp(arg, \"--ignore-blank-lines\"))\n \t\tDIFF_XDL_SET(options, IGNORE_BLANK_LINES);\n \telse if (!strcmp(arg, \"--indent-heuristic\"))\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex 1d3f8f0d22..4a49ec93b1 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -2251,6 +2251,8 @@ int parse_merge_opt(struct merge_options *o, const char *s)\n \t\tDIFF_XDL_SET(o, IGNORE_WHITESPACE);\n \telse if (!strcmp(s, \"ignore-space-at-eol\"))\n \t\tDIFF_XDL_SET(o, IGNORE_WHITESPACE_AT_EOL);\n+\telse if (!strcmp(s, \"ignore-cr-at-eol\"))\n+\t\tDIFF_XDL_SET(o, IGNORE_CR_AT_EOL);\n \telse if (!strcmp(s, \"renormalize\"))\n \t\to->renormalize = 1;\n \telse if (!strcmp(s, \"no-renormalize\"))\ndiff --git a/xdiff/xdiff.h b/xdiff/xdiff.h\nindex b090ad8eac..ff16243da9 100644\n--- a/xdiff/xdiff.h\n+++ b/xdiff/xdiff.h\n@@ -32,7 +32,8 @@ extern \"C\" {\n #define XDF_IGNORE_WHITESPACE (1 << 2)\n #define XDF_IGNORE_WHITESPACE_CHANGE (1 << 3)\n #define XDF_IGNORE_WHITESPACE_AT_EOL (1 << 4)\n-#define XDF_WHITESPACE_FLAGS (XDF_IGNORE_WHITESPACE | XDF_IGNORE_WHITESPACE_CHANGE | XDF_IGNORE_WHITESPACE_AT_EOL)\n+#define XDF_IGNORE_CR_AT_EOL (1 << 5)\n+#define XDF_WHITESPACE_FLAGS (XDF_IGNORE_WHITESPACE | XDF_IGNORE_WHITESPACE_CHANGE | XDF_IGNORE_WHITESPACE_AT_EOL | XDF_IGNORE_CR_AT_EOL)\n \n #define XDF_PATIENCE_DIFF (1 << 5)\n #define XDF_HISTOGRAM_DIFF (1 << 6)\ndiff --git a/xdiff/xutils.c b/xdiff/xutils.c\nindex 04d7b32e4e..8720e272b9 100644\n--- a/xdiff/xutils.c\n+++ b/xdiff/xutils.c\n@@ -156,6 +156,21 @@ int xdl_blankline(const char *line, long size, long flags)\n \treturn (i == size);\n }\n \n+/*\n+ * Have we eaten everything on the line, except for an optional\n+ * CR at the very end?\n+ */\n+static int ends_with_optional_cr(const char *l, long s, long i)\n+{\n+\tif (s && l[s-1] == '\\n')\n+\t\ts--;\n+\tif (s == i)\n+\t\treturn 1;\n+\tif (s == i + 1 && l[i] == '\\r')\n+\t\treturn 1;\n+\treturn 0;\n+}\n+\n int xdl_recmatch(const char *l1, long s1, const char *l2, long s2, long flags)\n {\n \tint i1, i2;\n@@ -170,7 +185,8 @@ int xdl_recmatch(const char *l1, long s1, const char *l2, long s2, long flags)\n \n \t/*\n \t * -w matches everything that matches with -b, and -b in turn\n-\t * matches everything that matches with --ignore-space-at-eol.\n+\t * matches everything that matches with --ignore-space-at-eol,\n+\t * which in turn matches everything that matches with --ignore-cr-at-eol.\n \t *\n \t * Each flavor of ignoring needs different logic to skip whitespaces\n \t * while we have both sides to compare.\n@@ -204,6 +220,14 @@ int xdl_recmatch(const char *l1, long s1, const char *l2, long s2, long flags)\n \t\t\ti1++;\n \t\t\ti2++;\n \t\t}\n+\t} else if (flags & XDF_IGNORE_CR_AT_EOL) {\n+\t\t/* Find the first difference and see how the line ends */\n+\t\twhile (i1 < s1 && i2 < s2 && l1[i1] == l2[i2]) {\n+\t\t\ti1++;\n+\t\t\ti2++;\n+\t\t}\n+\t\treturn (ends_with_optional_cr(l1, s1, i1) &&\n+\t\t\tends_with_optional_cr(l2, s2, i2));\n \t}\n \n \t/*\n@@ -230,9 +254,15 @@ static unsigned long xdl_hash_record_with_whitespace(char const **data,\n \t\tchar const *top, long flags) {\n \tunsigned long ha = 5381;\n \tchar const *ptr = *data;\n+\tint cr_at_eol_only = (flags & XDF_WHITESPACE_FLAGS) == XDF_IGNORE_CR_AT_EOL;\n \n \tfor (; ptr < top && *ptr != '\\n'; ptr++) {\n-\t\tif (XDL_ISSPACE(*ptr)) {\n+\t\tif (cr_at_eol_only) {\n+\t\t\tif (*ptr == '\\r' &&\n+\t\t\t (top <= ptr + 1 || ptr[1] == '\\n'))\n+\t\t\t\tcontinue;\n+\t\t}\n+\t\telse if (XDL_ISSPACE(*ptr)) {\n \t\t\tconst char *ptr2 = ptr;\n \t\t\tint at_eol;\n \t\t\twhile (ptr + 1 < top && XDL_ISSPACE(ptr[1])\n\n\n"},{"id":"331012","messageId":"alpine.DEB.2.21.1.1710251412100.6482@virtualbox","threadId":"47023","inReplyTo":"20171024190234.GA21077@tor.lan","subject":"Re: Consequences of CRLF in index?","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-10-25T12:13:54Z","receivedAt":"2017-10-25T12:14:08Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Tue, 24 Oct 2017, Torsten Bögershausen wrote:\n\n> The penalty may be low, but Dscho once reported that it [line endings\n> conversion] is measurable & painful on a \"big repo\".\n\nYes, I do not remember the details, but it must have been around 5-15%\nspeed improvement to prevent the autoCRLF stuff from doing its thing.\n\nAt work, we always switch it off, for that reason.\n\nCiao,\nDscho"},{"id":"331013","messageId":"alpine.DEB.2.21.1.1710251415040.6482@virtualbox","threadId":"47023","inReplyTo":"eda48d5e-a52c-9a39-8e55-a6828efdb3ef@kdbg.org","subject":"Re: Consequences of CRLF in index?","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-10-25T12:19:59Z","receivedAt":"2017-10-25T12:20:12Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Hannes,\n\nOn Tue, 24 Oct 2017, Johannes Sixt wrote:\n\n> Am 24.10.2017 um 19:48 schrieb Lars Schneider:\n> > I've migrated a large repo (110k+ files) with a lot of history (177k\n> > commits)\n> > and a lot of users (200+) to Git. Unfortunately, all text files in the index\n> > of the repo have CRLF line endings. In general this seems not to be a\n> > problem\n> > as the project is developed exclusively on Windows.\n> > \n> > However, I wonder if there are any \"hidden consequences\" of this setup?\n> \n> I've been working on a project with CRLF in every source file for a decade\n> now. It's C++ source, and it isn't even Windows-only: when checked out on\n> Linux, there are CRs in the files, with no bad consequences so far. GCC is\n> happy with them.\n\nI envy you for the blessing of such a clean C++ source that you do not\nhave any, say, Unix shell script in it. Try this, and weep:\n\n\t$ printf 'echo \\\\\\r\\n\\t123\\r\\n' >a1\n\n\t$ sh a1\n\n\ta1: 2: a1: 123: not found\n\nFor the same reason (Unix shell not handling CR/LF gracefull), I went\nthrough that painful work that finally landed as 00ddc9d13ca (Fix build\nwith core.autocrlf=true, 2017-05-09).\n\nCiao,\nDscho\n"},{"id":"331024","messageId":"CAGZ79kbgjgOhQuK0-zF7oTvuM_gWKF25+uAf-hsHK2Xg4yshhA@mail.gmail.com","threadId":"47023","inReplyTo":"xmqqshe7j0af.fsf@gitster.mtv.corp.google.com","subject":"Re: Consequences of CRLF in index?","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-10-25T16:44:32Z","receivedAt":"2017-10-25T16:44:44Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Oct 24, 2017 at 9:53 PM, Junio C Hamano wrote:\n>\n> Here is a lunch-time hack to add that option. As you can see from\n> the lack of docs, tests and a proper log message, I haven't played\n> with it long enough to know how buggy it is, though. I am not all\n> that interested in polishing it to completion myself and prefer to\n> leave it as #leftoverbits ;-)\n\nOk, nevertheless a review pointing out a couple things would be\nuseful for those who pick it up later, I assume.\n\n> Also I didn't bother teaching this to Stefan's color-moved thing, as\n> the line comparator it uses will hopefully be unified with the one I\n> am touching in xdiff/ with this patch.\n\nwhich will be rerolled shortly fixing just the parameter names as Eric\nmentioned.\n\n> diff.c | 5 ++++-\n> merge-recursive.c | 2 ++\n> xdiff/xdiff.h | 3 ++-\n> xdiff/xutils.c | 34 ++++++++++++++++++++++++++++++++--\n> 4 files changed, 40 insertions(+), 4 deletions(-)\n>\n> diff --git a/diff.c b/diff.c\n> index 6fd288420b..eeca0fd3b8 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -4202,7 +4202,8 @@ void diff_setup_done(struct diff_options *options)\n>\n> if (DIFF_XDL_TST(options, IGNORE_WHITESPACE) ||\n> DIFF_XDL_TST(options, IGNORE_WHITESPACE_CHANGE) ||\n> - DIFF_XDL_TST(options, IGNORE_WHITESPACE_AT_EOL))\n> + DIFF_XDL_TST(options, IGNORE_WHITESPACE_AT_EOL) ||\n> + DIFF_XDL_TST(options, IGNORE_CR_AT_EOL))\n\nThis highlights another part of the flag macros, that could be made nicer.\nAll these flags combined are XDF_WHITESPACE_FLAGS, so this\nif could be written without the macros as\n\n if (options->xdl_ops & XDF_WHITESPACE_FLAGS)\n\nwhich we might want to hide in a macro\n\n DIFF_XDL_MASK_TST(options, mask)\n\nor such?\n\n\n> #define XDF_IGNORE_WHITESPACE_AT_EOL (1 << 4)\n> -#define XDF_WHITESPACE_FLAGS (XDF_IGNORE_WHITESPACE | XDF_IGNORE_WHITESPACE_CHANGE | XDF_IGNORE_WHITESPACE_AT_EOL)\n> +#define XDF_IGNORE_CR_AT_EOL (1 << 5)\n> +#define XDF_WHITESPACE_FLAGS (XDF_IGNORE_WHITESPACE | XDF_IGNORE_WHITESPACE_CHANGE | XDF_IGNORE_WHITESPACE_AT_EOL | XDF_IGNORE_CR_AT_EOL)\n>\n> #define XDF_PATIENCE_DIFF (1 << 5)\n\n(1<<5) is taken twice now. Currently there is only one\nunused free bit (but that was used once upon a time);\nso we have to think how we revamp the flag system to\nsupport more than 32 bits.\n\nSee also the answers to\nhttps://public-inbox.org/git/20171024000931.14814-1-sbeller@google.com/\nas that started this discussion already.\n\n> #define XDF_HISTOGRAM_DIFF (1 << 6)\n> diff --git a/xdiff/xutils.c b/xdiff/xutils.c\n> index 04d7b32e4e..8720e272b9 100644\n> --- a/xdiff/xutils.c\n> +++ b/xdiff/xutils.c\n> @@ -156,6 +156,21 @@ int xdl_blankline(const char *line, long size, long flags)\n> return (i == size);\n> }\n>\n> +/*\n> + * Have we eaten everything on the line, except for an optional\n> + * CR at the very end?\n> + */\n> +static int ends_with_optional_cr(const char *l, long s, long i)\n> +{\n> + if (s && l[s-1] == '\\n')\n> + s--;\n\nso first we cut off the '\\n',\n\n> + if (s == i)\n> + return 1;\n\nthen we either have an ending without\n\n> + if (s == i + 1 && l[i] == '\\r')\n> + return 1;\n\nor with a '\\r' before.\n\nThat seems correct after some thought; I might offer\nanother easier to understand (for me) solution,\nwhich is\n{\n /* cut of ending LF */\n if (s && l[s-1] == '\\n')\n s--;\n /* do we only need to cut LF? */\n if (i == s)\n return 1;\n\n /* cut of ending CR */\n if (s && l[s-1] == '\\r')\n s--;\n /* was this everything to cut? */\n return i == s\n}\n\nThough this seems even more complicated\nafter having it written down.\n\n> * Each flavor of ignoring needs different logic to skip whitespaces\n> * while we have both sides to compare.\n> @@ -204,6 +220,14 @@ int xdl_recmatch(const char *l1, long s1, const char *l2, long s2, long flags)\n> i1++;\n> i2++;\n> }\n> + } else if (flags & XDF_IGNORE_CR_AT_EOL) {\n> + /* Find the first difference and see how the line ends */\n> + while (i1 < s1 && i2 < s2 && l1[i1] == l2[i2]) {\n> + i1++;\n> + i2++;\n> + }\n> + return (ends_with_optional_cr(l1, s1, i1) &&\n> + ends_with_optional_cr(l2, s2, i2));\n> }\n>\n> /*\n> @@ -230,9 +254,15 @@ static unsigned long xdl_hash_record_with_whitespace(char const **data,\n> char const *top, long flags) {\n> unsigned long ha = 5381;\n> char const *ptr = *data;\n> + int cr_at_eol_only = (flags & XDF_WHITESPACE_FLAGS) == XDF_IGNORE_CR_AT_EOL;\n>\n> for (; ptr < top && *ptr != '\\n'; ptr++) {\n> - if (XDL_ISSPACE(*ptr)) {\n> + if (cr_at_eol_only) {\n> + if (*ptr == '\\r' &&\n> + (top <= ptr + 1 || ptr[1] == '\\n'))\n> + continue;\n> + }\n> + else if (XDL_ISSPACE(*ptr)) {\n> const char *ptr2 = ptr;\n> int at_eol;\n> while (ptr + 1 < top && XDL_ISSPACE(ptr[1])\n>\n>\n"},{"id":"331025","messageId":"07309D89-F536-4DA8-9214-B605007845C6@gmail.com","threadId":"47023","inReplyTo":"20171024181415.3tvmc36aqi335v66@aiede.mtv.corp.google.com","subject":"Re: Consequences of CRLF in index?","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2017-10-25T17:04:23Z","receivedAt":"2017-10-25T17:04:31Z","isPatch":false,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\n> On 24 Oct 2017, at 20:14, Jonathan Nieder wrote:\n> \n> Hi,\n> \n> Lars Schneider wrote:\n> \n>> I've migrated a large repo (110k+ files) with a lot of history (177k commits)\n>> and a lot of users (200+) to Git. Unfortunately, all text files in the index\n>> of the repo have CRLF line endings. In general this seems not to be a problem\n>> as the project is developed exclusively on Windows.\n> \n> Sounds good.\n> \n>> However, I wonder if there are any \"hidden consequences\" of this setup?\n>> If there are consequences, then I see two options. Either I rebase the repo\n>> and fix the line endings for all commits or I add a single commit that fixes\n>> the line endings for all files. Both actions require coordination with the\n>> users to avoid repo trouble/merge conflicts. The \"single fixup commit\" options\n>> would also make diffs into the past look bad. Would a single large commit have\n>> any impact on the performance of standard Git operations?\n> \n> There are no hidden consequences that I'm aware of. If you later\n> decide that you want to become a cross-platform project, then you may\n> want to switch to LF endings, in which case I suggest the \"single\n> fixup commit\" strategy.\n> \n> In any event, you also probably want to declare what you're doing\n> using .gitattributes. By checking in the files as CRLF, you are\n> declaring that you do *not* want Git to treat them as text files\n> (i.e., you do not want Git to change the line endings), so something\n> as simple as\n> \n> \t* -text\n\nThat's sounds good. Does \"-text\" have any other implications?\nFor whatever reason I always thought this is the way to tell\nGit that a particular file is binary with the implication that\nGit should not attempt to diff it.\n\nThanks,\nLars"},{"id":"331026","messageId":"20171025171357.4nk4aj2c4m4b6kl3@aiede.mtv.corp.google.com","threadId":"47023","inReplyTo":"07309D89-F536-4DA8-9214-B605007845C6@gmail.com","subject":"Re: Consequences of CRLF in index?","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-10-25T17:13:57Z","receivedAt":"2017-10-25T17:14:08Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi again,\n\nLars Schneider wrote:\n>> On 24 Oct 2017, at 20:14, Jonathan Nieder wrote:\n\n>> In any event, you also probably want to declare what you're doing\n>> using .gitattributes. By checking in the files as CRLF, you are\n>> declaring that you do *not* want Git to treat them as text files\n>> (i.e., you do not want Git to change the line endings), so something\n>> as simple as\n>>\n>> \t* -text\n>\n> That's sounds good. Does \"-text\" have any other implications?\n> For whatever reason I always thought this is the way to tell\n> Git that a particular file is binary with the implication that\n> Git should not attempt to diff it.\n\nNo other implications. You're thinking of \"-diff\". There is also a\nshortcut \"binary\" which simply means \"-text -diff\".\n\nIdeas for wording improvements to gitattributes(5) on this subject?\n\nThanks,\nJonathan\n"},{"id":"331066","messageId":"xmqqy3nye9n8.fsf@gitster.mtv.corp.google.com","threadId":"47023","inReplyTo":"CAGZ79kbgjgOhQuK0-zF7oTvuM_gWKF25+uAf-hsHK2Xg4yshhA@mail.gmail.com","subject":"Re: Consequences of CRLF in index?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-10-26T05:54:51Z","receivedAt":"2017-10-26T05:54:58Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n>> diff --git a/diff.c b/diff.c\n>> index 6fd288420b..eeca0fd3b8 100644\n>> --- a/diff.c\n>> +++ b/diff.c\n>> @@ -4202,7 +4202,8 @@ void diff_setup_done(struct diff_options *options)\n>>\n>> if (DIFF_XDL_TST(options, IGNORE_WHITESPACE) ||\n>> DIFF_XDL_TST(options, IGNORE_WHITESPACE_CHANGE) ||\n>> - DIFF_XDL_TST(options, IGNORE_WHITESPACE_AT_EOL))\n>> + DIFF_XDL_TST(options, IGNORE_WHITESPACE_AT_EOL) ||\n>> + DIFF_XDL_TST(options, IGNORE_CR_AT_EOL))\n>\n> This highlights another part of the flag macros, that could be made nicer.\n> All these flags combined are XDF_WHITESPACE_FLAGS, so this\n> if could be written without the macros as\n>\n> if (options->xdl_ops & XDF_WHITESPACE_FLAGS)\n\nYes, and I think the codepath that matters most already uses that\nform. Perhaps it is a good idea to do the rewrite without a macro\n(XDF_WHITESPACE_FLAGS is already a macro enough).\n\n> (1<<5) is taken twice now.\n\nGood eyes. I think we use bits #1-#8 now (bit #0 is vacant, so are\n#9-#31).\n\n>> diff --git a/xdiff/xutils.c b/xdiff/xutils.c\n>> index 04d7b32e4e..8720e272b9 100644\n>> --- a/xdiff/xutils.c\n>> +++ b/xdiff/xutils.c\n>> @@ -156,6 +156,21 @@ int xdl_blankline(const char *line, long size, long flags)\n>> return (i == size);\n>> }\n>>\n>> +/*\n>> + * Have we eaten everything on the line, except for an optional\n>> + * CR at the very end?\n>> + */\n>> +static int ends_with_optional_cr(const char *l, long s, long i)\n>> +{\n>> + if (s && l[s-1] == '\\n')\n>> + s--;\n>\n> so first we cut off the '\\n',\n\n> That seems correct after some thought;\n\nI added the \"trim the LF at the end\" to the beginning of the\nfunction at the last minute to cheat, and it is debatable if it is\nentirely correct on an incomplete line.\n\nThe byte at the end of line, i.e. l[s-1], could be either '\\n' or\nsomething else, and the latter is an incompete line at the end of\nthe file. When we trimmed the LF and decremented s, CR at l[s-1]\nis the CR in CRLF, which we do want to ignore. If we didn't, then\nwhat is CR sitting at l[s-1]? It is a lone CR at the end of file,\nnot a part of CRLF. Do we really want to ignore it?\n\nIf we take the name of the option \"ignore-cr-at-eol\" literally, yes,\nit is a CR sitting at the end of a line, which happens to be an\nincomplete one, so we do want to ignore. But if we think about the\nreason why we are adding the option (i.e. to help conversion between\nCRLF and LF), it is somewhat iffy. The lone CR at the end of file\ncannot be something that came from CRLF<->LF conversion, and ignoring\nit would hide possible problems in conversion or the original data.\n\n> I might offer\n> another easier to understand (for me) solution,\n> ...\n> Though this seems even more complicated\n> after having it written down.\n\nThis happens to me quite often and my solution to it is to remove\nthe alternative I tried to formulate after convincing myself that it\nis not that much of an improvement ;-).\n"},{"id":"331072","messageId":"5e962f69-f9cb-bd7e-481f-050e6239813f@kdbg.org","threadId":"47023","inReplyTo":"alpine.DEB.2.21.1.1710251415040.6482@virtualbox","subject":"Re: Consequences of CRLF in index?","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2017-10-26T07:09:27Z","receivedAt":"2017-10-26T07:09:35Z","isPatch":false,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 25.10.2017 um 14:19 schrieb Johannes Schindelin:\n> I envy you for the blessing of such a clean C++ source that you do not\n> have any, say, Unix shell script in it. Try this, and weep:\n> \n> \t$ printf 'echo \\\\\\r\\n\\t123\\r\\n' >a1\n> \n> \t$ sh a1\n> \n> \ta1: 2: a1: 123: not found\n\nI was bitten by that, too. For this reason, I ensure that shell scripts \nand Makefiles begin their life on Linux. Fortunately, modern editors on \nWindows, includ^Wand vi, do not force CRLF line breaks, and such files \ncan be edited on Windows, too.\n\nOf course, I do not set core.autocrlf anywhere to avoid any changes \nbehind my back.\n\n> For the same reason (Unix shell not handling CR/LF gracefull), I went\n> through that painful work that finally landed as 00ddc9d13ca (Fix build\n> with core.autocrlf=true, 2017-05-09).\n\nThat's much appreciated!\n\n-- Hannes\n"},{"id":"331084","messageId":"30ED36AB-FAB1-4635-94AF-FCEC6F02CE01@gmail.com","threadId":"47023","inReplyTo":"5e962f69-f9cb-bd7e-481f-050e6239813f@kdbg.org","subject":"Re: Consequences of CRLF in index?","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2017-10-26T11:01:25Z","receivedAt":"2017-10-26T11:01:38Z","isPatch":false,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\n> On 26 Oct 2017, at 09:09, Johannes Sixt wrote:\n> \n> Am 25.10.2017 um 14:19 schrieb Johannes Schindelin:\n>> I envy you for the blessing of such a clean C++ source that you do not\n>> have any, say, Unix shell script in it. Try this, and weep:\n>> \t$ printf 'echo \\\\\\r\\n\\t123\\r\\n' >a1\n>> \t$ sh a1\n>> \ta1: 2: a1: 123: not found\n> \n> I was bitten by that, too. For this reason, I ensure that shell scripts and Makefiles begin their life on Linux. Fortunately, modern editors on Windows, includ^Wand vi, do not force CRLF line breaks, and such files can be edited on Windows, too.\n\nWouldn't this kind of .gitattributes setup solve the problem?\n\n* -text\n*.sh text eol=lf\n\nThanks,\nLars\n\n"},{"id":"331085","messageId":"0B4CDE3E-86E1-4919-980B-048577EDCBEE@gmail.com","threadId":"47023","inReplyTo":"20171025171357.4nk4aj2c4m4b6kl3@aiede.mtv.corp.google.com","subject":"Re: Consequences of CRLF in index?","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2017-10-26T11:06:15Z","receivedAt":"2017-10-26T11:06:24Z","isPatch":false,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\n> On 25 Oct 2017, at 19:13, Jonathan Nieder wrote:\n> \n> Hi again,\n> \n> Lars Schneider wrote:\n>>> On 24 Oct 2017, at 20:14, Jonathan Nieder wrote:\n> \n>>> In any event, you also probably want to declare what you're doing\n>>> using .gitattributes. By checking in the files as CRLF, you are\n>>> declaring that you do *not* want Git to treat them as text files\n>>> (i.e., you do not want Git to change the line endings), so something\n>>> as simple as\n>>> \n>>> \t* -text\n>> \n>> That's sounds good. Does \"-text\" have any other implications?\n>> For whatever reason I always thought this is the way to tell\n>> Git that a particular file is binary with the implication that\n>> Git should not attempt to diff it.\n> \n> No other implications. You're thinking of \"-diff\". There is also a\n> shortcut \"binary\" which simply means \"-text -diff\".\n\nYeah. Well, when I read \"-text\" then I think \"no text\" and that makes\nme think \"is binary\".\n\n\n> Ideas for wording improvements to gitattributes(5) on this subject?\n\nI think the wording in the docs is good. It is just the \"text\" keyword\nthat confused me. Maybe this could have been names \"eolnorm\" and\n\"-eolnorm\" or something. But it is too late for that now I guess :-)\n\nThanks,\nLars\n"},{"id":"331107","messageId":"20171026191543.GA27760@tor.lan","threadId":"47023","inReplyTo":"20171025171357.4nk4aj2c4m4b6kl3@aiede.mtv.corp.google.com","subject":"Re: Consequences of CRLF in index?","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2017-10-26T19:15:43Z","receivedAt":"2017-10-26T19:16:00Z","isPatch":false,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On Wed, Oct 25, 2017 at 10:13:57AM -0700, Jonathan Nieder wrote:\n> Hi again,\n> \n> Lars Schneider wrote:\n> >> On 24 Oct 2017, at 20:14, Jonathan Nieder wrote:\n> \n> >> In any event, you also probably want to declare what you're doing\n> >> using .gitattributes. By checking in the files as CRLF, you are\n> >> declaring that you do *not* want Git to treat them as text files\n> >> (i.e., you do not want Git to change the line endings), so something\n> >> as simple as\n> >>\n> >> \t* -text\n> >\n> > That's sounds good. Does \"-text\" have any other implications?\n> > For whatever reason I always thought this is the way to tell\n> > Git that a particular file is binary with the implication that\n> > Git should not attempt to diff it.\n> \n> No other implications. You're thinking of \"-diff\". There is also a\n> shortcut \"binary\" which simply means \"-text -diff\".\n\nNot 100% the same, as far as I know.\n\"binary\" means: Don't convert line endings, and there is now way to\ndo a readable diff.\nThe only thing to tell the user is: The binary blobs are different.\n\nThen we have \"text\". The \"old\" version of \"text\" was \"crlf\", which\nfor some people was more intuitive, and less intuitive for others.\n\"* crlf\" is the same as \"* text\" and means please convert line endings.\nAnd yes, the file is still line oriented.\n\"* -crlf\" means don't touch the line endings, the file is\nline-orinted and diff and merge will work.\n\"* -text\" is the same as \"* -crlf\"\n\n> \n> Ideas for wording improvements to gitattributes(5) on this subject?\n\nNone from me at the moment.\n\n> \n> Thanks,\n> Jonathan\n"},{"id":"331108","messageId":"20171026192201.GB27760@tor.lan","threadId":"47023","inReplyTo":"30ED36AB-FAB1-4635-94AF-FCEC6F02CE01@gmail.com","subject":"Re: Consequences of CRLF in index?","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2017-10-26T19:22:01Z","receivedAt":"2017-10-26T19:22:12Z","isPatch":false,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On Thu, Oct 26, 2017 at 01:01:25PM +0200, Lars Schneider wrote:\n> \n> > On 26 Oct 2017, at 09:09, Johannes Sixt wrote:\n> > \n> > Am 25.10.2017 um 14:19 schrieb Johannes Schindelin:\n> >> I envy you for the blessing of such a clean C++ source that you do not\n> >> have any, say, Unix shell script in it. Try this, and weep:\n> >> \t$ printf 'echo \\\\\\r\\n\\t123\\r\\n' >a1\n> >> \t$ sh a1\n> >> \ta1: 2: a1: 123: not found\n> > \n> > I was bitten by that, too. For this reason, I ensure that shell scripts and Makefiles begin their life on Linux. Fortunately, modern editors on Windows, includ^Wand vi, do not force CRLF line breaks, and such files can be edited on Windows, too.\n> \n> Wouldn't this kind of .gitattributes setup solve the problem?\n> \n> * -text\n> *.sh text eol=lf\n> \n\nYes, exactly. and for the snake-lovers:\n\n*.py text eol=lf\n"},{"id":"331112","messageId":"7a4396e8-7b97-9ec9-d028-c5ad29042c86@kdbg.org","threadId":"47023","inReplyTo":"30ED36AB-FAB1-4635-94AF-FCEC6F02CE01@gmail.com","subject":"Re: Consequences of CRLF in index?","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2017-10-26T20:20:58Z","receivedAt":"2017-10-26T20:21:06Z","isPatch":false,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 26.10.2017 um 13:01 schrieb Lars Schneider:\n>> On 26 Oct 2017, at 09:09, Johannes Sixt wrote:\n>> Am 25.10.2017 um 14:19 schrieb Johannes Schindelin:\n>>> I envy you for the blessing of such a clean C++ source that you do not\n>>> have any, say, Unix shell script in it. Try this, and weep:\n>>> \t$ printf 'echo \\\\\\r\\n\\t123\\r\\n' >a1\n>>> \t$ sh a1\n>>> \ta1: 2: a1: 123: not found\n>>\n>> I was bitten by that, too. For this reason, I ensure that shell scripts and Makefiles begin their life on Linux. Fortunately, modern editors on Windows, includ^Wand vi, do not force CRLF line breaks, and such files can be edited on Windows, too.\n> \n> Wouldn't this kind of .gitattributes setup solve the problem?\n\nBut there is no problem. Don't have CRs in shell scripts and be done \nwith it.\n\n> \n> * -text\n> *.sh text eol=lf\n\nWhy would that be necessary? I cannot have CRLF in shell scripts etc., \nnot even on Windows. (And in addition I do not mind CR in C++ source \ncode.) All I want is: Git, please, by all means, do not mess with my \nfiles, ever. Isn't the simplest way to achieve that to set *nothing* at \nall? Not even core.autocrlf?\n\n-- Hannes\n"},{"id":"331114","messageId":"20171026203046.fu3z5ngnw7hckfrn@aiede.mtv.corp.google.com","threadId":"47023","inReplyTo":"7a4396e8-7b97-9ec9-d028-c5ad29042c86@kdbg.org","subject":"Re: Consequences of CRLF in index?","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-10-26T20:30:47Z","receivedAt":"2017-10-26T20:31:27Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Johannes Sixt wrote:\n> Am 26.10.2017 um 13:01 schrieb Lars Schneider:\n\n>> * -text\n>> *.sh text eol=lf\n>\n> Why would that be necessary? I cannot have CRLF in shell scripts etc., not\n> even on Windows. (And in addition I do not mind CR in C++ source code.) All\n> I want is: Git, please, by all means, do not mess with my files, ever. Isn't\n> the simplest way to achieve that to set *nothing* at all? Not even\n> core.autocrlf?\n\nThat's why Lars suggests \"* -text\" in .gitattributes. That way, if\nsome user of the repository *does* set core.autocrlf, you still get\nthe \"do not mess with my files\" semantics you want.\n\nIf you control the configuration of all users of the repository, you\ndon't need that, but it doesn't hurt, either.\n\nWith that \"* -text\", you do not need \"*.sh text eol=lf\", but maybe\nyou'd want it in order to get some warnings when someone changes the\nline endings by mistake without running tests. (Better to have a\nculture of running tests, you might say. Fair enough, but as with the\nother .gitattributes line, it doesn't hurt.)\n\nJonathan\n"},{"id":"331116","messageId":"f56a02d6-fbf9-188f-d19f-3d48708d9268@kdbg.org","threadId":"47023","inReplyTo":"20171026203046.fu3z5ngnw7hckfrn@aiede.mtv.corp.google.com","subject":"Re: Consequences of CRLF in index?","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2017-10-26T20:51:27Z","receivedAt":"2017-10-26T20:51:33Z","isPatch":false,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Thank you for the clarification, it's much appreciated.\n\n-- Hannes\n\n"},{"id":"331125","messageId":"CAEVs+zbq_zcYjyoCPE=W3LuQxAtRz8G+qLXA31jUiw+b4A4kSg@mail.gmail.com","threadId":"47023","inReplyTo":"f56a02d6-fbf9-188f-d19f-3d48708d9268@kdbg.org","subject":"Re: Consequences of CRLF in index?","fromName":"Ross Kabus","fromEmail":"rkabus@aerotech.com","sentAt":"2017-10-26T22:27:26Z","receivedAt":"2017-10-26T22:28:12Z","isPatch":false,"sender":{"key":"rkabus@aerotech.com","avatar":"https://avatars.githubusercontent.com/u/18420751?v=4"},"body":"Is \"* -text\" in any way different than \"-text\" (without the * asterisk)? All\nof my .gitattributes files have \"-text\" (no * asterisk) and I haven't noticed\nany difference but could I be missing something subtle?\n\n~Ross\n"},{"id":"331130","messageId":"xmqqh8ule6yc.fsf@gitster.mtv.corp.google.com","threadId":"47023","inReplyTo":"CAEVs+zbq_zcYjyoCPE=W3LuQxAtRz8G+qLXA31jUiw+b4A4kSg@mail.gmail.com","subject":"Re: Consequences of CRLF in index?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-10-27T01:05:15Z","receivedAt":"2017-10-27T01:05:22Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ross Kabus writes:\n\n> Is \"* -text\" in any way different than \"-text\" (without the * asterisk)? All\n> of my .gitattributes files have \"-text\" (no * asterisk) and I haven't noticed\n> any difference but could I be missing something subtle?\n>\n> ~Ross\n\nA line in the .gitattibute file is of the form\n\n\t\t...\n\ni.e. a pattern to match paths, with a list of attribute definitions.\nThe asterisk they are showing in their description is the \npart, i.e. \"apply the '-text' thing to paths that match '*'\", which\nis equivalent to saying \"set text attribute to false for all paths\".\n\n"},{"id":"331140","messageId":"xmqqinf1ce3i.fsf_-_@gitster.mtv.corp.google.com","threadId":"47023","inReplyTo":"xmqqy3nye9n8.fsf@gitster.mtv.corp.google.com","subject":"Re* Consequences of CRLF in index?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-10-27T06:13:53Z","receivedAt":"2017-10-27T06:14:02Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Stefan Beller writes:\n>\n>> (1<<5) is taken twice now.\n>\n> Good eyes. I think we use bits #1-#8 now (bit #0 is vacant, so are\n> #9-#31).\n\nLet's do this bit-shuffling as a preliminary clean-up.\n\n-- >8 --\nSubject: [PATCH] xdiff: reassign xpparm_t.flags bits\n\nWe have packed the bits too tightly in such a way that it is not\neasy to add a new type of whitespace ignoring option, a new type\nof LCS algorithm, or a new type of post-cleanup heuristics.\n\nReorder bits a bit to give room for these three classes of options\nto grow.\n\nWhile at it, add a comment in front of the bit definitions to\nclarify in which structure these defined bits may appear.\n\nSigned-off-by: Junio C Hamano \n---\n xdiff/xdiff.h | 14 ++++++++------\n 1 file changed, 8 insertions(+), 6 deletions(-)\n\ndiff --git a/xdiff/xdiff.h b/xdiff/xdiff.h\nindex b090ad8eac..457cac32d8 100644\n--- a/xdiff/xdiff.h\n+++ b/xdiff/xdiff.h\n@@ -27,22 +27,24 @@\n extern \"C\" {\n #endif /* #ifdef __cplusplus */\n \n+/* xpparm_t.flags */\n+#define XDF_NEED_MINIMAL (1 << 0)\n \n-#define XDF_NEED_MINIMAL (1 << 1)\n #define XDF_IGNORE_WHITESPACE (1 << 2)\n #define XDF_IGNORE_WHITESPACE_CHANGE (1 << 3)\n #define XDF_IGNORE_WHITESPACE_AT_EOL (1 << 4)\n #define XDF_WHITESPACE_FLAGS (XDF_IGNORE_WHITESPACE | XDF_IGNORE_WHITESPACE_CHANGE | XDF_IGNORE_WHITESPACE_AT_EOL)\n \n-#define XDF_PATIENCE_DIFF (1 << 5)\n-#define XDF_HISTOGRAM_DIFF (1 << 6)\n+#define XDF_IGNORE_BLANK_LINES (1 << 7)\n+\n+#define XDF_PATIENCE_DIFF (1 << 14)\n+#define XDF_HISTOGRAM_DIFF (1 << 15)\n #define XDF_DIFF_ALGORITHM_MASK (XDF_PATIENCE_DIFF | XDF_HISTOGRAM_DIFF)\n #define XDF_DIFF_ALG(x) ((x) & XDF_DIFF_ALGORITHM_MASK)\n \n-#define XDF_IGNORE_BLANK_LINES (1 << 7)\n-\n-#define XDF_INDENT_HEURISTIC (1 << 8)\n+#define XDF_INDENT_HEURISTIC (1 << 23)\n \n+/* xdemitconf_t.flags */\n #define XDL_EMIT_FUNCNAMES (1 << 0)\n #define XDL_EMIT_FUNCCONTEXT (1 << 2)\n \n-- \n2.15.0-rc2-266-g8f92d095f4\n\n"},{"id":"331168","messageId":"alpine.DEB.2.21.1.1710271716330.6482@virtualbox","threadId":"47023","inReplyTo":"30ED36AB-FAB1-4635-94AF-FCEC6F02CE01@gmail.com","subject":"Re: Consequences of CRLF in index?","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-10-27T15:18:42Z","receivedAt":"2017-10-27T15:18:53Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Lars,\n\nOn Thu, 26 Oct 2017, Lars Schneider wrote:\n\n> > On 26 Oct 2017, at 09:09, Johannes Sixt wrote:\n> > \n> > Am 25.10.2017 um 14:19 schrieb Johannes Schindelin:\n> >> I envy you for the blessing of such a clean C++ source that you do not\n> >> have any, say, Unix shell script in it. Try this, and weep:\n> >> \t$ printf 'echo \\\\\\r\\n\\t123\\r\\n' >a1\n> >> \t$ sh a1\n> >> \ta1: 2: a1: 123: not found\n> > \n> > I was bitten by that, too. For this reason, I ensure that shell\n> > scripts and Makefiles begin their life on Linux. Fortunately, modern\n> > editors on Windows, includ^Wand vi, do not force CRLF line breaks, and\n\n ^^^^^^^^^^^^^^\n\nThis put a well-needed smile on my face. Thanks.\n\n> > such files can be edited on Windows, too.\n> \n> Wouldn't this kind of .gitattributes setup solve the problem?\n> \n> * -text\n> *.sh text eol=lf\n\nIf you look at the commit I mentioned, you will see examples where it\nbreaks down: when using Unix shell scripts *without* .sh file extension.\nMost notable offender: GIT-VERSION-GEN.\n\nCiao,\nDscho\n"},{"id":"331174","messageId":"CAGZ79kY0eL42fC+QC9ifvHt-Z_PO7FfVYaEZysGWq+LqLcjduA@mail.gmail.com","threadId":"47023","inReplyTo":"xmqqinf1ce3i.fsf_-_@gitster.mtv.corp.google.com","subject":"Re: Re* Consequences of CRLF in index?","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-10-27T17:06:18Z","receivedAt":"2017-10-27T17:06:29Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Oct 26, 2017 at 11:13 PM, Junio C Hamano wrote:\n> Subject: [PATCH] xdiff: reassign xpparm_t.flags bits\n>\n> We have packed the bits too tightly in such a way that it is not\n> easy to add a new type of whitespace ignoring option, a new type\n> of LCS algorithm, or a new type of post-cleanup heuristics.\n>\n> Reorder bits a bit to give room for these three classes of options\n> to grow.\n>\n> While at it, add a comment in front of the bit definitions to\n> clarify in which structure these defined bits may appear.\n>\n> Signed-off-by: Junio C Hamano \n\nReviewed-by: Stefan Beller \n\n> ---\n> xdiff/xdiff.h | 14 ++++++++------\n> 1 file changed, 8 insertions(+), 6 deletions(-)\n>\n> diff --git a/xdiff/xdiff.h b/xdiff/xdiff.h\n> index b090ad8eac..457cac32d8 100644\n> --- a/xdiff/xdiff.h\n> +++ b/xdiff/xdiff.h\n> @@ -27,22 +27,24 @@\n> extern \"C\" {\n> #endif /* #ifdef __cplusplus */\n>\n> +/* xpparm_t.flags */\n> +#define XDF_NEED_MINIMAL (1 << 0)\n>\n> -#define XDF_NEED_MINIMAL (1 << 1)\n\nThe whitespace flags are also stored in xpparm_t, though\nthese flags can also come in from other sources (e.g. we\njust pass it in manually via the interface).\n\n> #define XDF_IGNORE_WHITESPACE (1 << 2)\n> #define XDF_IGNORE_WHITESPACE_CHANGE (1 << 3)\n> #define XDF_IGNORE_WHITESPACE_AT_EOL (1 << 4)\n> #define XDF_WHITESPACE_FLAGS (XDF_IGNORE_WHITESPACE | XDF_IGNORE_WHITESPACE_CHANGE | XDF_IGNORE_WHITESPACE_AT_EOL)\n>\n> -#define XDF_PATIENCE_DIFF (1 << 5)\n> -#define XDF_HISTOGRAM_DIFF (1 << 6)\n> +#define XDF_IGNORE_BLANK_LINES (1 << 7)\n\nIs XDF_IGNORE_BLANK_LINES a whitespace option, just\non the lines level instead of the inside one line? I think\nit still makes sense to keep it here, slightly separated from\nthe IGNORE flags.\n\n> +#define XDF_PATIENCE_DIFF (1 << 14)\n> +#define XDF_HISTOGRAM_DIFF (1 << 15)\n> #define XDF_DIFF_ALGORITHM_MASK (XDF_PATIENCE_DIFF | XDF_HISTOGRAM_DIFF)\n> #define XDF_DIFF_ALG(x) ((x) & XDF_DIFF_ALGORITHM_MASK)\n>\n> -#define XDF_IGNORE_BLANK_LINES (1 << 7)\n> -\n> -#define XDF_INDENT_HEURISTIC (1 << 8)\n> +#define XDF_INDENT_HEURISTIC (1 << 23)\n>\n> +/* xdemitconf_t.flags */\n> #define XDL_EMIT_FUNCNAMES (1 << 0)\n> #define XDL_EMIT_FUNCCONTEXT (1 << 2)\n\nThis looked like it was carefully crafted to avoid accidental bit overlap\nwith XDF_NEED_MINIMAL; but these are in different flag fields, this\nshould be fine.\n\nThanks,\nStefan\n"},{"id":"331175","messageId":"20171027170745.2125-1-sbeller@google.com","threadId":"47023","inReplyTo":"xmqqinf1ce3i.fsf_-_@gitster.mtv.corp.google.com","subject":"[PATCH 0/2] Re* Consequences of CRLF in index?","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-10-27T17:07:43Z","receivedAt":"2017-10-27T17:07:57Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"> Let's do this bit-shuffling as a preliminary clean-up.\n\nThese 2 patches can go on top of that as well.\n\nThanks,\nStefan\n\nStefan Beller (2):\n xdiff/xdiff.h: remove unused flags\n xdiff/xdiffi.c: remove unneeded function declarations\n\n xdiff/xdiff.h | 8 --------\n xdiff/xdiffi.c | 17 -----------------\n 2 files changed, 25 deletions(-)\n\n-- \n2.15.0.rc2.443.gfcc3b81c0a\n\n"},{"id":"331176","messageId":"20171027170745.2125-2-sbeller@google.com","threadId":"47023","inReplyTo":"20171027170745.2125-1-sbeller@google.com","subject":"[PATCH 1/2] xdiff/xdiff.h: remove unused flags","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-10-27T17:07:44Z","receivedAt":"2017-10-27T17:08:00Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"These flags were there since the beginning (3443546f6e (Use a *real*\nbuilt-in diff generator, 2006-03-24), but were never used. Remove them.\n\nSigned-off-by: Stefan Beller \n---\n xdiff/xdiff.h | 8 --------\n 1 file changed, 8 deletions(-)\n\ndiff --git a/xdiff/xdiff.h b/xdiff/xdiff.h\nindex 457cac32d8..0a41f5e500 100644\n--- a/xdiff/xdiff.h\n+++ b/xdiff/xdiff.h\n@@ -48,14 +48,6 @@ extern \"C\" {\n #define XDL_EMIT_FUNCNAMES (1 << 0)\n #define XDL_EMIT_FUNCCONTEXT (1 << 2)\n \n-#define XDL_MMB_READONLY (1 << 0)\n-\n-#define XDL_MMF_ATOMIC (1 << 0)\n-\n-#define XDL_BDOP_INS 1\n-#define XDL_BDOP_CPY 2\n-#define XDL_BDOP_INSB 3\n-\n /* merge simplification levels */\n #define XDL_MERGE_MINIMAL 0\n #define XDL_MERGE_EAGER 1\n-- \n2.15.0.rc2.443.gfcc3b81c0a\n\n"},{"id":"331177","messageId":"20171027170745.2125-3-sbeller@google.com","threadId":"47023","inReplyTo":"20171027170745.2125-1-sbeller@google.com","subject":"[PATCH 2/2] xdiff/xdiffi.c: remove unneeded function declarations","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-10-27T17:07:45Z","receivedAt":"2017-10-27T17:08:02Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"There is no need to forward-declare these functions, as they are used\nafter their implementation only.\n\nSigned-off-by: Stefan Beller \n---\n xdiff/xdiffi.c | 17 -----------------\n 1 file changed, 17 deletions(-)\n\ndiff --git a/xdiff/xdiffi.c b/xdiff/xdiffi.c\nindex 93a65680a1..0a4ea948f9 100644\n--- a/xdiff/xdiffi.c\n+++ b/xdiff/xdiffi.c\n@@ -22,34 +22,17 @@\n \n #include \"xinclude.h\"\n \n-\n-\n #define XDL_MAX_COST_MIN 256\n #define XDL_HEUR_MIN_COST 256\n #define XDL_LINE_MAX (long)((1UL << (CHAR_BIT * sizeof(long) - 1)) - 1)\n #define XDL_SNAKE_CNT 20\n #define XDL_K_HEUR 4\n \n-\n-\n typedef struct s_xdpsplit {\n \tlong i1, i2;\n \tint min_lo, min_hi;\n } xdpsplit_t;\n \n-\n-\n-\n-static long xdl_split(unsigned long const *ha1, long off1, long lim1,\n-\t\t unsigned long const *ha2, long off2, long lim2,\n-\t\t long *kvdf, long *kvdb, int need_min, xdpsplit_t *spl,\n-\t\t xdalgoenv_t *xenv);\n-static xdchange_t *xdl_add_change(xdchange_t *xscr, long i1, long i2, long chg1, long chg2);\n-\n-\n-\n-\n-\n /*\n * See \"An O(ND) Difference Algorithm and its Variations\", by Eugene Myers.\n * Basically considers a \"box\" (off1, off2, lim1, lim2) and scan from both\n-- \n2.15.0.rc2.443.gfcc3b81c0a\n\n"},{"id":"331375","messageId":"CAGZ79kbNMWqPkk9PGDH44YYPC=Mgqk4+sTmRwQUGKF_JCmbpLA@mail.gmail.com","threadId":"47023","inReplyTo":"20171027170745.2125-1-sbeller@google.com","subject":"Re: [PATCH 0/2] Re* Consequences of CRLF in index?","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-10-30T17:20:59Z","receivedAt":"2017-10-30T17:21:16Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, Oct 27, 2017 at 10:07 AM, Stefan Beller wrote:\n>> Let's do this bit-shuffling as a preliminary clean-up.\n>\n> These 2 patches can go on top of that as well.\n\nActually these textually do not conflict with your patch,\nand they can be picked independently, e.g. they could\ngo on top of sb/diff-color-moved-use-xdl-recmatch\nas a diff cleanup.\n\n(I note this as you regard your patches as a lunch time hack\nin the cooking email; I am serious about these patches though.)\n\nThanks,\nStefan\n"},{"id":"331443","messageId":"xmqqpo942fzk.fsf@gitster.mtv.corp.google.com","threadId":"47023","inReplyTo":"CAGZ79kbNMWqPkk9PGDH44YYPC=Mgqk4+sTmRwQUGKF_JCmbpLA@mail.gmail.com","subject":"Re: [PATCH 0/2] Re* Consequences of CRLF in index?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-10-31T02:44:31Z","receivedAt":"2017-10-31T02:44:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> (I note this as you regard your patches as a lunch time hack\n> in the cooking email; I am serious about these patches though.)\n\nWe do not want to touch borrowed code unnecessarily. Are these\nlines and bits hampering further progress we are actively trying to\nmake right now?\n\n"},{"id":"331479","messageId":"CAGZ79kaSQzLEjmD3+YgQOz5cety9Y3MZmUo7EkKA-JdQRt_Diw@mail.gmail.com","threadId":"47023","inReplyTo":"xmqqpo942fzk.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 0/2] Re* Consequences of CRLF in index?","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-10-31T16:41:25Z","receivedAt":"2017-10-31T16:41:31Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, Oct 30, 2017 at 7:44 PM, Junio C Hamano wrote:\n> Stefan Beller writes:\n>\n>> (I note this as you regard your patches as a lunch time hack\n>> in the cooking email; I am serious about these patches though.)\n>\n> We do not want to touch borrowed code unnecessarily. Are these\n> lines and bits hampering further progress we are actively trying to\n> make right now?\n\nNo they are not, you are correct.\n\nI differ in opinion on 'borrowed code'. The latest release of xdiff\n(v0.23) was in Nov 13, 2008 according to http://freecode.com/projects/xdiff-lib\nor on March 23, 2006 according to https://directory.fsf.org/wiki/LibXDiff\nand given that we incorporated so many changes already to xdiff,\nI would argue it is sufficiently different from the original, we'll probably\nnever import another upstream version (if there will be a release at all).\n\nSo the code was rather taken and now we are the bag holders in\nmaintaining it, so we can make it pretty even only for the sake of\npleasing ourselves (or rather: not confusing ourselves with too\nmany unused flags).\n\nThanks,\nStefan\n"},{"id":"331480","messageId":"20171031170120.3tg3mrenyhmp5uwc@sigill.intra.peff.net","threadId":"47023","inReplyTo":"CAGZ79kaSQzLEjmD3+YgQOz5cety9Y3MZmUo7EkKA-JdQRt_Diw@mail.gmail.com","subject":"Re: [PATCH 0/2] Re* Consequences of CRLF in index?","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-10-31T17:01:20Z","receivedAt":"2017-10-31T17:01:31Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Oct 31, 2017 at 09:41:25AM -0700, Stefan Beller wrote:\n\n> On Mon, Oct 30, 2017 at 7:44 PM, Junio C Hamano wrote:\n> > Stefan Beller writes:\n> >\n> >> (I note this as you regard your patches as a lunch time hack\n> >> in the cooking email; I am serious about these patches though.)\n> >\n> > We do not want to touch borrowed code unnecessarily. Are these\n> > lines and bits hampering further progress we are actively trying to\n> > make right now?\n> \n> No they are not, you are correct.\n> \n> I differ in opinion on 'borrowed code'. The latest release of xdiff\n> (v0.23) was in Nov 13, 2008 according to http://freecode.com/projects/xdiff-lib\n> or on March 23, 2006 according to https://directory.fsf.org/wiki/LibXDiff\n> and given that we incorporated so many changes already to xdiff,\n> I would argue it is sufficiently different from the original, we'll probably\n> never import another upstream version (if there will be a release at all).\n> \n> So the code was rather taken and now we are the bag holders in\n> maintaining it, so we can make it pretty even only for the sake of\n> pleasing ourselves (or rather: not confusing ourselves with too\n> many unused flags).\n\nYes, I'd agree. For what it's worth both sides of this argument played\nout in my head when I saw your patches, and I ended up at the same \"we\nare the bag holders\" place. And there's certainly precedent for touching\nthat code and cleaning it up to make it easier to work with (just look\nat at \"git log -p xdiff\").\n\nI don't know that I would support a full-scale rewrite (for the same\nreason that I wouldn't on the rest of the code base -- avoiding churn).\nBut deleting unused bits seems like an easy win for readability.\n\n-Peff\n"},{"id":"331976","messageId":"20171107064011.18399-1-gitster@pobox.com","threadId":"47023","inReplyTo":"xmqqshe7j0af.fsf@gitster.mtv.corp.google.com","subject":"[PATCH v2 0/2] Teach \"diff\" to ignore only CR at EOL","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-11-07T06:40:09Z","receivedAt":"2017-11-07T06:40:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"This time with doc updates and tests. The previous one is at \n\nhttps://public-inbox.org/git/xmqqshe7j0af.fsf@gitster.mtv.corp.google.com\n\nA change that is supposed to only change the end-of-line convention\nbetween LF <-> CRLF and nothing else can be verified with \"diff -b\"\nor \"diff --ignore-space-at-eol\" in practice, but these hide changes\nin whitespaces other than the carriage return at the end of the lines.\n\nThese two patches introduce a new --ignore-cr-at-eol option so that\nonly CR at eol and no other whitespace changes are ignored while\nmaking a comparison, to allow a stricter validation.\n\nI am not sure how much practical value the new option has. The\npreliminary clean-up patch to shuffle the bis assignment for flags\ndoes have values, though ;-).\n\nJunio C Hamano (2):\n xdiff: reassign xpparm_t.flags bits\n diff: --ignore-cr-at-eol\n\n Documentation/diff-options.txt | 3 +++\n Documentation/merge-strategies.txt | 5 +++--\n diff.c | 6 +++---\n merge-recursive.c | 2 ++\n t/t4015-diff-whitespace.sh | 28 ++++++++++++++++++++++++++++\n xdiff/xdiff.h | 26 ++++++++++++++++----------\n xdiff/xutils.c | 38 ++++++++++++++++++++++++++++++++++++--\n 7 files changed, 91 insertions(+), 17 deletions(-)\n\n-- \n2.15.0-263-g47cc852023\n\n"},{"id":"331977","messageId":"20171107064011.18399-3-gitster@pobox.com","threadId":"47023","inReplyTo":"20171107064011.18399-1-gitster@pobox.com","subject":"[PATCH v2 2/2] diff: --ignore-cr-at-eol","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-11-07T06:40:11Z","receivedAt":"2017-11-07T06:40:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"A new option --ignore-cr-at-eol tells the diff machinery to treat a\ncarriage-return at the end of a (complete) line as if it does not\nexist.\n\nThis would make it easier to review a change whose only effect is to\nturn line endings from CRLF to LF or the other way around.\n\nSigned-off-by: Junio C Hamano \n---\n Documentation/diff-options.txt | 3 +++\n Documentation/merge-strategies.txt | 5 +++--\n diff.c | 2 ++\n merge-recursive.c | 2 ++\n t/t4015-diff-whitespace.sh | 28 ++++++++++++++++++++++++++++\n xdiff/xdiff.h | 4 +++-\n xdiff/xutils.c | 38 ++++++++++++++++++++++++++++++++++++--\n 7 files changed, 77 insertions(+), 5 deletions(-)\n\ndiff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt\nindex 89cc0f48de..aa2c0ff74d 100644\n--- a/Documentation/diff-options.txt\n+++ b/Documentation/diff-options.txt\n@@ -519,6 +519,9 @@ endif::git-format-patch[]\n --text::\n \tTreat all files as text.\n \n+--ignore-cr-at-eol::\n+\tIgnore carrige-return at the end of line when doing a comparison.\n+\n --ignore-space-at-eol::\n \tIgnore changes in whitespace at EOL.\n \ndiff --git a/Documentation/merge-strategies.txt b/Documentation/merge-strategies.txt\nindex 2eb92b9327..030744910e 100644\n--- a/Documentation/merge-strategies.txt\n+++ b/Documentation/merge-strategies.txt\n@@ -57,11 +57,12 @@ diff-algorithm=[patience|minimal|histogram|myers];;\n ignore-space-change;;\n ignore-all-space;;\n ignore-space-at-eol;;\n+ignore-cr-at-eol;;\n \tTreats lines with the indicated type of whitespace change as\n \tunchanged for the sake of a three-way merge. Whitespace\n \tchanges mixed with other changes to a line are not ignored.\n-\tSee also linkgit:git-diff[1] `-b`, `-w`, and\n-\t`--ignore-space-at-eol`.\n+\tSee also linkgit:git-diff[1] `-b`, `-w`,\n+\t`--ignore-space-at-eol`, and `--ignore-cr-at-eol`.\n +\n * If 'their' version only introduces whitespace changes to a line,\n 'our' version is used;\ndiff --git a/diff.c b/diff.c\nindex 790250fe86..dd14e4190c 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -3888,6 +3888,8 @@ int diff_opt_parse(struct diff_options *options,\n \t\tDIFF_XDL_SET(options, IGNORE_WHITESPACE_CHANGE);\n \telse if (!strcmp(arg, \"--ignore-space-at-eol\"))\n \t\tDIFF_XDL_SET(options, IGNORE_WHITESPACE_AT_EOL);\n+\telse if (!strcmp(arg, \"--ignore-cr-at-eol\"))\n+\t\tDIFF_XDL_SET(options, IGNORE_CR_AT_EOL);\n \telse if (!strcmp(arg, \"--ignore-blank-lines\"))\n \t\tDIFF_XDL_SET(options, IGNORE_BLANK_LINES);\n \telse if (!strcmp(arg, \"--indent-heuristic\"))\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex 7a7d55aabe..006b94baf2 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -2214,6 +2214,8 @@ int parse_merge_opt(struct merge_options *o, const char *s)\n \t\tDIFF_XDL_SET(o, IGNORE_WHITESPACE);\n \telse if (!strcmp(s, \"ignore-space-at-eol\"))\n \t\tDIFF_XDL_SET(o, IGNORE_WHITESPACE_AT_EOL);\n+\telse if (!strcmp(s, \"ignore-cr-at-eol\"))\n+\t\tDIFF_XDL_SET(o, IGNORE_CR_AT_EOL);\n \telse if (!strcmp(s, \"renormalize\"))\n \t\to->renormalize = 1;\n \telse if (!strcmp(s, \"no-renormalize\"))\ndiff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh\nindex 289806d0c7..32dd54c21d 100755\n--- a/t/t4015-diff-whitespace.sh\n+++ b/t/t4015-diff-whitespace.sh\n@@ -106,6 +106,8 @@ test_expect_success 'another test, without options' '\n \tgit diff -w -b --ignore-space-at-eol >out &&\n \ttest_cmp expect out &&\n \n+\tgit diff -w --ignore-cr-at-eol >out &&\n+\ttest_cmp expect out &&\n \n \ttr \"Q_\" \"\\015 \" <<-\\EOF >expect &&\n \tdiff --git a/x b/x\n@@ -128,6 +130,9 @@ test_expect_success 'another test, without options' '\n \tgit diff -b --ignore-space-at-eol >out &&\n \ttest_cmp expect out &&\n \n+\tgit diff -b --ignore-cr-at-eol >out &&\n+\ttest_cmp expect out &&\n+\n \ttr \"Q_\" \"\\015 \" <<-\\EOF >expect &&\n \tdiff --git a/x b/x\n \tindex d99af23..22d9f73 100644\n@@ -145,6 +150,29 @@ test_expect_success 'another test, without options' '\n \t CR at end\n \tEOF\n \tgit diff --ignore-space-at-eol >out &&\n+\ttest_cmp expect out &&\n+\n+\tgit diff --ignore-space-at-eol --ignore-cr-at-eol >out &&\n+\ttest_cmp expect out &&\n+\n+\ttr \"Q_\" \"\\015 \" <<-\\EOF >expect &&\n+\tdiff --git a/x b/x\n+\tindex_d99af23..22d9f73 100644\n+\t--- a/x\n+\t+++ b/x\n+\t@@ -1,6 +1,6 @@\n+\t-whitespace at beginning\n+\t-whitespace change\n+\t-whitespace in the middle\n+\t-whitespace at end\n+\t+_\twhitespace at beginning\n+\t+whitespace_\t_change\n+\t+white space in the middle\n+\t+whitespace at end__\n+\t unchanged line\n+\t CR at end\n+\tEOF\n+\tgit diff --ignore-cr-at-eol >out &&\n \ttest_cmp expect out\n '\n \ndiff --git a/xdiff/xdiff.h b/xdiff/xdiff.h\nindex cbf5d8e166..51f8926215 100644\n--- a/xdiff/xdiff.h\n+++ b/xdiff/xdiff.h\n@@ -33,9 +33,11 @@ extern \"C\" {\n #define XDF_IGNORE_WHITESPACE (1 << 1)\n #define XDF_IGNORE_WHITESPACE_CHANGE (1 << 2)\n #define XDF_IGNORE_WHITESPACE_AT_EOL (1 << 3)\n+#define XDF_IGNORE_CR_AT_EOL (1 << 4)\n #define XDF_WHITESPACE_FLAGS (XDF_IGNORE_WHITESPACE | \\\n \t\t\t XDF_IGNORE_WHITESPACE_CHANGE | \\\n-\t\t\t XDF_IGNORE_WHITESPACE_AT_EOL)\n+\t\t\t XDF_IGNORE_WHITESPACE_AT_EOL | \\\n+\t\t\t XDF_IGNORE_CR_AT_EOL)\n \n #define XDF_IGNORE_BLANK_LINES (1 << 7)\n \ndiff --git a/xdiff/xutils.c b/xdiff/xutils.c\nindex 04d7b32e4e..b2cbcc818f 100644\n--- a/xdiff/xutils.c\n+++ b/xdiff/xutils.c\n@@ -156,6 +156,24 @@ int xdl_blankline(const char *line, long size, long flags)\n \treturn (i == size);\n }\n \n+/*\n+ * Have we eaten everything on the line, except for an optional\n+ * CR at the very end?\n+ */\n+static int ends_with_optional_cr(const char *l, long s, long i)\n+{\n+\tint complete = s && l[s-1] == '\\n';\n+\n+\tif (complete)\n+\t\ts--;\n+\tif (s == i)\n+\t\treturn 1;\n+\t/* do not ignore CR at the end of an incomplete line */\n+\tif (complete && s == i + 1 && l[i] == '\\r')\n+\t\treturn 1;\n+\treturn 0;\n+}\n+\n int xdl_recmatch(const char *l1, long s1, const char *l2, long s2, long flags)\n {\n \tint i1, i2;\n@@ -170,7 +188,8 @@ int xdl_recmatch(const char *l1, long s1, const char *l2, long s2, long flags)\n \n \t/*\n \t * -w matches everything that matches with -b, and -b in turn\n-\t * matches everything that matches with --ignore-space-at-eol.\n+\t * matches everything that matches with --ignore-space-at-eol,\n+\t * which in turn matches everything that matches with --ignore-cr-at-eol.\n \t *\n \t * Each flavor of ignoring needs different logic to skip whitespaces\n \t * while we have both sides to compare.\n@@ -204,6 +223,14 @@ int xdl_recmatch(const char *l1, long s1, const char *l2, long s2, long flags)\n \t\t\ti1++;\n \t\t\ti2++;\n \t\t}\n+\t} else if (flags & XDF_IGNORE_CR_AT_EOL) {\n+\t\t/* Find the first difference and see how the line ends */\n+\t\twhile (i1 < s1 && i2 < s2 && l1[i1] == l2[i2]) {\n+\t\t\ti1++;\n+\t\t\ti2++;\n+\t\t}\n+\t\treturn (ends_with_optional_cr(l1, s1, i1) &&\n+\t\t\tends_with_optional_cr(l2, s2, i2));\n \t}\n \n \t/*\n@@ -230,9 +257,16 @@ static unsigned long xdl_hash_record_with_whitespace(char const **data,\n \t\tchar const *top, long flags) {\n \tunsigned long ha = 5381;\n \tchar const *ptr = *data;\n+\tint cr_at_eol_only = (flags & XDF_WHITESPACE_FLAGS) == XDF_IGNORE_CR_AT_EOL;\n \n \tfor (; ptr < top && *ptr != '\\n'; ptr++) {\n-\t\tif (XDL_ISSPACE(*ptr)) {\n+\t\tif (cr_at_eol_only) {\n+\t\t\t/* do not ignore CR at the end of an incomplete line */\n+\t\t\tif (*ptr == '\\r' &&\n+\t\t\t (ptr + 1 < top && ptr[1] == '\\n'))\n+\t\t\t\tcontinue;\n+\t\t}\n+\t\telse if (XDL_ISSPACE(*ptr)) {\n \t\t\tconst char *ptr2 = ptr;\n \t\t\tint at_eol;\n \t\t\twhile (ptr + 1 < top && XDL_ISSPACE(ptr[1])\n-- \n2.15.0-263-g47cc852023\n\n"},{"id":"331978","messageId":"20171107064011.18399-2-gitster@pobox.com","threadId":"47023","inReplyTo":"20171107064011.18399-1-gitster@pobox.com","subject":"[PATCH v2 1/2] xdiff: reassign xpparm_t.flags bits","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-11-07T06:40:10Z","receivedAt":"2017-11-07T06:40:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"We have packed the bits too tightly in such a way that it is not\neasy to add a new type of whitespace ignoring option, a new type\nof LCS algorithm, or a new type of post-cleanup heuristics.\n\nReorder bits a bit to give room for these three classes of options\nto grow. Also make use of XDF_WHITESPACE_FLAGS macro where we check\nany of these bits are on, instead of using DIFF_XDL_TST() macro on\nindividual possibilities. That way, the \"is any of the bits on?\"\ncode does not have to change when we add more ways to ignore\nwhitespaces.\n\nWhile at it, add a comment in front of the bit definitions to\nclarify in which structure these defined bits may appear.\n\nSigned-off-by: Junio C Hamano \n---\n diff.c | 4 +---\n xdiff/xdiff.h | 24 ++++++++++++++----------\n 2 files changed, 15 insertions(+), 13 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 74283d9001..790250fe86 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -3434,9 +3434,7 @@ void diff_setup_done(struct diff_options *options)\n \t * inside contents.\n \t */\n \n-\tif (DIFF_XDL_TST(options, IGNORE_WHITESPACE) ||\n-\t DIFF_XDL_TST(options, IGNORE_WHITESPACE_CHANGE) ||\n-\t DIFF_XDL_TST(options, IGNORE_WHITESPACE_AT_EOL))\n+\tif ((options->xdl_opts & XDF_WHITESPACE_FLAGS))\n \t\tDIFF_OPT_SET(options, DIFF_FROM_CONTENTS);\n \telse\n \t\tDIFF_OPT_CLR(options, DIFF_FROM_CONTENTS);\ndiff --git a/xdiff/xdiff.h b/xdiff/xdiff.h\nindex b090ad8eac..cbf5d8e166 100644\n--- a/xdiff/xdiff.h\n+++ b/xdiff/xdiff.h\n@@ -27,22 +27,26 @@\n extern \"C\" {\n #endif /* #ifdef __cplusplus */\n \n+/* xpparm_t.flags */\n+#define XDF_NEED_MINIMAL (1 << 0)\n \n-#define XDF_NEED_MINIMAL (1 << 1)\n-#define XDF_IGNORE_WHITESPACE (1 << 2)\n-#define XDF_IGNORE_WHITESPACE_CHANGE (1 << 3)\n-#define XDF_IGNORE_WHITESPACE_AT_EOL (1 << 4)\n-#define XDF_WHITESPACE_FLAGS (XDF_IGNORE_WHITESPACE | XDF_IGNORE_WHITESPACE_CHANGE | XDF_IGNORE_WHITESPACE_AT_EOL)\n+#define XDF_IGNORE_WHITESPACE (1 << 1)\n+#define XDF_IGNORE_WHITESPACE_CHANGE (1 << 2)\n+#define XDF_IGNORE_WHITESPACE_AT_EOL (1 << 3)\n+#define XDF_WHITESPACE_FLAGS (XDF_IGNORE_WHITESPACE | \\\n+\t\t\t XDF_IGNORE_WHITESPACE_CHANGE | \\\n+\t\t\t XDF_IGNORE_WHITESPACE_AT_EOL)\n \n-#define XDF_PATIENCE_DIFF (1 << 5)\n-#define XDF_HISTOGRAM_DIFF (1 << 6)\n+#define XDF_IGNORE_BLANK_LINES (1 << 7)\n+\n+#define XDF_PATIENCE_DIFF (1 << 14)\n+#define XDF_HISTOGRAM_DIFF (1 << 15)\n #define XDF_DIFF_ALGORITHM_MASK (XDF_PATIENCE_DIFF | XDF_HISTOGRAM_DIFF)\n #define XDF_DIFF_ALG(x) ((x) & XDF_DIFF_ALGORITHM_MASK)\n \n-#define XDF_IGNORE_BLANK_LINES (1 << 7)\n-\n-#define XDF_INDENT_HEURISTIC (1 << 8)\n+#define XDF_INDENT_HEURISTIC (1 << 23)\n \n+/* xdemitconf_t.flags */\n #define XDL_EMIT_FUNCNAMES (1 << 0)\n #define XDL_EMIT_FUNCCONTEXT (1 << 2)\n \n-- \n2.15.0-263-g47cc852023\n\n"},{"id":"331988","messageId":"alpine.DEB.2.21.1.1711071329280.6482@virtualbox","threadId":"47023","inReplyTo":"20171107064011.18399-1-gitster@pobox.com","subject":"Re: [PATCH v2 0/2] Teach \"diff\" to ignore only CR at EOL","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-11-07T12:30:54Z","receivedAt":"2017-11-07T12:31:50Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Tue, 7 Nov 2017, Junio C Hamano wrote:\n\n> This time with doc updates and tests. The previous one is at \n> \n> https://public-inbox.org/git/xmqqshe7j0af.fsf@gitster.mtv.corp.google.com\n> \n> A change that is supposed to only change the end-of-line convention\n> between LF <-> CRLF and nothing else can be verified with \"diff -b\"\n> or \"diff --ignore-space-at-eol\" in practice, but these hide changes\n> in whitespaces other than the carriage return at the end of the lines.\n\nGood. I was wishing for such a feature in the past.\n\nHowever, the short and sweet `-b` or `-w` switches are really, really\nnice. `--ignore-cr-at-eol` is just very cumbersome to type out. So I think\nyou will want to add this patch to your patch series:\n\n-- snipsnap --\ndiff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\nindex 0e16f017a41..b7a45e8df29 100644\n--- a/contrib/completion/git-completion.bash\n+++ b/contrib/completion/git-completion.bash\n@@ -1400,7 +1400,7 @@ __git_diff_common_options=\"--stat --numstat --shortstat --summary\n \t\t\t--patch-with-stat --name-only --name-status --color\n \t\t\t--no-color --color-words --no-renames --check\n \t\t\t--full-index --binary --abbrev --diff-filter=\n-\t\t\t--find-copies-harder\n+\t\t\t--find-copies-harder --ignore-cr-at-eol\n \t\t\t--text --ignore-space-at-eol --ignore-space-change\n \t\t\t--ignore-all-space --ignore-blank-lines --exit-code\n \t\t\t--quiet --ext-diff --no-ext-diff\n"},{"id":"331990","messageId":"alpine.DEB.2.21.1.1711071336120.6482@virtualbox","threadId":"47023","inReplyTo":"20171107064011.18399-2-gitster@pobox.com","subject":"Re: [PATCH v2 1/2] xdiff: reassign xpparm_t.flags bits","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-11-07T12:44:44Z","receivedAt":"2017-11-07T12:45:09Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Tue, 7 Nov 2017, Junio C Hamano wrote:\n\n> We have packed the bits too tightly in such a way that it is not\n> easy to add a new type of whitespace ignoring option, a new type\n> of LCS algorithm, or a new type of post-cleanup heuristics.\n> \n> Reorder bits a bit to give room for these three classes of options\n> to grow. Also make use of XDF_WHITESPACE_FLAGS macro where we check\n> any of these bits are on, instead of using DIFF_XDL_TST() macro on\n> individual possibilities. That way, the \"is any of the bits on?\"\n> code does not have to change when we add more ways to ignore\n> whitespaces.\n> \n> While at it, add a comment in front of the bit definitions to\n> clarify in which structure these defined bits may appear.\n\nMakes sense.\n\n> diff --git a/diff.c b/diff.c\n> index 74283d9001..790250fe86 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -3434,9 +3434,7 @@ void diff_setup_done(struct diff_options *options)\n> \t * inside contents.\n> \t */\n> \n> -\tif (DIFF_XDL_TST(options, IGNORE_WHITESPACE) ||\n> -\t DIFF_XDL_TST(options, IGNORE_WHITESPACE_CHANGE) ||\n> -\t DIFF_XDL_TST(options, IGNORE_WHITESPACE_AT_EOL))\n> +\tif ((options->xdl_opts & XDF_WHITESPACE_FLAGS))\n\nNot only shorter now, but a lot clearer, too: nobody needs to wonder now\nwhether one whitespace flag was excluded on purpose.\n\n> \t\tDIFF_OPT_SET(options, DIFF_FROM_CONTENTS);\n> \telse\n> \t\tDIFF_OPT_CLR(options, DIFF_FROM_CONTENTS);\n> diff --git a/xdiff/xdiff.h b/xdiff/xdiff.h\n> index b090ad8eac..cbf5d8e166 100644\n> --- a/xdiff/xdiff.h\n> +++ b/xdiff/xdiff.h\n> @@ -27,22 +27,26 @@\n> extern \"C\" {\n> #endif /* #ifdef __cplusplus */\n> \n> +/* xpparm_t.flags */\n> +#define XDF_NEED_MINIMAL (1 << 0)\n> \n> -#define XDF_NEED_MINIMAL (1 << 1)\n\nThis change makes me wonder whether the least significant bit was omitted\non purpose originally. You probably looked at that? May be worth\nmentioning in the commit message.\n\n> -#define XDF_IGNORE_WHITESPACE (1 << 2)\n> -#define XDF_IGNORE_WHITESPACE_CHANGE (1 << 3)\n> -#define XDF_IGNORE_WHITESPACE_AT_EOL (1 << 4)\n> -#define XDF_WHITESPACE_FLAGS (XDF_IGNORE_WHITESPACE | XDF_IGNORE_WHITESPACE_CHANGE | XDF_IGNORE_WHITESPACE_AT_EOL)\n> +#define XDF_IGNORE_WHITESPACE (1 << 1)\n> +#define XDF_IGNORE_WHITESPACE_CHANGE (1 << 2)\n> +#define XDF_IGNORE_WHITESPACE_AT_EOL (1 << 3)\n> +#define XDF_WHITESPACE_FLAGS (XDF_IGNORE_WHITESPACE | \\\n> +\t\t\t XDF_IGNORE_WHITESPACE_CHANGE | \\\n> +\t\t\t XDF_IGNORE_WHITESPACE_AT_EOL)\n> \n> -#define XDF_PATIENCE_DIFF (1 << 5)\n> -#define XDF_HISTOGRAM_DIFF (1 << 6)\n> +#define XDF_IGNORE_BLANK_LINES (1 << 7)\n> +\n> +#define XDF_PATIENCE_DIFF (1 << 14)\n> +#define XDF_HISTOGRAM_DIFF (1 << 15)\n> #define XDF_DIFF_ALGORITHM_MASK (XDF_PATIENCE_DIFF | XDF_HISTOGRAM_DIFF)\n> #define XDF_DIFF_ALG(x) ((x) & XDF_DIFF_ALGORITHM_MASK)\n> \n> -#define XDF_IGNORE_BLANK_LINES (1 << 7)\n> -\n> -#define XDF_INDENT_HEURISTIC (1 << 8)\n> +#define XDF_INDENT_HEURISTIC (1 << 23)\n> \n> +/* xdemitconf_t.flags */\n> #define XDL_EMIT_FUNCNAMES (1 << 0)\n> #define XDL_EMIT_FUNCCONTEXT (1 << 2)\n\nIt is a pity that this diff is not easier to review, but it shows how much\nit was in need of cleaning up. Looks much nicer now.\n\nI wonder, however, what your guiding principle was in determining the\ngaps? I would have expected consecutive bits, except for the one gap to\nmake room for the upcoming flag, of course.\n\nFuture patch series could always start by making room for new flags, as\nneeded, after all.\n\nCiao,\nDscho\n"},{"id":"331991","messageId":"alpine.DEB.2.21.1.1711071345430.6482@virtualbox","threadId":"47023","inReplyTo":"20171107064011.18399-3-gitster@pobox.com","subject":"Re: [PATCH v2 2/2] diff: --ignore-cr-at-eol","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-11-07T13:23:00Z","receivedAt":"2017-11-07T13:23:21Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Tue, 7 Nov 2017, Junio C Hamano wrote:\n\n> A new option --ignore-cr-at-eol tells the diff machinery to treat a\n> carriage-return at the end of a (complete) line as if it does not\n> exist.\n> \n> This would make it easier to review a change whose only effect is to\n> turn line endings from CRLF to LF or the other way around.\n\nIf the goal is to make CR/LF -> LF conversions easier to review (or for\nthat matter, LF -> CR/LF), then this option may not be *completely*\nsatisfactory, as it would hide mixed changes (i.e. where some lines are\nconverted from CR/LF to LF and others are converted in the other direction\n*in the same patch*).\n\nI wonder whether it would make this patch series even more useful if you\nwould instead introduce --hide-crlf-to-lf and --hide-lf-to-crlf options\n(not even necessarily mutually exclusive, in case the reviewer really\nwants to ignore all line ending conversions).\n\n> diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt\n> index 89cc0f48de..aa2c0ff74d 100644\n> --- a/Documentation/diff-options.txt\n> +++ b/Documentation/diff-options.txt\n> @@ -519,6 +519,9 @@ endif::git-format-patch[]\n> --text::\n> \tTreat all files as text.\n> \n> +--ignore-cr-at-eol::\n> +\tIgnore carrige-return at the end of line when doing a comparison.\n\nI am not a native speaker, either, yet I have the impression that \"do a\ncomparison\" may be more colloquial than not. Also, it is a carriage-return\n(as in Sinatra's famous song about Love and Marriage) not a carrige-return.\n\nHow about \"Hide changed line endings\"?\n\n> diff --git a/xdiff/xutils.c b/xdiff/xutils.c\n> index 04d7b32e4e..b2cbcc818f 100644\n> --- a/xdiff/xutils.c\n> +++ b/xdiff/xutils.c\n> @@ -156,6 +156,24 @@ int xdl_blankline(const char *line, long size, long flags)\n> \treturn (i == size);\n> }\n> \n> +/*\n> + * Have we eaten everything on the line, except for an optional\n> + * CR at the very end?\n> + */\n> +static int ends_with_optional_cr(const char *l, long s, long i)\n> +{\n> +\tint complete = s && l[s-1] == '\\n';\n> +\n> +\tif (complete)\n> +\t\ts--;\n> +\tif (s == i)\n> +\t\treturn 1;\n\nWhat is the role of `s`, what of `i`? Maybe `length` and `current_offset`?\n\n> +\t/* do not ignore CR at the end of an incomplete line */\n> +\tif (complete && s == i + 1 && l[i] == '\\r')\n> +\t\treturn 1;\n\nThis made me scratch my head: too many negations. The comment may better\nread \"ignore CR only at the end of a complete line\".\n\nAnd now I understand even less why `1` is returned if `s == i`? Is this\nnot an empty line (complete or incomplete) *without* a CR?\n\n> @@ -204,6 +223,14 @@ int xdl_recmatch(const char *l1, long s1, const char *l2, long s2, long flags)\n> \t\t\ti1++;\n> \t\t\ti2++;\n> \t\t}\n> +\t} else if (flags & XDF_IGNORE_CR_AT_EOL) {\n> +\t\t/* Find the first difference and see how the line ends */\n> +\t\twhile (i1 < s1 && i2 < s2 && l1[i1] == l2[i2]) {\n> +\t\t\ti1++;\n> +\t\t\ti2++;\n> +\t\t}\n> +\t\treturn (ends_with_optional_cr(l1, s1, i1) &&\n> +\t\t\tends_with_optional_cr(l2, s2, i2));\n\nThere are extra parentheses around the `return` expression.\n\nTo accommodate the tentative --hide-crlf-to-lf and --hide-lf-to-crlf\noptions that I suggested earlier, this would simply become something like\nthis:\n\n\t} else if (flags & (XDF_IGNORE_CRLF_TO_LF | XDF_IGNORE_LF_TO_CRLF)) {\n\t\t/* Early exit: length must be equal or differ by 1 */\n\t\tif (s1 - i1 != s2 - i2 &&\n\t\t s1 - i1 != s2 + 1 - i2 && s1 + 1 - i1 != s2 - i2)\n\t\t\treturn 0;\n\n\t\t/* Early exit: skip incomplete lines */\n\t\tif (!s1 || !s2 || l1[s1-1] != '\\n' || l2[s2-1] != '\\n')\n\t\t\treturn 0;\n\n\t\t/* Find the first difference and see how the line ends */\n\t\twhile (i1 < s1 && i2 < s2 && l1[i1] == l2[i2]) {\n\t\t\ti1++;\n\t\t\ti2++;\n\t\t}\n\n\t\t/* Lines must be identical or have the indicated EOL change */\n\t\treturn ((i1 == s1 && i2 == s2) ||\n\t\t\t((flags & XDF_IGNORE_CRLF_TO_LF) &&\n\t\t\t i1 + 2 == s1 && l1[i1] == '\\r' && i2 + 1 == s2) ||\n\t\t\t((flags & XDF_IGNORE_LF_TO_CRLF) &&\n\t\t\t i1 + 1 == s1 && i2 + 2 == s2 && l2[i2] == '\\r');\n\nNote: I do not even know whether the code in this function has to assume\nthat the lines can be byte-wise identical or not, I just erred on the side\nof caution. I also do not know whether the return value 0 indicates a\nmistmatch, I just assumed it did. I really wish that I could have reviewed\nthe real code, not a patch in a mail program that lacks the context.\n\nCaveat emptor: my proposed code change has been written in the mail\nprogram (if we had a more code-centric review process, you would have a PR\nwith a suggested alternative already, I am really sorry for the\ninconvenience).\n\nCiao,\nDscho\n"},{"id":"331998","messageId":"xmqqr2tadtdi.fsf@gitster.mtv.corp.google.com","threadId":"47023","inReplyTo":"alpine.DEB.2.21.1.1711071336120.6482@virtualbox","subject":"Re: [PATCH v2 1/2] xdiff: reassign xpparm_t.flags bits","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-11-07T15:02:33Z","receivedAt":"2017-11-07T15:02:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n>> diff --git a/xdiff/xdiff.h b/xdiff/xdiff.h\n>> index b090ad8eac..cbf5d8e166 100644\n>> --- a/xdiff/xdiff.h\n>> +++ b/xdiff/xdiff.h\n>> @@ -27,22 +27,26 @@\n>> extern \"C\" {\n>> #endif /* #ifdef __cplusplus */\n>> \n>> +/* xpparm_t.flags */\n>> +#define XDF_NEED_MINIMAL (1 << 0)\n>> \n>> -#define XDF_NEED_MINIMAL (1 << 1)\n>\n> This change makes me wonder whether the least significant bit was omitted\n> on purpose originally. You probably looked at that? May be worth\n> mentioning in the commit message.\n>\n>> -#define XDF_IGNORE_WHITESPACE (1 << 2)\n>> -#define XDF_IGNORE_WHITESPACE_CHANGE (1 << 3)\n>> -#define XDF_IGNORE_WHITESPACE_AT_EOL (1 << 4)\n>> -#define XDF_WHITESPACE_FLAGS (XDF_IGNORE_WHITESPACE | XDF_IGNORE_WHITESPACE_CHANGE | XDF_IGNORE_WHITESPACE_AT_EOL)\n>> +#define XDF_IGNORE_WHITESPACE (1 << 1)\n>> +#define XDF_IGNORE_WHITESPACE_CHANGE (1 << 2)\n>> +#define XDF_IGNORE_WHITESPACE_AT_EOL (1 << 3)\n>> +#define XDF_WHITESPACE_FLAGS (XDF_IGNORE_WHITESPACE | \\\n>> +\t\t\t XDF_IGNORE_WHITESPACE_CHANGE | \\\n>> +\t\t\t XDF_IGNORE_WHITESPACE_AT_EOL)\n>> \n>> -#define XDF_PATIENCE_DIFF (1 << 5)\n>> -#define XDF_HISTOGRAM_DIFF (1 << 6)\n>> +#define XDF_IGNORE_BLANK_LINES (1 << 7)\n>> +\n>> +#define XDF_PATIENCE_DIFF (1 << 14)\n>> +#define XDF_HISTOGRAM_DIFF (1 << 15)\n>> #define XDF_DIFF_ALGORITHM_MASK (XDF_PATIENCE_DIFF | XDF_HISTOGRAM_DIFF)\n>> #define XDF_DIFF_ALG(x) ((x) & XDF_DIFF_ALGORITHM_MASK)\n>> \n>> -#define XDF_IGNORE_BLANK_LINES (1 << 7)\n>> -\n>> -#define XDF_INDENT_HEURISTIC (1 << 8)\n>> +#define XDF_INDENT_HEURISTIC (1 << 23)\n>> \n>> +/* xdemitconf_t.flags */\n>> #define XDL_EMIT_FUNCNAMES (1 << 0)\n>> #define XDL_EMIT_FUNCCONTEXT (1 << 2)\n>\n> It is a pity that this diff is not easier to review, but it shows how much\n> it was in need of cleaning up. Looks much nicer now.\n>\n> I wonder, however, what your guiding principle was in determining the\n> gaps? I would have expected consecutive bits, except for the one gap to\n> make room for the upcoming flag, of course.\n\nThere wasn't that deep a thought went into it, actually. \n\nI wanted to give room for each \"groupsof bits\" to grow, and gave\neach group a byte. Whitespace bits sits in the first byte (but\nignore-blank-lines work quite different so it grabs the bit from the\nother end), fundamental choices of LCS algos is another group that\nsits in the second byte, then hunk shifting and other heuristics as\nanother group. I guess that the \"need minimal\" thing could have\nbeen thrown into the last group, but somehow I didn't think it would\ngrow that much, so I left it at the lowest place.\n"},{"id":"331999","messageId":"xmqqinemdsxm.fsf@gitster.mtv.corp.google.com","threadId":"47023","inReplyTo":"alpine.DEB.2.21.1.1711071329280.6482@virtualbox","subject":"Re: [PATCH v2 0/2] Teach \"diff\" to ignore only CR at EOL","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-11-07T15:12:05Z","receivedAt":"2017-11-07T15:12:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> Good. I was wishing for such a feature in the past.\n>\n> However, the short and sweet `-b` or `-w` switches are really, really\n> nice. `--ignore-cr-at-eol` is just very cumbersome to type out. So I think\n> you will want to add this patch to your patch series:\n\nYeah, I should probably have added that to 2/2 from the beginning.\n\n> -- snipsnap --\n> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\n> index 0e16f017a41..b7a45e8df29 100644\n> --- a/contrib/completion/git-completion.bash\n> +++ b/contrib/completion/git-completion.bash\n> @@ -1400,7 +1400,7 @@ __git_diff_common_options=\"--stat --numstat --shortstat --summary\n> \t\t\t--patch-with-stat --name-only --name-status --color\n> \t\t\t--no-color --color-words --no-renames --check\n> \t\t\t--full-index --binary --abbrev --diff-filter=\n> -\t\t\t--find-copies-harder\n> +\t\t\t--find-copies-harder --ignore-cr-at-eol\n> \t\t\t--text --ignore-space-at-eol --ignore-space-change\n> \t\t\t--ignore-all-space --ignore-blank-lines --exit-code\n> \t\t\t--quiet --ext-diff --no-ext-diff\n"},{"id":"332019","messageId":"CAGZ79kY3OH_o9w0XqcxevuKTqNWDpjVPn3P8gttO_DgcDekFSw@mail.gmail.com","threadId":"47023","inReplyTo":"xmqqinemdsxm.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2 0/2] Teach \"diff\" to ignore only CR at EOL","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-11-07T17:42:28Z","receivedAt":"2017-11-07T17:42:38Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Nov 7, 2017 at 7:12 AM, Junio C Hamano wrote:\n> Johannes Schindelin writes:\n>\n>> Good. I was wishing for such a feature in the past.\n>>\n>> However, the short and sweet `-b` or `-w` switches are really, really\n>> nice. `--ignore-cr-at-eol` is just very cumbersome to type out. So I think\n>> you will want to add this patch to your patch series:\n>\n> Yeah, I should probably have added that to 2/2 from the beginning.\n>\n>> -- snipsnap --\n>> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\n>> index 0e16f017a41..b7a45e8df29 100644\n>> --- a/contrib/completion/git-completion.bash\n>> +++ b/contrib/completion/git-completion.bash\n>> @@ -1400,7 +1400,7 @@ __git_diff_common_options=\"--stat --numstat --shortstat --summary\n>> --patch-with-stat --name-only --name-status --color\n>> --no-color --color-words --no-renames --check\n>> --full-index --binary --abbrev --diff-filter=\n>> - --find-copies-harder\n>> + --find-copies-harder --ignore-cr-at-eol\n>> --text --ignore-space-at-eol --ignore-space-change\n>> --ignore-all-space --ignore-blank-lines --exit-code\n>> --quiet --ext-diff --no-ext-diff\n\nThat series, with this squash, looks good to me.\n"},{"id":"332049","messageId":"xmqqh8u5ob0h.fsf@gitster.mtv.corp.google.com","threadId":"47023","inReplyTo":"alpine.DEB.2.21.1.1711071345430.6482@virtualbox","subject":"Re: [PATCH v2 2/2] diff: --ignore-cr-at-eol","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-11-08T00:43:42Z","receivedAt":"2017-11-08T00:43:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> On Tue, 7 Nov 2017, Junio C Hamano wrote:\n>\n>> A new option --ignore-cr-at-eol tells the diff machinery to treat a\n>> carriage-return at the end of a (complete) line as if it does not\n>> exist.\n>> \n>> This would make it easier to review a change whose only effect is to\n>> turn line endings from CRLF to LF or the other way around.\n>\n> If the goal is to make CR/LF -> LF conversions easier to review (or for\n> that matter, LF -> CR/LF), then this option may not be *completely*\n> satisfactory, as it would hide mixed changes (i.e. where some lines are\n> converted from CR/LF to LF and others are converted in the other direction\n> *in the same patch*).\n\nYou are 100% right.\n\nThis feature is not about helping to review a patch that wanted to\ndo CRLF-to-LF (or the other way around) conversion at all. Just\nlike the --ignore-space-at-eol is not a feature to make sure that\nthe only thing you did was to remove trailing whitespaces---it will\nalso ignore lines you added trailing whitespaces as irrelevant and\nuninteresting.\n\nIn general, selling these \"--ignore-*\" whitespace options as a tool\nfor such a verification is incorrect.\n\nThese \"--ignore-*\" whitespace options are to help reviewing _other_\nchanges without getting distracted by the class of changes these\noptions represent. I guess I may have to update the log message (I\ndo not think I wrote anything like that in the documentation update).\n\nThanks for pointing it out.\n"},{"id":"332052","messageId":"xmqqd14toaqr.fsf@gitster.mtv.corp.google.com","threadId":"47023","inReplyTo":"xmqqh8u5ob0h.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2 2/2] diff: --ignore-cr-at-eol","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-11-08T00:49:32Z","receivedAt":"2017-11-08T00:49:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> These \"--ignore-*\" whitespace options are to help reviewing _other_\n> changes without getting distracted by the class of changes these\n> options represent. I guess I may have to update the log message (I\n> do not think I wrote anything like that in the documentation update).\n\nHere is what I just did with \"commit --amend\" (there is no change\nin the contents of the patch).\n\n diff: --ignore-cr-at-eol\n \n A new option --ignore-cr-at-eol tells the diff machinery to treat a\n carriage-return at the end of a (complete) line as if it does not\n exist.\n \n Just like other \"--ignore-*\" options to ignore various kinds of\n whitespace differences, this will help reviewing the real changes\n you made without getting distracted by spurious CRLF<->LF conversion\n made by your editor program.\n \n Signed-off-by: Junio C Hamano \n"},{"id":"332595","messageId":"xmqq60ac17yv.fsf@gitster.mtv.corp.google.com","threadId":"47023","inReplyTo":"alpine.DEB.2.21.1.1711071345430.6482@virtualbox","subject":"Re: [PATCH v2 2/2] diff: --ignore-cr-at-eol","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-11-15T04:28:24Z","receivedAt":"2017-11-15T04:28:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\nI notice that I left a few things unanswered even after giving\nanswers to the most important part (i.e. \"what is this for was\nsold incorrectly\"). Here are the leftover bits.\n\n>> diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt\n>> index 89cc0f48de..aa2c0ff74d 100644\n>> --- a/Documentation/diff-options.txt\n>> +++ b/Documentation/diff-options.txt\n>> @@ -519,6 +519,9 @@ endif::git-format-patch[]\n>> --text::\n>> \tTreat all files as text.\n>> \n>> +--ignore-cr-at-eol::\n>> +\tIgnore carrige-return at the end of line when doing a comparison.\n>\n> I am not a native speaker, either, yet I have the impression that \"do a\n> comparison\" may be more colloquial than not. Also, it is a carriage-return\n> (as in Sinatra's famous song about Love and Marriage) not a carrige-return.\n>\n> How about \"Hide changed line endings\"?\n\nThat felt like a good suggestion when I saw your reaction,\nespecially with only the parts visible in the patch and its context,\nbut after reviewing descriptions of other --ignore-* options, I no\nlonger think so. The existing description of \"--ignore-*\" matches\nmanpages of GNU diff and they do not say \"hide\", either.\n\n>> diff --git a/xdiff/xutils.c b/xdiff/xutils.c\n>> index 04d7b32e4e..b2cbcc818f 100644\n>> --- a/xdiff/xutils.c\n>> +++ b/xdiff/xutils.c\n>> @@ -156,6 +156,24 @@ int xdl_blankline(const char *line, long size, long flags)\n>> \treturn (i == size);\n>> }\n>> \n>> +/*\n>> + * Have we eaten everything on the line, except for an optional\n>> + * CR at the very end?\n>> + */\n>> +static int ends_with_optional_cr(const char *l, long s, long i)\n>> +{\n>> +\tint complete = s && l[s-1] == '\\n';\n>> +\n>> +\tif (complete)\n>> +\t\ts--;\n>> +\tif (s == i)\n>> +\t\treturn 1;\n>\n> What is the role of `s`, what of `i`? Maybe `length` and `current_offset`?\n\nI'd agree with that sentiment if this file were not borrowed code\nbut our own, but after looking at xdiff/ code, I think the names of\nthese variables follow the convention used there better, which\nconsistently names the variable for lines they deal with l1, l2, etc.,\ntheir sizes s1, s2, etc., and the indices into the line i1, i2, etc.\n\n>> +\t/* do not ignore CR at the end of an incomplete line */\n>> +\tif (complete && s == i + 1 && l[i] == '\\r')\n>> +\t\treturn 1;\n>\n> This made me scratch my head: too many negations. The comment may better\n> read \"ignore CR only at the end of a complete line\".\n\nPerhaps. \"incomplete line\" is a term with a specific definition\n(and I think by \"complete line\" you mean a line that is not an\nincomplete line), so I do not see the above comment as having too\nmany negations, though. If you feel strongly about it, you could\n\"fix\" it with a follow-up patch.\n\n>> @@ -204,6 +223,14 @@ int xdl_recmatch(const char *l1, long s1, const char *l2, long s2, long flags)\n>> \t\t\ti1++;\n>> \t\t\ti2++;\n>> \t\t}\n>> +\t} else if (flags & XDF_IGNORE_CR_AT_EOL) {\n>> +\t\t/* Find the first difference and see how the line ends */\n>> +\t\twhile (i1 < s1 && i2 < s2 && l1[i1] == l2[i2]) {\n>> +\t\t\ti1++;\n>> +\t\t\ti2++;\n>> +\t\t}\n>> +\t\treturn (ends_with_optional_cr(l1, s1, i1) &&\n>> +\t\t\tends_with_optional_cr(l2, s2, i2));\n>\n> There are extra parentheses around the `return` expression.\n\nYes, everybody knows that return is not a function that needs a\nparentheses around its parameter. I would drop them if this\nexpression were not split into two lines, but because the expression\nis split at &&, I think it reads better with the extra parens. So\nI'll leave them as-is.\n\nThanks.\n\n"}]}