commands: add git-lfs-migrate(1) 'info' subcommand by ttaylorr · Pull Request #2313 · git-lfs/git-lfs

ttaylorr · 2017-06-10T01:15:04Z

This pull request adds documentation and an implementation for the 'info' subcommand of the git-lfs-migrate(1) suite.

The 'info' subcommand is designed to gather statistics regarding the size of data stored in Git per filepath extension. The output of the command looks as follows:

~/g/git-lfs (migrate-subcommand-info) $ git-lfs migrate info \
  --include-ref=refs/heads/migrate-subcommand-info \
  --exclude-ref=refs/heads/master \
  --top=3
*.go 2.0 MB
*.sh 325.3 KB
*.md 225.6 KB

The following arguments are permitted:

--include-ref: a set of references of which commits reachable by those refs are included in the traversal.
--exclude-ref: a set of references of which commits reachable by those refs are excluded in the traversal.
--include: a filepath filter include set
--exclude: a filepath filter exclude set
<branch ...>: an optional set of branches to include, defaulting to the currently checked out branch, or those references given in --{include,exclude}-ref.

Here's a summary of the changes:

fd837f4...11b8d1f: document and specify command arguments in the man pages
b47255d: add scaffolding for subcommands to git-lfs-migrate(1).
58fda2d...1a67cec: helper functions necessary to complete a migration.
d2c6b0c: teach tools.ClampInt.
1612c44: add test fixtures for the info subcommand
af3871b: implement and test the info subcommand

This is the first command to be implemented as described in the git-lfs-migrate(1) proposal issue: (see discussion: #2146).

/cc @git-lfs/core

…ole repo

…teFn

technoweenie

The code looks good, but I have some minor review comments.

One thing: I don't think showing cumulative file size for a file type is that helpful. How can the info command help you decide what to migrate?

Allow user to pass in some kind of arbitrary size threshold. git lfs migrate info --size=5MB
Show some kind of histogram of file sizes. Median, 75/95/99 percentile?

technoweenie · 2017-06-12T20:40:22Z

commands/command_migrate.go

+		// `git-lfs-migrate(1)` command as a subcommand (child).
+
+		for _, subcommand := range []*cobra.Command{
+			MigrateInfoCommand,


I don't think it makes sense to declare this MigrateInfoCommand var in commands/command_migrate_info.go if some of the flags are added here. They should all be set up in this init() func. What do you think?

I think that's a great idea; it should make readers more easily aware of the various migrate subcommands. I made this change via: 1f443d0.

technoweenie · 2017-06-12T20:41:04Z

docs/man/git-lfs-migrate.1.ronn

+## MODES
+
+* `info`
+    Perform no migration, instead show information about repository size.


I think you trim the first part of this sentence. Talk about what it does.

Thanks for the suggestion, I much prefer your wording: d453d9d.

technoweenie · 2017-06-12T20:41:15Z

docs/man/git-lfs-migrate.1.ronn

+
+* [branch ...]:
+    Migrate only the set of branches listed. If not given, `git-lfs-migrate(1)`
+    will the currently checked out branch.


Looks like you're missing a word here :)

😅: 471fc9e.

ttaylorr · 2017-06-13T22:06:45Z

@technoweenie good call re: surfacing more relevant information. Here's an updated format I implemented in f3e0af6. What do you think?

~/g/git-lfs (migrate-subcommand-info) $ ./bin/git-lfs migrate info \
  --top=5 --above=10kb \
  --include-ref=refs/heads/migrate-subcommand-info
  --exclude-ref=refs/heads/master
Files above 10 KB:
*.go    906 KB, 48/479 files(s)  10%
*.bmp      154 KB, 1/2 files(s)  50%
*.sh      149 KB, 8/75 files(s)  11%
*.md      114 KB, 7/45 files(s)  16%
*.ico       34 KB, 1/1 files(s) 100%

technoweenie

This is looking really great. A few comments:

technoweenie · 2017-06-13T22:38:34Z

commands/command_migrate_info.go

+			}
+
+			// TODO(@ttaylorr): needed?
+			exts[ext] = entry


I think it's either needed here, or in the entry == nil block.

I think either works, but I dig the explicitness of exts[ext] = entry. Removed the TODO comment in 6b94d8b.

technoweenie · 2017-06-13T22:42:31Z

commands/command_migrate_info.go

+}
+
+// MigrateInfoEntry represents a tuple of filetype to total size taken by that
+// file type.


I think this description is a bit out of date, since the file type to entry mapping is the exts var.

Nice catch! Updated those docs and added some godoc to the fields on this type in 7c77064.

technoweenie · 2017-06-13T23:15:34Z

commands/command_migrate_info.go

+		percentage := fmt.Sprintf("%.0f%%", percentAbove)
+
+		percentages = append(percentages, percentage)
+	}


Two minor things about this:

