Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

fix(files): add slash for dir #5605

Merged
merged 2 commits into from
Oct 18, 2018

Conversation

overbool
Copy link
Contributor

Fixes: #5601

License: MIT
Signed-off-by: Overbool overbool.xu@gmail.com

License: MIT
Signed-off-by: Overbool <overbool.xu@gmail.com>
@overbool overbool requested a review from Kubuxu as a code owner October 16, 2018 14:16
@overbool
Copy link
Contributor Author

@schomatis I add the slash for dir when using -l flag(#5601 (comment))

@schomatis
Copy link
Contributor

Yes, we can later evaluate adding it for both cases if you feel like it, but this seems like an inexpensive first approach.

@schomatis
Copy link
Contributor

Could you add a test case for it? (Or modify the existing ones, I'm not sure if this change will break them, we sometimes check with grep for certain strings like the file name.)

@overbool
Copy link
Contributor Author

@schomatis I had added the test case

Copy link
Contributor

@schomatis schomatis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@@ -260,6 +260,12 @@ test_files_api() {
verify_dir_contents /cats/this/is/a/dir
'

test_expect_success "dir has correct name" '
echo "this/" > ls_dir_expected &&
ipfs files ls -l /cats | grep this/ | awk '\''{print $1}'\'' > ls_dir_actual &&
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the standard way is to output "filename hash size" in the ls_dir_actual instead of using awk+grep.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@schomatis yeah, I also think that "filename hash size" is a better way. However, the hash is unfixed, because ipfs files mkdir uses different arg:
https://github.com/ipfs/go-ipfs/blob/9bf4e4145ea897ced2f9aa2087296b3beda6e941/test/sharness/t0250-files-api.sh#L109-L116

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should then save the dir hash generated on every call, although I'm not sure if that's the best approach, @magik6k WDYT?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep, we should grab the generated hash, save it and compare that

@schomatis schomatis requested a review from magik6k October 16, 2018 15:54
License: MIT
Signed-off-by: Overbool <overbool.xu@gmail.com>
@overbool overbool force-pushed the feat/add-slash-in-files-ls branch from 89574c8 to ec8e451 Compare October 16, 2018 17:06
@overbool
Copy link
Contributor Author

@schomatis @magik6k I had modified the test case.

@schomatis schomatis added RFM topic/commands Topic commands topic/files Topic files labels Oct 16, 2018
@Stebalien Stebalien merged commit 084c7d1 into ipfs:master Oct 18, 2018
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
RFM topic/commands Topic commands topic/files Topic files
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants