{"thread":{"id":"6335","subject":"Bug-ish: CRLF endings and conflict markers","startedAt":"2007-01-11T09:41:17Z","lastAt":"2007-01-11T11:36:22Z","messageCount":8,"participants":["Andy Parkins","Johannes Schindelin","Shawn O. Pearce"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"31448","messageId":"200701110941.22024.andyparkins@gmail.com","threadId":"6335","inReplyTo":null,"subject":"Bug-ish: CRLF endings and conflict markers","fromName":"Andy Parkins","fromEmail":"andyparkins@gmail.com","sentAt":"2007-01-11T09:41:17Z","receivedAt":"2007-01-11T09:41:17Z","isPatch":false,"sender":{"key":"andyparkins@gmail.com","avatar":null},"body":"Hello,\n\nI am sure the response to this will be \"tough\", however I'll mention it \nanyway. It wasn't a problem for me, as I know git is LF endings only, but \nothers might be a bit confused.\n\n * Track a file that has CR-LF endings. Not a problem, git works fine with \n these and treats the CR as an extra character at the end of the line.\n * Have two branches make conflicting changes to that file\n * Merge, conflict\n * Open the file\n * Find that (almost) every line now displays with a \"^M\" at the end. vim has\n found some lines that don't have CR-LF ends so dropped back to UNIX mode.\n * Be careful that your conflict resolution doesn't introduce any new lines as \n they will not have CR-LF endings.\n\nThe \"(almost)\" above refers of course to the conflict markers. These have LF \nendings only so force the editor into UNIX mode. Assuming a binary safe \neditor, things will be fine if the conflict resolution is simply to remove \nthe markers, and edit the existing lines. However, if the user adds any \nlines during conflict resolution, those new lines will only have LF ends.\n\nThis is a problem for compilers that are expecting CR-LF input.\n\nNote: this is not on Windows (spit), this is simply editing a CR-LF file on \nUNIX.\n\nThe best solution is probably to use the line ending of the conflicted lines. \nI've had a look, but I can only fine builtin-rerere.c that generates the \nmarkers - would that be the place to make this change?\n\n\nAndy\n-- \nDr Andy Parkins, M Eng (hons), MIEE\nandyparkins@gmail.com\n"},{"id":"31450","messageId":"Pine.LNX.4.63.0701111043440.22628@wbgn013.biozentrum.uni-wuerzburg.de","threadId":"6335","inReplyTo":"200701110941.22024.andyparkins@gmail.com","subject":"Re: Bug-ish: CRLF endings and conflict markers","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2007-01-11T09:46:51Z","receivedAt":"2007-01-11T09:46:51Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Thu, 11 Jan 2007, Andy Parkins wrote:\n\n> [describes that conflict markers have LF-only line ending, even if file \n> has CR-LF line endings]\n> \n> The best solution is probably to use the line ending of the conflicted \n> lines.\n\nQuestion is: how to find out. Especially if your file already has mixed \nline endings...\n\n> I've had a look, but I can only fine builtin-rerere.c that generates the \n> markers - would that be the place to make this change?\n\nNo. It would be in xdiff/xmerge.c:{139,147,160}. But I think that xdiff \nreally is LF-only throughout, so you'd have to do much more work anyway.\n\nCiao,\nDscho\n"},{"id":"31452","messageId":"20070111095046.GA28309@spearce.org","threadId":"6335","inReplyTo":"200701110941.22024.andyparkins@gmail.com","subject":"Re: Bug-ish: CRLF endings and conflict markers","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2007-01-11T09:50:46Z","receivedAt":"2007-01-11T09:50:46Z","isPatch":false,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Andy Parkins wrote:\n> The best solution is probably to use the line ending of the conflicted lines. \n> I've had a look, but I can only fine builtin-rerere.c that generates the \n> markers - would that be the place to make this change?\n\nbuiltin-rerere may need to change but the code that's actually\ncreating the conflict markers isn't there. Its somewhere in\nxdiff/xmerge.c. I say somewhere as I haven't dredged down into\nthat code myself, but I know that's where xdl_merge() is and I\nknow its xdl_merge() that actually created the content of the\nconflict file during the merge.\n\nThat said I don't really care about this problem that much.\nThe problem that I care about is its far too easy to convert the\nlineendings in a file (e.g. CRLF->LF, LF->CRLF). This causes the\nentire file to differ, making merges very difficult. I really\nshould just fix it (in the one place where it matters to me) by\nmodifying the pre-commit hook to look for such a case and abort.\n\n-- \nShawn.\n"},{"id":"31454","messageId":"Pine.LNX.4.63.0701111057110.22628@wbgn013.biozentrum.uni-wuerzburg.de","threadId":"6335","inReplyTo":"20070111095046.GA28309@spearce.org","subject":"Re: Bug-ish: CRLF endings and conflict markers","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2007-01-11T09:59:18Z","receivedAt":"2007-01-11T09:59:18Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Thu, 11 Jan 2007, Shawn O. Pearce wrote:\n\n> That said I don't really care about this problem that much. The problem \n> that I care about is its far too easy to convert the lineendings in a \n> file (e.g. CRLF->LF, LF->CRLF). This causes the entire file to differ, \n> making merges very difficult. I really should just fix it (in the one \n> place where it matters to me) by modifying the pre-commit hook to look \n> for such a case and abort.\n\nWhy not just introduce a config variable, and do the conversion in-flight?\n\nOr, alternatively, do the merge ignoring white space? (Of course, this is \nsomewhat pointless when merging whitespace fixes...)\n\nCiao,\nDscho\n"},{"id":"31457","messageId":"20070111101653.GC28309@spearce.org","threadId":"6335","inReplyTo":"Pine.LNX.4.63.0701111057110.22628@wbgn013.biozentrum.uni-wuerzburg.de","subject":"Re: Bug-ish: CRLF endings and conflict markers","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2007-01-11T10:16:53Z","receivedAt":"2007-01-11T10:16:53Z","isPatch":false,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Johannes Schindelin wrote:\n> On Thu, 11 Jan 2007, Shawn O. Pearce wrote:\n> \n> > That said I don't really care about this problem that much. The problem \n> > that I care about is its far too easy to convert the lineendings in a \n> > file (e.g. CRLF->LF, LF->CRLF). This causes the entire file to differ, \n> > making merges very difficult. I really should just fix it (in the one \n> > place where it matters to me) by modifying the pre-commit hook to look \n> > for such a case and abort.\n> \n> Why not just introduce a config variable, and do the conversion in-flight?\n\nThat's a lot of work and goes very much against the Git mindset that\nwe never alter content, just store it as-is. If Linus sees this\nthread I'm suspecting he will jump in and point out that altering\ncontent transparently like this just wrong. I think he's stated\nsomething like that in the past. :-)\n\nAll I want is to make the user realize what they have done. \"Hey\ndummy, you just changed the entire file and the only difference I see\nfor most lines is simply the addition/removal of a CR. You shouldn't\ndo that!\". The pre-commit hook is the perfect place for that.\n\nIt should be pretty easy to do. For every line in the diff stuff\nit into a perl hash after removing any trailing CR. If both an\nadd and a delete for the exact same line of text appear in the diff\n(with the only difference being the CR on the end) and the number\nof such lines number is at least 50% of the modified lines in the\nfile, something's amiss. Smack the user and tell them to fix the\nfile.\n \n> Or, alternatively, do the merge ignoring white space? (Of course, this is \n> somewhat pointless when merging whitespace fixes...)\n\nLets not go down that road. That's just asking for trouble.\nAnd it sounds like a lot of work from what you pointed out in\nanother message.\n\n-- \nShawn.\n"},{"id":"31458","messageId":"Pine.LNX.4.63.0701111123280.22628@wbgn013.biozentrum.uni-wuerzburg.de","threadId":"6335","inReplyTo":"20070111101653.GC28309@spearce.org","subject":"Re: Bug-ish: CRLF endings and conflict markers","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2007-01-11T10:26:12Z","receivedAt":"2007-01-11T10:26:12Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Thu, 11 Jan 2007, Shawn O. Pearce wrote:\n\n> Johannes Schindelin wrote:\n> > On Thu, 11 Jan 2007, Shawn O. Pearce wrote:\n> > \n> > > That said I don't really care about this problem that much. The problem \n> > > that I care about is its far too easy to convert the lineendings in a \n> > > file (e.g. CRLF->LF, LF->CRLF). This causes the entire file to differ, \n> > > making merges very difficult. I really should just fix it (in the one \n> > > place where it matters to me) by modifying the pre-commit hook to look \n> > > for such a case and abort.\n> > \n> > Why not just introduce a config variable, and do the conversion in-flight?\n> \n> That's a lot of work and goes very much against the Git mindset that\n> we never alter content, just store it as-is.\n\nWhile that is correct, we also _use_ the content, and very much alter it \nall the time. A diff, for example, is nothing but altered content.\n\n> All I want is to make the user realize what they have done. \"Hey dummy, \n> you just changed the entire file and the only difference I see for most \n> lines is simply the addition/removal of a CR. You shouldn't do that!\". \n> The pre-commit hook is the perfect place for that.\n\nThis sounds sensible. I mistook your task to be integrator for people you \ncan't smack.\n\n> > Or, alternatively, do the merge ignoring white space? (Of course, this \n> > is somewhat pointless when merging whitespace fixes...)\n> \n> Lets not go down that road. That's just asking for trouble. And it \n> sounds like a lot of work from what you pointed out in another message.\n\nBut to the contrary, ignoring white space in xdl_merge() is just a \nquestion of passing XDF_IGNORE_WHITESPACE_CHANGE as part of the xpparam's.\n\nCiao,\nDscho\n"},{"id":"31460","messageId":"20070111104111.GF28309@spearce.org","threadId":"6335","inReplyTo":"Pine.LNX.4.63.0701111123280.22628@wbgn013.biozentrum.uni-wuerzburg.de","subject":"Re: Bug-ish: CRLF endings and conflict markers","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2007-01-11T10:41:11Z","receivedAt":"2007-01-11T10:41:11Z","isPatch":false,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Johannes Schindelin wrote:\n> On Thu, 11 Jan 2007, Shawn O. Pearce wrote:\n> > That's a lot of work and goes very much against the Git mindset that\n> > we never alter content, just store it as-is.\n> \n> While that is correct, we also _use_ the content, and very much alter it \n> all the time. A diff, for example, is nothing but altered content.\n\nYes, of course. But we don't take A and store A' in the ODB during\na commit. We always assert that if you give us A, you'll get back A.\nYou can always ask us to compute some sort of A' later on, but that\nis always based on the originally stored A.\n\nYou can also ask us to combine an A and B and get some new thing C.\nBut again, we don't then go off and store C' instead of C.\n \n> > All I want is to make the user realize what they have done. \"Hey dummy, \n> > you just changed the entire file and the only difference I see for most \n> > lines is simply the addition/removal of a CR. You shouldn't do that!\". \n> > The pre-commit hook is the perfect place for that.\n> \n> This sounds sensible. I mistook your task to be integrator for people you \n> can't smack.\n\nTo some extent I am. But I have patched the local Cygwin package\nI use to distribute Git to everyone to have a 1 line pre-commit\nupdate hook (and no other hooks) which just invokes a Perl script\nin /usr/local/bin, and that Perl script is also in the same Cygwin\npackage.\n\nSo Cygwin's \"feature\" of making all Git hooks executable by default\nin a new repository plays in my favor here. The pre-commit hook is\nactive in every repository by default, and it just execs a standard\nscript which can be modified at any time. So I can easily update\neveryone's pre-commit hook at once.\n\nCurrently the hook script is just the stock one that comes with Git,\nexcept it ignores CRLF line endings. I'll likely tweak it as I'm\nsuggesting, make the CRLF->LF/LF->CRLF check an optional feature\nthat can be toggled on/off in the top of the script, and submit the\npatch to update the stock hook. Others may find the check useful.\n\n-- \nShawn.\n"},{"id":"31464","messageId":"200701111136.23864.andyparkins@gmail.com","threadId":"6335","inReplyTo":"Pine.LNX.4.63.0701111043440.22628@wbgn013.biozentrum.uni-wuerzburg.de","subject":"Re: Bug-ish: CRLF endings and conflict markers","fromName":"Andy Parkins","fromEmail":"andyparkins@gmail.com","sentAt":"2007-01-11T11:36:22Z","receivedAt":"2007-01-11T11:36:22Z","isPatch":false,"sender":{"key":"andyparkins@gmail.com","avatar":null},"body":"On Thursday 2007 January 11 09:46, Johannes Schindelin wrote:\n\n> > The best solution is probably to use the line ending of the conflicted\n> > lines.\n>\n> Question is: how to find out. Especially if your file already has mixed\n> line endings...\n\nThat's why I said use the line endings of the conflicted lines. Even if the \nfile is mixed, at least lines added are the same as the ones near them. \nLet's say for example a conflict like this:\n\n<<<<\nWhole file has DOS endings^M\n====\nThe whole file has DOS endings^M\n>>>>\n\nAs both of the conflicted parts end in CRLF, the conflict markers would \ndefault to having CRLF. This would cope with mixed-ending files as well, as \nthe ending is always determined locally.\n\nIf we were being really clever, we could make each marker use the ending of \nthe line following it; making it impossible to accuse git of doing anything \nworse than already existed.\n\n> No. It would be in xdiff/xmerge.c:{139,147,160}. But I think that xdiff\n\nThanks; I see why I couldn't find it: it's generated as a loop \"marker_size\" \nlong, rather than the literal that is in rerere. I've had a quick glance at \nxdl_fill_merge_buffer() and can see it's not an easy thing to do (at least \nfor me). Maybe if it itches me a bit more I'll put some effort into my \nscratching :-)\n\n> really is LF-only throughout, so you'd have to do much more work anyway.\n\nThat doesn't seem to be a problem. git is performing very nicely on the CR-LF \nfile I'm tracking. As I mentioned, it's treating the CR as just another \ncharacter on the line - perfect: merges, diffs, diffstats all work just fine.\n\n\nAndy\n-- \nDr Andy Parkins, M Eng (hons), MIEE\nandyparkins@gmail.com\n"}]}