Re: [PATCH v3 1/2] worktree: fix worktree add race.

[Eric Sunshine <sunshine@sunshineco.com>]

On Wed, Feb 20, 2019 at 11:17 AM Michal Suchanek <msuchanek@suse.de> wrote:
> Git runs a stat loop to find a worktree name that's available and then does
> mkdir on the found name. Turn it to mkdir loop to avoid another invocation of
> worktree add finding the same free name and creating the directory first.
>
> Signed-off-by: Michal Suchanek <msuchanek@suse.de>
> ---
> diff --git a/builtin/worktree.c b/builtin/worktree.c
> @@ -295,8 +295,12 @@ static int add_worktree(const char *path, const char *refname,
>         if (safe_create_leading_directories_const(sb_repo.buf))
>                 die_errno(_("could not create leading directories of '%s'"),
>                           sb_repo.buf);
> -       while (!stat(sb_repo.buf, &st)) {
> +       while (mkdir(sb_repo.buf, 0777)) {
>                 counter++;
> +               if ((errno != EEXIST) || !counter /* overflow */)
> +                       die_errno(_("could not create directory of '%s'"),
> +                                 sb_repo.buf);
>                 strbuf_setlen(&sb_repo, len);
>                 strbuf_addf(&sb_repo, "%d", counter);
>         }
> @@ -306,8 +310,6 @@ static int add_worktree(const char *path, const char *refname,
>         atexit(remove_junk);
>         sigchain_push_common(remove_junk_on_signal);
> -       if (mkdir(sb_repo.buf, 0777))
> -               die_errno(_("could not create directory of '%s'"), sb_repo.buf);
>         junk_git_dir = xstrdup(sb_repo.buf);
>         is_junk = 1;

Did you audit this "junk" handling to verify that stuff which ought to
be cleaned up still is cleaned up now that the mkdir() and die() have
been moved above the atexit(remove_junk) invocation?

I did just audit it, and I _think_ that it still works as expected,
but it would be good to hear that someone else has come to the same
conclusion.