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

[Junio C Hamano <gitster@pobox.com>]

Michal Suchanek <msuchanek@suse.de> 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@suse.de>
> ---
>  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".

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.

>  	}
> +	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".

>  	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;