Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CRLF preservation #11

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

CRLF preservation #11

wants to merge 10 commits into from

Conversation

dra27
Copy link
Member

@dra27 dra27 commented Jun 22, 2018

This PR makes the following changes:

  • OpamLexer requires some additional state which allows it to track the line-endings of the file being parsed.
  • Based on this state, OpamBaseParser's EOF token is augmented with a bool option indicating whether the file had CRLF-encoded line endings, LF-encoded line endings or a mixture.
  • OpamParser then treats mixed endings as CRLF on Windows and LF elsewhere and returns an opamfile record augmented with a file_crlf field.
  • OpamPrinter uses this information to control how it converts opamfile and related values back to strings.

Important notes:

  • The Normalise functions are unchanged - in particular, they use LF-endings on Windows
  • The Preserved functions now no longer LF-endings for changed entries and CRLF-endings for any unchanged values in a file which was CRLF-encoded
  • opam format upgrade should not magically convert all CRLF-encoded files to LF-encoded files.

This is still partially a work-in-progress, but is ready for initial review - in particular, I plan to add CI to this repo and some tests of these various behaviours and also produce a parallel PR in opam itself, as some changes are required there for CRLF-preservation.

In addition, the API is now documented.

If a string contains newlines, it is printed using triple-quoting
format, however this introduces a newline at the start of the string.

If the string does not begin with a newline, then add an escaped one.
The file_crlf field records whether the opam file was read from a file
where the line endings were CRLF-encoded. This changes the signatures
for the lexer, as state must now be threaded through each call. The EOF
token in OpamBaseParser has a bool option parameter added to it which
indicates if the file line-endings were CRLF-encoded (Some true),
LF-encoded (Some false), or mixed (None). If the file endings are mixed,
then OpamParser will default to assuming CRLF-encoding on Windows and
LF-encoding everywhere else, including Cygwin.
@dra27
Copy link
Member Author

dra27 commented Jun 22, 2018

3632a6d contains an unrelated, but important, change to the encoding of strings containing newlines.

@dra27
Copy link
Member Author

dra27 commented Jun 29, 2018

Tests to follow - @AltGr, is AppVeyor enabled as well?

@avsm
Copy link
Member

avsm commented Jan 31, 2019

just before i rebase #13 -- is this mergeable now?

This was referenced Dec 18, 2019
@rjbou
Copy link
Contributor

rjbou commented Dec 18, 2019

Thanks @dra27! I ported half of the PR in #21 for CI & #22 for doc.
I'll ping @AltGr for CLRF review

dra27 added a commit to dra27/opam-file-format that referenced this pull request May 13, 2020
These are part of the documentation for ocaml#11, which isn't yet merged.
dra27 added a commit to dra27/opam-file-format that referenced this pull request May 13, 2020
These are part of the documentation for ocaml#11, which isn't yet merged.
dra27 added a commit to dra27/opam-file-format that referenced this pull request May 14, 2020
These are part of the documentation for ocaml#11, which isn't yet merged.
@dra27 dra27 mentioned this pull request May 22, 2020
dra27 added a commit to dra27/opam-file-format that referenced this pull request May 26, 2020
These are part of the documentation for ocaml#11, which isn't yet merged.
dra27 added a commit to dra27/opam-file-format that referenced this pull request May 26, 2020
These are part of the documentation for ocaml#11, which isn't yet merged.
@avsm
Copy link
Member

avsm commented Aug 7, 2020

Dev meeting discussion: this is for opam 2.2 at the earlier

@dra27 dra27 mentioned this pull request Oct 21, 2020
@dra27 dra27 mentioned this pull request Dec 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants