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

Fix the empty result issue in add -p #664

Merged
merged 3 commits into from Jul 22, 2021
Merged

Conversation

norisio
Copy link
Contributor

@norisio norisio commented Jul 22, 2021

Related Issues: #328, #557

Details of the issue

Current delta can't handle CRLF-ending diff hunks at least when used through git add -p. The reason here:

  • bytelines library (used here) strips CRs (b'\r', 0x0D) as well as LFs (b'\n', 0x0A) when it splits bytes into lines.
    • Find the operations here.
  • So there shouldn't be any CRs in lines: ByteLines<BufRead>.
  • However, git inserts some ansi control characters between CR and LF:
    image
  • So bytelines library can't find and leaves the CR in lines.
  • CR will be emitted directly into a terminal (at least when delta used through git add -p). Thus the rendered result will be blank (CR erases the line).

What I changed

When ingest_line, delta strips the trailing CR.

Before:

image

After:

image

@dandavison
Copy link
Owner

This (with the explanation) looks great @norisio, thank you! I've set the tests running on Github actions. Would you be able to add a test for the bug you've fixed? There are lots of examples in the codebase to follow, for example one possible style of test is the end-to-end type tests in test_example_diffs.rs, but if you prefer more of a unit test style that would be good too.

@norisio
Copy link
Contributor Author

norisio commented Jul 22, 2021

OK, will try.

@norisio
Copy link
Contributor Author

norisio commented Jul 22, 2021

I added a testcase. GIT_DIFF_SINGLE_HUNK_WITH_SEQUENCE_OF_CR_ESCAPE_SEQUENCES_LF contains the "harmful" pattern CR-ansi-LF.

@dandavison Could you check the content?

On master + testcase:

$ cargo test  orphan
    Finished test [unoptimized + debuginfo] target(s) in 0.05s
     Running target/debug/deps/delta-170397dd0efbb98e

running 1 test
test tests::test_example_diffs::tests::test_orphan_carriage_return_is_stripped ... FAILED

failures:
(snip)

On this branch:

$ cargo test  orphan
    Finished test [unoptimized + debuginfo] target(s) in 4.75s
     Running target/debug/deps/delta-170397dd0efbb98e

running 1 test
test tests::test_example_diffs::tests::test_orphan_carriage_return_is_stripped ... ok

test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 233 filtered out; finished in 1.86s

+ println!(\"added line\");\r
}\r
";

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, thank you!

@dandavison dandavison merged commit 63fdebb into dandavison:master Jul 22, 2021
@dandavison
Copy link
Owner

Thanks very much for this work @norisio.

Kr1ss-XD added a commit to Kr1ss-XD/delta that referenced this pull request Jul 30, 2021
* upstream/master:
  Change example config in README
  Recognize GitHub SSH remote URLs that don't start with `git@` for hyperlinks (dandavison#668)
  Fix the empty result issue in add -p (dandavison#664)
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.

None yet

2 participants