Stefan Beller[PATCHv17 00/11] Expose submodule parallelism to the user
This replaces origin/sb/submodule-parallel-update and applies on origin/sb/submodule-parallel-fetch.
Thanks Jonathan for review!
* fixing all the small nits of v16 found by Jonathan!
Thanks, Stefan
Interdiff to v15: (current origin/sb/submodule-parallel-update)
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c index 32254cd..9d94406 100644 --- a/builtin/submodule--helper.c +++ b/builtin/submodule--helper.c @@ -259,32 +259,36 @@ struct submodule_update_clone { /* index into 'list', the list of submodules to look into for cloning */ int current; struct module_list list; - int warn_if_uninitialized : 1; - /* update parameter passed via commandline*/ + unsigned warn_if_uninitialized : 1; + + /* update parameter passed via commandline */ struct submodule_update_strategy update; + /* configuration parameters which are passed on to the children */ int quiet; const char *reference; const char *depth; const char *recursive_prefix; const char *prefix; - /* lines to be output */ + + /* Machine-readable status lines to be consumed by git-submodule.sh */ struct string_list projectlines; + /* If we want to stop as fast as possible and return an error */ - int quickstop : 1; + unsigned quickstop : 1; }; #define SUBMODULE_UPDATE_CLONE_INIT {0, MODULE_LIST_INIT, 0, \ SUBMODULE_UPDATE_STRATEGY_INIT, 0, NULL, NULL, NULL, NULL, \ STRING_LIST_INIT_DUP, 0} /** - * Inspect if 'ce' needs to be cloned. If so, prepare the 'child' to be running - * the clone and return non zero. + * Determine whether 'ce' needs to be cloned. If so, prepare the 'child' to + * run the clone. Returns 1 if 'ce' needs to be cloned, 0 otherwise. */ static int prepare_to_clone_next_submodule(const struct cache_entry *ce, struct child_process *child, struct submodule_update_clone *suc, - struct strbuf *err) + struct strbuf *out) { const struct submodule *sub = NULL; struct strbuf displaypath_sb = STRBUF_INIT; @@ -295,10 +299,10 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce, if (ce_stage(ce)) { if (suc->recursive_prefix) { - strbuf_addf(err, "Skipping unmerged submodule %s/%s\n", + strbuf_addf(out, "Skipping unmerged submodule %s/%s\n", suc->recursive_prefix, ce->name); } else { - strbuf_addf(err, "Skipping unmerged submodule %s\n", + strbuf_addf(out, "Skipping unmerged submodule %s\n", ce->name); } goto cleanup; @@ -315,7 +319,7 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce, if (suc->update.type == SM_UPDATE_NONE || (suc->update.type == SM_UPDATE_UNSPECIFIED && sub->update_strategy.type == SM_UPDATE_NONE)) { - strbuf_addf(err, "Skipping submodule '%s'\n", + strbuf_addf(out, "Skipping submodule '%s'\n", displaypath); goto cleanup; } @@ -330,11 +334,11 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce, git_config_get_string(sb.buf, &url); if (!url) { /* - * Only mention uninitialized submodules when its + * Only mention uninitialized submodules when their * path have been specified */ if (suc->warn_if_uninitialized) - strbuf_addf(err, _("Submodule path '%s' not initialized\n" + strbuf_addf(out, _("Submodule path '%s' not initialized\n" "Maybe you want to use 'update --init'?\n"), displaypath); goto cleanup; @@ -435,7 +439,7 @@ static int update_clone(int argc, const char **argv, const char *prefix) struct submodule_update_clone suc = SUBMODULE_UPDATE_CLONE_INIT; struct option module_update_clone_options[] = { - OPT_STRING(0, "prefix", &prefix, + OPT_STRING(0, "prefix", &suc.prefix, N_("path"), N_("path into the working tree")), OPT_STRING(0, "recursive-prefix", &suc.recursive_prefix, @@ -460,7 +464,6 @@ static int update_clone(int argc, const char **argv, const char *prefix) N_("git submodule--helper update_clone [--prefix=<path>] [<path>...]"), NULL }; - suc.prefix = prefix; argc = parse_options(argc, argv, prefix, module_update_clone_options, git_submodule_helper_usage, 0); @@ -475,8 +478,8 @@ static int update_clone(int argc, const char **argv, const char *prefix) if (pathspec.nr) suc.warn_if_uninitialized = 1; - gitmodules_config(); /* Overlay the parsed .gitmodules file with .git/config */ + gitmodules_config(); git_config(submodule_config, NULL); if (max_jobs < 0) diff --git a/run-command.c b/run-command.c index 8abaae4..6fad42f 100644 --- a/run-command.c +++ b/run-command.c @@ -897,29 +897,29 @@ struct parallel_processes { struct pollfd *pfd; unsigned shutdown : 1; - unsigned ended_with_newline: 1; int output_owner; struct strbuf buffered_output; /* of finished children */ }; int default_start_failure(struct child_process *cp, - struct strbuf *err, + struct strbuf *out, void *pp_cb, void *pp_task_cb) { int i; - strbuf_addstr(err, "Starting a child failed:"); + strbuf_addstr(out, "Starting a child failed:"); for (i = 0; cp->argv[i]; i++) - strbuf_addf(err, " %s", cp->argv[i]); + strbuf_addf(out, " %s", cp->argv[i]); + strbuf_addch(out, '\n'); return 0; } int default_task_finished(int result, struct child_process *cp, - struct strbuf *err, + struct strbuf *out, void *pp_cb, void *pp_task_cb) { @@ -928,9 +928,10 @@ int default_task_finished(int result, if (!result) return 0; - strbuf_addf(err, "A child failed with return code %d:", result); + strbuf_addf(out, "A child failed with return code %d:", result); for (i = 0; cp->argv[i]; i++) - strbuf_addf(err, " %s", cp->argv[i]); + strbuf_addf(out, " %s", cp->argv[i]); + strbuf_addch(out, '\n'); return 0; } @@ -980,7 +981,6 @@ static void pp_init(struct parallel_processes *pp, pp->nr_processes = 0; pp->output_owner = 0; pp->shutdown = 0; - pp->ended_with_newline = 1; pp->children = xcalloc(n, sizeof(*pp->children)); pp->pfd = xcalloc(n, sizeof(*pp->pfd)); strbuf_init(&pp->buffered_output, 0); @@ -1013,7 +1013,7 @@ static void pp_cleanup(struct parallel_processes *pp) * When get_next_task added messages to the buffer in its last * iteration, the buffered output is non empty. */ - fputs(pp->buffered_output.buf, stderr); + strbuf_write(&pp->buffered_output, stderr); strbuf_release(&pp->buffered_output); sigchain_pop_common(); @@ -1055,7 +1055,6 @@ static int pp_start_one(struct parallel_processes *pp) pp->data, &pp->children[i].data); strbuf_addbuf(&pp->buffered_output, &pp->children[i].err); - strbuf_addch(&pp->buffered_output, '\n'); strbuf_reset(&pp->children[i].err); if (code) pp->shutdown = 1; @@ -1098,11 +1097,9 @@ static void pp_buffer_stderr(struct parallel_processes *pp, int output_timeout) static void pp_output(struct parallel_processes *pp) { int i = pp->output_owner; - size_t len = pp->children[i].err.len; - if (pp->children[i].state == GIT_CP_WORKING && len) { - fputs(pp->children[i].err.buf, stderr); - pp->ended_with_newline = - (pp->children[i].err.buf[len - 1] == '\n'); + if (pp->children[i].state == GIT_CP_WORKING && + pp->children[i].err.len) { + strbuf_write(&pp->children[i].err, stderr); strbuf_reset(&pp->children[i].err); } } @@ -1112,7 +1109,6 @@ static int pp_collect_finished(struct parallel_processes *pp) int i, code; int n = pp->max_processes; int result = 0; - ssize_t len; while (pp->nr_processes > 0) { for (i = 0; i < pp->max_processes; i++) @@ -1137,22 +1133,15 @@ static int pp_collect_finished(struct parallel_processes *pp) pp->pfd[i].fd = -1; child_process_init(&pp->children[i].process); - len = pp->children[i].err.len - 1; - if (len >= 0 && pp->children[i].err.buf[len] != '\n') - strbuf_addch(&pp->children[i].err, '\n'); - if (i != pp->output_owner) { strbuf_addbuf(&pp->buffered_output, &pp->children[i].err); strbuf_reset(&pp->children[i].err); } else { - if (len == -1 && !pp->ended_with_newline) - strbuf_addch(&pp->children[i].err, '\n'); - fputs(pp->children[i].err.buf, stderr); + strbuf_write(&pp->children[i].err, stderr); strbuf_reset(&pp->children[i].err); - pp->ended_with_newline = 1; /* Output all other finished child processes */ - fputs(pp->buffered_output.buf, stderr); + strbuf_write(&pp->buffered_output, stderr); strbuf_reset(&pp->buffered_output); /* diff --git a/run-command.h b/run-command.h index a054fa6..2bd8fee 100644 --- a/run-command.h +++ b/run-command.h @@ -139,7 +139,7 @@ int in_async(void); * return the negative signal number. */ typedef int (*get_next_task_fn)(struct child_process *cp, - struct strbuf *err, + struct strbuf *out, void *pp_cb, void **pp_task_cb); @@ -148,7 +148,7 @@ typedef int (*get_next_task_fn)(struct child_process *cp, * a new process. * * You must not write to stdout or stderr in this function. Add your - * message to the strbuf err instead, which will be printed without + * message to the strbuf out instead, which will be printed without * messing up the output of the other parallel processes. * * pp_cb is the callback cookie as passed into run_processes_parallel, @@ -159,7 +159,7 @@ typedef int (*get_next_task_fn)(struct child_process *cp, * the negative signal number. */ typedef int (*start_failure_fn)(struct child_process *cp, - struct strbuf *err, + struct strbuf *out, void *pp_cb, void *pp_task_cb); @@ -168,7 +168,7 @@ typedef int (*start_failure_fn)(struct child_process *cp, * exact command which failed. */ int default_start_failure(struct child_process *cp, - struct strbuf *err, + struct strbuf *out, void *pp_cb, void *pp_task_cb); @@ -176,7 +176,7 @@ int default_start_failure(struct child_process *cp, * This callback is called on every child process that finished processing. * * You must not write to stdout or stderr in this function. Add your - * message to the strbuf err instead, which will be printed without + * message to the strbuf out instead, which will be printed without * messing up the output of the other parallel processes. * * pp_cb is the callback cookie as passed into run_processes_parallel, @@ -188,7 +188,7 @@ int default_start_failure(struct child_process *cp, */ typedef int (*task_finished_fn)(int result, struct child_process *cp, - struct strbuf *err, + struct strbuf *out, void *pp_cb, void *pp_task_cb); @@ -198,7 +198,7 @@ typedef int (*task_finished_fn)(int result, */ int default_task_finished(int result, struct child_process *cp, - struct strbuf *err, + struct strbuf *out, void *pp_cb, void *pp_task_cb); diff --git a/strbuf.c b/strbuf.c index 38686ff..71345cd 100644 --- a/strbuf.c +++ b/strbuf.c @@ -395,6 +395,12 @@ ssize_t strbuf_read_once(struct strbuf *sb, int fd, size_t hint) return cnt; } +ssize_t strbuf_write(struct strbuf *sb, FILE *f) +{ + return fwrite(sb->buf, 1, sb->len, f); +} + + #define STRBUF_MAXLINK (2*PATH_MAX) int strbuf_readlink(struct strbuf *sb, const char *path, size_t hint) diff --git a/strbuf.h b/strbuf.h index 2bf90e7..d4f2aa1 100644 --- a/strbuf.h +++ b/strbuf.h @@ -387,6 +387,12 @@ extern ssize_t strbuf_read_file(struct strbuf *sb, const char *path, size_t hint extern int strbuf_readlink(struct strbuf *sb, const char *path, size_t hint); /** + * Write the whole content of the strbuf to the stream not stopping at + * NUL bytes. + */ +extern ssize_t strbuf_write(struct strbuf *sb, FILE *stream); + +/** * Read a line from a FILE *, overwriting the existing contents * of the strbuf. The second argument specifies the line * terminator character, typically `'\n'`. diff --git a/submodule.c b/submodule.c index 8fbc847..051f722 100644 --- a/submodule.c +++ b/submodule.c @@ -219,7 +219,7 @@ void gitmodules_config(void) int parse_submodule_update_strategy(const char *value, struct submodule_update_strategy *dst) { - free((void *)dst->command); + free((void*)dst->command); dst->command = NULL; if (!strcmp(value, "none")) dst->type = SM_UPDATE_NONE; diff --git a/t/t0061-run-command.sh b/t/t0061-run-command.sh index 5c6822c..12228b4 100755 --- a/t/t0061-run-command.sh +++ b/t/t0061-run-command.sh @@ -77,32 +77,6 @@ test_expect_success 'run_command runs in parallel with more tasks than jobs avai test_cmp expect actual ' -test_expect_success 'run_command ensures each command ends in LF' ' - test-run-command run-command-parallel 3 sh -c "printf \"%s\n%s\" Hello World" 2>actual && - test_cmp expect actual -' - -cat >expect <<-EOF -preloaded output of a child -preloaded output of a child -preloaded output of a child -preloaded output of a child -EOF - -test_expect_success 'run_command ensures each command ends in LF when output is only in starting cb' ' - test-run-command run-command-parallel 3 sh -c true 2>actual && - test_cmp expect actual -' - -cat >expect <<-EOF -EOF - -test_expect_success 'run_command ensures each command ends in LF except when there is no output' ' - test-run-command run-command-parallel-silent 3 sh -c true 2>actual && - test_cmp expect actual -' - - cat >expect <<-EOF preloaded output of a child asking for a quick stop diff --git a/test-run-command.c b/test-run-command.c index ca1ee97..fbe0a27 100644 --- a/test-run-command.c +++ b/test-run-command.c @@ -31,20 +31,6 @@ static int parallel_next(struct child_process *cp, return 1; } -static int parallel_next_silent(struct child_process *cp, - struct strbuf *err, - void *cb, - void **task_cb) -{ - struct child_process *d = cb; - if (number_callbacks >= 4) - return 0; - - argv_array_pushv(&cp->args, d->argv); - number_callbacks++; - return 1; -} - static int no_job(struct child_process *cp, struct strbuf *err, void *cb, @@ -85,10 +71,6 @@ int main(int argc, char **argv) jobs = atoi(argv[2]); proc.argv = (const char **)argv + 3; - if (!strcmp(argv[1], "run-command-parallel-silent")) - exit(run_processes_parallel(jobs, parallel_next_silent, - NULL, NULL, &proc)); - if (!strcmp(argv[1], "run-command-parallel")) exit(run_processes_parallel(jobs, parallel_next, NULL, NULL, &proc)); Stefan Beller (11): submodule-config: keep update strategy around submodule-config: drop check against NULL fetching submodules: respect `submodule.fetchJobs` config option submodule update: direct error message to stderr run_processes_parallel: treat output of children as byte array run-command: expose default_{start_failure, task_finished} run_processes_parallel: rename parameters for the callbacks run_processes_parallel: correctly terminate callbacks with an LF git submodule update: have a dedicated helper for cloning submodule update: expose parallelism to the user clone: allow an explicit argument for parallel submodule clones Documentation/config.txt | 6 + Documentation/git-clone.txt | 6 +- Documentation/git-submodule.txt | 7 +- builtin/clone.c | 19 ++- builtin/fetch.c | 2 +- builtin/submodule--helper.c | 254 ++++++++++++++++++++++++++++++++++++++++ git-submodule.sh | 56 ++++----- run-command.c | 36 +++--- run-command.h | 29 ++++- strbuf.c | 6 + strbuf.h | 6 + submodule-config.c | 19 ++- submodule-config.h | 2 + submodule.c | 37 +++++- submodule.h | 18 +++ t/t5526-fetch-submodules.sh | 14 +++ t/t7400-submodule-basic.sh | 4 +- t/t7406-submodule-update.sh | 27 +++++ 18 files changed, 477 insertions(+), 71 deletions(-)
-- 2.7.2.373.g1655498.dirty
