mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Eric Sunshine <>
To: Marketa Calabkova <>
Cc: "Git List" <>,
	"Nguyễn Thái Ngọc Duy" <>
Subject: Re: Worktree creation race
Date: Fri, 1 Feb 2019 01:27:31 -0500	[thread overview]
Message-ID: <> (raw)
In-Reply-To: <>

On Mon, Jan 28, 2019 at 7:58 AM Marketa Calabkova <> 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.

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).

  reply	other threads:[~2019-02-01  6:27 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-01-15 14:03 Worktree creation race Marketa Calabkova
2019-01-28 12:58 ` Marketa Calabkova
2019-02-01  6:27   ` Eric Sunshine [this message]
2019-02-01  7:06     ` Duy Nguyen
2019-02-01 13:16       ` Duy Nguyen
2019-02-01 19:54         ` Eric Sunshine
2019-02-01 18:06     ` Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:

  List information:

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='' \ \ \ \ \

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).