{"thread":{"id":"51009","subject":"[PATCH 0/1] Fix cvsexportcommit with CR/LF line endings","startedAt":"2019-04-29T21:58:50Z","lastAt":"2019-05-08T10:59:59Z","messageCount":7,"participants":["Johannes Schindelin via GitGitGadget","Dustin Spicuzza via GitGitGadget","Junio C Hamano","Dustin Spicuzza","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":1},"messages":[{"id":"374673","messageId":"pull.132.git.gitgitgadget@gmail.com","threadId":"51009","inReplyTo":null,"subject":"[PATCH 0/1] Fix cvsexportcommit with CR/LF line endings","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-04-29T21:58:46Z","receivedAt":"2019-04-29T21:58:50Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"When using CVSNT, we need to be prepared to grok CVS output with CR/LF line\nendings.\n\nWhile we do not use CVSNT in Git for Windows, this patch made its way into\nGit's source code via the Git for Windows project (probably because of the\nease of GitHub PRs), and we had not a single complaint for over 2 years, so\nI would consider this patch ready to be included in core Git.\n\nDustin Spicuzza (1):\n cvsexportcommit: force crlf translation\n\n git-cvsexportcommit.perl | 1 +\n 1 file changed, 1 insertion(+)\n\n\nbase-commit: 39ffebd23b1ef6830bf86043ef0b5c069d9299a9\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-132%2Fdscho%2Fcvsexportcommit-crlf-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-132/dscho/cvsexportcommit-crlf-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/132\n-- \ngitgitgadget\n"},{"id":"374674","messageId":"9b508af11be125ce5fd80c40c732dbcaf45067fb.1556575126.git.gitgitgadget@gmail.com","threadId":"51009","inReplyTo":"pull.132.git.gitgitgadget@gmail.com","subject":"[PATCH 1/1] cvsexportcommit: force crlf translation","fromName":"Dustin Spicuzza via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-04-29T21:58:47Z","receivedAt":"2019-04-29T21:58:51Z","isPatch":true,"sender":{"key":"dustin@virtualroadside.com","avatar":null},"body":"From: Dustin Spicuzza \n\nWhen using cvsnt + msys + git, it seems like the output of cvs status\nhad \\r\\n in it, and caused the command to fail.\n\nThis fixes that.\n\nSigned-off-by: Dustin Spicuzza \nSigned-off-by: Johannes Schindelin \n---\n git-cvsexportcommit.perl | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/git-cvsexportcommit.perl b/git-cvsexportcommit.perl\nindex d13f02da95..fc00d5946a 100755\n--- a/git-cvsexportcommit.perl\n+++ b/git-cvsexportcommit.perl\n@@ -431,6 +431,7 @@ END\n sub safe_pipe_capture {\n my @output;\n if (my $pid = open my $child, '-|') {\n+\tbinmode($child, \":crlf\");\n \t@output = (<$child>);\n \tclose $child or die join(' ',@_).\": $! $?\";\n } else {\n-- \ngitgitgadget\n"},{"id":"375002","messageId":"xmqqtve6lito.fsf@gitster-ct.c.googlers.com","threadId":"51009","inReplyTo":"9b508af11be125ce5fd80c40c732dbcaf45067fb.1556575126.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/1] cvsexportcommit: force crlf translation","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-05-07T09:22:59Z","receivedAt":"2019-05-07T09:23:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Dustin Spicuzza via GitGitGadget\" writes:\n\n> From: Dustin Spicuzza \n>\n> When using cvsnt + msys + git, it seems like the output of cvs status\n> had \\r\\n in it, and caused the command to fail.\n\nThis is a bit under-explained in that it does not make it clear\nwhere the right place to fix would be. From \"X did Y which caused\nthe command to fail\", a possible right fix could be \"so fix it by\ntelling X not to do Y\", but of course a patch to fix cvsnt won't\ncome to this list ;-)\n\nI haven't thought things through, so let's think it aloud and\nenumerate what should have been explained in the log message here.\n\n - With binmode(\":crlf\"), (i.e. if the platform uses CRLF convert\n external CRLF into internal '\\n'), the change hopes not to affect\n platforms that do not use CRLF.\n\n - \"it SEEMS LIKE the output of cvs status had CRLF in it\", i.e. it\n is uncertain if everybody on the platform has the same issue.\n But by using binmode(\":crlf\"), if some other implementations of\n \"cvs status\" on the platform used LF, they won't get negatively\n affected by this change, either.\n\nSo, I guess the change is safe enough that it does not hurt other\npeople.\n\n>\n> This fixes that.\n>\n> Signed-off-by: Dustin Spicuzza \n> Signed-off-by: Johannes Schindelin \n> ---\n> git-cvsexportcommit.perl | 1 +\n> 1 file changed, 1 insertion(+)\n>\n> diff --git a/git-cvsexportcommit.perl b/git-cvsexportcommit.perl\n> index d13f02da95..fc00d5946a 100755\n> --- a/git-cvsexportcommit.perl\n> +++ b/git-cvsexportcommit.perl\n> @@ -431,6 +431,7 @@ END\n> sub safe_pipe_capture {\n> my @output;\n> if (my $pid = open my $child, '-|') {\n> +\tbinmode($child, \":crlf\");\n> \t@output = (<$child>);\n> \tclose $child or die join(' ',@_).\": $! $?\";\n> } else {\n"},{"id":"375027","messageId":"4da677fb-47c3-a1ac-e7e0-6b74cdb7334c@virtualroadside.com","threadId":"51009","inReplyTo":"xmqqtve6lito.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 1/1] cvsexportcommit: force crlf translation","fromName":"Dustin Spicuzza","fromEmail":"dustin@virtualroadside.com","sentAt":"2019-05-07T13:51:55Z","receivedAt":"2019-05-07T13:57:08Z","isPatch":true,"sender":{"key":"dustin@virtualroadside.com","avatar":null},"body":"Yes, your interpretation is exactly correct, even the interpretation of\nthe uncertainty of the message.\n\nI didn't send the patch to this list though, not sure why gitgitgadget\ndecided to do so (I've never heard of it before now). The patch was\noriginally submitted to git for windows (PR #938), with an even worse\ncommit message. :)\n\nHowever, I'm not opposed to it being merged elsewhere. As you pointed\nout, it shouldn't negatively affect non-CRLF platforms.\n\nDustin\n\nOn 5/7/19 5:22 AM, Junio C Hamano wrote:\n> \"Dustin Spicuzza via GitGitGadget\" writes:\n>\n>> From: Dustin Spicuzza \n>>\n>> When using cvsnt + msys + git, it seems like the output of cvs status\n>> had \\r\\n in it, and caused the command to fail.\n> This is a bit under-explained in that it does not make it clear\n> where the right place to fix would be. From \"X did Y which caused\n> the command to fail\", a possible right fix could be \"so fix it by\n> telling X not to do Y\", but of course a patch to fix cvsnt won't\n> come to this list ;-)\n>\n> I haven't thought things through, so let's think it aloud and\n> enumerate what should have been explained in the log message here.\n>\n> - With binmode(\":crlf\"), (i.e. if the platform uses CRLF convert\n> external CRLF into internal '\\n'), the change hopes not to affect\n> platforms that do not use CRLF.\n>\n> - \"it SEEMS LIKE the output of cvs status had CRLF in it\", i.e. it\n> is uncertain if everybody on the platform has the same issue.\n> But by using binmode(\":crlf\"), if some other implementations of\n> \"cvs status\" on the platform used LF, they won't get negatively\n> affected by this change, either.\n>\n> So, I guess the change is safe enough that it does not hurt other\n> people.\n>\n>> This fixes that.\n>>\n>> Signed-off-by: Dustin Spicuzza \n>> Signed-off-by: Johannes Schindelin \n>> ---\n>> git-cvsexportcommit.perl | 1 +\n>> 1 file changed, 1 insertion(+)\n>>\n>> diff --git a/git-cvsexportcommit.perl b/git-cvsexportcommit.perl\n>> index d13f02da95..fc00d5946a 100755\n>> --- a/git-cvsexportcommit.perl\n>> +++ b/git-cvsexportcommit.perl\n>> @@ -431,6 +431,7 @@ END\n>> sub safe_pipe_capture {\n>> my @output;\n>> if (my $pid = open my $child, '-|') {\n>> +\tbinmode($child, \":crlf\");\n>> \t@output = (<$child>);\n>> \tclose $child or die join(' ',@_).\": $! $?\";\n>> } else {\n\n"},{"id":"375029","messageId":"xmqqd0kul5w0.fsf@gitster-ct.c.googlers.com","threadId":"51009","inReplyTo":"4da677fb-47c3-a1ac-e7e0-6b74cdb7334c@virtualroadside.com","subject":"Re: [PATCH 1/1] cvsexportcommit: force crlf translation","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-05-07T14:02:23Z","receivedAt":"2019-05-07T14:02:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Dustin Spicuzza writes:\n\n> Yes, your interpretation is exactly correct, even the interpretation of\n> the uncertainty of the message.\n>\n> I didn't send the patch to this list though, not sure why gitgitgadget\n> decided to do so (I've never heard of it before now). The patch was\n> originally submitted to git for windows (PR #938), with an even worse\n> commit message. :)\n\nThe credit goes to Dscho, making effort to slim down patches Git for\nWindows carries on top of my tree by upstreaming them.\n"},{"id":"375109","messageId":"nycvar.QRO.7.76.6.1905081248190.44@tvgsbejvaqbjf.bet","threadId":"51009","inReplyTo":"xmqqtve6lito.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 1/1] cvsexportcommit: force crlf translation","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-05-08T10:56:26Z","receivedAt":"2019-05-08T10:56:34Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Tue, 7 May 2019, Junio C Hamano wrote:\n\n> \"Dustin Spicuzza via GitGitGadget\" writes:\n>\n> > From: Dustin Spicuzza \n> >\n> > When using cvsnt + msys + git, it seems like the output of cvs status\n> > had \\r\\n in it, and caused the command to fail.\n>\n> This is a bit under-explained in that it does not make it clear\n> where the right place to fix would be. From \"X did Y which caused\n> the command to fail\", a possible right fix could be \"so fix it by\n> telling X not to do Y\", but of course a patch to fix cvsnt won't\n> come to this list ;-)\n\nRight.\n\nHow about this:\n\n\tThe offical CVS for Windows (called CVS NT) produces DOS line\n\tendings in its `cvs status` output. Let's teach our own\n\t`cvsexportcommit` command to handle that gracefully.\n\nIt is unlikely that anybody wants to spend time \"fixing\" this in CVS NT,\neven less likely that anybody would take that patch, and even if that was\nthe case, there will still be plenty of CVS NT versions out there that\n`cvsexportcommit` cannot handle.\n\nI think it would be best to just integrate this change in Git and be done\nwith it. It's not like it adds a ton of maintenance burden there.\n\n(BTW this was also my thinking when I accepted that patch into Git for\nWindows, and thereby accepted the responsibility of upstreaming it.)\n\nCiao,\nDscho\n"},{"id":"375110","messageId":"xmqqr299fbyw.fsf@gitster-ct.c.googlers.com","threadId":"51009","inReplyTo":"nycvar.QRO.7.76.6.1905081248190.44@tvgsbejvaqbjf.bet","subject":"Re: [PATCH 1/1] cvsexportcommit: force crlf translation","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-05-08T10:59:51Z","receivedAt":"2019-05-08T10:59:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> How about this:\n>\n> \tThe offical CVS for Windows (called CVS NT) produces DOS line\n> \tendings in its `cvs status` output. Let's teach our own\n> \t`cvsexportcommit` command to handle that gracefully.\n>\n> It is unlikely that anybody wants to spend time \"fixing\" this in CVS NT,\n> even less likely that anybody would take that patch, and even if that was\n> the case, there will still be plenty of CVS NT versions out there that\n> `cvsexportcommit` cannot handle.\n>\n> I think it would be best to just integrate this change in Git and be done\n> with it. It's not like it adds a ton of maintenance burden there.\n\nOne thing that took some time (at least to me) to audit was that the\nhelper being touched is *NOT* limited to running \"cvs status\". If\nother commands cared about passing binary data intact, this change\nwould have been disasterous. After seeing what external commands it\nis told to drive, I think it is OK. It's not like we'd be updating\nthe program often and reusing the function blindly to accept possibly\nbinary data in a near future.\n\n\n"}]}