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

[Regression] Bad merge when adding new files #189

Closed
piranna opened this issue Aug 15, 2017 · 16 comments
Closed

[Regression] Bad merge when adding new files #189

piranna opened this issue Aug 15, 2017 · 16 comments

Comments

@piranna
Copy link
Contributor

piranna commented Aug 15, 2017

When adding a new file, first line of new file is removed from file beggining and added at the end of the new file, corrupting data. This works correctly with diff 3.0.1, so it got broken in some moment after that.

@piranna
Copy link
Contributor Author

piranna commented Aug 22, 2017

Doing a git bisect it shows the regression was added at d8a3635

@piranna
Copy link
Contributor Author

piranna commented Aug 23, 2017

I'm having troubles to fix the regression... reverting the changes my test works but the added ones fails, adding the changes to the end. @tyomitch, can you help here?

tyomitch added a commit to tyomitch/jsdiff that referenced this issue Aug 23, 2017
@tyomitch
Copy link
Contributor

@piranna would you try #192

@piranna
Copy link
Contributor Author

piranna commented Aug 23, 2017

@piranna would you try #192

Tests seems to pass, do you ask that I try it on real code?

@tyomitch
Copy link
Contributor

Yes, on whatever alerted you of this bug in the first place.

@piranna
Copy link
Contributor Author

piranna commented Aug 23, 2017

Yes, on whatever alerted you of this bug in the first place.

Ok, I tried it on https://github.com/piranna/BarebonesOS-linux and it failed :-(

@tyomitch
Copy link
Contributor

@piranna how do I reproduce the failure with BarebonesOS-linux?
npm install succeeds for me, then npm run build fails saying "missing script: build" (and it's indeed missing).

@piranna
Copy link
Contributor Author

piranna commented Aug 24, 2017

@tyomitch it was my fault, I was using an in-development version of BarebonesOS-linux that in fact was not working. I re-check it again with master and it works, so I can confirm that your pull-requests fix the regression also on real world code. I propose to merge it and publish a new version.

@piranna
Copy link
Contributor Author

piranna commented Aug 28, 2017

I have check it again with the in-development version after fixing it and can confirm again that @tyomitch changes has fixed my issue :-) Can we merge and publish them?

@piranna
Copy link
Contributor Author

piranna commented Aug 28, 2017

Sorry, didn't see it was merged yesterday. Can we publish it to npm so we can close this issue? And what we do with the pull-request with the faling test? Should we merge it too?

@kpdecker
Copy link
Owner

kpdecker commented Aug 28, 2017 via email

@tyomitch
Copy link
Contributor

And what we do with the pull-request with the faling test? Should we merge it too?

#192 included your test case.

@piranna
Copy link
Contributor Author

piranna commented Aug 28, 2017

My plan is to release a version early this week. Want to try to get all
outstanding PRs in first.

That would be cool, thanks :-)

@piranna
Copy link
Contributor Author

piranna commented Sep 3, 2017

Since this is already fixed on master, can it be published a new version on npm? I have several packages broken and development stopped due to this dependency...

@kpdecker
Copy link
Owner

kpdecker commented Sep 3, 2017

Released in v3.3.1

@kpdecker kpdecker closed this as completed Sep 3, 2017
@piranna
Copy link
Contributor Author

piranna commented Sep 3, 2017

Thank you :-)

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

No branches or pull requests

3 participants