Thanks for including me. Apparently I did miss some emails :) On Fri, Feb 1, 2019 at 1:27 PM Eric Sunshine <sunshine@xxxxxxxxxxxxxx> wrote: > > On Mon, Jan 28, 2019 at 7:58 AM Marketa Calabkova <mcalabkova@xxxxxxx> wrote: > > On 15/01/2019 15:03, Marketa Calabkova wrote: > > > I am writing to report a bug. The original report is from my colleague, I am also providing his suggestions. > > > > > > There is insufficient locking for worktree addition. Adding worktree may fail. > > > > > > The problem is that git reads the directory entries in $GIT_DIR/worktrees, > > > finds a worktree name that does not exist, tries to create it, and if an > > > error is returned adding the worktree fails. When multiple git processes > > > do this in parallel only one adds a worktree and the others fail. Git should > > > reread the directory and find a new name that does not exist when creating > > > the worktree directory fails because another git process already created it. > > > > > > I suppose adding PID in the tree name would mitigate the issue to the point it will be very unlikely to encounter. > > > > > > I need more than the tree in the temporary directory so using the temporary directory directly as a tree is out of question. > > > > > > cd gitrepo > > > git commit --allow-empty -m Empty > > > for n in $(seq 10000) ; do ( tmp=$(mktemp -d /dev/shm/gittest/test.XXXXXXXXXXX) ; mkdir $tmp/test ; git worktree add --detach $tmp/test ; ) & done > > > > > > (you should see many messages like: > > > fatal: could not create directory of '.git/worktrees/test284': File exists) > > > > > Does anyone has a suggestion what to do with this bug? It looks like a > > one-line fix probably in builtin/worktree.c, but I have no idea how to > > do it. Sorry. > > I doubt this is a one-line fix, and I don't think it has anything to > do with reading entries in $GIT_DIR/worktrees. I never thought people would create worktrees "like crazy" to end up worrying about races like this. The mkdir loop would be one way to go. But I'm going to add a new option to let the user control this directory name. This is necessary since this name is now exposed via "worktrees/<name>" reference space and should also be reported in "git worktree list". Avoiding the race is a nice bonus. > add_worktree() already attempts to give a unique identifier to each > worktree by adding a numeric suffix and incrementing that suffix if > the name already exists (such as the 284 in your example error > message) but there is definitely a race-condition between the time it > stat()s the name and the time it mkdir()s it. > > One possible fix would be to unconditionally use the PID, as you > suggest, though, this is not necessarily foolproof against races > either (though it makes collisions very unlikely). > > Another possibility would be to skip the stat() and instead do the > mkdir() in a loop, incrementing the sequence number each time through > the loop. That should eliminate the race entirely (I think). -- Duy
