commands/prune: unify formatting #2757

ttaylorr · 2017-11-28T03:53:02Z

This pull request removes the use of the *progress.Spinner type in git-lfs-prune(1) in favor of a more consistent logging scheme.

Before:

~/example (master) $ git lfs prune
Pruning 1 files, (18 B)tained
✔ 3 local objects, 2 retained, done
✔ Deleted 1 files, done

After:

~/example (master) $ git lfs prune
prune: 3 local object(s), 2 retained, done
prune: Deleting objects: 100% (1/1), done

This removes one of two existing uses of the *progress.Spinner implementation, and the next pull-request in this series will remove the other use-case, which will allow *progress.Spinner to be deleted.

Please let me know if you have any thoughts on the formatting of these messages. I tried to make them as similar as I would imagine Git writing them, with these thoughts in mind:

Lowercase prefix of the command that is being run.
", done\n" on the end (courtesy of *tasklog.Logger).
Use percentages where appropriate, text where otherwise.

/cc @git-lfs/core

ttaylorr · 2017-11-29T18:19:31Z

~~TODO: looks like there are some tests failing here that I have to look into.~~

ttaylorr · 2017-12-02T01:34:07Z

All fixed. Here's a quick breakdown of what was going wrong --

The final update sent from *SimpleTasks were sometimes being dropped since close()-ing the updates channel does not wait for acknowledgement from the tasklog.Logger. By adding a "secret" OnComplete() method that is called if the Task implements it, we can wait for the logger to close.
Some of the final updates were being dropped in a race-y fashion because we were failing to call logger.Close() (fixed in 4b33121). Adding this call waits until not only all active tasks have closed, but also until the logger has logged all messages.

technoweenie

This is a great start. I made some suggestions relating to where the tasks are initialized and cleaned up.

technoweenie · 2017-12-04T19:46:00Z

commands/command_prune.go

+		pruneDeleteFiles(prunableObjects, percentage)
 	}

+	logger.Close()


Should this be moved to a defer right after the logger is initialized? That ensures that it's closed if this func returns early, such as when prunableObjects is empty.

technoweenie · 2017-12-04T19:47:42Z

commands/command_prune.go

 }

