Zip::File#write_buffer doesn't assume we're writing to the original IO

[casperisfine]

write_buffer may be called with another IO than the one that was used to open/read the file. Hence even if there is no change, the caller may expect the archive to be written in the IO.

NB: a bunch of test fails, but I'm not sure I fully understand the API and that my assumptions are correct, hence why I'm opening early.

Perhaps we're using the API incorrectly and we ought to rewrite our code?

[hainesr]

To be honest this is an especially horrible bit of the API, and not one where I really understand the thinking behind it - it's from way before my time as maintainer.

The issue with doing what you've done in this PR is that it regresses the other fix we put in place to make sure that write_buffer doesn't change things when it simply opens a buffer for reading (#280, #512, et al).

My gut feeling is that this is a bit hairy to fix properly in a minor release. Probably what needs to happen is that Zip::File keeps a hold of the underlying IO so that it doesn't need to be passed in explicitly to write_buffer, then it can just return the unchanged (or changed) IO. The other thing we could do is go back to returning nil if there's no change, then at least you could test for that: if you get an IO back it has changed/been updated, and if you get nil back your original IO is still valid and unchanged.

I would be interested in your thoughts on this.

[casperisfine]

My gut feeling is that this is a bit hairy to fix properly in a minor release.

Right, but it also got broken in a minor release 😛 .

I would be interested in your thoughts on this.

So we're not huge users of the gem, and I am only the guy that's updating the dependency and has limited knowledge about the code that uses this API.

But out of the top of my head, what you say make sense but seems like a long term refectoring given that right now File isn't tied to an IO.

Perhaps a short term solution is either:

• write_buffer takes an extra parameter to always write regardless of changes
• Add another method (no idea about the name) that essentially work like write_buffer used to.

[hainesr]

My gut feeling is that this is a bit hairy to fix properly in a minor release.

Right, but it also got broken in a minor release 😛 .

Yes, you're right.

I feel like I really should revert write_buffer to it's broken-in-some-ways state and fix the issue people were seeing a different way. I could change open_buffer to only write if needed, and still return the IO either way (we have the IO at that point). I think that would fix the issue and preserve the functionality we have now.

[casperisfine]

Up to you. Since we don't use the library that much, it's not that hard for me to work around it. I know have a green build with 2.4rc1.