Skip to content
This repository has been archived by the owner on Feb 12, 2024. It is now read-only.

fix: cat: test file existence after filtering #1148

Merged
merged 4 commits into from
Dec 15, 2017
Merged

Conversation

pgte
Copy link
Contributor

@pgte pgte commented Dec 14, 2017

Should fix #1142

@codecov
Copy link

codecov bot commented Dec 14, 2017

Codecov Report

Merging #1148 into master will increase coverage by 0.01%.
The diff coverage is 91.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1148      +/-   ##
==========================================
+ Coverage   83.64%   83.66%   +0.01%     
==========================================
  Files         122      122              
  Lines        2740     2749       +9     
==========================================
+ Hits         2292     2300       +8     
- Misses        448      449       +1
Impacted Files Coverage Δ
src/core/components/files.js 94.53% <91.66%> (-0.43%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5bfde87...abbaebd. Read the comment docs.

@daviddias
Copy link
Member

@alanshaw can you test with this PR?

@alanshaw
Copy link
Member

screen shot 2017-12-14 at 22 12 25

I'm getting "No such file" error now

@daviddias
Copy link
Member

.cat is returning two files for a directory?

@daviddias daviddias added the P1 High: Likely tackled by core team if no one steps up label Dec 15, 2017
@pgte
Copy link
Contributor Author

pgte commented Dec 15, 2017

@diasdavid the exporter is exporting the dir, and apparently cat is using that export stream, so it has to filter based on path.

The problem I see here is that the ipfsPath that is being passed in has the /ipfs/ trailer, which I guess was supposed to have been removed before being passed in.

@alanshaw could you tell me the API call are you doing to trigger this cat?

@alanshaw
Copy link
Member

It's a direct call to js-ipfs in the browser - ipfs.files.cat.

@alanshaw
Copy link
Member

@pgte
Copy link
Contributor Author

pgte commented Dec 15, 2017

@alanshaw thanks for the test!
Looks like the latest abbaebd fixes this:

web_inspector_ _localhost

@daviddias daviddias merged commit 34f28ef into master Dec 15, 2017
@daviddias daviddias deleted the fix/cat-unknown branch December 15, 2017 11:09
@ghost ghost removed the status/in-progress In progress label Dec 15, 2017
@alanshaw
Copy link
Member

Directories now display so \o/

I'm so sorry @pgte but this change may have caused an issue elsewhere...listings for nested directories (using ipfs.ls) are now not correct:

Expected (using go-ipfs):

screen shot 2017-12-15 at 12 13 10

Actual (using js-ipfs):

screen shot 2017-12-15 at 12 13 21

I'll check now if this was an existing bug or not and report back.

@daviddias
Copy link
Member

@alanshaw I believe that is another bug , see #1079

richardschneider pushed a commit that referenced this pull request Dec 17, 2017
* fix: cat: test file existence *after* filtering. Should fix #1142

* fix: cat: filtering out result files

* fix: cat: for when the ipfs path is a buffer

* fix: cat: remove /ipfs/ prefix if there is one
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
P1 High: Likely tackled by core team if no one steps up
Projects
None yet
Development

Successfully merging this pull request may close these issues.

jsipfs cat on a directory not working correctly
3 participants