-func pruneTaskDisplayProgress(progressChan PruneProgressChan, waitg *sync.WaitGroup, spinner *progress.Spinner) {
+func pruneTaskDisplayProgress(progressChan PruneProgressChan, waitg *sync.WaitGroup, task *tasklog.SimpleTask) {


I'm not crazy about this function receiving a task, and then closing it. What do you think about sending it the logger instead, so that the task is created, enqueued, used, and completed all in this func?

technoweenie · 2017-12-04T19:49:15Z

commands/command_prune.go

-		logger.Enqueue(spinner)
+		// enqueue a _new_ percentage task to track deletion progress.
+		percentage := tasklog.NewPercentageTask("prune: Deleting objects", uint64(len(prunableObjects)))
+		logger.Enqueue(percentage)


Since this percentage is only used inside pruneDeleteFiles(), why not pass logger to it instead. Same justification as my note on pruneTaskDisplayProgress().

technoweenie · 2017-12-04T19:49:51Z

tasklog/percentage_task_test.go

 }

 func TestPercentageTaskCallsDoneWhenComplete(t *testing.T) {
+	t.Skip()


Is this skip temporary, or should the test be removed?

ttaylorr · 2017-12-04T20:02:48Z

@technoweenie thanks for the feedback, I agree with everything you said 😄. I un-skipped the test in 088b2ab, and changed the above methods to take a *tasklog.Logger, not a tasklog.Task in cde392b.

Can you take another look? 🙇

technoweenie · 2017-12-04T20:05:50Z

commands/command_prune.go

-func pruneDeleteFiles(prunableObjects []string, task *tasklog.PercentageTask) {
+func pruneDeleteFiles(prunableObjects []string, logger *tasklog.Logger) {
+	task := tasklog.NewPercentageTask("prune: Deleting objects", uint64(len(prunableObjects)))
+	logger.Enqueue(task)


Small idea for another PR:

task := logger.NewPercentageTask(...) // create and enqueue task defer task.Complete()

🤦‍♂️ I'm silly -- this exists! Changed in 65b4f34.

technoweenie · 2017-12-04T21:00:00Z

commands/command_prune.go

+		info.Logf("prune: %d file(s) would be pruned (%s)", len(prunableObjects), humanize.FormatBytes(uint64(totalSize)))
 		if verbose {
-			Print(verboseOutput.String())
+			for _, item := range verboseOutput {


I think you can take out the two extra if verbose (here and L194), since verboseOutput will be empty in that case.

Good, call -- I changed this in 782c58c.

technoweenie · 2017-12-04T21:03:25Z

commands/command_prune.go

+				info.Logf("\n%s", item)
+			}
 		}
+		info.Complete()


This could be moved to a defer, allowing the one on L192 to be removed too. I point it out because I think having a consistent pattern for dealing with loggers and tasks is important. I really like how the tasklog package is shaping up :)

Hm, I don't think that these calls to Complete() can be deferred, since pruneDeleteFiles creates a new task, which (for the *tasklog.Logger to output), requires there to be no actively running task.

technoweenie

I have a few nitpicky things, but overall I think this is a nice improvement. The old prune output was fine, but I very much prefer the Git style output for some reason.

In PR git-lfs#2757 the output of the "git lfs prune" command was revised to use the methods of the Logger structure of the "tasklog" package to output progress messages, replacing use of the Spinner structure from the older "progress" package. The Spinner structure and its methods were then removed in PR git-lfs#2759, and the "progress" package was removed entirely in PR git-lfs#2762. As part of this change, the "git lfs prune" command's progress messages were reformatted so as to always begin with the Git LFS subcommand name, for example, "prune: 8 local objects, 9 retained, done." Two other Git LFS commands follow the same design, the "git lfs fetch" and "git lfs migrate" commands, but no others. Git commands also do not prefix their progress messages with the name of the subcommand (although messages from a Git remote may be reported with the prefix "remote:"). To help bring all our commands' progress messages into alignment, we simply remove the "prune:" prefix from the messages output by the "git lfs prune" command. We expect to revise the progress messages from the "git lfs fetch" and "git lfs migrate" commands in the same way in subsequent commits of this PR.

ttaylorr added the review label Nov 28, 2017

ttaylorr added this to the v2.4.0 milestone Nov 28, 2017

ttaylorr requested a review from technoweenie November 28, 2017 03:53

commands: drop use of *Spinner in git-lfs-prune(1)

71b60d1

ttaylorr force-pushed the prune-unify-formatting branch from 2778479 to 71b60d1 Compare November 29, 2017 04:08

This was referenced Nov 29, 2017

commands/fetch: unify formatting #2758

Merged

progress: remove spinner.go #2759

Merged

command/prune: use *SimpleTask for printing to avoid clobbering

c237cf3

ttaylorr added 12 commits December 1, 2017 13:42

commands/prune: write verbose output per-line

1fceed9

Merge branch 'master' into prune-unify-formatting

1d4ccd4

tasklog/log: call OnComplete before accepting new tasks, if implemented

84c5dfe

tasklog/simple_task: wait for call to OnComplete

d438eab

tasklog: update tests

4276e1b

commands/prune: re-organize formatting for verboseOutput

cd7d883

tasklog: try this

b8fc8f1

tasklog: skip this for now

07be1f5

tasklog: revert adding *sync.WaitGroup

bb43fc0

commands/prune: ensure that the logger is closed before exiting

4b33121

tasklog: remove 'wg' reference from *PercentageTask

51d0fe4

commands/prune: ensure that info is Complete()'d

588c46f

technoweenie reviewed Dec 4, 2017

View reviewed changes

ttaylorr added 2 commits December 4, 2017 12:01

tasklog: un-t.Skip() TestPercentageTaskCalculatesPercentWithoutTotal

088b2ab

commands/prune: pass *tasklog.Logger, not task

cde392b

technoweenie reviewed Dec 4, 2017

View reviewed changes

commands/prune: use tasklog API to create percentage task

65b4f34

ttaylorr mentioned this pull request Dec 4, 2017

tasklog: teach *Logger how to enqueue new *SimpleTask's #2767

Merged

technoweenie reviewed Dec 4, 2017

View reviewed changes

technoweenie approved these changes Dec 4, 2017

View reviewed changes

commands/prune: remove unnecessary if verbose { ... }

782c58c

ttaylorr merged commit a98f6a9 into master Dec 5, 2017

ttaylorr deleted the prune-unify-formatting branch December 5, 2017 01:29

chrisd8088 mentioned this pull request May 27, 2023

Ensure all logging tasks are closed and simplify log task dispatching #5375

Merged

commands/prune: unify formatting #2757

commands/prune: unify formatting #2757

Uh oh!

Conversation

ttaylorr commented Nov 28, 2017

Uh oh!

ttaylorr commented Nov 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ttaylorr commented Dec 2, 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!

ttaylorr commented Dec 4, 2017

Uh oh!

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

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 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ttaylorr commented Nov 29, 2017 •

edited

Loading

technoweenie Dec 4, 2017 •

edited

Loading