Support ls --dired#5280
Conversation
|
GNU testsuite comparison: |
sylvestre
requested review from
cakebaker and
tertsdiepraam
and removed request for
tertsdiepraam
tertsdiepraam
left a comment
tertsdiepraam
left a comment
There was a problem hiding this comment.
I think this is already much cleaner than when I looked at it this morning. However, I still think there's a lot of ways this could break and there seem to be a lot of ``moving parts'', if that makes sense. For example, some calculation might break if we change the output slightly.
Have you tried my suggestion of a wrapper around a writer that counts the number of bytes printed? I think that has several advantages:
- we don't need to store a string
- we don't need to lookup the last item on positions
- we don't need to do any manual calculations
- we can ignore total (because we don't need the string anymore)
It feels to me like a more natural way of dealing with this. What do you think?
|
GNU testsuite comparison: |
That perfect is the enemy of the good :) |
|
Alright! |
|
GNU testsuite comparison: |
|
GNU testsuite comparison: |
| fn test_ls_dired_incompatible() { | ||
| let scene = TestScenario::new(util_name!()); | ||
|
|
||
| scene | ||
| .ucmd() | ||
| .arg("--dired") | ||
| .fails() | ||
| .code_is(1) | ||
| .stderr_contains("--dired requires --format=long"); | ||
| } |
There was a problem hiding this comment.
This test seems to be incorrect. At least with GNU ls 9.3, running ls --dired doesn't fail. It simply ignores the --dired option and outputs the same as ls.
There was a problem hiding this comment.
Yeah, but it is a GNU issue. I will fix it there too.
There was a problem hiding this comment.
| let result = cmd.succeeds(); | ||
| // Number of blocks | ||
| #[cfg(target_os = "linux")] | ||
| result.stdout_contains(" total 4"); |
There was a problem hiding this comment.
For some reason this line fails on my machine because the total is 0 :| It works fine, and the total is 4, if I do the steps manually...
There was a problem hiding this comment.
and you run linux ?
There was a problem hiding this comment.
yep, I'm running Arch Linux.
There was a problem hiding this comment.
Here is the debug output:
Output:
total 0
-rw-r--r-- 1 dho dho 0 Sep 18 08:00 a1
-rw-r--r-- 1 dho dho 0 Sep 18 08:00 a22
-rw-r--r-- 1 dho dho 0 Sep 18 08:00 a333
-rw-r--r-- 1 dho dho 0 Sep 18 08:00 a4444
drwxr-xr-x 2 dho dho 40 Sep 18 08:00 d
//DIRED// 51 53 95 98 140 144 186 191 233 234
//DIRED-OPTIONS// --quoting-style=literal
The two odd things are the timestamps (they're off by two hours) and the size of the directory (it's 40 instead of 4096)
There was a problem hiding this comment.
fun!
as you know, it isn't the fault of this PR :)
There was a problem hiding this comment.
That's true, I added it to my todo list to investigate further
Co-authored-by: Daniel Hofstetter <daniel.hofstetter@42dh.com>
Co-authored-by: Daniel Hofstetter <daniel.hofstetter@42dh.com>
|
GNU testsuite comparison: |
| /// Calculates the byte positions for DIRED | ||
| pub fn calculate_dired_byte_positions( | ||
| output_display_len: usize, | ||
| dfn_len: usize, |
There was a problem hiding this comment.
Display File Name
we should rename it (it is from ls.rs)
I will create an issue
Co-authored-by: Daniel Hofstetter <daniel.hofstetter@42dh.com>
Co-authored-by: Daniel Hofstetter <daniel.hofstetter@42dh.com>
| pub struct DiredOutput { | ||
| pub dired_positions: Vec<BytePosition>, | ||
| pub subdired_positions: Vec<BytePosition>, | ||
| pub just_printed_total: bool, |
There was a problem hiding this comment.
I'm struggling with this field, it somehow doesn't fit to the other fields because it doesn't contain any data like the other fields. Though I don't know where it should be instead :|
There was a problem hiding this comment.
yeah, i used to have it in BytePosition but terts didn't like it and suggested this place :)
However, I would not overthink this, this feature is probably not used much and won't get many updates
|
GNU testsuite comparison: |
cakebaker
left a comment
cakebaker
left a comment
There was a problem hiding this comment.
Once the fmt issue is fixed I think it is ready to be merged.
|
GNU testsuite comparison: |
3 participants
No description provided.