Regroup marshling/unmarshaling and lint

puellanivis commented 3 years ago

This covers a few different things that were somewhat unrelated to starting to use filexfer but will make that transfer easier.

Collects and isolates marshal/unmarshal code into packets.go
Removing clamp and min functions that both did the same thing, and didn’t really contribute to more understandable code.
Fix corner cases in os.ModeType → uint32 where we treat the various types as bitfields that can be OR’ed together rather than mutually exclusive values.
a small bit of linting on request-server.go
fileInfo can use the values in the wrapped FileStat, which we also don’t need to treat as an any.
bring over the ls_formatting from PR-432, as this is an improvement beyond just bringing in filexfer as it extracts more UID/GID values for the various GOOS, supports setting UID/GID values from a FileStat, and also converts UIDs/GIDs to usernames and groupnames when possible. (Do we really want to use system-wide username/groupnames? Might be good to provide an ability to override this? Since request-server might be operating entirely disjoint from the underlying OS.)
removed a few cgo build conditions that were not actually necessary
verified that the code compiles for all GOOS supported in go1.16 (not verified that it works, just that it compiles)

puellanivis commented 3 years ago

I should run a regression test on this with FileZilla… since I’m touching the longname field…

drakkan commented 3 years ago

Hi,

I just tested with FileZilla and it seems ok. SFTPGo test cases also passed.

I'm not sure about UIDs/GIDs to usernames and groupnames conversion, however we may allow you to override the conversion in the future if needed. Thanks!

drakkan commented 3 years ago

The directory listing is now something like this

sftp> ls -la
drwxr-xr-x    1 0        0               0 Aug 16 07:19 .
drwxr-xr-x    1 0        0               0 Aug 16 07:19 local
-rw-r--r--    1 nicola   nicola       4034 Aug 16 07:19 file.txt
drwxr-xr-x    1 0        0               0 Aug 16 07:19 vdir1
drwxr-xr-x    1 0        0               0 Aug 16 07:19 vdir2
drwxr-xr-x    1 0        0               0 Aug 16 07:19 vdir3

., local, vdir1, vdir2, vdir3 are virtual directory and so UIDs/GIDs are not converted. The same happen for cloud storage based accounts. For non local files/dirs the type assertion fails and so no conversion happen. I think this is ok for now. Maybe if future we could allow to disable this conversion (it could slow down directory listing if we have a lot of entries) using a setting or provide an optional interface

puellanivis commented 3 years ago

Yeah, I was thinking of abstracting it away to an optional interface as well. It shouldn’t be too hard. 🤔

puellanivis commented 3 years ago

I’ve spun off the username and groupname lookup into an extension type on FileLister, default behavior for RequestServer should end up as “do not convert UID/GID from numbers”, and default unoverridable behavior for Server is to use os/user for username and groupname lookup.

drakkan commented 3 years ago

it looks good for me, thanks

pkg / sftp

Regroup marshling/unmarshaling and lint #456