> And, I may be missing something, but later in that function we do:
>
> if (repo_read_index(r) < 0)
> die(_("index file corrupt"));
>
> Do we need to read the index there if we're just invoking a "status"
> command, isn't it reading it for us & reporting back?
You're correct I don't need to read the index there
> > +static struct status_task *
> > +get_status_task_from_index(struct submodule_parallel_status *sps,
> > + struct strbuf *err)
> > +{
> > + for (; sps->index_count < sps->submodule_names->nr; sps->index_count++) {
> > + struct submodule_status_util *util = sps->submodule_names->items[sps->index_count].util;
> > + const struct cache_entry *ce = util->ce;
> > + struct status_task *task;
> > + struct strbuf buf = STRBUF_INIT;
> > + const char *git_dir;
> > +
> > + strbuf_addf(&buf, "%s/.git", ce->name);
> > + git_dir = read_gitfile(buf.buf);
>
> Okey, so the "NULL" variant of read_gitfile_gently(), as we don't care
> about the sort of error we got, but...
>
> > + if (!git_dir)
> > + git_dir = buf.buf;
> > + if (!is_git_directory(git_dir)) {
>
> Isn't this something we could have asked read_gitfile_gently() about
> instead, i.e. the READ_GITFILE_ERR_NOT_A_REPO condition?
> > + if (is_directory(git_dir))
> > + die(_("'%s' not recognized as a git repository"), git_dir);
>
> It would be a detour from this, but perhaps setup.c can be tasked with
> knowing about this edge case and returning an error code, but even if we
> punt on that we can just do the is_directory() here, but get the
> !is_git_directory() for free it seems.
So there are two non-fatal errors in read_gitfile_gently() that return
NULL rather than dieing:
READ_GITFILE_ERR_STAT_FAILED
READ_GITFILE_ERR_NOT_A_FILE
Then the edge case becomes:
Not a git file (or stat failed)
Not a git directory
Is a directory
I'm not seeing how we would get !is_git_directory() for free.
> > + task->path = ce->name;
> > + task->dirty_submodule = util->dirty_submodule;
> > + task->ignore_untracked = util->ignore_untracked;
>
> Cn do we the same readability trick here that we did with "struct
> submodule_status_util tmp = {" earlier & the memcpy()? Looks like it...
Yes, if I make the readability change, then I should still be able to
use xmalloc
> > +
> > + child_process_init(cp);
> > + prepare_submodule_repo_env_in_gitdir(&cp->env);
> > +
> > + strvec_init(&cp->args);
> > + strvec_pushl(&cp->args, "status", "--porcelain=2", NULL);
> > + if (task->ignore_untracked)
> > + strvec_push(&cp->args, "-uno");
>
> Nit: Maybe worth spelling out --untracked-files=no (or maybe "-uno" is
> more idiomatic at this point...)
Same spelling as in is_submodule_modified(). Probably not worth
changing /shrug.
> > +int get_submodules_status(struct repository *r,
> > + struct string_list *submodules,
> > + int max_parallel_jobs);
>
> s/int/size_t/ because at this point you've already die()'d in the one
> caller if it's <0 from the config parser, so we know it's unsigned
> (actually >1, but unsigned will have to do :).
The caller is passing in an int anyways so I'm going to keep it as is
for consistency.
Took the rest of your suggestions. Thanks!
