-
-
Notifications
You must be signed in to change notification settings - Fork 3k
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
cmd: fix files ls
to report hash and size for files
#5045
cmd: fix files ls
to report hash and size for files
#5045
Conversation
I'll add some tests (or probably some already existing tests should be fixed). |
The test is failing because the reported size varies for reasons I don't quite understand, I'll raise a different issue for that and leave this as blocked in the meanwhile. |
I wonder if we need fo flush first or something... |
Now that you mention it, maybe we need to lock the directory, that's what (but that may just be due to the |
Sorry, I missed that commit before my comment. It looks like you fixed the size issue, right? The code looks corect, at least. |
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.
LGTM modulo the magic number.
core/commands/files.go
Outdated
|
||
out := &filesLsOutput{[]mfs.NodeListing{mfs.NodeListing{ | ||
Name: name, | ||
Type: 1, |
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.
Should probably use unixfs.TFile
to avoid magic numbers.
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.
Yes, I should directly call the Type()
function, 1
isn't even the actual value of TFile
.., thanks!
@Stebalien looks like @schomatis addressed your PR comments, can this be merged? |
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.
I'm not sure but I think this might change the output of ipfs ls <file>
when -l
is not used. Please add a test for that.
ipfs files ls -l /cats/file1 > ls_l_actual && | ||
test_cmp ls_l_expected ls_l_actual | ||
' | ||
|
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.
Please add a test for the ls output without -l
is unchanged when used on a file.
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.
Nice catch!
@kevina The field of the which was already set in the (unmodified) That being clarified, always happy to add more tests :) |
Okay the reason I bring it up is that 'ls /afile' should not fail if the hash for /afile is not available. But a quick test on the exiting version of IPFS verified this is not the case, so this is not a regression but something that could be cleaned up later. so LGTM. |
Good point, I should be replicating the logic of the |
Fixed, @kevina could you take another look please? Just to clarify, in the file case, the node has already been fetched through MFS, so getting its hash (or any other information) won't fail, in contrast, in the directory case, after the directory node is fetched other nodes need to be looked in the DAG (files inside the directory) for the command to build its output, leaving open the door for an potential error. However, even if this case can't fail, it should do the amount of work it needs to do and not more, so it needs to also (as the directory case) discriminate the |
License: MIT Signed-off-by: Lucas Molas <[email protected]>
License: MIT Signed-off-by: Lucas Molas <[email protected]>
Oh, fun fact, I've just realized that the |
Fixes #5044.