-
-
Notifications
You must be signed in to change notification settings - Fork 392
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 create archive to a continuous writing source file failed #388
Conversation
mholt#387 Signed-off-by: Yan Zhu <[email protected]>
c55f399
to
85ad2b9
Compare
Thanks! Why does this fix it, though? Wouldn't Copy() just copy the 2 GB and finish at EOF? Why do we need to limit the number of bytes? |
I suppose the problem is caused by writing a header with the original file size and the reader reading the new append content. https://github.com/mholt/archiver/blob/v4.0.0-alpha.8/tar.go#L74-L100
|
ping @mholt |
Thanks, sorry for the delay. Just had a baby. I just need to think about this one more time but I suppose it is likely to be merged soon 🙂 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I mean, I think I have no problems with this, but I also don't really know what the implications are for not your use case (which I don't fully understand still from code screenshots).
Could we add a comment maybe explaining why we're doing it this way?
Even better, tests to ensure correct behavior in your use case and the 'standard' use cases. I haven't really gotten around to doing a lot of testing in this package yet but if you want to make me even more confident in this change, then that'd be good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the improvement, we'll try it out
Fix #387