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

ls: Report real file size #5906

Merged
merged 4 commits into from
Jan 17, 2019
Merged

ls: Report real file size #5906

merged 4 commits into from
Jan 17, 2019

Conversation

magik6k
Copy link
Member

@magik6k magik6k commented Jan 8, 2019

Need this for ipfs/go-ipfs-http-client#1 to implement fast api.Unixfs().Get().

@magik6k magik6k requested a review from Kubuxu as a code owner January 8, 2019 16:41
@ghost ghost assigned magik6k Jan 8, 2019
@ghost ghost added the status/in-progress In progress label Jan 8, 2019
@magik6k magik6k force-pushed the feat/ls-filesize branch 2 times, most recently from 329a35b to 34ab008 Compare January 8, 2019 16:42
@magik6k magik6k mentioned this pull request Jan 8, 2019
Merged
1 task
@Stebalien
Copy link
Member

Stebalien commented Jan 8, 2019 •
edited
Loading

So, what if we made this the default? That is:

  1. Replace --resolve-size with just --size.
  2. Default this to true.
  3. Always use the bytes size for files.
  4. Consider adding a --disk-usage (or similar) flag. Not sure if we really care about this as, IIRC, this will be going away with UnixFSv2.

(issue for discussion: ipfs-inactive/interface-js-ipfs-core#427)

@Stebalien Stebalien mentioned this pull request Jan 8, 2019
Closed
@magik6k magik6k changed the title ls: option to report real file size ls: Report real file size Jan 11, 2019
@magik6k magik6k force-pushed the feat/ls-filesize branch 3 times, most recently from 7e6b597 to c4d041a Compare January 11, 2019 16:14
Stebalien
Stebalien requested changes Jan 11, 2019
View reviewed changes
Copy link
Member

@Stebalien Stebalien left a comment

Choose a reason for hiding this comment

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

Mostly looks good but I'm concerned with backwards compatibility. I expect that using actual file sizes will reduce confusion and actually fix bugs however, we should probably print something for directory sizes to avoid breaking column-based parsing.

test/sharness/t0045-ls.sh Outdated Show resolved Hide resolved
@magik6k magik6k mentioned this pull request Jan 12, 2019
Open
51 tasks
@magik6k magik6k added the need/review Needs a review label Jan 14, 2019
@magik6k @Stebalien
ls: option to report real file size
6bb02a5 
License: MIT
Signed-off-by: Łukasz Magiera <magik6k@gmail.com>
@magik6k @Stebalien
ls: report real size by default
e63c06c 
License: MIT
Signed-off-by: Łukasz Magiera <magik6k@gmail.com>
@magik6k @Stebalien
ls: skip size for directories
39c2e01 
License: MIT
Signed-off-by: Łukasz Magiera <magik6k@gmail.com>
@magik6k @Stebalien
ls: dummy value in place of size for dirs
3b5c071 
License: MIT
Signed-off-by: Łukasz Magiera <magik6k@gmail.com>
@ghost ghost assigned Stebalien Jan 17, 2019
@Stebalien Stebalien merged commit ac26948 into master Jan 17, 2019
@ghost ghost removed the status/in-progress In progress label Jan 17, 2019
@ghost ghost deleted the feat/ls-filesize branch January 20, 2019 18:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Stebalien Stebalien Stebalien approved these changes

@Kubuxu Kubuxu Awaiting requested review from Kubuxu

Assignees

@Stebalien Stebalien

@magik6k magik6k

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

Successfully merging this pull request may close these issues.
2 participants
@magik6k @Stebalien