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

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

Jeff King <peff@peff.net> writes:

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