Gitscanner callbacks by technoweenie · Pull Request #1725 · git-lfs/git-lfs

technoweenie · 2016-11-29T21:16:52Z

This changes the gitscanner API to use callbacks, instead of returning a PointerChannelWrapper. See the fsck command for an ideal usage of it:

gitscanner := lfs.NewGitScanner(func(p *lfs.WrappedPointer, err error) {
  // handle pointers or scan errors
})

if err := gitscanner.ScanRefWithDeleted(ref.Sha, nil); err != nil {
  // handle cmd error
}

gitscanner.Close()

You can now pass a callback to NewGitScanner() for all the .Scan*() funcs, or pass a callback to individual functions if you want those pointers to be handled differently.

Unfortunately, this introduces some ugly code in the core push/pull/checkout related commands. A lot of code still expects to receive a []*lfs.WrappedPointer after the scan has completed. This cleanup will have to happen in future PRs. The main blockers are the progress meter and transfer queues need to accept a stream of pointers.

My hunch is that the transfer queue is all ready setup for this, but our use of it prefers receiving a []*lfs.WrappedPointer. However, the progress meter definitely expects to receive the total number and size of the pointers up front. Since this process is becoming async, updating the progress meter stuff is my next target.

…leted(), ScanRefRange(), and ScanLeftToRemote()

ttaylorr

Looking good, I just left a few comments that I'd love to 👂 your 💭 's on.

ttaylorr · 2016-11-30T16:24:56Z

commands/command_checkout.go

