I think returning a *Lock would be appropriate here, since this method doesn't guarantee that locking a file will succeed all of the time, and the zero-value Lock{} doesn't make sense.

Actually I remember this was deliberately by value now. Returning a non-pointer is important because I don't want to allow the client to modify the original lock, because it's cached and therefore they could corrupt the cache by doing that. This outweighs the zero value Lock concern IMO. Yes I could return a pointer and copy it but it's unnecessary complexity. A zero value in the event of an error is idiomatic anyway (nil is just a zero value pointer after all).

I wouldn't mind implementing a (l *Lock) Copy() *Lock function, but I don't feel strongly.

That depends on callers doing the right thing (not modifying return values from the locking client. I think cache corruption is a good enough reason to return a value.

s/tells you/is the/

😻

I follow this logic, but I think we can make it clearer:

for _, l := range cachedLocks {
        matchesPath := path == l.Path
        matchesId := id == l.Id

        if (filterByPath && matchesPath) || (filterById && matchesId) {
                // ...
        }
}

This does add a level of indentation to the body of the loop, but I think the extra clarity is worth it.

I hate to be the optimisation nitpicker here but doing it my way avoids comparison operations in the case of !filterByPath or !filterById whereas your way requires the comparison all the time, so I prefer my way 😄 I think it's clear enough.

I agree that the original is clear enough.

👍

Not sure if (c *Client) is the right receiver for this method. What do you think about wrapping tools/kv in this package with functions that understand *Lock and etc, and putting this method there?

package locking

const (
       idPrefix string = "*id*://"
)

type LockCache struct {
        kv *kv.Store
}

func (c *LockCache) CacheLock(l Lock) error {
        // ...
}

func (c *LockCache) encodeId(id string) string {
       if !c.isId(id) {
               return idPrefix+id
        }

       return id
}

func (c *LockCache) isId(key string) bool {
       return strings.HasPrefix(key, idPrefix)
}

I like this approach a lot. I think it's a nice, clear line of abstraction that we can draw. The *locking.Client would own a *LockCache underneath and delegate into the cache's public methods from there.

Yeah that's a good shout.

Here's another place where I think the approach I proposed above would yield a win. This test is testing the behavior of caching a lock, but creates a locking client to do so. Moving that behavior down into a more isolated part of the system would mean that this test can be closer to it's subject.

Yep, it made it better :)

Not that this is likely behavior, but I think that using the two-tuple variant of this cast would be safer in that it'd prevent an unlikely panic:

func (c *Client) cacheUnlockByPath(filePath string) error {
        ilock := c.cache.Get(filePath)
        if ilock == nil { // <- needed to prevent `(*Lock)(nil)`
                return nil
        }
        if l, ok := ilock.(*Lock); ok {
                // ...
        }

        return nil
}

Cool, I've just collapsed this into:

if lock, ok := ilock.(*Lock); ok && lock != nil {

Since the actual casting of nil is safe anyway so long as we check.

Do you think we should return an error if we couldn't find the lock? I think a sentinel value here would be fine:

package locking

var ErrMissingLock error = errors.New("locking: could not find lock")

I always think it's better to fail silently when requesting to remove a non-existent item from any collection; this is idiomatic with core collections and what I would expect from an API.

This API is awesome 👍

I think having functions with a Cache prefix is redundant. Especially in the *Client usage: c.cache.CacheUnlockById(id). What do you think about method names like this:

• CacheLock -> Set
• CacheUnlockByPath -> DeleteByPath
• CacheUnlockById -> DeleteById
• CachedLocks -> GetAll or just All

Fair point; to keep it distinct from kv I'll make it Add / RemoveByBlah / Locks.

I agree that the original is clear enough.

I think something like refreshLocksCache() would be a better name.