push optimizations (take 2) #1128
Conversation
|
@dluksza: I applied #969 on top of this in https://github.com/github/git-lfs/compare/skip-pre-push-dupes-2...include-refs-in-transfers?expand=1. Still working on it :) I wouldn't check that branch out yet, I plan to rebase it against this PR once it's merged. |
| return nil, fmt.Errorf("Failed to call git show-ref: %v", err) | ||
| } | ||
| cmd.Start() | ||
| defer cmd.Wait() |
There was a problem hiding this comment.
I think any error from cmd.Wait() should be returned at the end. (edit) ideally with the contents of stderr, that's proving important in diagnosing git errors instead of getting the generic exit code 128
There was a problem hiding this comment.
Fixed in 0935817.
|
👍 just stylistic & potential optimisation comments from me. This is a really big improvement for overlapping refs. |
|
Big refactor in 0935817 taking most of your feedback into account.
|
|
👍 Like it, lots of iteration has collapsed too & I like the new methods. I have maybe one more suggestion but it's optional, this would be fine. |
|
|
||
| // AddUpload adds the given oid to the set of oids that have been uploaded in | ||
| // the current process. | ||
| func (c *uploadContext) AddUpload(oid string) { |
There was a problem hiding this comment.
Nit picky comment: AddUpload sounds like a request to add an upload to be done, how about either AddUploaded or SetUploaded to match HasUploaded?
This PR brings back the push optimizations from #1040. Hit up #1040 and #1071 to get the original context. This makes a few changes based on feedback:
Add ref information to upload request #969 is no longer included. That will come in the next PR.
extract common pre-push and push uploading code #1071 (comment)
It's now called
uploadContext. It's still unexported, so I expect the type to change and move from thecommandspackage some day.https://github.com/github/git-lfs/pull/1071/files#diff-d540c48abbcd5b13b0acd4cbef433700R57
Handled in
newUploadContext()https://github.com/github/git-lfs/pull/1071/files#r55989899
Agreed. There are now fewer test changes. The reason the "skipped" output changed, is because it was being handled in the old filter() before the
*lfs.TransferQueueobject for the upload is created. Now, the only objects the*uploadContextskips those that have already been uploaded in the current process (usually from another ref).