From: Patrick Steinhardt <ps@pks.im>
Date: Tue, 03 Nov 2026 08:01:08 GMT
Subject: Re: [PATCH 1/2] credential-cache--daemon: allow a concurrently created socket dir
Message-ID: <aumUxF1uVtVC4qYj@pks.im>
In-Reply-To: <20261101-fz-credential-cache-race-v1-1-28f0bac51e11@gmail.com>

On Sun, Nov 01, 2026 at 03:29:25PM -0800, Farid Zakaria wrote:
> diff --git a/builtin/credential-cache--daemon.c b/builtin/credential-cache--daemon.c
> index 65cc619bec..0dfa11e2ec 100644
> --- a/builtin/credential-cache--daemon.c
> +++ b/builtin/credential-cache--daemon.c
> @@ -273,8 +273,18 @@ static void init_socket_directory(const char *path)
>  		 */
>  		if (safe_create_leading_directories_const(the_repository, dir) < 0)
>  			die_errno("unable to create directories for '%s'", dir);

It's quite curious that we use `safe_create_leading_directories()` here.
As documented in "path.h", the `safe_...()` functions should only ever
be used for creating directories in the ".git" directory itself. In
particular, applying "core.sharedRepostiory" to the leading directories
of our credential daemon is never going to result in anything good at
all.

So we should call `safe_create_leadingdirectories_no_share_const()`
instead, which does not apply "core.sharedRepository". I know that's not
introduced by your patch, but would you mind maybe fixing that in a
preparatory commit?

> -		if (mkdir(dir, 0700) < 0)
> -			die_errno("unable to mkdir '%s'", dir);
> +		if (mkdir(dir, 0700) < 0) {
> +			if (errno != EEXIST)
> +				die_errno("unable to mkdir '%s'", dir);

Makes sense.

> +			/*
> +			 * Another daemon created it first; check it like one
> +			 * that already existed.
> +			 */
> +			if (stat(dir, &st))
> +				die_errno("unable to stat '%s'", dir);
> +			if (st.st_mode & 077)
> +				die(_(permissions_advice), dir);
> +		}

A bit of a shame that we duplicate this logic, but I don't see an
obvious way for how to avoid that.

Patrick