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(plugin-patch): avoid creating rename diffs #4557

Merged
merged 1 commit into from
Jun 21, 2022
Merged

fix(plugin-patch): avoid creating rename diffs #4557

merged 1 commit into from
Jun 21, 2022

Conversation

redabacha
Copy link
Contributor

@redabacha redabacha commented Jun 17, 2022

What's the problem this PR addresses?

helps to avoid #3341 by generating diffs without renames

How did you fix it?

passed --no-renames option when calling git diff

Checklist

  • I have set the packages that need to be released for my changes to be effective.
  • I will check that all automated PR checks pass before the PR gets reviewed.

@arcanis
Copy link
Member

arcanis commented Jun 20, 2022

Shouldn't we rather implement rename in ZipFS using zip_file_rename? Otherwise the produced patches will be very large (especially if the renamed file is a bundle or something like this).

@redabacha
Copy link
Contributor Author

redabacha commented Jun 20, 2022

hi @arcanis, yes agreed this is more of a simple stopgap solution until someone smarter than me or when i have more time maybe can implement the libzip file rename. i do personally think it's a better dx to have the patch apply successfully rather than receive a very ambiguous error message despite the larger diff 😅.

@arcanis
Copy link
Member

arcanis commented Jun 21, 2022

Fair enough! 🙂

@arcanis arcanis merged commit 483e1c2 into yarnpkg:master Jun 21, 2022
@paul-soporan
Copy link
Member

Shouldn't we rather implement rename in ZipFS using zip_file_rename?

I've tried to do it multiple times but for some reason I can't seem to make it work because zip_file_rename always returns an error code early and I can't figure out why.

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