[PATCH v8 0/6] submodule: parallelize diff
- From
Calvin Wan <calvinwan@google.com>
- Date
- Feb 9, 2023, 00:02 UTC
- Message-ID
- <20230209000212.1892457-1-calvinwan@google.com>
- In-Reply-To
- <20230207181706.363453-1-calvinwan@google.com>
Original cover letter for context: https://lore.kernel.org/git/20221011232604.839941-1-calvinwan@google.com/
This reroll contains stylistic changes suggested by Avar and Phillip, and includes a range-diff below.
Calvin Wan (6): run-command: add duplicate_output_fn to run_processes_parallel_opts submodule: strbuf variable rename submodule: move status parsing into function submodule: refactor is_submodule_modified() diff-lib: refactor out diff_change logic diff-lib: parallelize run_diff_files for submodules
Documentation/config/submodule.txt | 12 ++ diff-lib.c | 133 +++++++++++---- run-command.c | 16 +- run-command.h | 25 +++ submodule.c | 266 ++++++++++++++++++++++++----- submodule.h | 9 + t/helper/test-run-command.c | 20 +++ t/t0061-run-command.sh | 39 +++++ t/t4027-diff-submodule.sh | 31 ++++ t/t7506-status-submodule.sh | 25 +++ 10 files changed, 497 insertions(+), 79 deletions(-)
Range-diff against v7:
1: 311b1abfbe ! 1: 5d51250c67 run-command: add duplicate_output_fn to run_processes_parallel_opts
@@ run-command.c: static void pp_init(struct parallel_processes *pp,
if (!opts->get_next_task)
BUG("you need to specify a get_next_task function");
-+ if (opts->duplicate_output && opts->ungroup)
-+ BUG("duplicate_output and ungroup are incompatible with each other");
++ if (opts->ungroup) {
++ if (opts->duplicate_output)
++ BUG("duplicate_output and ungroup are incompatible with each other");
++ }
+
CALLOC_ARRAY(pp->children, n);
if (!opts->ungroup)
@@ run-command.c: static void pp_buffer_stderr(struct parallel_processes *pp,
+ } else if (n < 0) {
if (errno != EAGAIN)
die_errno("read");
-+ } else {
-+ if (opts->duplicate_output)
-+ opts->duplicate_output(&pp->children[i].err,
-+ strlen(pp->children[i].err.buf) - n,
-+ opts->data,
-+ pp->children[i].data);
++ } else if (opts->duplicate_output) {
++ opts->duplicate_output(&pp->children[i].err,
++ pp->children[i].err.len - n,
++ opts->data, pp->children[i].data);
+ }
}
}
@@ run-command.h: typedef int (*start_failure_fn)(struct strbuf *out,
+ *
+ * This function is incompatible with "ungroup"
+ */
-+typedef void (*duplicate_output_fn)(struct strbuf *out,
-+ size_t offset,
-+ void *pp_cb,
-+ void *pp_task_cb);
++typedef void (*duplicate_output_fn)(struct strbuf *out, size_t offset,
++ void *pp_cb, void *pp_task_cb);
+
/**
* This callback is called on every child process that finished processing.
@@ run-command.h: struct run_process_parallel_opts
start_failure_fn start_failure;
+ /**
-+ * duplicate_output: See duplicate_output_fn() above. This should be
-+ * NULL unless process specific output is needed
++ * duplicate_output: See duplicate_output_fn() above. Unless you need
++ * to capture output from child processes, leave this as NULL.
+ */
+ duplicate_output_fn duplicate_output;
+
@@ t/helper/test-run-command.c: static int no_job(struct child_process *cp,
+ void *pp_task_cb UNUSED)
+{
+ struct string_list list = STRING_LIST_INIT_DUP;
++ struct string_list_item *item;
+
+ string_list_split(&list, out->buf + offset, '\n', -1);
-+ for (size_t i = 0; i < list.nr; i++) {
-+ if (strlen(list.items[i].string) > 0)
-+ fprintf(stderr, "duplicate_output: %s\n", list.items[i].string);
-+ }
++ for_each_string_list_item(item, &list)
++ fprintf(stderr, "duplicate_output: %s\n", item->string);
+ string_list_clear(&list, 0);
+}
+
@@ t/t0061-run-command.sh: test_expect_success 'run_command runs in parallel with m
+ test_must_be_empty out &&
+ test 4 = $(grep -c "duplicate_output: Hello" err) &&
+ test 4 = $(grep -c "duplicate_output: World" err) &&
-+ sed "/duplicate_output/d" err > err1 &&
++ sed "/duplicate_output/d" err >err1 &&
+ test_cmp expect err1
+'
+
@@ t/t0061-run-command.sh: test_expect_success 'run_command runs in parallel with a
+ test_must_be_empty out &&
+ test 4 = $(grep -c "duplicate_output: Hello" err) &&
+ test 4 = $(grep -c "duplicate_output: World" err) &&
-+ sed "/duplicate_output/d" err > err1 &&
++ sed "/duplicate_output/d" err >err1 &&
+ test_cmp expect err1
+'
+
@@ t/t0061-run-command.sh: test_expect_success 'run_command runs in parallel with m
+ test_must_be_empty out &&
+ test 4 = $(grep -c "duplicate_output: Hello" err) &&
+ test 4 = $(grep -c "duplicate_output: World" err) &&
-+ sed "/duplicate_output/d" err > err1 &&
++ sed "/duplicate_output/d" err >err1 &&
+ test_cmp expect err1
+'
+
2: d00a18dd84 = 2: 6ded5b6788 submodule: strbuf variable rename
3: dcda518922 = 3: 0c71cea8cd submodule: move status parsing into function
4: c6fc5ba13b ! 4: 5c8cc93f9f submodule: refactor is_submodule_modified()
@@ submodule.c: static int config_update_recurse_submodules = RECURSE_SUBMODULES_OF
static int initialized_fetch_ref_tips;
static struct oid_array ref_tips_before_fetch;
static struct oid_array ref_tips_after_fetch;
-+static const char *status_porcelain_start_error =
-+ N_("could not run 'git status --porcelain=2' in submodule %s");
-+static const char *status_porcelain_fail_error =
-+ N_("'git status --porcelain=2' failed in submodule %s");
++#define STATUS_PORCELAIN_START_ERROR \
++ N_("could not run 'git status --porcelain=2' in submodule %s")
++#define STATUS_PORCELAIN_FAIL_ERROR \
++ N_("'git status --porcelain=2' failed in submodule %s")
/*
* Check if the .gitmodules file is unmerged. Parsing of the .gitmodules file
@@ submodule.c: unsigned is_submodule_modified(const char *path, int ignore_untrack
+ prepare_status_porcelain(&cp, path, ignore_untracked);
if (start_command(&cp))
- die(_("Could not run 'git status --porcelain=2' in submodule %s"), path);
-+ die(_(status_porcelain_start_error), path);
++ die(_(STATUS_PORCELAIN_START_ERROR), path);
fp = xfdopen(cp.out, "r");
while (strbuf_getwholeline(&buf, fp, '\n') != EOF) {
@@ submodule.c: unsigned is_submodule_modified(const char *path, int ignore_untrack
if (finish_command(&cp) && !ignore_cp_exit_code)
- die(_("'git status --porcelain=2' failed in submodule %s"), path);
-+ die(_(status_porcelain_fail_error), path);
++ die(_(STATUS_PORCELAIN_FAIL_ERROR), path);
strbuf_release(&buf);
return dirty_submodule;
5: 1ea8eae9c9 = 5: 6c2b62abc8 diff-lib: refactor out diff_change logic
6: 0d35fcc38d < -: ---------- diff-lib: refactor match_stat_with_submodule
7: fd1eec974d ! 6: bb25dadbe5 diff-lib: parallelize run_diff_files for submodules
@@ diff-lib.c: static int check_removed(const struct index_state *istate, const str
+ unsigned *ignore_untracked)
{
int changed = ie_match_stat(diffopt->repo->index, ce, st, ce_option);
- struct diff_flags orig_flags;
+- if (S_ISGITLINK(ce->ce_mode)) {
+- struct diff_flags orig_flags = diffopt->flags;
+- if (!diffopt->flags.override_submodule_config)
+- set_diffopt_flags_from_submodule_config(diffopt, ce->name);
+- if (diffopt->flags.ignore_submodules)
+- changed = 0;
+- else if (!diffopt->flags.ignore_dirty_submodules &&
+- (!changed || diffopt->flags.dirty_submodules))
++ struct diff_flags orig_flags;
+ int defer = 0;
-
- if (!S_ISGITLINK(ce->ce_mode))
-- return changed;
++
++ if (!S_ISGITLINK(ce->ce_mode))
+ goto ret;
-
- orig_flags = diffopt->flags;
- if (!diffopt->flags.override_submodule_config)
-@@ diff-lib.c: static int match_stat_with_submodule(struct diff_options *diffopt,
- goto cleanup;
- }
- if (!diffopt->flags.ignore_dirty_submodules &&
-- (!changed || diffopt->flags.dirty_submodules))
-- *dirty_submodule = is_submodule_modified(ce->name,
++
++ orig_flags = diffopt->flags;
++ if (!diffopt->flags.override_submodule_config)
++ set_diffopt_flags_from_submodule_config(diffopt, ce->name);
++ if (diffopt->flags.ignore_submodules) {
++ changed = 0;
++ goto cleanup;
++ }
++ if (!diffopt->flags.ignore_dirty_submodules &&
+ (!changed || diffopt->flags.dirty_submodules)) {
+ if (defer_submodule_status && *defer_submodule_status) {
+ defer = 1;
+ *ignore_untracked = diffopt->flags.ignore_untracked_in_submodules;
+ } else {
-+ *dirty_submodule = is_submodule_modified(ce->name,
- diffopt->flags.ignore_untracked_in_submodules);
+ *dirty_submodule = is_submodule_modified(ce->name,
+- diffopt->flags.ignore_untracked_in_submodules);
+- diffopt->flags = orig_flags;
++ diffopt->flags.ignore_untracked_in_submodules);
+ }
-+ }
- cleanup:
- diffopt->flags = orig_flags;
+ }
++cleanup:
++ diffopt->flags = orig_flags;
+ret:
+ if (defer_submodule_status)
+ *defer_submodule_status = defer;
@@ diff-lib.c: int run_diff_files(struct rev_info *revs, unsigned int option)
changed, istate, ce))
continue;
}
-+ if (submodules.nr > 0) {
-+ int parallel_jobs;
-+ if (git_config_get_int("submodule.diffjobs", ¶llel_jobs))
++ if (submodules.nr) {
++ unsigned long parallel_jobs;
++ struct string_list_item *item;
++
++ if (git_config_get_ulong("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(&submodules, parallel_jobs))
+ die(_("submodule status failed"));
-+ for (size_t i = 0; i < submodules.nr; i++) {
-+ struct submodule_status_util *util = submodules.items[i].util;
++ for_each_string_list_item(item, &submodules) {
++ struct submodule_status_util *util = item->util;
+
+ if (diff_change_helper(&revs->diffopt, util->newmode,
+ util->dirty_submodule, util->changed,
@@ submodule.c: int submodule_touches_in_range(struct repository *r,
+ int result;
+
+ struct string_list *submodule_names;
-+
-+ /* Pending statuses by OIDs */
-+ struct status_task **oid_status_tasks;
-+ int oid_status_tasks_nr, oid_status_tasks_alloc;
+};
+
struct submodule_parallel_fetch {
@@ submodule.c: unsigned is_submodule_modified(const char *path, int ignore_untrack
+ struct status_task *task = task_cb;
+
+ sps->result = 1;
-+ strbuf_addf(err,
-+ _(status_porcelain_start_error),
-+ task->path);
++ strbuf_addf(err, _(STATUS_PORCELAIN_START_ERROR), task->path);
+ return 0;
+}
+
@@ submodule.c: unsigned is_submodule_modified(const char *path, int ignore_untrack
+
+ if (retvalue) {
+ sps->result = 1;
-+ strbuf_addf(err,
-+ _(status_porcelain_fail_error),
-+ task->path);
++ strbuf_addf(err, _(STATUS_PORCELAIN_FAIL_ERROR), task->path);
+ }
+
+ parse_status_porcelain_strbuf(&task->out,-- 2.39.1.519.gcb327c4b5f-goog