Re: [PATCH v3 6/6] diff-lib: parallelize run_diff_files for submodules

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

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

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

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

> > +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 :).