First, it's given an io.Writer, but the output is buffered. It's not exactly a lot of data, but I'm just used to writing it out with fmt.Fprintf if the output doesn't need post-processing. However, having to deal with just 1 err from fmt.Fprintln is kinda nice.

Second: You could reduce how often e is looped by building extensions, files, and percentages at the same time. At a high level, the structure of this func would be something like:

Init extensions, files, percentages

Loop through e, appending to those slices

Rjust or Ljust them

Write header

Write each line with the justified extensions, files, percentages

I think structurally it makes more sense, but 36179de saves a whopping 3 LOC (7 if I didn't add comments).

However, having to deal with just 1 err from fmt.Fprintln is kinda nice.

Totally agree.

Second: You could reduce how often e is looped by building extensions, files, and percentages at the same time.

Thank you! This is such an awesome suggestion. I changed this in 0dbc358 and it made the code much cleaner.

… type

technoweenie · 2017-06-13T23:24:38Z

I think this output looks great, but the file sizes should be aligned too:

Files above 10 KB:
*.go    906 KB, 48/479 files(s)  10%
*.bmp      154 KB, 1/2 files(s)  50%
*.sh      149 KB, 8/75 files(s)  11%
*.md      114 KB, 7/45 files(s)  16%
*.ico       34 KB, 1/1 files(s) 100%

Aligned, it'd look something like:

*.go 	906 KB	48/480 files(s)	 10%
*.bmp	154 KB	   1/2 files(s)	 50%
*.sh 	149 KB	  8/75 files(s)	 11%
*.md 	114 KB	  7/45 files(s)	 16%
*.ico	34 KB 	   1/1 files(s)	100%

In chat, you questioned out loud about how to parse this output with scripts:

I know I suggested moving it to a header, but maybe Files above 10 KB: is not necessary? I don't think I need the command to repeat the --above flag to me.
While humanized sizes are definitely the way to go by default, IMO, what about introducing a flag to specify the unit? --size-unit=[ bytes | M | etc ].

technoweenie · 2017-06-13T23:25:37Z

commands/command_migrate_info.go

+	migrateInfoAboveFmt string
+	// migrateInfoAbove is the number of bytes parsed from the above
+	// migrateInfoAboveFmt flag.
+	migrateInfoAbove uint64


I think migrateInfoAbove should have a sensible default, like 1MB.

I think this output looks great, but the file sizes should be aligned too:

Good call: I built up a size array as well, and aligned it in 7d2dbd6. I think it looks much better this way!

ttaylorr · 2017-06-13T23:39:56Z

I know I suggested moving it to a header, but maybe Files above 10 KB: is not necessary? I don't think I need the command to repeat the --above flag to me.

I agree, and I really dig this reasoning. I think it a) makes the output easier to look at, b) doesn't repeat already known information, and c) makes the output easier to parse with Unix-y tools. Removed the header in ae76e72.

While humanized sizes are definitely the way to go, IMO, what about introducing a flag to specify the unit? --size-unit=[ bytes | M | etc ].

Totally possible, and I think this would make sense for a lot of command-line tools that want to parse the output.

ttaylorr · 2017-06-14T17:38:05Z

I think migrateInfoAbove should have a sensible default, like 1MB.

Good call, I made the default 1MB in 97b0f36. This can be easily changed in the future if 1MB is too high/low.

While humanized sizes are definitely the way to go, IMO, what about introducing a flag to specify the unit? --size-unit=[ bytes | M | etc ].

Totally possible, and I think this would make sense for a lot of command-line tools that want to parse the output.

Started looking into this, but I think this may be best done in a separate PR after this one, since it will involve changes in both the humanize package and the info command.

ttaylorr · 2017-06-14T17:38:39Z

@technoweenie so long as I follow up on the --size-unit flag in a separate PR, I think this is ready to go.

technoweenie

Looks great 🤘

ttaylorr · 2017-06-16T16:45:20Z

Thanks for the review!

The ClampInt() function does not always return a value between its "min" and "max" arguments because it reverses their meaning, and the relevant tests in the corresponding tools/math_test.go file do not report the problem because do not run when the "go test ./tools" command is run. The latter bug is a result of the test functions lacking the necessary Test* prefix to be recognized by the Go "test" command. After we fix the ClampInt() function, we rename all the test functions in tools/math_test.go to follow the Test*() naming scheme, which ensures they now run properly. Lastly, we change the one caller of the ClampInt() function, the migrateInfoCommand() function, which previously worked correctly because it also reversed the order of the arguments it passed to the function. That reversal was introduced in commit bd2e1b2 in PR git-lfs#2313, as part of the development of the "migrate info" command, presumably because the author discovered the call was not working as expected.

