> You make it sound as if this is only for "git status", but shouldn't
> it benefit the usual "git diff" the same way when you have
> submodules that can be dirty?
It should also! I'll reword the commit message.
> > Add config option status.parallelSubmodules to set the maximum number
> > of parallel jobs.
>
> Configuration is fine, but I am having a hard time justifying the
> addition of an extra parameter to run_diff_files(). It might be
> more palatable to give a new bit (default off) in the rev structure
> that tells it that it is OK to go into the "defer_submodule_status"
> mode, if we absolutely want to change the behaviour of
> run_diff_files() only for a single caller or something, but I doubt
> it should even be needed.
>
> I cannot think of a sane user interface that says "when
> run_diff_files() is invoked as an implementation detail of 'status',
> use this value, and when it is running for another command 'foo',
> use this other value". How would a user decide to pick a different
> value for different commands?
>
> Letting a single configuration variable to decide the degree of
> parallelism inside run_diff_files() would be sufficient, and we
> shouldn't have to touch all the existing calling sites of
> run_diff_files() all over the place. If you absolutely want to do
> this, I'd rather see the new member for the configuration variable
> not in wt_status but in rev_info.
I agree the configuration variable should be in rev_info and not
be specific to status since other callers can take advantage of it.
>
> > +status.parallelSubmodules::
> > + When linkgit:git-status[1] is run in a superproject with
> > + submodules, a status subprocess is spawned for every submodule.
> > + This option specifies the number of submodule status subprocesses
> > + to run in parallel. If unset, it defaults to 1.
>
> As I said, I am not sure <cmd>.parallelSubmodules per command makes
> much sense. "If unset, it defaults to" sounds a bit redundant (if
> it is explicitly set, the default value should not matter).
ack.
> > @@ -138,6 +153,9 @@ int run_diff_files(struct rev_info *revs, unsigned int option)
> > struct cache_entry *ce = istate->cache[i];
> > int changed;
> > unsigned dirty_submodule = 0;
> > + int defer_submodule_status = revs && revs->repo &&
> > + !strcmp(revs->repo->gitdir, ".git");
>
> What is this overly deeply indented comparison with ".git" doing?
> What are we checking and why? Is that something we need to do for
> each and every active_cache[] entry, or isn't it constant over the
> loop?
It is checking to see whether we are in the superproject or not, since
it doesn't make sense to parallelize status in a submodule subprocess.
It doesn't need to be in the loop though -- will move out.
> > + int ignore_untracked_in_submodules;
> >
> > if (diff_can_quit_early(&revs->diffopt))
> > break;
> > @@ -269,11 +287,36 @@ int run_diff_files(struct rev_info *revs, unsigned int option)
> > }
> >
> > changed = match_stat_with_submodule(&revs->diffopt, ce, &st,
> > + ce_option, &dirty_submodule,
> > + &defer_submodule_status,
> > + &ignore_untracked_in_submodules);
> > newmode = ce_mode_from_stat(ce, st.st_mode);
> > + if (defer_submodule_status) {
> > + struct string_list_item *item =
> > + string_list_append(&submodules, ce->name);
> > + struct submodule_status_util *util = xmalloc(sizeof(*util));
> > + util->changed = changed;
> > + util->dirty_submodule = 0;
> > + util->ignore_untracked = ignore_untracked_in_submodules;
> > + util->newmode = newmode;
> > + util->ce = ce;
> > + item->util = util;
> > + continue;
>
> This makes me wonder if defer_submodule_status should be a string
> list that receives the punted submodules---iow, don't these details
> belong to match_stat_with_submodule() rather than its caller here?
That makes sense. I can definitely clean up this section and
match_stat_with_submodules() more.
>
> Even better may be to start a new child task for the submodule here,
> letting it work while the parent process moves on to the next entry
> in the active_cache[]. I do not know if you thought about doing it
> that way.
I think the implementation complexity of doing it this way would be
very high -- basically reimplementing run_processes_parallel(), but
within this loop. And the benefit of doing so I think is not worth it -- at
that point, I might as well parallelize the entire function.
> > + for (int i = 0; i < submodules.nr; i++) {
>
> We still do not allow "for (type var = init; ...)" if I am not
> mistaken. Check the coding guidelines.
ack.
