I think it makes more sense if this check is moved higher, so it can skip initializing the lock client if readonly files are disabled.

Any thoughts on adding a git config value? That would let users configure this per repo, if they wanted.

Oh yes indeed, good point, having granularity at the repo level on whether this is enabled or not would be very welcome.

Good shout, adding that now and a wrapper Configuration method to combine the two.

Would returning an unbound channel here let this start parsing the .gitattributes contents while tools.FastWalkGitRepo is doing its thing?

Unfortunately this is not possible; see the end of the function where the order of the files is reversed to ensure the correct precedence of attributes is maintained.

This brings back the config import to the locking package. I'd prefer locking.Client get LocalWorkingDir and LocalGitDir properties, set in locking.NewClient(*config.Configuration).

The API refactor will move that to this commands helper.

OK; even after doing this it will still require the config package, until the per-package config comes in, but at least it puts it in one place ready for that I guess.

I'm with @technoweenie, let's keep this import light.

I'd like this to be a property on locking.Client too, set with the default value in locking.NewClient().

This function looks good, but I'm thinking that this line would lock a file named "lockable" (or one that contains the substring "lockable") as lockable on disk, even if the user didn't want it to be. This is sort of edgecase-y, but I think it's still behavior worth checking. Maybe a more proper .giattributes parser would be worth investigating?

That's a good point, if the file entry in .gitattributes was specific enough that could be a problem - it's not likely but could be a nasty edge case in that 0.0001% and experience says that's worth addressing.

Turns out there's a cheap way to do it, we're already splitting on whitespace so just need to check fields[1:] instead. .gitattributes always splits on plain whitespace (you have to use [[:space:]] in file patterns) so this works.

Nit: I'd prefer a smaller if statement here:

if len(errs) == 0 {
      return nil
}

// ...

It'd be nice if we didn't need the if statement at all, but I think this is at least a small step forward 👍

👍 I submit PRs to projects with opposing code styles for this case and sometimes I forget 😄

This read isn't guarded by a mutex-lock. I think using a sync.RWMutex would be appropriate here.

A read-only mutex here wouldn't do anything useful unless the caller also locked it.

ensureLockablesLoaded ensures that c.lockablePatterns is only updated by one thread, and refreshLockablePatterns deliberately replaces the entire map (rather than clearing and re-populating it) so that previous callers to GetLockablePatterns can continue to use the old map. Therefore a read-only mutex is not required provided the return from GetLockablePatterns is not modified. Unfortunately Go doesn't have a const marker for returns, I thought about copying the map each time but ultimately thought it wasn't worth it.

I'm with @technoweenie, let's keep this import light.

This can be initialized after calling git.GetAttributePaths to avoid resizing:

paths := git.GetAttributePaths(...)
c.lockablePatterns = make([]string, 0, len(paths))

for _, p := range paths {
        if p.Lockable {
                c.lockablePatterns = append(c.lockablePatterns, p.Path)
        }
}
// ...

I dislike the pattern of returning a single error via a method call in the return signature, because I think this hides too much. I prefer:

if err := tools.SetFileWriteFlag(path, true); err != nil {
        return Lock{}, err
}

return lock, nil

I'm sure I've had the opposite feedback before 😆 Fair enough.

I'd prefer to use a text/template instead of string replacement, since the later is a common Go convention and is well understood by looking at the hookBaseContent variable alone.

That would be a lot of cruft for what will boil down to a global find/replace even with a template. If you like I can make the string look like a template and still use strings.Replace but I see no reason to clutter this with all the init/struct param/execute patterns of templates.

This command needs a manpage entry.

Very nice 👍

This command needs a manpage entry.

@ttaylorr If the lockable feature is on, then Git LFS cannot skip the post-checkout command. Consequently, Git LFS needs to search for all .gitattributes files to check them for lockable patterns.

My team is working with a large repo (>20k directories, >300k files) and just searching for the .gitattributes file in all directories takes ~6 sec on macOS with a fast SSD. Consider these timings:

$ export GIT_LFS_SET_LOCKABLE_READONLY=1; time git-lfs post-checkout a0c5174a19f2dece44c09bc76f91feb69799e66f a0c5174a19f2dece44c09bc76f91feb69799e66f 0
git-lfs post-checkout a0c5174a19f2dece44c09bc76f91feb69799e66f  0  

35.09s user 6.41s system 730% cpu 5.679 total


$ export GIT_LFS_SET_LOCKABLE_READONLY=0; time git-lfs post-checkout a0c5174a19f2dece44c09bc76f91feb69799e66f a0c5174a19f2dece44c09bc76f91feb69799e66f 0
git-lfs post-checkout a0c5174a19f2dece44c09bc76f91feb69799e66f  0  

0.01s user 0.02s system 89% cpu 0.030 total

I plan to distribute the setting lfs.setlockablereadonly=false in my company gitconfig but it took me a while to figure out the culprit of the slowness. I don't know how widely the lockable feature is used, but I wonder if lfs.setlockablereadonly=falseis a more sensible default? Alternatively we could find ways to speed up the .gitattributes discovery.

I don't know how widely the lockable feature is used, but I wonder if lfs.setlockablereadonly=false is a more sensible default?

I think =true is an important default value to keep. Here are a few thoughts on how we can get the best of both worlds:

1. Speed up the time it takes to search for .gitattributes. This has been optimized by @sinbad in Fast directory walk #1616. I think that this work could be augmented by first stat()-ing directories to search, and then seeing if the stat is out-of-date with the last time that the directory was traversed, effectively memoizing the search. I presume that this would improve the speed a great deal.

2. Warn when the search is taking too long. We can set a timer (similar to what Microsoft does with GVFS when the --ahead-behind check takes too long) and warn if Git LFS is unable to determine the set of lockable files quickly.

What do you think? Perhaps we should have this discussion in a new issue, as well.