Support multiple patches per package

[jamesreggio]

I've been manually patching React Native for quite some time and I love the idea of this tool. It makes extracting a patch so much easier than the manual process I'm used to using.

The one thing that stands in the way of my complete adoption of patch-package is its lack of support for multiple patches upon the same package. I like to separate my patches by concern, and it'd be nice if patch-package allowed a third segment in the patch name that can be customized by the user.

For example, it'd be great if I could split my one mega-patch into the following three patch files:

patches/react-native+0.52.0+babel.patch
patches/react-native+0.52.0+cookies.patch
patches/react-native+0.52.0+scrollview.patch

patch-package could apply the patches in lexically-sorted order. If the order of patch application matters, the user could include a sorting ordinal in the custom third segment, like such:

patches/react-native+0.52.0+00-scrollview.patch
patches/react-native+0.52.0+01-cookies.patch

The trickiest part of this will be excluding existing patches at the time that a new patch is extracted from a package. I'm not entirely sure how to handle that, but I'm also unconvinced that support for multiple patches needs to hinge on first-class support for extracting and isolating multiple patches. For now, if you want to use this power-user feature, you would just need to ensure that each patch is authored on a clean base package.

[ds300]

Cool feature request! I'd love to have this as an option for power users.

I'm happy to provide guidance if you're interested in working on this. Probably won't get around to it myself any time soon.

[jamesreggio]

I'd be happy to do the implementation if you're okay with me just tackling the patch application portion first. How does that sound to you?

(And any pointers would be appreciated.)

[ds300]

I think that's the trivial part and it makes sense to prototype the patch generation/updating first, since design decisions there might affect how the patch application works.

[jamesreggio]

Aw shucks, you're gonna make me eat my vegetables before I get my steak.

Alright then. I probably won't circle back to this for a while yet, but we'll see.

[ds300]

Hah, sorry, I was in work mode. I forgot that people also code for fun :-) If you want to build the patch application first, go for it. Often doing the easier bits first is a great way to get motivated for tackling the larger problem.

…

On Wed, 28 Feb 2018, 23:29 James Reggio, ***@***.***> wrote: Aw shucks, you're gonna make me eat my vegetables before I get my steak. Alright then. I probably won't circle back to this for a while yet, but we'll see. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#43 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABL1qfZe4oYEo3wT8VfkGiXUl5oEx194ks5tZeFdgaJpZM4SXIiJ> .

[jamesreggio]

Sounds good.

I slept on it and think that a good approach to capturing the patch would be similar to git add -p, where you get an interactive prompt for staging the patch. Given that you're already using git under the hood, this actually shouldn't be too much of an undertaking.

In summary, I'd add the following options to the CLI:

---

• --interactive, -i

  Interactively choose hunks of changes to include in the patch. Similar to git add --patch.

• --name [name], -n [name]

  Add an optional name identifier to the patch's filename.

  [name] may contain letters, numbers, hyphens, and underscores.

---

The applicator would then be modified to lexically sort the patches and then ignore the name's patch segment (though It would echo the name it as it reports status).

// The patch filename regex would go from this:

/^(.+?)(:|\+)(.+)\.patch$/

// to this:

/^(.+?)(:|\+)(.+)(?:(:|\+)(.+))?\.patch$/

// In case you haven't seen it before, (?:[pattern]) indicates a non-captured group.

WDYT of that approach?

[RocketPuppy]

This would be really helpful in normal usage as well. I see added #37 to fix a bug with cached node_modules and partially applied patches. A sequence of multiple patches to be applied would also be helpful there, and shouldn't require reverting old patches.

[DanielRuf]

Bump, we would also need this feature.

[DanielRuf]

Workaround for us at the moment: some sort of structured collection which I am currently building at https://github.com/DanielRuf/patches

[DanielRuf]

Should similar or same like it is done in https://github.com/cweagans/composer-patches

[avrahamcool]

Bump, I would love to see this features live.

[ds300]

Not sure whether I want to support this feature or not.

On the one hand, it makes it easier to group and understand related changes.

On the other hand, it's a very complicated UX problem, and would likely balloon the surface area of the cli.

Also part of me feels like if the changes are so significant that grouping them makes them significantly easier to understand, then it's probably a good indication that forking the library is the way to go.

So feature is very low on my priority list. I don't think I'll ever work on in myself or approve a pull request for it unless someone designs a UX that feels really good for generating and managing the stacked patch files.

[avrahamcool]

if I understood the other comments correctly, the main thing we want here is to be able to group changes into small independent chunks, so we can have more control on which patches we need to apply over the original package.

lets say one of the changes we patched is later fixed by the maintainer of the package, but the other are not - we can just delete this specific patch file.

the part that is bothering you is the interactive UI for choosing the parts of the changes that should be grouped together.
what if we took a different approach at this? we don't need to specify anything.

• if it's the first time we patch a package:
  every change that is made to the original package will be taken into the patch file.

• if the package already been patched before:
  we take only the "new" changes, and create a patch file for them.
  this can be done by installing a clean version of the package - and applying all of the old patches in a sequence, and only then applying the diff between the result and the user edited package.

