search

psanford / wormhole-william

End-to-end encrypted file transfer. A magic wormhole CLI and API in Go (golang).
MIT License
1.08k stars 55 forks source link

Small dir receive improvements and example, fixes #26 #33

Closed Jacalz closed 3 years ago

Jacalz commented 3 years ago

Fixes #26

I recently rewrote the zip handling in wormhole-gui for the upcoming 2.2.0 release and I managed to make a few improvements to it at the same time. I figured that I would be a good open source citizen and port it upstream so that everyone can use it.

Jacalz commented 3 years ago

I have now fixed an annoying bug with the code. Should be ready for review now. @psanford FYI, I don't know why some of the tests are failing. Does not seem to be my happening on my machine.

psanford commented 3 years ago

The code sets permission to 0777 with the assumption that the user's umask will remove the permission bits the user doesn't want. For example my umask is set to:

0002
$ umask -S
u=rwx,g=rwx,o=rx

so even though the application sets the permission to 0777 it still results in a directory that is 775. This seems to match the behavior of the official python Magic Wormhole client.

As for the unzip logic, what is the actual issue you are trying to address? You mentioned a problem with a deferred .Close() but I don't see any code doing that in the original implementation.

Jacalz commented 3 years ago

The code sets permission to 0777 with the assumption that the user's umask will remove the permission bits the user doesn't want. For example my umask is set to:

0002
$ umask -S
u=rwx,g=rwx,o=rx

so even though the application sets the permission to 0777 it still results in a directory that is 775. This seems to match the behavior of the official python Magic Wormhole client.

Okay. I still think that we are setting unnecessarily high, but if it’s what the official tool is doing, I guess we can keep it that way.

As for the unzip logic, what is the actual issue you are trying to address? You mentioned a problem with a deferred .Close() but I don't see any code doing that in the original implementation.

The issue in that case is that you are not actually deferring the close at all in some cases ( look inside the for-loop for zip extraction). Sorry if I was unclear, I merely used the defer close issue as a motivational for what I set it up that way (instead of just deferring without checking errors).

psanford commented 3 years ago

The issue in that case is that you are not actually deferring the close at all in some cases ( look inside the for-loop for zip extraction).

I'm still not seeing what you are talking about. Can you point to the lines of code that have the problem?

Jacalz commented 3 years ago

Looks like the autocomplete in Visual Studio Code had decided to switch back to the regular archive/zip package. Sorting that out made the CI a lot more happy (don't know why the cross-tests were the only ones to complain about it though).

Jacalz commented 3 years ago

Comments should now be mostly addressed. I also fixed a bug where subdirectories would not be created unless each folder was part of the zip file. It is working really well now :slightly_smiling_face:

Jacalz commented 3 years ago

This has now been rebased and squashed. Though, I still think that https://github.com/psanford/wormhole-william/pull/33#discussion_r593926165 maybe should be commented on before it lands.

Jacalz commented 3 years ago

Commits have now been squashed and rebased on latest master. Should be ready for a re-review.

Jacalz commented 3 years ago

@psanford Could you please look at getting this merged?