Jeff King <p...@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.