The ClampInt() function does not always return a value between its "min" and "max" arguments because it reverses their meaning, and the relevant tests in the corresponding tools/math_test.go file do not report the problem because they do not execute when the "go test ./tools" command is run. The latter bug is a result of the test functions lacking the necessary Test* prefix to be recognized by the Go "test" command. After we fix the ClampInt() function, we rename all the test functions in tools/math_test.go to follow the Test*() naming scheme, which ensures they now run properly. Lastly, we change the one caller of the ClampInt() function, the migrateInfoCommand() function, which previously worked correctly because it also reversed the order of the arguments it passed to the function. That reversal was introduced in commit bd2e1b2 in PR git-lfs#2313, as part of the development of the "migrate info" command, presumably because the author discovered the call was not working as expected.

ttaylorr added 22 commits June 8, 2017 16:55

docs/man: add entry for git-lfs-migrate(1)

fd837f4

commands: add skeleton 'migrate' command

c86e34f

docs/man: remove export mode from git-lfs-migrate(1)

d2f1802

docs/man: make git-lfs-migrate(1) migrate single branch instead of wh…

a4c5352

…ole repo

docs/man: refer to migration 'types' as 'modes'

2f2430c

docs/man: remove DECSRIPTION section

fd531d3

docs/man: remove 'import' mode from git-lfs-migrate(1)

f751df5

docs/man: teach git-lfs-migrate(1) --{include,exclude}-ref=<ref>

11b8d1f

commands/command_migrate: wire up subcommands

b47255d

commands/command_migrate: implement getObjectDatabase

58fda2d

commands/command_migrate: teach finding current ref

d557056

commands/command_migrate: teach how to format ref names

2aeb148

commands/command_migrate: teach how to find all remote refs

ce89eb3

commands/command_migrate: teach how to determine included/excluded refs

3e26421

commands/command_migrate: teach how to form rewrite options

547749d

commands/command_migrate: teach how to form a history rewriter

1a67cec

commands/migrate: teach how to migrate a repository given a BlobRewri…

169abf6

…teFn

tools/math: teach tools.ClampInt to clamp integers

d2c6b0c

test: add repository fixtures for git-lfs-migrate(1) info

1612c44

commands/command_migrate: add 'info' subcommand

af3871b

Merge branch 'master' into migrate-cmd

897ac48

commands/migrate_info: fix arguments to tools.ClampInt

bd2e1b2

ttaylorr added the review label Jun 10, 2017

ttaylorr added this to the v2.2.0 milestone Jun 10, 2017

ttaylorr requested a review from technoweenie June 10, 2017 01:15

Merge branch 'master' into migrate-subcommand-info

1b0d830

technoweenie suggested changes Jun 12, 2017

View reviewed changes

ttaylorr added 3 commits June 12, 2017 15:22

commands/migrate: declare info subcommand in main init()

1f443d0

docs/man: explicitly state purpose of 'info' mode

d453d9d

docs/man: add a missing 'migrate' 😅

471fc9e

ttaylorr added 4 commits June 13, 2017 16:00

tools: implement tools.Rjust([]string{...})

4f1ae98

tools: implement tools.Ljust

bea1d63

commands/migrate: teach --above

f3e0af6

tools: ensure Longest() is called

481c6be

technoweenie reviewed Jun 13, 2017

View reviewed changes

ttaylorr added 2 commits June 13, 2017 17:22

commands/command_migrate_info: remove TODO comment

6b94d8b

commands/command_migrate_info: add documentation to *MigrateInfoEntry…

7c77064

… type

technoweenie reviewed Jun 13, 2017

View reviewed changes

ttaylorr added 4 commits June 13, 2017 17:28

commands/command_migrate_info: build cols all at once

0dbc358

commands/command_migrate_info: rename 'files' to 'stats'

8f573f8

commands/commands_migrate_info: align stat, size

7d2dbd6

commands/command_migrate_info: remove header

ae76e72

ttaylorr added 2 commits June 14, 2017 11:34

commands/migrate: make --above default to 1 MB

97b0f36

docs/man: add additional 'info' options

62462e3

Merge branch 'master' into migrate-subcommand-info

ca31ac0

commands/command_migrate_info: print entries to os.Stdout

aff6edc

ttaylorr mentioned this pull request Jun 15, 2017

commands: add logging to git-lfs-migrate-info(1) #2329

Merged

technoweenie approved these changes Jun 16, 2017

View reviewed changes

ttaylorr merged commit 56871cf into master Jun 16, 2017

ttaylorr deleted the migrate-subcommand-info branch June 19, 2017 15:53

chrisd8088 mentioned this pull request Mar 11, 2021

Fix up handling of the "migrate info" command's --top option #4434

Merged

Conversation

ttaylorr commented Jun 10, 2017

Uh oh!

technoweenie left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ttaylorr commented Jun 13, 2017

Uh oh!

technoweenie left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

technoweenie commented Jun 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ttaylorr commented Jun 13, 2017

Uh oh!

ttaylorr commented Jun 14, 2017

Uh oh!

ttaylorr commented Jun 14, 2017

Uh oh!

technoweenie left a comment

Choose a reason for hiding this comment

Uh oh!

ttaylorr commented Jun 16, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

technoweenie commented Jun 13, 2017 •

edited

Loading