Patch #44330
closed
Convert CRLF line endings to LF in three test fixture files
Description
I am one of the Debian Developers working on maintaining Redmine in Debian. I noticed that there are three files in the codebase with CRLF (Carriage Return, Line Feed) line endings, which is the Windows standard for line terminations in text files. All of the rest of the text files in the Redmine codebase use LF (Line Feed) line endings, the Linux standard.
Generally, it is recommended that files in a repository use LF line endings. Many repository clients, like git, default to automatically converting CRLF to LF when uploading to a repository. In most cases, there is no downside to having all text files use LF. The only exception I have encountered is when a standard requires CRLF (like IMAP) and test files need to be formatted that way to verify correct output.
The three files in Redmine that have CRLF line endings are:
test/fixtures/files/2006/07/060719210727_changeset_utf8.diff test/fixtures/files/2019/04/190430092344_redmine_logo.ai.unknown test/fixtures/files/testfile.txt
In reviewing these files, there does not appear to be any reason why they would need to be CRLF. My guess is they were uploaded by someone whose client was set to not automatically convert CRLF to LF.
In the Debian packaging workflow we are using, the code is checked in and out of a git repository various times as the package is built and tested. At some points, the contents of the code is checked against the upstream tarball to make sure that no files have changed. Because git with default settings converts CRLF to LF, these three files are flagged as having changed. If there is no downside to the project in doing so, I think it would be preferable to convert these files to LF. If for some reason these files do need to be CRLF, I have a way of working around that in the Debian packaging.
Attached is a patch that converts these files to LF. It was generated by simply running dos2unix on each file.
Files
Updated by Go MAEDA 14 days ago
- File 0001-Convert-CRLF-line-endings-to-LF-in-three-test-fixtur.patch 0001-Convert-CRLF-line-endings-to-LF-in-three-test-fixtur.patch added
- File 0002-Fix-filesize-and-digest-of-attachment-fixtures-to-ma.patch 0002-Fix-filesize-and-digest-of-attachment-fixtures-to-ma.patch added
- Subject changed from Convert three CRLF files to LF to Convert CRLF line endings to LF in three test fixture files
- Target version set to Candidate for next minor release
Thank you for reporting this and for the patch.
I agree that these three files should use LF line endings.
Converting source:trunk/test/fixtures/files/testfile.txt@24886 changes its size and its digest, so the tests that check those values needed to be updated as well. I have attached an updated patch that includes those changes, together with the matching filesize and digest in test/fixtures/attachments.yml:
0001-Convert-CRLF-line-endings-to-LF-in-three-test-fixtur.patch
While working on this, I also found that the filesize and digest of several other fixtures in source:trunk/test/fixtures/attachments.yml@24886 do not match the actual files. This is not related to the line endings, so I put it in a separate patch. Fixtures whose file does not exist on disk are left unchanged:
0002-Fix-filesize-and-digest-of-attachment-fixtures-to-ma.patch
Below is why I think converting these files to LF is safe.
test/fixtures/files/testfile.txt:
CRLF line endings were added to this file in r2085. At that time, Attachment.digest opened the file to compute its digest, and on Windows the result was wrong because the file was read in text mode (#2264). The CRLF line endings made it possible to test that the file was read in binary mode.
In r2670 the digest was changed to be computed from the uploaded data, and both Attachment.digest and its test (test_digest) were removed. Nothing has depended on these line endings since then. The CRLF is only a leftover.
test/fixtures/files/2006/07/060719210727_changeset_utf8.diff:
In r4604, the existing 060719210727_changeset.diff was renamed to 060719210727_changeset_iso8859-1.diff, and a copy of it converted to UTF-8 was added as this file. The line endings changed from LF to CRLF at that point. The tests added in r4604 do not require CRLF, so this looks like an accident during the conversion.
test/fixtures/files/2019/04/190430092344_redmine_logo.ai.unknown:
This file is the output of an Adobe tool of that time, with its extension changed. It was probably written with CRLF line endings by that tool. No test in Redmine requires CRLF for this file. I also confirmed that the converted file can still be opened by a current Adobe tool, so the conversion causes no problem.
Updated by