patch, 2 partsworktree: fix worktree add race.

> When adding wotktrees git can die in get_common_dir_noenv while
> examining existing worktrees because the commondir file does not exist.
> Handle ENOENT so adding a worktree does not fail because of incompletely
> set-up other worktree.
>
> Signed-off-by: Michal Suchanek <msuchanek@suse.de>
> ---
> diff --git a/setup.c b/setup.c
> @@ -274,22 +274,25 @@ int get_common_dir_noenv(struct strbuf *sb, const char *gitdir)
> +               if (strbuf_read_file(&data, path.buf, 0) <= 0) {
> +                       if (errno != ENOENT)
> +                               die_errno(_("failed to read %s"), path.buf);

> On Fri, Feb 15, 2019 at 1:16 PM Michal Suchanek <msuchanek@suse.de> wrote:
> > When adding wotktrees git can die in get_common_dir_noenv while
> > examining existing worktrees because the commondir file does not exist.
> > Handle ENOENT so adding a worktree does not fail because of incompletely
> > set-up other worktree.
> >
> > Signed-off-by: Michal Suchanek <msuchanek@suse.de>
> > ---
> > diff --git a/setup.c b/setup.c
> > @@ -274,22 +274,25 @@ int get_common_dir_noenv(struct strbuf *sb, const char *gitdir)
> > +               if (strbuf_read_file(&data, path.buf, 0) <= 0) {
> > +                       if (errno != ENOENT)
> > +                               die_errno(_("failed to read %s"), path.buf);  
> 
> Documentation for strbuf_read_file() in strbuf.h does not state that
> 'errno' has any meaningful value when this function fails, however,

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

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

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

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