Test fails on Windows with a git checkout using dos line endings in comments

[rpavlik]

Found this when setting up a new CI system: get these failures in the tests, Windows with VS2013 32-bit building. They pass locally, where the file mentioned has unix endlines, but on the CI system they have dos ones and fail. I think this may have found an actual bug in the comments handling.

cc @godbyk

  Failure details:
  * Test C:\jenkins-node\workspace\jsoncpp-win-matrix\BIT\32\VS\12\host\windows\test\data\test_comment_02.json
    Difference in input at line 11:
    Expected: '// Multiline comment cpp-style'
    Actual:   '// Multiline comment cpp-style// Second line'


  * Test C:\jenkins-node\workspace\jsoncpp-win-matrix\BIT\32\VS\12\host\windows\test\data\test_real_08.json
    Difference in input at line 1:
    Expected: '// Out of 32-bit integer range, switch to double in 32-bit mode. Length the'
    Actual:   '// Out of 32-bit integer range, switch to double in 32-bit mode. Length the// same as UINT_MAX in base 10 and digit less than UINT_MAX's last digit in// order to catch a bug in the parsing code.'


  * Test C:\jenkins-node\workspace\jsoncpp-win-matrix\BIT\32\VS\12\host\windows\test\data\test_real_09.json
    Difference in input at line 1:
    Expected: '// Out of 64-bit integer range, switch to double in all modes. Length the same'
    Actual:   '// Out of 64-bit integer range, switch to double in all modes. Length the same// as ULONG_MAX in base 10 and digit less than ULONG_MAX's last digit in order// to catch a bug in the parsing code.'


  * Test C:\jenkins-node\workspace\jsoncpp-win-matrix\BIT\32\VS\12\host\windows\test\data\test_real_10.json
    Difference in input at line 1:
    Expected: '// Out of 32-bit signed integer range, switch to double in all modes. Length'
    Actual:   '// Out of 32-bit signed integer range, switch to double in all modes. Length// the same as INT_MIN in base 10 and digit less than INT_MIN's last digit in// order to catch a bug in the parsing code.'


  * Test C:\jenkins-node\workspace\jsoncpp-win-matrix\BIT\32\VS\12\host\windows\test\data\test_real_11.json
    Difference in input at line 1:
    Expected: '// Out of 64-bit signed integer range, switch to double in all modes. Length'
    Actual:   '// Out of 64-bit signed integer range, switch to double in all modes. Length// the same as LONG_MIN in base 10 and digit less than LONG_MIN's last digit in// order to catch a bug in the parsing code.'


  Test results: 51 passed, 5 failed.

[rpavlik]

Both systems have core.autocrlf set to false but the one that doesn't show the bug has eol = lf while the buggy one lacks that line in git config.

[cdunn2001]

Thanks. I'll fix this soon. I'm updating mac-ports for 10.10 today...

[cdunn2001]

Try again. You might have to delete the old-dir first.

[mzeren-vmw]

I ran into this recently and my solution was to normalize EOL in comments to '\n' while reading. I.e. I think this is a design bug, not a .gitattributes problem.

[rpavlik]

Yeah, this makes the test pass but doesn't mean it handles dos line endings in comments correctly.

[cdunn2001]

Right. Ideally, we would record the line-endings so that we can dump the original.

[mzeren-vmw]

It doesn't seem necessary to preserve "exact bytes" of comment EOLs, only to record the fact that there was a line break. The proposed patch simply records line endings in a canonical form, which conform's to json_writer's interpretation of what a new line should be (always '\n'). Saying that on output EOL is always \n may seem simplistic, but it's likely to work in practice even on windows systems, and avoid these types of bugs. If non-unix EOL is desired a simple \n --> \r\n pass can be written outside of the main logic -- as is done in the test code by opening the files in "t" mode.

[cdunn2001]

I wouldn't mind requiring \n on input, but it seems unexpected to accept \r\n and to write only \n.

Didn't the .gitattributes fix the tests?

This change also fixes dereferencing past the end of the comment string in
StyledWriter::writeCommentBeforeValue.

We definitely want that fixed. And we probably should have tests for \r\n if we don't.

The test code assumes that
comments contain \n EOLs and opens the .actual files (etc.) with "wt" which
converts \n to \r\n.

I guess we should change that too.

[mzeren-vmw]

I wouldn't mind requiring \n on input,

Isn't it important to support DOS input files? (It is to my project.)

but it seems unexpected to accept \r\n and to write only \n.

json_writer always writes \n today except for multi-line comments that come from DOS files. In that case it writes "mixed EOLs", which is the root cause of this issue. Ideally there would be some way to control output EOL (preserve, local, unix, dos, mac?) but to implement that we probably want a representation for canonical EOLs in multi-line comments internally, or perhaps comment lines could be stored in a list or vector?

Didn't the .gitattributes fix the tests?

Sure, and it's a step forward since we would need that change to test DOS files on non-DOS platforms any way. But I'm still not convinced it fixes the root issue. For now I'm happy to cherry-pick 8f3aa22 on top of 1.1 to unblock my build.

[cdunn2001]

Yes, cherry-pick that for now. I reverted because it interferes with diffs of test-data changes.

So we have some choices:

1. Use rt/wt in C (which is less reliable than in Python).
   • This would add cr to every line, not just comments.
2. Retain the carriage returns and write them back.
   • Note that we do not preserve white-space for indentation.
3. Strip carriage-return on input, and write only line-feed.
   • Seems consistent with the spirit of JSON.

So I'm leaning toward @mzeren-vmw's appoach. Mark, would you rebase pull-request #122? (git rebase origin/master; git push -f my-repo my-branch:my-branch)