Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix "ipfs ls" so it works correctly with raw leaves. #3557

Merged
merged 3 commits into from
Jan 4, 2017
Merged

Fix "ipfs ls" so it works correctly with raw leaves. #3557

merged 3 commits into from
Jan 4, 2017

Conversation

kevina
Copy link
Contributor

@kevina kevina commented Jan 3, 2017

Closes #3548

@kevina kevina added the status/in-progress In progress label Jan 3, 2017
@kevina kevina mentioned this pull request Jan 3, 2017
Closed
@kevina
Copy link
Contributor Author

kevina commented Jan 3, 2017

Note, the first commit "Add test for adding directory with --raw-leaves option." is not strictly required, but still I good idea to get more coverage when the "--raw-leaves" option is used (I wrote it thinking ipfs ls was tested in that file, was mistaken, but decided to keep it anyway).

@kevina kevina changed the title Fix "ipfs ls" so it works correctly with raw leaves. WIP: Fix "ipfs ls" so it works correctly with raw leaves. Jan 3, 2017
@kevina
Copy link
Contributor Author

kevina commented Jan 3, 2017 •
edited
Loading

Note: There doesn't seam to be any tests for the --resolve-type option in the ipfs ls command. Before I am comfortable with this comment I will need to add some tests for that as I needed to modify that code to fix #3548 and I just caught a bug in the p.r.

I will do that some time Tuesday.

@kevina
Fix "ipfs ls" so it works correctly with raw leaves.
6cc0903 
License: MIT
Signed-off-by: Kevin Atkinson <[email protected]>
@kevina
Copy link
Contributor Author

kevina commented Jan 4, 2017

Okay this should be ready for review now.

@kevina kevina added need/review Needs a review and removed status/in-progress In progress labels Jan 4, 2017
@kevina kevina changed the title WIP: Fix "ipfs ls" so it works correctly with raw leaves. Fix "ipfs ls" so it works correctly with raw leaves. Jan 4, 2017
@Kubuxu
Copy link
Member

Kubuxu commented Jan 4, 2017 •
edited
Loading

The 3526c26 is before the fix, does it cause the tests to fail? If it does it should be moved to after the fix.

This way git-bisect will still work.

Kubuxu
Kubuxu reviewed Jan 4, 2017
View reviewed changes
test/sharness/t0045-ls.sh
# should not exist on the network, but we don't want to wait for a
# timeout so we will kill the request after a few seconds
test_expect_success "'ipfs ls --resolve-type=false' ok and does not hang" '
ipfs ls --resolve-type=false $DIR > /dev/null &
Copy link
Member

Choose a reason for hiding this comment

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

You can use go-timeout here.

Kubuxu
Kubuxu reviewed Jan 4, 2017
View reviewed changes
test/sharness/t0045-ls.sh
PID=$!
sleep 2
kill $PID 2> /dev/null
wait $PID
Copy link
Member

Choose a reason for hiding this comment

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

Wait after kill is not guaranteed to return valid return code, safer to use go-timeout.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

@kevina
Copy link
Contributor Author

kevina commented Jan 4, 2017

The 3526c26 is before the fix, does it cause the tests to fail? If it does it should be moved to after the fix?

No. It was an extra test that turned out to be unnecessary, but I decided to use it anyway.

@kevina
Add tests for "ls --resolve-type=false".
980e341 
License: MIT
Signed-off-by: Kevin Atkinson <[email protected]>
whyrusleeping
whyrusleeping approved these changes Jan 4, 2017
View reviewed changes
Copy link
Member

@whyrusleeping whyrusleeping left a comment

Choose a reason for hiding this comment

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

LGTM

@whyrusleeping whyrusleeping merged commit 12d51a6 into master Jan 4, 2017
@whyrusleeping whyrusleeping deleted the kevina/raw-leaves-ls-fix branch January 4, 2017 21:02
@ghost ghost mentioned this pull request Jan 27, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@whyrusleeping whyrusleeping whyrusleeping approved these changes

@Kubuxu Kubuxu Kubuxu approved these changes

Assignees
No one assigned
Labels
need/review Needs a review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@kevina @Kubuxu @whyrusleeping