On Thu, Oct 20 2022, Calvin Wan wrote:
> @@ -103,6 +117,7 @@ int run_diff_files(struct rev_info *revs, unsigned int option)
> ? CE_MATCH_RACY_IS_DIRTY : 0);
> uint64_t start = getnanotime();
> struct index_state *istate = revs->diffopt.repo->index;
> + struct string_list submodules = STRING_LIST_INIT_NODUP;
>
> diff_set_mnemonic_prefix(&revs->diffopt, "i/", "w/");
>
> @@ -227,6 +242,8 @@ int run_diff_files(struct rev_info *revs, unsigned int option)
> newmode = ce->ce_mode;
> } else {
> struct stat st;
> + unsigned ignore_untracked = 0;
> + int defer_submodule_status = !!revs->repo;
>
> changed = check_removed(istate, ce, &st);
> if (changed) {
> @@ -248,8 +265,24 @@ int run_diff_files(struct rev_info *revs, unsigned int option)
> }
>
> changed = match_stat_with_submodule(&revs->diffopt, ce, &st,
> - ce_option, &dirty_submodule);
> + ce_option, &dirty_submodule,
> + &defer_submodule_status,
> + &ignore_untracked);
> newmode = ce_mode_from_stat(ce, st.st_mode);
> + if (defer_submodule_status) {
> + struct submodule_status_util tmp = {
> + .changed = changed,
> + .dirty_submodule = 0,
> + .ignore_untracked = ignore_untracked,
> + .newmode = newmode,
> + .ce = ce
Nit: We usually add the trailing "," to lists, except as a marker of
"don't add anything after this", i.e. only omit it when we have NULL as
the last element, but it's good to have it here.
> + };
> + struct string_list_item *item;
Nit: Another \n here to seperate the variables from the code, maybe...
> + item = string_list_append(&submodules, ce->name);
> + item->util = xmalloc(sizeof(tmp));
> + memcpy(item->util, &tmp, sizeof(tmp));
This whole thing is much more readable, thanks.
> + continue;
> + }
> }
>
> if (!changed && !dirty_submodule) {
> @@ -268,6 +301,40 @@ int run_diff_files(struct rev_info *revs, unsigned int option)
> ce->name, 0, dirty_submodule);
>
> }
> + if (submodules.nr > 0) {
> + int parallel_jobs;
> + if (git_config_get_int("submodule.diffjobs", ¶llel_jobs))
> + parallel_jobs = 1;
> + else if (!parallel_jobs)
> + parallel_jobs = online_cpus();
> + else if (parallel_jobs < 0)
> + die(_("submodule.diffjobs cannot be negative"));
> +
> + if (get_submodules_status(revs->repo, &submodules, parallel_jobs))
> + BUG("submodule status failed");
A BUG() is probably too strong if we're invoking a subprocess,
i.e. maybe the OS got tired of us and killed it, which is just an
exception, not a bug in git.
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?
> +struct submodule_parallel_status {
> + size_t index_count;
> + int result;
> +
> + struct string_list *submodule_names;
> + struct repository *r;
> +
> + /* Pending statuses by OIDs */
> + struct status_task **oid_status_tasks;
> + int oid_status_tasks_nr, oid_status_tasks_alloc;
> +};
> +
> +#define SPS_INIT { 0 }
Nit: It's good to add *_INIT macros sometimes, but here we just have a
private API that's only used once, which is...
> [...]
> +int get_submodules_status(struct repository *r,
> + struct string_list *submodules,
> + int max_parallel_jobs)
> +{
> + struct submodule_parallel_status sps = SPS_INIT;
...here, to copy some context around, and...
> + const struct run_process_parallel_opts opts = {
> + .tr2_category = "submodule",
> + .tr2_label = "parallel/status",
> +
> + .processes = max_parallel_jobs,
> + .hide_output = 1,
> +
> + .get_next_task = get_next_submodule_status,
> + .start_failure = status_start_failure,
> + .pipe_output = status_pipe_output,
> + .task_finished = status_finish,
> + .data = &sps,
> + };
> +
> + sps.r = r;
> +
> + if (repo_read_index(r) < 0)
> + die(_("index file corrupt"));
> +
> + sps.submodule_names = submodules;
...the net result is needing to first assign SPS_INIT, then add the "r"
member, then "submodule_names". In this case this looks easier:
struct submodule_parallel_status sps = {
.r = r,
.submodule_names = submodules,
};
const struct run_process_parallel_opts opts = {
[...]
> + string_list_sort(sps.submodule_names);
> + run_processes_parallel(&opts);
> +
> struct submodule_parallel_fetch {
> /*
> * The index of the last index entry processed by
> @@ -1445,6 +1459,13 @@ struct fetch_task {
> struct oid_array *commits; /* Ensure these commits are fetched */
> };
>
> +struct status_task {
> + struct repository *repo;
> + const char *path;
> + unsigned dirty_submodule;
> + int ignore_untracked;
> +};
> +
> /**
> * When a submodule is not defined in .gitmodules, we cannot access it
> * via the regular submodule-config. Create a fake submodule, which we can
> @@ -1956,6 +1977,142 @@ unsigned is_submodule_modified(const char *path, int ignore_untracked)
> return dirty_submodule;
> }
>
> +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.
> + strbuf_release(&buf);
> + /* The submodule is not checked out, so it is not modified */
> + util->dirty_submodule = 0;
> + continue;
> + }
> + strbuf_release(&buf);
> +
> + task = xmalloc(sizeof(*task));
> + memset(task, 0, sizeof(*task));
s/xmalloc/xcalloc/?
> + 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...
> +static int get_next_submodule_status(struct child_process *cp,
> + struct strbuf *err,
> + void *data, void **task_cb)
Nit: Too early wrapping, "void *data" should be at the previous line
(which won't exceed 79 chars)?
> +{
> + struct submodule_parallel_status *sps = data;
> + struct status_task *task = get_status_task_from_index(sps, err);
> +
> + if (!task) {
> + return 0;
> + }
Nit: Don't need {}-braces here.
> +
> + 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...)
> +
> + prepare_submodule_repo_env(&cp->env);
> + cp->git_cmd = 1;
> + cp->no_stdin = 1;
> + cp->dir = task->path;
> + *task_cb = task;
> + return 1;
> +}
> +
> +static int status_start_failure(struct strbuf *err,
> + void *cb, void *task_cb)
> +{
> + struct submodule_parallel_status *sps = cb;
> +
> + sps->result = 1;
> + return 0;
> +}
> +
> +static void status_pipe_output(struct strbuf *process_out,
> + void *cb, void *task_cb)
> +{
> + struct status_task *task = task_cb;
> + struct string_list list = STRING_LIST_INIT_DUP;
> +
> + string_list_split(&list, process_out->buf, '\n', -1);
> +
> + for (size_t i = 0; i < list.nr; i++) {
> + if (parse_status_porcelain(list.items[i].string,
> + strlen(list.items[i].string),
Nit: I think you can use for_each_string_list_item() here, saving some
verbosity..
> +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 :).
