Skip to content

RFC: add option "--size" to ls-files to print the file size#1829

Closed
larsxschneider wants to merge 2 commits intogit-lfs:masterfrom
larsxschneider:lars/print-size
Closed

RFC: add option "--size" to ls-files to print the file size#1829
larsxschneider wants to merge 2 commits intogit-lfs:masterfrom
larsxschneider:lars/print-size

Conversation

@larsxschneider
Copy link
Member

@larsxschneider larsxschneider commented Jan 5, 2017

This was useful for me to inspect the size of LFS files.

There are two somewhat controversial decisions in there and I would like to get your feedback on them:

  1. I print size in MB only. Everything else made the code more complicated for little gain IMHO.
  2. I pad the size with 6 chars to align it nicely... this would of course break if someone tracks a crazy large file

@technoweenie
Copy link
Contributor

This seems fine to me. Why not make it default though?

@larsxschneider
Copy link
Member Author

I thought about making it the default. I didn't do it because I thought maybe someone is using the current git lfs ls-files output format? (I spent to much time on the git-core mailing list and adopted their backwards compatibility thinking, haha).

@larsxschneider
Copy link
Member Author

Up to you. Default would be fine with me!

@technoweenie
Copy link
Contributor

How about we merge this and backport to v1.5.x, but make it default in a follow-up PR for 2.x (master)?

@ttaylorr
Copy link
Contributor

ttaylorr commented Jan 5, 2017 via email

@larsxschneider
Copy link
Member Author

In the default version we wouldn't need the -s / --size switch anymore, right? In that case I would rather remove all this cruft. I don't think this thing requires back-porting...

@ttaylorr no rush. I thought about something like that. I wonder if this makes automated parsing harder (I use the output in scripts)? Maybe an automated MB/GB conversion could be the default and with a special argument we make it always print bytes without padding? Would that be too complicated?

@technoweenie
Copy link
Contributor

I say we keep size in MB as default. Anything special should be handled in some kind of --format flag, as proposed in #1814 (comment). That definitely does not need to happen in this PR though.

@ttaylorr Sure, I was hoping to get a third opinion before merging :)

@ttaylorr
Copy link
Contributor

ttaylorr commented Jan 5, 2017

Here's the piece of code that I was thinking about, but I'm not sure that it's relevant to the suggestion I'm going to make. I think a good compromise on this would be making --size take an argument, i.e.: --size=mb, --size=b, --size=gb, etc. The default --size= could just assume MBs. I think this would be a good start on our way towards full support of --porcelain/--format. @larsxschneider @technoweenie what are your thoughts on this?

The code looks 👍, but I think adding an integration test asserting that the sizes get printed out as expected would be good before merging.

@technoweenie
Copy link
Contributor

The default --size= could just assume MBs. I think this would be a good start on our way towards full support of --porcelain/--format.

How so? A custom arg like --size seems the opposite of a general --format arg.

@ttaylorr
Copy link
Contributor

ttaylorr commented Jan 9, 2017 via email

@technoweenie
Copy link
Contributor

Since there's not a huge need to backport to 1.5, I'd rather just display the size in MB by default, and potentially add --format before a 2.0 release. I don't think incrementally adding --format support over several releases is a bad thing.

@ttaylorr
Copy link
Contributor

ttaylorr commented Jan 9, 2017 via email

@larsxschneider
Copy link
Member Author

Superseded by #2540

@larsxschneider larsxschneider deleted the lars/print-size branch August 31, 2017 08:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants