{"thread":{"id":"44767","subject":"git-apply: warn/fail on *changed* end of line (eol) *only*?","startedAt":"2016-12-25T23:49:33Z","lastAt":"2016-12-27T22:15:07Z","messageCount":6,"participants":["Igor Djordjevic BugA","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"308329","messageId":"ac97f925-d930-0592-0a2a-66c9218b1417@gmail.com","threadId":"44767","inReplyTo":null,"subject":"git-apply: warn/fail on *changed* end of line (eol) *only*?","fromName":"Igor Djordjevic BugA","fromEmail":"igor.d.djordjevic@gmail.com","sentAt":"2016-12-25T23:49:24Z","receivedAt":"2016-12-25T23:49:33Z","isPatch":false,"sender":{"key":"igor.d.djordjevic@gmail.com","avatar":null},"body":"Hi to all,\n\nThis is a follow-up of a message posted at git-users Google group[1],\nas the topic may actually be better suited for this mailing list\ninstead. If needed, some elaboration on the context can be found there,\nas well as a detailed example describing the motive for the question\nitself (or directly here[2], second half of the message).\n\nIn short -- git-apply warns on applying the patch with CRLF line endings\n(new), considered whitespace errors, even when previous hunk version\n(old) has/had that very same CRLF line endings, too, so nothing actually\nchanged in this regards. Even worse, it happily applies a patch with LF\nline endings (new) without any warning/hint, even though previous (old)\nline endings were CRLF, thus effectively (and silently) breaking the\n(previous) line endings.\n\nI understand that what should be considered \"correct\" line endings can\nbe set explicitly (and should be communicated on the project level in\nthe first place), but would it make sense to have an additional\ngit-apply --whitespace option (like \"warn-changed\" and \"error-changed\",\nor something) to warn/fail on *changed* end of line *only*, without any\nfurther knowledge needed?\n\nSo not to have Git need to assume or know which line endings should be\nconsidered \"correct\", nor to think/bother too much with it, but just to\nwarn/fail on actual line ending *change* (in comparison between old and\nnew part/hunk of the patch).\n\nThis seems more intuitive to me, and much more cross-platform\ncollaboration friendly, at least as an option, as one wouldn`t need to\nthink much about \"correct\" project/file line endings (which would still\nbe advised, anyway), but would be promptly warned about possibly\nbreaking previous/existing line endings, maybe even with an option to\nkeep those previous ones, or force the new ones... yet as I`m still new\naround, I accept that I might be missing some bigger picture here :)\n\nThanks, BugA\n\nP.S. I`m discussing git-apply and end-of-line handling here in\nparticular as that`s what I`m currently concerned with, and for the sake\nof simplicity, but might be that whole thing could theoretically be\nbroadened to other Git tools (like git-diff) and whitespace (error)\nhandling in general, too (warn/fail only if actually introduced in new\nhunk, not previously being present in the old one).\n--\n[1] https://groups.google.com/d/msg/git-users/9Os_48yJdn0/j9S_W7h-EAAJ\n[2] https://groups.google.com/d/msg/git-users/9Os_48yJdn0/5S9Ja1fEEAAJ\n"},{"id":"308330","messageId":"a3c1d05a-79f7-f3c8-6771-e68e8d5bb4f5@gmail.com","threadId":"44767","inReplyTo":"ac97f925-d930-0592-0a2a-66c9218b1417@gmail.com","subject":"Re: git-apply: warn/fail on *changed* end of line (eol) *only*?","fromName":"Igor Djordjevic BugA","fromEmail":"igor.d.djordjevic@gmail.com","sentAt":"2016-12-25T23:55:59Z","receivedAt":"2016-12-25T23:56:25Z","isPatch":false,"sender":{"key":"igor.d.djordjevic@gmail.com","avatar":null},"body":"On 26/12/2016 00:49, Igor Djordjevic BugA wrote:\n> This is a follow-up of a message posted at git-users Google group[1],\n> as the topic may actually be better suited for this mailing list\n> instead. If needed, some elaboration on the context can be found there,\n> as well as a detailed example describing the motive for the question\n> itself (or directly here[2], second half of the message).\n\nFor some reason reference links got excluded, so here they are again:\n\n[1] https://groups.google.com/d/msg/git-users/9Os_48yJdn0/j9S_W7h-EAAJ\n[2] https://groups.google.com/d/msg/git-users/9Os_48yJdn0/5S9Ja1fEEAAJ\n"},{"id":"308331","messageId":"xmqqvau7cqy1.fsf@gitster.mtv.corp.google.com","threadId":"44767","inReplyTo":"ac97f925-d930-0592-0a2a-66c9218b1417@gmail.com","subject":"Re: git-apply: warn/fail on *changed* end of line (eol) *only*?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-12-26T03:25:10Z","receivedAt":"2016-12-26T03:25:19Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Igor Djordjevic BugA <igor.d.djordjevic@gmail.com> writes:\n\n> In short -- git-apply warns on applying the patch with CRLF line endings\n> (new), considered whitespace errors, even when previous hunk version\n> (old) has/had that very same CRLF line endings, too, so nothing actually\n> changed in this regards. Even worse, it happily applies a patch with LF\n> line endings (new) without any warning/hint, even though previous (old)\n> line endings were CRLF, thus effectively (and silently) breaking the\n> (previous) line endings.\n\nLet me see if I understood your problem description correctly.\n\nImagine that the project wants LF line endings, i.e. it considers\nthat a line with CRLF ending has an unwanted \"whitespace\" at the\nend.  Now, you start from this source file:\n\n    1 <CRLF>\n    3 <CRLF>\n    5 <CRLF>\n\nand a patch like this comes in:\n\n     1 <CRLF>\n    -3 <CRLF>\n    +three <CRLF>\n     5 <CRLF>\n\nYou think that \"3 <CRLF>\" was replaced by \"three <CRLF>\", and the\nclaim is \"the 'previous' contents already had <CRLF> ending, so the\nchange is not making things worse\".\n\nBut what if the patch was like this?\n\n     1 <CRLF>\n    -3 <CRLF>\n    +three <CRLF>\n    +four <CRLF>\n     5 <CRLF>\n\nDo you want to warn on \"four <CRLF>\" because it does not have any\n\"previous\" corresponding line?\n\nExtending the thought further, which line do you want to warn and\nwhich line do you not want to, if the patch were like this instead?\n\n     1 <CRLF>\n    -3 <CRLF>\n    +four <CRLF>\n    +three <CRLF>\n     5 <CRLF>\n\nExtending this thought experiment further, you would realize that\nfundamentally the concept of \"previous contents\" has no sensible\ndefinition.\n\nIncidentally, not realizing \"there is no sensible definition of\n'previous contents'\" is a source of another often seen confusion by\nnew users of Git and any version control systems regarding the\n\"blame\" feature.  When given the final answer by \"blame\", they often\nsay \"what content did the line have _before_ the commit blame gave\nme?\"  As there is no sensible definition of 'previous contents' that\ncorresponds to the line, that question does not in general have a\nsensible answer.\n"},{"id":"308363","messageId":"e08ebc36-1f1a-871d-53aa-b42770ace044@gmail.com","threadId":"44767","inReplyTo":"xmqqvau7cqy1.fsf@gitster.mtv.corp.google.com","subject":"Re: git-apply: warn/fail on *changed* end of line (eol) *only*?","fromName":"Igor Djordjevic BugA","fromEmail":"igor.d.djordjevic@gmail.com","sentAt":"2016-12-27T00:26:55Z","receivedAt":"2016-12-27T00:27:06Z","isPatch":false,"sender":{"key":"igor.d.djordjevic@gmail.com","avatar":null},"body":"Hi Junio, and thanks for sharing your thoughts.\n\nYou understood it pretty well, except the context \"scope\". Example does\nshow a single line change, and a single line old/new comparison, but\nthat is for simplicity sake of the initial example. What I was\ndiscussing about was the whole patch \"hunk\" instead (mentioned inside\nthe part you quoted, too).\n\n> But what if the patch was like this?\n> \n>      1 <CRLF>\n>     -3 <CRLF>\n>     +three <CRLF>\n>     +four <CRLF>\n>      5 <CRLF>\n> \n> Do you want to warn on \"four <CRLF>\" because it does not have any\n> \"previous\" corresponding line?\n\nNo, because the old hunk (single - line) has <CRLF> line ending(s), the\nsame as the new hunk (both + lines), so effectively there is no end of\nline change between old and new _hunks_, so no warning needed.\n\n> Extending the thought further, which line do you want to warn and\n> which line do you not want to, if the patch were like this instead?\n> \n>      1 <CRLF>\n>     -3 <CRLF>\n>     +four <CRLF>\n>     +three <CRLF>\n>      5 <CRLF>\n\nAgain, no warnings here, same as described above.\n\n> Extending this thought experiment further, you would realize that\n> fundamentally the concept of \"previous contents\" has no sensible\n> definition.\n\nThis one is interesting, and maybe arguable - I certainly don`t have\nenough knowledge about the overall matter (Git heuristics for diff\ncreation, in terms of which parts get marked as \"old\" (-), and which as\n\"new\" (+), which probably even depends on user defined settings)...\n\n... BUT, the overall idea here seems rather simple (to me :P) - if *all*\nremoved lines inside the old hunk have the same line endings (<CRLF>,\nfor example), where there is *any* (at least one) added line inside the\nnew hunk that has a different line ending than the ones in the old hunk\n(like <LF>), then warning or failure might be preferred (optionally,\nat least), as it does seem like something fishy is going on.\n\nI appreciate and understand a need for Git being able to know \"correct\"\nline endings, but in case where the whole previous hunk (or the whole\nfile, even) has \"incorrect\" line endings (<CRLF>, as far as Git is\nconcerned, as in the given example), it seems more sensible to me for\nGit to warn you about _that_ first, rather than keep silent and apply a\nnew hunk introducing <LF> line endings without you even knowing - heck,\nmaybe your core end of line setting is incorrect, indeed, so actually\nhaving someone to let you know seems nice, before potentially corrupting\nyour file.\n\nIt felt like - \"Hey, I do have a mean to know that your _whole file_\nhas incorrect line endings (as per my current settings), but I don`t\nwant to tell you, yet I`ll rather happily apply this patch which\nintroduces n lines with _correct_ line endings... So, now you still have\nyour whole incorrect file, but don`t worry, I fixed those n lines for\nyou behind your back, life is good, and you`re welcome.\"\n\nOk, I might be exaggerating, but it just doesn`t seem right, the end\nresult seems even worse - having \"incorrect\" and \"correct\" endings\nsilently mixed.\n\nTo prevent this, the most important question seems to be - do we have a\nsensible way to tell \"this is THE line ending of the old hunk (the only\nline ending used)\"?\n\nAnd the worst case answer would probably be - you need to check the\nwhole (old) file, if all line endings are the same, that is THE line\nending you`ll use for comparison against line endings of the newly added\nlines.\n\nFor the best case scenario, I don`t know enough of Git diff heuristics\nto say if we would even be able to determine THE line endings based on\nthe old _hunk_ only, not to examine the whole old file.\n\n_If_ we can find THE old hunk (or file) line endings, and patch\nintroduces different ones, then warn/fail option seems sensible...?\n\nOf course, in case where old hunk/file already has mixed line endings\n(like both <CRLF> and <LF>), we can either choose to warn right away (as\nmixed lines endings are found already), or maybe even not warn at all,\nas nothing actually changes in regards to line endings no matter which\none gets added, and we`re discussing \"warn/fail on eol *change* only\".\n\nOtherwise, I agree about \"previous contents\" sensibility, especially in\nthe \"blame\" case, but this does seem a bit different... or does it?\n\nRegards, BugA\n"},{"id":"308391","messageId":"xmqq8tr1cmf9.fsf@gitster.mtv.corp.google.com","threadId":"44767","inReplyTo":"xmqqvau7cqy1.fsf@gitster.mtv.corp.google.com","subject":"Re: git-apply: warn/fail on *changed* end of line (eol) *only*?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-12-27T17:27:22Z","receivedAt":"2016-12-27T17:27:39Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Imagine that the project wants LF line endings, i.e. it considers\n> that a line with CRLF ending has an unwanted \"whitespace\" at the\n> end.  Now, you start from this source file:\n>\n>     1 <CRLF>\n>     3 <CRLF>\n>     5 <CRLF>\n>\n> and a patch like this comes in:\n>\n>      1 <CRLF>\n>     -3 <CRLF>\n>     +three <CRLF>\n>      5 <CRLF>\n>\n> You think that \"3 <CRLF>\" was replaced by \"three <CRLF>\", and the\n> claim is \"the 'previous' contents already had <CRLF> ending, so the\n> change is not making things worse\".\n\nTo see the problem with \"check existing lines\", it probably is\neasier to extend the above example to start from a file with one\nmore line, like this:\n\n    1 <CRLF>\n    3 <CRLF>\n    4 <LF>\n    5 <CRLF>\n\nand extend all the example patches to remove \"4 <LF>\" line as well,\nwhere they remove \"3 <CRLF>\", making the first example patch like\nso:\n\n     1 <CRLF>\n    -3 <CRLF>\n    -4 <LF>\n    +three <CRLF>\n     5 <CRLF>\n\nNow, if you take \"three <CRLF>\" to be replacing \"3 <CRLF>\", then you\nmay feel that not warning on the CRLF would be the right thing, but\nthere is no reason (other than the fact you, a human, understand\nwhat 'three' means) to choose \"3 <CRLF>\" over \"4 <LF>\" as the\noriginal.  If you take \"three <CRLF>\" to be replacing \"4 <LF>\", you\nwould need to warn.\n\nA totally uninteresting special case is when the original is all\n<CRLF>, but in that case, as you already said in the original\nmessage, the project wants <CRLF> and you can configure it as such.\nThen <CR> in the line-end <CRLF> won't be mistaken as if it is a\nwhitespace character <CR> at the end of a line terminated with <LF>.\n"},{"id":"308422","messageId":"00a4a54c-bc8e-9fef-0f70-232c85b0139b@gmail.com","threadId":"44767","inReplyTo":"xmqq8tr1cmf9.fsf@gitster.mtv.corp.google.com","subject":"Re: git-apply: warn/fail on *changed* end of line (eol) *only*?","fromName":"Igor Djordjevic BugA","fromEmail":"igor.d.djordjevic@gmail.com","sentAt":"2016-12-27T22:14:40Z","receivedAt":"2016-12-27T22:15:07Z","isPatch":false,"sender":{"key":"igor.d.djordjevic@gmail.com","avatar":null},"body":"On 27/12/2016 18:27, Junio C Hamano wrote:\n\n> To see the problem with \"check existing lines\", it probably is\n> easier to extend the above example to start from a file with one\n> more line, like this:\n>\n>     1 <CRLF>\n>     3 <CRLF>\n>     4 <LF>\n>     5 <CRLF>\n>\n> and extend all the example patches to remove \"4 <LF>\" line as well,\n> where they remove \"3 <CRLF>\", making the first example patch like\n> so:\n>\n>      1 <CRLF>\n>     -3 <CRLF>\n>     -4 <LF>\n>     +three <CRLF>\n>      5 <CRLF>\n>\n> Now, if you take \"three <CRLF>\" to be replacing \"3 <CRLF>\", then you\n> may feel that not warning on the CRLF would be the right thing, but\n> there is no reason (other than the fact you, a human, understand\n> what 'three' means) to choose \"3 <CRLF>\" over \"4 <LF>\" as the\n> original.  If you take \"three <CRLF>\" to be replacing \"4 <LF>\", you\n> would need to warn.\n\nHmm, but why do you keep insisting on knowing what the lines are about,\nand still making a 'per line' end of line comparison? Besides, your\nexample falls within the (only?) special case I mentioned as the one Git\nprobably shouldn`t be acting upon, as both <LF> and <CRLF> are present\namong the the old hunk lines already (lines starting with '-').\n\nThe logic should be simple, what I tried to describe in the previous\nmessage:\n\n  1. Examine all '-' lines line endings.\n\n  1.1. If not all line endings are the same (like in your example, no\n       matter the line content), do nothing.\n\t\n  1.2. If all line endings _are_ the same within the old hunk ('-'\n       lines), check if any of the '+' lines (new hunk) has a different\n       line ending (still no matter the line content).\n\t\n  1.2.1. If no different line endings among '+' lines in comparison to\n         unique line ending found in '-' lines, do nothing.\n\n  1.2.2. If _any_ of the '+' lines _has_ a different line ending in\n         comparison to unique line ending found in '-' lines, then do\n         warn/fail.\n\t\t\nThis even might seem as the most sensible approach, no matter the\noverall project end of line setting, which is exactly why I find it\ninteresting - it seems to 'just work', being beginner friendly _and_\nalso watching your back on unexpected end of line changes.\n\n> A totally uninteresting special case is when the original is all\n> <CRLF>, but in that case, as you already said in the original\n> message, the project wants <CRLF> and you can configure it as such.\n> Then <CR> in the line-end <CRLF> won't be mistaken as if it is a\n> whitespace character <CR> at the end of a line terminated with <LF>.\n\nBut this is exactly the most confusing case, especially for beginners,\nwhere project configuration is incorrect, and you get warned about\n'whitespace errors' where all line endings (old/new) are in fact still\nthe same (confusing).\n\nYet worse, you don`t get warned of different line endings being applied,\nas these are considered 'correct' due to current (incorrect) setting, no\nmatter the whole file is actually different, which you don`t get warned\nabout in the first place, and you may discover the file line ending\nbreakage only when other contributors start complaining.\n\nAlso, would it be sensible to account for a possible file inside the\nproject having different line endings than project in general...? This\nwould help there as well, without unintentionally breaking the file.\n\nRegards, BugA\n\nP.S. I guess you don`t need me to tell you that, but please feel free\nto drop out of this discussion at your own discretion, even though I\nenjoy sharing thoughts (and learning new stuff!), I do kind of feel I`m\nwasting your time for something that might not be that important, if at\nall, otherwise someone would have probably addressed it so far :/\n"}]}