(it;s clear that the first scenario is actually a private case of the second one.. but it's clearer to understand in that way)

I hope I didn't miss the point completely..

[ds300]

Yeah that would work for creating patch files! But what if you want to go back and make changes to a previous patch file?

[avrahamcool]

you just delete this specific patch file, and run 'patch package' again after you make your desired changes.
all of the changes he's responsible for will be packed again in a new patch file - because they will be caught as diff from the original package.

[ds300]

That assumes the patch files don't modify contiguous or overlapping areas of code. Otherwise the patch-application process would break.

[avrahamcool]

Alright.
Maybe instead of deleting the previous patch, you can just create a new patch that fixes the thing you want on top of the previous patch.
When they will be run sequentially, it will eventualy have the fixed code as desired.

And if you know you have a patch that is independent of other patches, you are free to just delete as I said before.

Its actually resemble the .Net EF migration system.
Where each migration calculates the diff between the current entities you have with the result of running all previous migration in sequens. Thus getting only the "new" stuff into the newly created migration file.

[ds300]

Thanks for all your input! 👍

Maybe instead of deleting the previous patch, you can just create a new patch that fixes the thing you want on top of the previous patch.
When they will be run sequentially, it will eventualy have the fixed code as desired.

Then you might end up with related changes being in separate files, while this feature is supposed to promote related changes being grouped together.

Maybe it seems like I'm nitpicking. I feel that the simplicity of patch-package's UX is one of its biggest strengths and that this feature would compromise that. Also I have no need for it myself, so the thought of implementing it and/or maintaining it is something I'm going to push back against naturally.

Maybe a good alternative to having multiple patch files is to add comments explaining the changes?

[avrahamcool]

Thank you again for all of this work, and for your detailed answers. 👍

[jlsjonas]

Then you might end up with related changes being in separate files, while this feature is supposed to promote related changes being grouped together.

I think for several (including me) the issue is also several small changes/fixes to a certain monolith-like package where each fix might get fixed over time.

A secondary issue the pragmatic approach explained in #43 (comment) would also fix is conflict-handing: right now when there's a conflict in a patch, all you know is which file it happened in.
If this is a single fix/change, that's fine
If you made several (small, but distinct) changes to the same package this gives no useful information.

[adgarcia]

My use cases mirrors jlsjonas’; I apply a number of small changes to react-native and after any given RN update some of those changes may no longer be necessary. It would be nice to just yank the individual patches that I don’t need

[adgarcia]

Being able to generate patches on a per file basis might be a nice compromise?

[DanielRuf]

Normally these are bundled in one patch file.

Why not append them if wanted and skip those which can not be applied (x chunks / y chunks).

Or use --forward and so on and check the return values.

[timri]

I haven't looked into this, I just stumbled about this issue and wanted to add some further information that might be relevant:
In other contexts (i.e. not javascript/npm) I used quilt for similar use-cases
https://linux.die.net/man/1/quilt
This is f.e. also used in the debian linux distribution to track/manage changes to upstream code when deb-packages are created, see f.e. https://wiki.debian.org/UsingQuilt or https://www.debian.org/doc/manuals/maint-guide/modify.en.html

So, if anyone wants to work on this issue, quilt might provide some ideas for workflows.
(Or you might simply use quilt in your use-cases, at least if working on linux)

[landsman]

I would love to have this and some naming convetion for identify what exactly patch doing.

This can be done by using some kind of config in package.json - I think that loading files by file system is not a good way at all.

Check this out working solution in PHP world, Drupal: https://groups.drupal.org/node/518975.

This is really nice solution with documentation of all patches in project on the first view.
It would be BC break but it can bring really easy another functionality: Fetch patch(es) from url

[a-eid]

I was looking for some feature similar to this, would love to see it implemented.

[ericraio]

Any updates to this?

[Nantris]

This would be amazing, but it doesn't really seem feasible, or at least, not without a tremendous amount of work. I'd love for someone to prove me wrong though.

[gabrielmaldi]

Perhaps just the naming convention isn't too much work?

patches/react-native+0.52.0+babel.patch

Text after the second + (babel in this case) would be interpreted (and ignored) as author description instead of part of the version, in order to avoid the long warning:

Warning: patch-package detected a patch file version mismatch

  Don't worry! This is probably fine. The patch was still applied
  successfully. Here's the deets:

  Patch file created for

    react-native+0.52.0+babel

  applied to

    react-native+0.52.0

  At path

    node_modules/react-native

  This warning is just to give you a heads-up. There is a small chance of
  breakage even though the patch was applied successfully. Make sure the package
  still behaves like you expect (you wrote tests, right?) and then run

    patch-package react-native

  to update the version in the patch file name and make this warning go away.

---
patch-package finished with 1 warning(s).

[Andriiklymiuk]

Amy update on this one?) would be really cool feature

[tuiza]

Any updates?

[ds300]

This feature is going ahead. Expensify is kindly funding the effort. I'm working on it in #474 and expect it to be ready to use in a few weeks.

[ds300]

This is finished and was released in v8.0.0