+	// use new gitscanner so mapping has all the scanned pointers before continuing
+	mapping := make(map[string][]*lfs.WrappedPointer)
+	chgitscanner := lfs.NewGitScanner(nil)
+	err = chgitscanner.ScanTree(ref.Sha, func(p *lfs.WrappedPointer, err error) {


Do you think this callback should be moved into the line above since (I think) we're leaning towards getting rid of the per-operation callbacks?

ttaylorr · 2016-11-30T16:52:57Z

commands/command_fetch.go

+	tempgitscanner := lfs.NewGitScanner(func(p *lfs.WrappedPointer, err error) {
+		if err != nil {
+			if multiErr != nil {
+				multiErr = fmt.Errorf("%v\n%v", multiErr, err)


What do you think about extracting this into a function in the errors package? I'd prefer to use errors.Wrap(), but I think it defaults to "Error" if err is nil.

package errors func CombineErrors(e1, e2 error) error { if e1 == nil { return e2 } return fmt.Errorf("%v\n%v", e1, e2) }

There's probably a pretty cool opportunity to do something like:

func CombineErrors(car error, cdr ...error) error { ... }

but I don't think we need it.

Coming back to this, if CombineErrors were to go in the errors package, I think it'd be better to just call it errors.Combine.

Yeah, we could probably expose the inner errors somehow too. I'll experiment with this in another PR.

I added errors.Combine in my original locking PR so 👍 (although it only concatenated with a line break as you're doing here, nothing smart like storing inner errors)

ttaylorr · 2016-11-30T16:58:32Z

commands/command_fetch.go

+	var pointers []*lfs.WrappedPointer
+
+	tempgitscanner := lfs.NewGitScanner(nil)
+	err := tempgitscanner.ScanPreviousVersions(ref, since, func(p *lfs.WrappedPointer, err error) {


Same comment here about inlining the callback to the above line (and then shortening this if statement to not leak the err variable).

ttaylorr · 2016-11-30T16:59:32Z

commands/command_fsck.go

-			return false, err
+	for _, oid := range corruptOids {
+		badFile := filepath.Join(badDir, oid)
+		if err := os.Rename(lfs.LocalMediaPathReadOnly(oid), badFile); err != nil {


Huh, I didn't know you could rename like this!

How else do you rename?

Ha, I definitely meant to type "move" in my original comment. I thought there was a os.MoveFile function, but I was mistaken 😅

ttaylorr · 2016-11-30T17:01:56Z

commands/command_fsck.go


-	if ok {
-		Print("Git LFS fsck OK")
+	recalculatedOid := hex.EncodeToString(oidHash.Sum(nil))


Oh man, this is so much better than

fmt.Sprintf("%x", oidHash.Sum(nil))

ttaylorr · 2016-11-30T17:02:47Z

commands/command_pre_push.go

+		pointers = append(pointers, p)
+	}
+
+	if err := g.ScanLeftToRemote(ref, cb); err != nil {


Are we OK to do this if multiErr != nil?

Yes. If this returns an error, the command that ScanLeftToRemote() attempts to spin up errored, so the callback potentially sets multiErr is never called.

Awesome, thanks for the explanation 👍

ttaylorr · 2016-11-30T17:04:59Z

lfs/gitscanner.go

+		return cb, nil
+	}
+
+	return nil, errors.New("No callback given")


We talked about this in 🍖 -space, but this would be a good candidate to extract to a sentinel value if you want to keep the firstGitScannerCallback business 👍

ttaylorr · 2016-11-30T18:48:28Z

lfs/gitscanner.go

+
+// CallbackMissing returns a boolean indicating whether the error is reporting
+// that a GitScanner is missing a required GitScannerCallback.
+func CallbackMissing(err error) bool {


I dig IsCallbackMissing instead of just CallbackMissing.

It was a 50/50 toss-up for me, but your recommendation made it a 51/49 split :)

sinbad · 2016-12-05T14:52:38Z

It feels a little odd to me to supply the callback in NewGitScanner instead of per Scan... function. Are you really likely to want to use the same callback for more than one invocation of Scan...? Feels to me like you'd mostly want different callbacks each time, and the callback implementation flowing after the Scan.. call feels more natural and readable to having to look back at the construction.

technoweenie · 2016-12-05T17:59:36Z

Part of the point in the git scanner redesign is spawning the git cat-file processes a single time for large pushes, reducing how many git commands are spawned overall. Most uses of the git scanner can be changed to pass the pointers asynchronously to the transfer queue, but a lot of cruft prevents us from doing it in this PR. I plan to tackle the progress meter next, while @ttaylorr works on the transfer queue.

That said, I do think passing the callback on the constructor is pretty strange from an API perspective. There's still time before the API is finalized a git/gitscanner package, so I expect the callback situation will change.

technoweenie added 7 commits November 29, 2016 09:40

introduce gitscanner callbacks for ScanUnpushed()

1ffbea6

introduce gitscanner callbacks for ScanIndex()

52c9965

gitscanner Scan funcs should take optional callbacks

0a38194

introduce gitscanner callbacks to ScanPreviousVersions()

d8ca610

introduce gitscanner callbacks to ScanTree()

ea95f21

introduce gitscanner callbacks to ScanAll(), ScanRef(), ScanRefWithDe…

3085dc3

…leted(), ScanRefRange(), and ScanLeftToRemote()

rewrite fsck command to use gitscanner api

0569c17

technoweenie added the review label Nov 29, 2016

ttaylorr reviewed Nov 30, 2016

View reviewed changes

technoweenie added 2 commits November 30, 2016 10:30

gitscanner: prefer callbacks in the constructor

2040427

gitscanner: sentinel value for missing callback error

f307bc5

ttaylorr reviewed Nov 30, 2016

View reviewed changes

gitscanner: ask the question

a10f909

technoweenie merged this pull request into fast-walk-no-channels Dec 6, 2016

technoweenie deleted the gitscanner-callbacks branch December 6, 2016 16:25

technoweenie restored the gitscanner-callbacks branch December 6, 2016 22:37

technoweenie deleted the gitscanner-callbacks branch December 6, 2016 22:39

technoweenie restored the gitscanner-callbacks branch December 6, 2016 22:40

technoweenie mentioned this pull request Dec 6, 2016

Gitscanner callbacks master #1743

Merged

ttaylorr deleted the gitscanner-callbacks branch December 16, 2016 22:47

Conversation

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

Uh oh!

Uh oh!

ttaylorr 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!

ttaylorr Nov 30, 2016 • 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!

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!

Choose a reason for hiding this comment

Uh oh!

sinbad commented Dec 5, 2016

Uh oh!

technoweenie commented Dec 5, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

technoweenie commented Nov 29, 2016 •

edited

Loading

ttaylorr Nov 30, 2016 •

edited

Loading