Conversation
lfs/client.go
Outdated
There was a problem hiding this comment.
Just to clarify, closing these channels will stop the for loops in the goroutines once they've processed the last messages. Right? Not that it matters much since the command will exit right after this happens.
Also, I think Wait() is a strange name. It makes sense here, looking at the implementation of the queue. But I don't want the upload queue in the push command to wait, I want it to process or run or something. Prior art: http://golang.org/pkg/os/exec/#Cmd.Run
There was a problem hiding this comment.
Correct on the goroutines.
For Wait(), starting or running is implicit, we don't need to wait until the loop that figures out what to upload to finish before starting uploads. See http://golang.org/pkg/os/exec/#Cmd.Wait and http://golang.org/pkg/sync/#WaitGroup.Wait (which is what it's actually doing).
There was a problem hiding this comment.
<bike shedding comment>. Cool 👍
It might be cool to restructure Probably out of scope for this PR. Just thinking.... |
|
How will upload progress work on the cli output? We almost need something like the pb package that can track multiple, separate progress bars. |
|
I think posting an array would be good, that'll save a lot of round trips when there's a lot of files. I would agree that's out of scope for this PR, that would require an API change because I don't think we currently say that is allowed. For progress, it probably will have to be something with pb showing multiple bars. That should have been added to the TODO list. |
This refactoring will make a number of things easier. These didn't really need to be in their own packages. Having them separate packages, there are a few refactorings I've tried to do that end up with circular dependencies due to things outside of `lfs` depending on `lfs`. Pushing these into `lfs` makes refactoring simpler.
|
A progress bar that shows multiple entries is going to be problematic. It's not too complicated on unixy environments with ansi codes, but windows is a different story. I think you can access some terminal manipulation routines through the windows syscalls and kernel32, but that's a bit beyond my knowledge. There are a couple cross platform terminal ui libraries (e.g. go-termbox), but those want to take over the entire screen, and with git also dumping output to the screen that's not very practical. We have a few options we can do:
|
I like this idea. Instead of a single "uploading foo.bar (100MB)" message per file, have a single message showing the number of files and total storage, as well as a single progress bar for the whole thing. |
|
That's my leaning as well, because it's the simplest :). |
If wg.Add is called from the loop reading from the channel, there are scenarios (number of uploads <= number of workers) where Wait() will shut down the channels before they're read from.
|
Super cool 👍 |
To prevent being asked for credentials n times (where n = # of concurrent uploads), we process the upload queue synchronously until one of the uploads succeeds, and then allow concurrent uploads. This could be a bit faster if the condition is fired after the first successful API hit, rather than the first successful total upload, but some deeper refactoring will need to be done.
This lets the api client code broadcast success/fail events that the upload queue can listen for. The queue will process synchronously until it receives a success event, then it will run concurrently.
Conflicts: commands/command_push.go lfs/pointer.go lfs/pointer_clean.go lfs/pointer_smudge.go
commands/command_ls_files.go
Outdated
There was a problem hiding this comment.
I wonder if this should be ScanRefs or something?
|
This is now ready for 👀 The number of uploads that will happen concurrently is configured via When Progress output is now a single bar that covers the combined sizes of all uploads (see gif above). Edit to add: any upload errors that occur are collected and displayed after processing is complete. |
|
Found a small issue: So I used the new I'm using this to generate 50 Git LFS files to upload. require "time"
require "fileutils"
now = Time.now.strftime("%Y%m%d-%I%M%S")
FileUtils.mkdir_p(now)
50.times do |i|
File.open("#{now}/#{i}.dat", "w") do |f|
f.write(`openssl rand -hex 102400`)
end
end
puts `git add #{now}`
puts `git commit -m "files for #{now}"` |
An easy way to keep upload progress bars up to date when the file doesn't need to be uploaded is to just drain the callback reader into ioutil.Discard.
|
Here's how it looks now: The progress bar looks good. There's a lot of overhead, but I think we'll need to make some major API changes to solve. |

This PR will add support to allow uploads to happen concurrently.
git configcall