Re: [PATCH v6 1/6] submodules: add helper functions to determine presence of submodules

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 1, 2016, 18:46 UTC
Message-ID: <xmqqwpfja3nk.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20161201042926.mr2qdta7hviizcya@sigill.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 33 quoted lines

> On Wed, Nov 30, 2016 at 05:28:29PM -0800, Brandon Williams wrote:
>
>> +/*
>> + * Determine if a submodule has been populated at a given 'path'
>> + */
>> +int is_submodule_populated(const char *path)
>> +{
>> +	int ret = 0;
>> +	struct stat st;
>> +	char *gitdir = xstrfmt("%s/.git", path);
>> +
>> +	if (!stat(gitdir, &st))
>> +		ret = 1;
>> +
>> +	free(gitdir);
>> +	return ret;
>> +}
>
> I don't know if it's worth changing or not, but this could be a bit
> shorter:
>
>   int is_submodule_populated(const char *path)
>   {
> 	return !access(mkpath("%s/.git", path), F_OK);
>   }
>
> There is a file_exists() helper, but it uses lstat(), which I think you
> don't want (because you'd prefer to bail on a broken .git symlink). But
> access(F_OK) does what you want, I think.
>
> mkpath() is generally an unsafe function because it uses a static
> buffer, but it's handy and safe for handing values to syscalls like
> this.

I think your "unsafe" is not about thread-safety but about "the caller cannot rely on returned value staying valid for long haul". If this change since v5 is about thread-safety, I am not sure if it is safe to use mkpath here.

I am a bit wary of making the check too sketchy like this, but this is not about determining if a random "path" that has ".git" in a superproject working tree is a submodule or not (that information primarily comes from the superproject index), so I tend to agree with the patch that it is sufficient to check presence of ".git" alone.