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 panic when showing a diff which contains a diff #77

Merged
merged 5 commits into from
Mar 2, 2020

Conversation

Ma27
Copy link
Contributor

@Ma27 Ma27 commented Dec 28, 2019

When committing e.g. a patch-file in a git repository that has a line
which contains --- only, delta fails with the following error (as it
expects such a line to contain keywords for the diff):

thread 'delta::tests::test_diff_in_diff' panicked at 'byte index 2 is out of bounds of ``', src/libcore/str/mod.rs:2017:9
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace.

This patch changes the diffing behavior to ensure that lines containing
--- only won't be parsed.

When committing e.g. a patch-file in a git repository that has a line
which contains `--- ` only, `delta` fails with the following error (as it
expects such a line to contain keywords for the diff):

```
thread 'delta::tests::test_diff_in_diff' panicked at 'byte index 2 is out of bounds of ``', src/libcore/str/mod.rs:2017:9
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace.
```

This patch changes the diffing behavior to ensure that lines containing
`--- ` only won't be parsed.
@dandavison
Copy link
Owner

Hi @Ma27, thanks very much for this! (I'm going to be offline for the next ten days so will be slow to review and merge this, but will do ASAP.)

@Ma27
Copy link
Contributor Author

Ma27 commented Feb 27, 2020

@dandavison would you mind taking another look at this? :)

@dandavison
Copy link
Owner

Hi @Ma27, thanks for the ping and sorry for forgetting about this!

I've been trying to understand some history here... I think what's happened is that, in the interim, #90 merged a similar change to yours to master, so that your test now passes, without any change to the non-test code. Accordingly, I've pushed some commits to this branch, leaving the new unit test as the change that this branch will introduce when merged.

Do you buy all that and does it SGTY?

@Ma27
Copy link
Contributor Author

Ma27 commented Mar 2, 2020

Do you buy all that and does it SGTY?

LGTM, thanks! 👍

@dandavison dandavison merged commit 73bdb6b into dandavison:master Mar 2, 2020
@dandavison
Copy link
Owner

Great, thanks very much!

@Ma27 Ma27 deleted the fix-diff-in-diff branch March 2, 2020 15:43
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.

2 participants