locking: teach pre-push hook to check for locks#1815
Conversation
0633c83 to
44a7bd8
Compare
sinbad
left a comment
There was a problem hiding this comment.
My main comment here is that although I agree with these changes, catching the conflict at pre-push time should be the last resort, because at that point someone may well have to lose some work. We should aim to catch conflicts like this much earlier; that's part of the read-only work which I have in progress (rewritten from previous PR). But as a safety check it's fine.
I wondered whether we should try to allow --force to override this, but I don't think we have sight of that option in the hook (without platform-specific hacks like calling ps). The force unlock command is the safety valve there.
commands/command_pre_push.go
Outdated
| } | ||
|
|
||
| if len(myLocks) > 0 { | ||
| Print("Pushing your locked files:") |
There was a problem hiding this comment.
I think it would be good to explain more what this means in practice - i.e. that the user should consider unlocking the files if they have no further changes to publish after this push.
commands/command_pre_push.go
Outdated
| } | ||
|
|
||
| if len(lockConflicts) > 0 { | ||
| Error("Some files are locked in %s...%s", left, cfg.CurrentRemote) |
There was a problem hiding this comment.
Maybe we could be explicit like "You cannot push changes because someone else has these files locked"
commands/commands.go
Outdated
| func findLocks(lc *locking.Client, filter map[string]string, limit int, localOnly bool) (map[string]locking.Lock, error) { | ||
| locks, err := lc.SearchLocks(filter, limit, localOnly) | ||
| if err != nil { | ||
| return nil, errors.Wrap(err, "error finding locks") |
There was a problem hiding this comment.
I think just "finding locks" is a good error wrap message.
There was a problem hiding this comment.
My bad! Went ahead and cleaned this up in c0047f6.
commands/commands.go
Outdated
| // returned immediately, otherise a map of lock.Path -> lock will be returned | ||
| // instead. | ||
| func findLocks(lc *locking.Client, filter map[string]string, limit int, localOnly bool) (map[string]locking.Lock, error) { | ||
| locks, err := lc.SearchLocks(filter, limit, localOnly) |
There was a problem hiding this comment.
Does the client do anything special for 404/410 errors? Merging this PR would halt all pushes for LFS servers without the locking endpoints.
There was a problem hiding this comment.
Thanks for brining this up 😄 . I went ahead and changed the implementation of the Search() method on the *locking.Client to only try and deserialize the response if it was a 200 OK. If the response was a 404 or 410, the lack of deserialization would return an empty slice of locks, making the lock-related parts of the pre-push command effectively a no-op.
I don't think any changes are needed for the Lock() and Unlock() methods, since those are mutative operations and should error out if the server doesn't implement them.
Changes are in: 6fb728a.
d68f60d to
6fb728a
Compare
6fb728a to
b92cac5
Compare
commands/uploader.go
Outdated
| var canUpload bool = true | ||
|
|
||
| lockQuery := map[string]string{"path": p.Name} | ||
| locks, err := c.lockClient.SearchLocks(lockQuery, 1, false) |
There was a problem hiding this comment.
Making an API call per file seems like a lot of overhead. I think our early attempts queried all the locks up front, and checked each file locally. That does mean that we'll have to maintain a potentially huge list of locks in memory locally, so we may want to look at an improved interface for verifying groups of files in a single POST request.
There was a problem hiding this comment.
Really good call; I implemented this change in 25bd4a1.
|
Some things I'd like to follow up on, post-merge:
|
This pull-request teaches the
pre-pushhook to warn when you are pushing files that you own the lock on, and to error (preventing the push) when you are pushing files that you don't own the lock of.Here's a breakdown of what happened:
pre-pushhook to search for locks in rangefindLockshelper to make searching for locks more concise--no-verifyflag to avoid calling thepre-commitflag introduced in locking: add pre-commit hook to detect inappropriately modified files #1816.Some things that I'd like to take a look at before merging:
pre-pushhook be configurable where to search for locks? It seems that callers should be able to decide between searching the local lock cache, or searching the remote's locks. A flag seems like the wrong approach, since you'd have to update.git/hooks/pre-pusheach time, but a config option seems reasonable.pre-pushhook warn about your own locks? Should this be configurable?commandspackage own a singleton, private, instance of a*locking.Client?/cc @git-lfs/core