Re: [PATCH 1/1] cvsexportcommit: force crlf translation

From: Dustin Spicuzza <dustin@virtualroadside.com>
Date: May 7, 2019, 13:51 UTC
Message-ID: <4da677fb-47c3-a1ac-e7e0-6b74cdb7334c@virtualroadside.com>
In-Reply-To: <xmqqtve6lito.fsf@gitster-ct.c.googlers.com>

Yes, your interpretation is exactly correct, even the interpretation of the uncertainty of the message.

I didn't send the patch to this list though, not sure why gitgitgadget decided to do so (I've never heard of it before now). The patch was originally submitted to git for windows (PR #938), with an even worse commit message. :)

However, I'm not opposed to it being merged elsewhere. As you pointed out, it shouldn't negatively affect non-CRLF platforms.

Dustin

On 5/7/19 5:22 AM, Junio C Hamano wrote:

Show 48 quoted lines

> "Dustin Spicuzza via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
>> From: Dustin Spicuzza <dustin@virtualroadside.com>
>>
>> When using cvsnt + msys + git, it seems like the output of cvs status
>> had \r\n in it, and caused the command to fail.
> This is a bit under-explained in that it does not make it clear
> where the right place to fix would be.  From "X did Y which caused
> the command to fail", a possible right fix could be "so fix it by
> telling X not to do Y", but of course a patch to fix cvsnt won't
> come to this list ;-)
>
> I haven't thought things through, so let's think it aloud and
> enumerate what should have been explained in the log message here.
>
>  - With binmode(":crlf"), (i.e. if the platform uses CRLF convert
>    external CRLF into internal '\n'), the change hopes not to affect
>    platforms that do not use CRLF.
>
>  - "it SEEMS LIKE the output of cvs status had CRLF in it", i.e. it
>    is uncertain if everybody on the platform has the same issue.
>    But by using binmode(":crlf"), if some other implementations of
>    "cvs status" on the platform used LF, they won't get negatively
>    affected by this change, either.
>
> So, I guess the change is safe enough that it does not hurt other
> people.
>
>> This fixes that.
>>
>> Signed-off-by: Dustin Spicuzza <dustin@virtualroadside.com>
>> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
>> ---
>>  git-cvsexportcommit.perl | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/git-cvsexportcommit.perl b/git-cvsexportcommit.perl
>> index d13f02da95..fc00d5946a 100755
>> --- a/git-cvsexportcommit.perl
>> +++ b/git-cvsexportcommit.perl
>> @@ -431,6 +431,7 @@ END
>>  sub safe_pipe_capture {
>>      my @output;
>>      if (my $pid = open my $child, '-|') {
>> +	binmode($child, ":crlf");
>>  	@output = (<$child>);
>>  	close $child or die join(' ',@_).": $! $?";
>>      } else {