Adapted non-force fetching with locking #1
Conversation
Update like to storage GoDoc
examples: update link to GoDoc in _examples/storage
When the revlist is computing the set of hashes needed to transfer, it doesn't need to walk over commits it has already processed. So, it can instruct the commit walker not to walk those commits by passing in its own `seen` map. For a 36K object repo, this brought the time for `revlist.Objects` down from 50s to 30s.
One use of go-git is to transfer git data from a non-standard git repo (not stored in a file system, for example) to a "remote" backed by a standard, local .git repo. In this scenario, delta compression is not needed to reduce transfer time over the "network", because there is no network. The underlying storage layer has already taken care of the data tranfer, and sending the objects to local .git storage doesn't require compression. So this PR gives the user the option to turn off compression when it isn't needed. Of course, this results in a larger, uncompressed local .git repo, but the user can then run git gc or git repack on that repo if they care about the storage costs. Turning the pack window to 0 on reduces total push time of a 36K repo by 50 seconds (out of a pre-PR total of 3m26s).
…aster config: support a configurable, and turn-off-able, pack.window
…gh-master plumbing: the commit walker can skip externally-seen commits
This change is the fixed version of the previous performance improvement that was reverted due to some bogus logic. Now it's fixed and only stops the iteration if and only if all of the branches we've come across have been visited, being a branch a parent commit of a commit we've visited. Signed-off-by: Miguel Molina <[email protected]>
…-ancestors-fixed revlist: do not revisit ancestors as long as all branches are visited
|
And conflicts between src-d/master and keybase/master it seems. |
|
Can you please rebase this on top of |
There was a problem hiding this comment.
Actually I was able to review the relevant two patches separately. Looks good with some minor suggestions, thanks! But please rebase before merging into master. The go-git folks are going to request some tests as well, so it would be good to write some, but that needn't block the merge here.
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| defer ioutil.CheckClose(f, &err) | ||
|
|
||
| err = f.Lock() |
There was a problem hiding this comment.
Shouldn't there be a corresponding f.Unlock() somewhere, maybe as a defer?
There was a problem hiding this comment.
Actually Close takes care of it, but should probably document that. (If we were unlocking the file it would probably(?) need a fsync before the unlock, but go-billy lacks fsync too).
There was a problem hiding this comment.
@taruti: I don't understand how Close() takes care of it. Is that a new requirement of the billy.File.Close() method? Our current implementation doesn't do that, though I guess maybe for posfix flock and Windows, Close() handles it automatically? We will need something that explicitly calls Unlock() on our own libfs.File implementation; if libfs.File.Close() is supposed to do that, we can make that happen. Was just confused. A comment here would definitely help, and maybe one over in go-billy as well if that's a new assumption.
There was a problem hiding this comment.
But flocks are released by closing the file? From e.g. Linux Furthermore, the lock is released either by an explicit LOCK_UN operation on any of these duplicate file descriptors, or when all such file descriptors have been closed.. But yes more documentation would be good.
There was a problem hiding this comment.
Our implementation of file locking will run into trouble with other applications if we keep the locks after all file descriptors are closed.
There was a problem hiding this comment.
The implementation of our KBFS locks won't be flock-based, so we'll need some way to know to release them. Doing it as part of the Close() call is totally fine, we should just document it.
(@songgao -- we should change our libfs.File.Close() method to call Unlock()).
| @@ -142,6 +145,9 @@ type FetchOptions struct { | |||
| // Tags describe how the tags will be fetched from the remote repository, | |||
| // by default is TagFollowing. | |||
| Tags TagMode | |||
| // Force allows the fetch to update a local branch even when the remote | |||
| // branch does not descend from it. | |||
| Force bool | |||
There was a problem hiding this comment.
I know this is from someone else's PR, but I don't think this is strictly needed because the RefSpecs above already have an IsForce() method that says whether that particular ref should be forced or not. (For PullOptions, the default RefSpec is defined in the RemoteConfig.) Could you please update it to use the IsForce method instead?
There was a problem hiding this comment.
Now when actually doing this, are you sure we want this? CheckoutOptions and PullOptions have the Force bool in addition to FetchOptions. Removing it from one but not the others would feel inconsistent.
There was a problem hiding this comment.
You can keep the Force bool if you want, but in that case please also add support for the IsForce() method. The git remote protocol supports both force and non-force fetches in the same batch, so we'll need to be able to specify the force on individual refs.
|
Made a rebased clear PR (to not mess up diffs here): #2 |
|
Closing this and merging the other one. |
No description provided.