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

[Stefan Beller <sbeller@google.com>]

On Mon, Oct 31, 2016 at 3:38 PM, Brandon Williams <bmwill@google.com> wrote:
> +int is_submodule_checked_out(const char *path)
> +{
> +       int ret = 0;
> +       struct strbuf buf = STRBUF_INIT;
> +
> +       strbuf_addf(&buf, "%s/.git", path);
> +       ret = file_exists(buf.buf);

I think we can be more tight here; instead of checking
if the file or directory exists, we should be checking if
it is a valid git directory, i.e. s/file_exists/resolve_gitdir/
which returns a path to the actual git dir (in case of a .gitlink)
or NULL when nothing is found that looks like a git directory or
pointer to it.


> +
> +       strbuf_release(&buf);
> +       return ret;
> +}
> +
>  int parse_submodule_update_strategy(const char *value,
>                 struct submodule_update_strategy *dst)
>  {
> diff --git a/submodule.h b/submodule.h
> index d9e197a..bd039ca 100644
> --- a/submodule.h
> +++ b/submodule.h
> @@ -37,6 +37,8 @@ void set_diffopt_flags_from_submodule_config(struct diff_options *diffopt,
>                 const char *path);
>  int submodule_config(const char *var, const char *value, void *cb);
>  void gitmodules_config(void);
> +extern int is_submodule_initialized(const char *path);
> +extern int is_submodule_checked_out(const char *path);

no need to put extern for function names. (no other functions in this
header are extern. so local consistency maybe? I'd also claim that
all other extern functions in headers ought to be declared without
being extern)

Also naming: I'd go with

    is_submodule_populated ;)

as it will tell whether this function will tell you if there is a valid
submodule (and not just an empty dir as a place holder).

You don't have to run "git checkout" to arrive in that state,
but a plumbing command such as read_tree may have been used.