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

[Stefan Beller <sbeller@google.com>]

On Wed, Nov 30, 2016 at 8:29 PM, Jeff King <peff@peff.net> wrote:
> 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 say "I don't know if it's worth it" because what you've written is
> fine, and while more lines, it's fairly obvious and safe.

OK, chiming in here as well. :)

I plan on making use of the is_submodule_populated method in
the checkout --recurse-submodules series, and for that I am
undecided whether a cheap stat is the right approach or if we want
to have the result of resolve_gitdir as that fails in weird corner cases.

Anyway, I'd propose to change the name when going with either the
code as is or what Jeff proposes to be one of

    is_submodule_populated_cheaply
    is_submodule_populated_with_no_sanity_check
    is_submodule_dot_git_present
    have_submodule_dot_git

I think I'd prefer the last one as that describes what the function
actually does in a concise way?

Thanks,
Stefan