Implement ls - Githubissues

jeremyvii commented 3 years ago

This PR implements the ls utility. Below lists the arguments with the short and long flags.

[x] -A|--almost-all
[x] -C|--order-top-to-bottom
[x] -F|--classify
[x] -H|--no-dereference
[x] -L|--dereference
[x] -R|--recursive
[x] -S|--sort-size
[x] -a|--all
[x] -c|--file-status-modification
[x] -d|--directory
[x] -f|--no-sort
[x] -g|--no-owner
[x] -i|--inode
[x] -k|--block-size
[x] -l|--list
[x] -m|--comma-separate
[x] -n|--numeric-uid-gid
[x] -o|--no-group
[x] -p|--indicator
[x] -q|--hide-control-chars
[x] -r|--reverse
[x] -s|--size
[x] -t|--time
[x] -u|--last-accessed
[x] -x|--order-left-to-write
[x] -1|--one-per-line

jeremyvii commented 3 years ago

Any suggestions for writing tests for ls? My thinking is to create a temporary directory with various temporary files. However, I am not sure how to handle metadata, like user/group, that will vary from system to system.

GrayJack commented 3 years ago

Sorry for the late reply, I had some exam these days.

Any suggestions for writing tests for ls? My thinking is to create a temporary directory with various temporary files. However, I am not sure how to handle metadata, like user/group, that will vary from system to system.

Yeah, this is something I struggle with as well, when it's something simple as exist-or-not or the contents of the files testing is easier, there some work to it, but it's easier, when it depends on metadata that it's different from system to system, I have no idea how to do it. The utilities that depends on that I manually tested on several system and (virtual)machines. I do want to have a better way to test them.

One way to tackle this problem maybe it's to call the installed system wide utility and check the output, I think that for the POSIX standard features that should be enough, but for extensions... some flags exist on one implementation but no the other and vice-versa.

jeremyvii commented 3 years ago

The total of blocks diverge from the system ls

This seems to be the case for me as well. But I think it is because GNU's ls defaults to 1024 bytes. Metadata::blocks() is dividing by 512 bytes. I think the way Metadata is calculating the block value seems more in line with the POSIX standard (see the -s and -k flag). My plan was to eventually implement the -k option which will use 1024 bytes.

POSIX on the -s flag:

Indicate the total number of file system blocks consumed by each file displayed. If the -k option is also specified, the block size shall be 1024 bytes; otherwise, the block size is implementation-defined.

The output is misaligned

Strange, I cannot recreate this. But after I implement your suggestions on the column widths this may be resolved.

jeremyvii commented 3 years ago

Should I be using the bstr crate exported from coreutils_core to deal with non UTF-8 characters?

GrayJack commented 3 years ago

Since it already is using coreutils_core, yeah, using it is best

jeremyvii commented 3 years ago

Do you intend to finish the last missing item in the check list? No hurries! The work here is amazing as is, and it's perfectly fine to merge without a complete implementation.

Also I would like to keep track of the open PRs, because sometimes the author forgets to finish the draft, rendering us unable to merge the changes if finished! (I did this once )

In this review I just added 2 comments, one is a nitpick, another one is that I did a breaking change in the coreutils_core API

My apologies! I have been sitting on https://github.com/GrayJack/coreutils/pull/130/commits/03f3ab57a62ce995792df3ae6a8e9e87213f0201 since December 17th (forgot to push this commit). Work and the holidays had me distracted. I will take care of your reviews, see if there is any clean up I can do, and move this PR out of a draft state.

GrayJack commented 3 years ago

No need to apologize! We need to rest a little bit in the Holidays, I wasn't expecting a reply this month because of the Holidays LOL

Take your time, no need to hurry!

jeremyvii commented 3 years ago

I am unsure when this began exactly, but I noticed today that my BufWriter was not longer outputting anything when passing it into the write! macro. I messed with it for quiet a while but couldn't determine why. I got around this by creating a new BufWriter in 9daf166ab36be52536ef5249c996bc6f50ce5ea4 in the output function but I am not sure that this is a good solution. Do you have any thoughts or suggestions?

I would also like to note that the term_grid crate works excellently for the displaying the files in a grid. However, there is a bug in the terminal size calculation that causes the grid to display in one column even when there is enough room in the terminal (see https://github.com/ogham/rust-term-grid/issues/11).

GrayJack commented 3 years ago

I am unsure when this began exactly, but I noticed today that my BufWriter was not longer outputting anything when passing it into the write! macro. I messed with it for quiet a while but couldn't determine why. I got around this by creating a new BufWriter in 9daf166 in the output function but I am not sure that this is a good solution. Do you have any thoughts or suggestions?

I think the reason that is happening is because the BufWriter is failing to flush when it gets out of the scope (end of main function), because you're process::exiting it. My guess is that if you call writer.flush() before the process::exit it will return to output.

If I'm correct, I think a more clean solution is to only process::exit if exit_code is different than zero. That way, when it's zero (no errors happened ) the main function exits normally and drops the BufWriter, making it flush it's contents to the underlining writer.

GrayJack commented 3 years ago

LGTM! bors r+

GrayJack commented 3 years ago

bors r+

bors[bot] commented 3 years ago

Build succeeded:

GrayJack / coreutils

Implement ls #130