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

[github] update _strip_stack_information() to recognize \r\n #863

Closed
wants to merge 1 commit into from

Conversation

bolinfest
Copy link
Contributor

[github] update _strip_stack_information() to recognize \r\n

By default, Sapling programmatically writes the PR body on GitHub
and will continue to overwrite it every time the user runs
sl pr submit. In so doing, Sapling always uses \n as the
line terminator for the content it generates.

However, if the user has github.preserve-pull-request-description=true
set and edits the pull request body on GitHub, then the next time
Sapling pulls down the pull request body to parse it, it will find that
the original line terminators it wrote will have been changed to \r\n.

Previous to this commit, _strip_stack_information() always looked
for _HORIZONTAL_RULE and _SAPLING_FOOTER_MARKER joined with \n,
which failed to match the newly written version containing \r\n.
This meant that users with github.preserve-pull-request-description=true
set would end up getting an extra copy of the stack information
written to the PR body after running sl pr submit because the first
instance was considered the source of the PR body that Sapling was
not supposed to modify based on github.preserve-pull-request-description=true.

This commit changes _strip_stack_information() to match the pattern
using a regex that recognizes both \n and \r\n as the line terminator.

Added a doctest to _strip_stack_information() and verified the following
passes locally:

./tests/run-tests.py ./tests/test-doctest.py

By default, Sapling programmatically writes the PR body on GitHub
and will continue to overwrite it every time the user runs
`sl pr submit`. In so doing, Sapling always uses `\n` as the
line terminator for the content it generates.

However, if the user has `github.preserve-pull-request-description=true`
set and edits the pull request body on GitHub, then the next time
Sapling pulls down the pull request body to parse it, it will find that
the original line terminators it wrote will have been changed to `\r\n`.

Previous to this commit, `_strip_stack_information()` always looked
for `_HORIZONTAL_RULE` and `_SAPLING_FOOTER_MARKER` joined with `\n`,
which failed to match the newly written version containing `\r\n`.
This meant that users with `github.preserve-pull-request-description=true`
set would end up getting an extra copy of the stack information
written to the PR body after running `sl pr submit` because the first
instance was considered the source of the PR body that Sapling was
not supposed to modify based on `github.preserve-pull-request-description=true`.

This commit changes `_strip_stack_information()` to match the pattern
using a regex that recognizes both `\n` and `\r\n` as the line terminator.

Added a doctest to `_strip_stack_information()` and verified the following
passes locally:

```
./tests/run-tests.py ./tests/test-doctest.py
```
@facebook-github-bot
Copy link
Contributor

@facebook-github-bot has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@quark-zju merged this pull request in cf8b16f.

bolinfest added a commit to bolinfest/sapling that referenced this pull request Apr 19, 2024
…adding \n to commit message.

Summary:
While facebook#863 fixed some important
issues with respect to recognizing `\r\n`, it also introduced a new
issue where running `sl pr s` with `github.preserve-pull-request-description=true`
would inadvertently add an extra `\n` before the horizontal rule
delimiting the Sapling stack information.

This fixes `create_pull_request_title_and_body()` to preserve whatever
whitespace was originally present before the horizontal rule,
only adding a new `\n` to the end of the commit message if it did not
already have one.

Test Plan:
```
./tests/run-tests.py ./tests/test-doctest.py
```
bolinfest added a commit to bolinfest/sapling that referenced this pull request Apr 23, 2024
…adding \n to commit message.

Summary:
While facebook#863 fixed some important
issues with respect to recognizing `\r\n`, it also introduced a new
issue where running `sl pr s` with `github.preserve-pull-request-description=true`
would inadvertently add an extra `\n` before the horizontal rule
delimiting the Sapling stack information.

This fixes `create_pull_request_title_and_body()` to preserve whatever
whitespace was originally present before the horizontal rule,
only adding a new `\n` to the end of the commit message if it did not
already have one.

Test Plan:
```
./tests/run-tests.py ./tests/test-doctest.py
```
bolinfest added a commit to bolinfest/sapling that referenced this pull request Apr 23, 2024
…adding \n to commit message.

Summary:
While facebook#863 fixed some important
issues with respect to recognizing `\r\n`, it also introduced a new
issue where running `sl pr s` with `github.preserve-pull-request-description=true`
would inadvertently add an extra `\n` before the horizontal rule
delimiting the Sapling stack information.

This fixes `create_pull_request_title_and_body()` to preserve whatever
whitespace was originally present before the horizontal rule,
only adding a new `\n` to the end of the commit message if it did not
already have one.

Test Plan:
```
./tests/run-tests.py ./tests/test-doctest.py
```
facebook-github-bot pushed a commit that referenced this pull request Apr 23, 2024
…adding \n to commit message. (#880)

Summary:
Fix an issue where github.preserve-pull-request-description=true was adding \n to commit message.
While #863 fixed some important
issues with respect to recognizing `\r\n`, it also introduced a new
issue where running `sl pr s` with `github.preserve-pull-request-description=true`
would inadvertently add an extra `\n` before the horizontal rule
delimiting the Sapling stack information.

This fixes `create_pull_request_title_and_body()` to preserve whatever
whitespace was originally present before the horizontal rule,
only adding a new `\n` to the end of the commit message if it did not
already have one.

Pull Request resolved: #880

Test Plan:
```
./tests/run-tests.py ./tests/test-doctest.py
```

Reviewed By: quark-zju

Differential Revision: D56337507

Pulled By: sggutier

fbshipit-source-id: d675d5b0df4aa95f781440e0342300860a1cb7ed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants