On Fri, 15 Feb 2019 10:59:33 -0800
Junio C Hamano <gitster@xxxxxxxxx> wrote:
> Michal Suchanek <msuchanek@xxxxxxx> writes:
>
> > 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.
>
> Yeah, relying on the atomicity of mkdir(2) is much saner approach
> than "check -- ah we can use the name -- try to create" that is race
> prone.
>
> Thanks for working on this.
>
> > Signed-off-by: Michal Suchanek <msuchanek@xxxxxxx>
> > ---
> > builtin/worktree.c | 11 ++++++-----
> > 1 file changed, 6 insertions(+), 5 deletions(-)
> >
> > diff --git a/builtin/worktree.c b/builtin/worktree.c
> > index 3f9907fcc994..e1a2a56c03c5 100644
> > --- a/builtin/worktree.c
> > +++ b/builtin/worktree.c
> > @@ -268,10 +268,9 @@ static int add_worktree(const char *path, const char *refname,
> > struct strbuf sb_git = STRBUF_INIT, sb_repo = STRBUF_INIT;
> > struct strbuf sb = STRBUF_INIT;
> > const char *name;
> > - struct stat st;
> > struct child_process cp = CHILD_PROCESS_INIT;
> > struct argv_array child_env = ARGV_ARRAY_INIT;
> > - int counter = 0, len, ret;
> > + int counter = 1, len, ret;
> > struct strbuf symref = STRBUF_INIT;
> > struct commit *commit = NULL;
> > int is_branch = 0;
> > @@ -295,19 +294,21 @@ 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(!counter) break; /* don't loop forever */
> > strbuf_setlen(&sb_repo, len);
> > strbuf_addf(&sb_repo, "%d", counter);
>
> Style:
>
> if (!counter)
> break; /* don't loop forever */
>
> More importantly, how long would it take to loop thru all possible
> integers (can be simulated by making the parent directory
> unwritable)? Don't we want to cut off with more conservative upper
> limit, say 1000 rounds or even 100 rounds or so?
>
> Also, is the behaviour for a signed integer wrapping around due to
> getting incremented too many times well defined? I'd feel safer,
> especially if you are willing to spin for 4 billion times like this
> patch does, if you changed the counter to "unsigned int".
If there are 4 billion worktrees ..
but there is no need to spin if the failure reason is not EEXIST.
>
> I see you changed "counter" to start from 1, but that would mean
> that these fallback names would start with suffix 2, not 1. Which
> would look funny.
>
> I would have expected ".1", ".2", etc. as suffix, but the original
> used "1", "2", etc. so I won't complain on the format, but I do find
> it questionable to start counting from 2.
Yes, there is no need to change the starting counter.
>
> > }
> > + if (!counter)
> > + die_errno(_("could not create directory of '%s'"), sb_repo.buf);
>
> It would have saved reviewer's time if this die() were inside the
> loop where you punted with "break".
Yes, that's a leftover of the existing code structure.
>
> > name = strrchr(sb_repo.buf, '/') + 1;
> >
> > junk_pid = getpid();
> > 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;
Thanks
Michal
