From: Stefan Beller <hidden> Date: 2016-06-15 23:06:35
I didn't say this in the previous round because it smelled like an
RFC, but for a real submission, 2/2 may be doing too many things at
once. I suspect this is more or less "taste" thing, so I won't mind
too much as long as the reviewers are OK with it.
The patch 2/2 is now broken up into the first five patches.
There are only 2 minor changes:
* If a number of tasks <= 0 is specified, use the number of cpus instead.
* Renamed the command line option in test-run-command.c to "run-command-parallel-4"
as the 4 is hardcoded there.
The patch 6,7 are small cleanups (6 should add a test in a reroll)
Patches 7,8,9 are a preview of how I want to proceed in the near future:
After these functions are split out, we can add another patch on top which
rewrites the short main loop in cmd_update to be in C in submodule--helper
running in parallel.
It took me a while to get the idea how to realize parallelism with the
parallel run command structure now as opposed to the thread pool I proposed
earlier, but I think it will be straightforward from here.
Stefan
Stefan Beller (10):
strbuf: Add strbuf_read_noblock
run-command: factor out return value computation
run-command: add an asynchronous parallel child processor
fetch_populated_submodules: use new parallel job processing
submodules: Allow parallel fetching, add tests and documentation
git submodule update: Redirect any output to stderr
git submodule update: pass --prefix only with a non empty prefix
git submodule update: cmd_update_recursive
git submodule update: cmd_update_recursive
git submodule update: cmd_update_fetch
Documentation/fetch-options.txt | 7 +
builtin/fetch.c | 6 +-
builtin/pull.c | 6 +
git-submodule.sh | 242 ++++++++++++++++++----------------
run-command.c | 281 ++++++++++++++++++++++++++++++++++++----
run-command.h | 36 +++++
strbuf.c | 25 +++-
strbuf.h | 6 +
submodule.c | 119 ++++++++++++-----
submodule.h | 2 +-
t/t0061-run-command.sh | 20 +++
t/t5526-fetch-submodules.sh | 19 +++
test-run-command.c | 24 ++++
13 files changed, 620 insertions(+), 173 deletions(-)
--
2.6.0.rc0.131.gf624c3d
From: Stefan Beller <hidden> Date: 2016-06-15 23:06:35
We need to read from pipes without blocking in a later patch.
Signed-off-by: Stefan Beller <redacted>
---
strbuf.c | 25 +++++++++++++++++++++++--
strbuf.h | 6 ++++++
2 files changed, 29 insertions(+), 2 deletions(-)
From: Stefan Beller <hidden> Date: 2016-06-15 23:06:35
We will need computing the return value in a later patch without the
wait.
Signed-off-by: Stefan Beller <redacted>
---
run-command.c | 54 ++++++++++++++++++++++++++++++++----------------------
1 file changed, 32 insertions(+), 22 deletions(-)
@@ -232,6 +232,35 @@ static inline void set_cloexec(int fd)fcntl(fd,F_SETFD,flags|FD_CLOEXEC);}+staticintdetermine_return_value(intwait_status,+int*result,+int*error_code,+constchar*argv0)+{+if(WIFSIGNALED(wait_status)){+*result=WTERMSIG(wait_status);+if(*result!=SIGINT&&*result!=SIGQUIT)+error("%s died of signal %d",argv0,*result);+/*+*Thisreturnvalueischosensothatcode&0xff+*mimicstheexitcodethataPOSIXshellwouldreportfor+*aprogramthatdiedfromthissignal.+*/+*result+=128;+}elseif(WIFEXITED(wait_status)){+*result=WEXITSTATUS(wait_status);+/*+*Convertspecialexitcodewhenexecvpfailed.+*/+if(*result==127){+*result=-1;+*error_code=ENOENT;+}+}else+return1;+return0;+}+staticintwait_or_whine(pid_tpid,constchar*argv0){intstatus,code=-1;
@@ -244,29 +273,10 @@ static int wait_or_whine(pid_t pid, const char *argv0)if(waiting<0){failed_errno=errno;error("waitpid for %s failed: %s",argv0,strerror(errno));-}elseif(waiting!=pid){-error("waitpid is confused (%s)",argv0);-}elseif(WIFSIGNALED(status)){-code=WTERMSIG(status);-if(code!=SIGINT&&code!=SIGQUIT)-error("%s died of signal %d",argv0,code);-/*-*Thisreturnvalueischosensothatcode&0xff-*mimicstheexitcodethataPOSIXshellwouldreportfor-*aprogramthatdiedfromthissignal.-*/-code+=128;-}elseif(WIFEXITED(status)){-code=WEXITSTATUS(status);-/*-*Convertspecialexitcodewhenexecvpfailed.-*/-if(code==127){-code=-1;-failed_errno=ENOENT;-}}else{-error("waitpid is confused (%s)",argv0);+if(waiting!=pid+||(determine_return_value(status,&code,&failed_errno,argv0)<0))+error("waitpid is confused (%s)",argv0);}clear_child_for_cleanup(pid);
@@ -607,6 +607,24 @@ cmd_update_recursive()fi}+cmd_update_clone()+{+command="git checkout $subforce -q"+die_msg="$(eval_gettext"Unable to checkout '\$sha1' in submodule path '\$displaypath'")"+say_msg="$(eval_gettext"Submodule path '\$displaypath': checked out '\$sha1'")"++gitsubmodule--helperclone${GIT_QUIET:+--quiet}${prefix:+--prefix "$prefix"}--path"$sm_path"--name"$name"--url"$url""$reference""$depth"||exit++if(clear_local_git_env;cd"$sm_path"&&$command"$sha1")+then+say"$say_msg"+else+err="${err};$die_msg"+return+fi+cmd_update_recursive+}+## Update each submodule path to correct revision, using clone and checkout as needed#
@@ -725,9 +742,8 @@ Maybe you want to use 'update --init'?")"if!test-d"$sm_path"/.git&&!test-f"$sm_path"/.gitthen-gitsubmodule--helperclone${GIT_QUIET:+--quiet}${prefix:+--prefix "$prefix"}--path"$sm_path"--name"$name"--url"$url""$reference""$depth"||exit-cloned_modules="$cloned_modules;$name"-subsha1=+cmd_update_clone+continueelsesubsha1=$(clear_local_git_env;cd"$sm_path"&&gitrev-parse--verifyHEAD)||
@@ -767,13 +783,6 @@ Maybe you want to use 'update --init'?")"die"$(eval_gettext"Unable to fetch in submodule path '\$displaypath'")"fi-# Is this something we just cloned?-case";$cloned_modules;"in-*";$name;"*)-# then there is no local change to integrate-update_module=checkout;;-esac-must_die_on_failure=case"$update_module"incheckout)
From: Stefan Beller <hidden> Date: 2016-06-15 23:06:35
In a later patch we enable parallel processing of submodules, this
only adds the possibility for it. So this change should not change
any user facing behavior.
Signed-off-by: Stefan Beller <redacted>
---
builtin/fetch.c | 3 +-
submodule.c | 119 +++++++++++++++++++++++++++++++++++++++-----------------
submodule.h | 2 +-
3 files changed, 87 insertions(+), 37 deletions(-)
@@ -625,6 +625,89 @@ cmd_update_clone()cmd_update_recursive}+cmd_update_fetch()+{+subsha1=$(clear_local_git_env;cd"$sm_path"&&+gitrev-parse--verifyHEAD)||+die"$(eval_gettext"Unable to find current revision in submodule path '\$displaypath'")"++iftest-n"$remote"+then+iftest-z"$nofetch"+then+# Fetch remote before determining tracking $sha1+(clear_local_git_env;cd"$sm_path"&&git-fetch)||+die"$(eval_gettext"Unable to fetch in submodule path '\$sm_path'")"+fi+remote_name=$(clear_local_git_env;cd"$sm_path"&&get_default_remote)+sha1=$(clear_local_git_env;cd"$sm_path"&&+gitrev-parse--verify"${remote_name}/${branch}")||+die"$(eval_gettext"Unable to find current ${remote_name}/${branch} revision in submodule path '\$sm_path'")"+fi++iftest"$subsha1"!="$sha1"||test-n"$force"+then+subforce=$force+# If we don't already have a -f flag and the submodule has never been checked out+iftest-z"$subsha1"&&test-z"$force"+then+subforce="-f"+fi++iftest-z"$nofetch"+then+# Run fetch only if $sha1 isn't present or it+# is not reachable from a ref.+(clear_local_git_env;cd"$sm_path"&&+((rev=$(gitrev-list-n1$sha1--not--all2>/dev/null)&&+test-z"$rev")||git-fetch))||+die"$(eval_gettext"Unable to fetch in submodule path '\$displaypath'")"+fi++must_die_on_failure=+case"$update_module"in+checkout)+command="git checkout $subforce -q"+die_msg="$(eval_gettext"Unable to checkout '\$sha1' in submodule path '\$displaypath'")"+say_msg="$(eval_gettext"Submodule path '\$displaypath': checked out '\$sha1'")"+;;+rebase)+command="git rebase"+die_msg="$(eval_gettext"Unable to rebase '\$sha1' in submodule path '\$displaypath'")"+say_msg="$(eval_gettext"Submodule path '\$displaypath': rebased into '\$sha1'")"+must_die_on_failure=yes+;;+merge)+command="git merge"+die_msg="$(eval_gettext"Unable to merge '\$sha1' in submodule path '\$displaypath'")"+say_msg="$(eval_gettext"Submodule path '\$displaypath': merged in '\$sha1'")"+must_die_on_failure=yes+;;+!*)+command="${update_module#!}"+die_msg="$(eval_gettext"Execution of '\$command \$sha1' failed in submodule path '\$prefix\$sm_path'")"+say_msg="$(eval_gettext"Submodule path '\$prefix\$sm_path': '\$command \$sha1'")"+must_die_on_failure=yes+;;+*)+die"$(eval_gettext"Invalid update mode '$update_module' for submodule '$name'")"+esac++if(clear_local_git_env;cd"$sm_path"&&$command"$sha1")+then+say"$say_msg"+eliftest-n"$must_die_on_failure"+then+die_with_status2"$die_msg"+else+err="${err};$die_msg"+return+fi+fi++cmd_update_recursive+}+## Update each submodule path to correct revision, using clone and checkout as needed#
@@ -743,88 +826,9 @@ Maybe you want to use 'update --init'?")"if!test-d"$sm_path"/.git&&!test-f"$sm_path"/.gitthencmd_update_clone-continueelse-subsha1=$(clear_local_git_env;cd"$sm_path"&&-gitrev-parse--verifyHEAD)||-die"$(eval_gettext"Unable to find current revision in submodule path '\$displaypath'")"+cmd_update_fetchfi--iftest-n"$remote"-then-iftest-z"$nofetch"-then-# Fetch remote before determining tracking $sha1-(clear_local_git_env;cd"$sm_path"&&git-fetch)||-die"$(eval_gettext"Unable to fetch in submodule path '\$sm_path'")"-fi-remote_name=$(clear_local_git_env;cd"$sm_path"&&get_default_remote)-sha1=$(clear_local_git_env;cd"$sm_path"&&-gitrev-parse--verify"${remote_name}/${branch}")||-die"$(eval_gettext"Unable to find current ${remote_name}/${branch} revision in submodule path '\$sm_path'")"-fi--iftest"$subsha1"!="$sha1"||test-n"$force"-then-subforce=$force-# If we don't already have a -f flag and the submodule has never been checked out-iftest-z"$subsha1"&&test-z"$force"-then-subforce="-f"-fi--iftest-z"$nofetch"-then-# Run fetch only if $sha1 isn't present or it-# is not reachable from a ref.-(clear_local_git_env;cd"$sm_path"&&-((rev=$(gitrev-list-n1$sha1--not--all2>/dev/null)&&-test-z"$rev")||git-fetch))||-die"$(eval_gettext"Unable to fetch in submodule path '\$displaypath'")"-fi--must_die_on_failure=-case"$update_module"in-checkout)-command="git checkout $subforce -q"-die_msg="$(eval_gettext"Unable to checkout '\$sha1' in submodule path '\$displaypath'")"-say_msg="$(eval_gettext"Submodule path '\$displaypath': checked out '\$sha1'")"-;;-rebase)-command="git rebase"-die_msg="$(eval_gettext"Unable to rebase '\$sha1' in submodule path '\$displaypath'")"-say_msg="$(eval_gettext"Submodule path '\$displaypath': rebased into '\$sha1'")"-must_die_on_failure=yes-;;-merge)-command="git merge"-die_msg="$(eval_gettext"Unable to merge '\$sha1' in submodule path '\$displaypath'")"-say_msg="$(eval_gettext"Submodule path '\$displaypath': merged in '\$sha1'")"-must_die_on_failure=yes-;;-!*)-command="${update_module#!}"-die_msg="$(eval_gettext"Execution of '\$command \$sha1' failed in submodule path '\$prefix\$sm_path'")"-say_msg="$(eval_gettext"Submodule path '\$prefix\$sm_path': '\$command \$sha1'")"-must_die_on_failure=yes-;;-*)-die"$(eval_gettext"Invalid update mode '$update_module' for submodule '$name'")"-esac--if(clear_local_git_env;cd"$sm_path"&&$command"$sha1")-then-say"$say_msg"-eliftest-n"$must_die_on_failure"-then-die_with_status2"$die_msg"-else-err="${err};$die_msg"-continue-fi-fi--cmd_update_recursivedoneiftest-n"$err"
@@ -100,6 +100,13 @@ ifndef::git-pull[] reference to a commit that isn't already in the local submodule clone.+-j::+--jobs=<n>::+ Number of parallel children to be used for fetching submodules.+ Each will fetch from different submodules, such that fetching many+ submodules will be faster. By default submodules will be fetched+ one at a time.+ --no-recurse-submodules:: Disable recursive fetching of submodules (this has the same effect as using the '--recurse-submodules=no' option).
@@ -99,6 +100,8 @@ static struct option builtin_fetch_options[] = {N_("fetch all tags and associated objects"),TAGS_SET),OPT_SET_INT('n',NULL,&tags,N_("do not fetch all tags (--no-tags)"),TAGS_UNSET),+OPT_INTEGER('j',"jobs",&max_children,+N_("number of submodules fetched in parallel")),OPT_BOOL('p',"prune",&prune,N_("prune remote-tracking branches no longer on remote")),{OPTION_CALLBACK,0,"recurse-submodules",NULL,N_("on-demand"),
@@ -71,6 +71,16 @@ test_expect_success "fetch --recurse-submodules recurses into submodules" 'test_i18ncmpexpect.erractual.err'+test_expect_success"fetch --recurse-submodules -j2 has the same output behaviour"'+add_upstream_commit&&+(+cddownstream&&+gitfetch--recurse-submodules-j22>../actual.err+)&&+test_must_be_emptyactual.out&&+test_i18ncmpexpect.erractual.err+'+ test_expect_success"fetch alone only fetches superproject"'add_upstream_commit&&(
@@ -140,6 +150,15 @@ test_expect_success "--quiet propagates to submodules" '!test-sactual.err'+test_expect_success"--quiet propagates to parallel submodules"'+(+cddownstream&&+gitfetch--recurse-submodules-j2--quiet>../actual.out2>../actual.err+)&&+!test-sactual.out&&+!test-sactual.err+'+ test_expect_success"--dry-run propagates to submodules"'add_upstream_commit&&(
From: Stefan Beller <hidden> Date: 2016-06-15 23:06:35
This allows to run external commands in parallel with ordered output
on stderr.
If we run external commands in parallel we cannot pipe the output directly
to the our stdout/err as it would mix up. So each process's output will
flow through a pipe, which we buffer. One subprocess can be directly
piped to out stdout/err for a low latency feedback to the user.
Example:
Let's assume we have 5 submodules A,B,C,D,E and each fetch takes a
different amount of time as the different submodules vary in size, then
the output of fetches in sequential order might look like this:
time -->
output: |---A---| |-B-| |----C-----------| |-D-| |-E-|
When we schedule these submodules into maximal two parallel processes,
a schedule and sample output over time may look like this:
thread 1: |---A---| |-D-| |-E-|
thread 2: |-B-| |----C-----------|
output: |---A---|B|------C-------|DE
So A will be perceived as it would run normally in the single child
version. As B has finished by the time A is done, we can dump its whole
progress buffer on stderr, such that it looks like it finished in no time.
Once that is done, C is determined to be the visible child and its progress
will be reported in real time.
So this way of output is really good for human consumption,
as it only changes the timing, not the actual output.
For machine consumption the output needs to be prepared in
the tasks, by either having a prefix per line or per block
to indicate whose tasks output is displayed.
Signed-off-by: Stefan Beller <redacted>
---
run-command.c | 228 +++++++++++++++++++++++++++++++++++++++++++++++++
run-command.h | 36 ++++++++
t/t0061-run-command.sh | 20 +++++
test-run-command.c | 24 ++++++
4 files changed, 308 insertions(+)
@@ -862,3 +863,230 @@ int capture_command(struct child_process *cmd, struct strbuf *buf, size_t hint)close(cmd->out);returnfinish_command(cmd);}++structparallel_processes{+intmax_number_processes;+void*data;+get_next_taskfn;+handle_child_starting_failurefn_err;+handle_child_return_valuefn_exit;++intnr_processes;+intall_tasks_started;+intforeground_child;+char*slots;+structchild_process*children;+structpollfd*pfd;+structstrbuf*err;+structstrbuffinished_children;+};++staticvoidrun_processes_parallel_init(structparallel_processes*pp,+intn,void*data,+get_next_taskfn,+handle_child_starting_failurefn_err,+handle_child_return_valuefn_exit)+{+inti;++if(n<1)+n=online_cpus();++pp->max_number_processes=n;+pp->data=data;+pp->fn=fn;+pp->fn_err=fn_err;+pp->fn_exit=fn_exit;++pp->nr_processes=0;+pp->all_tasks_started=0;+pp->foreground_child=0;+pp->slots=xcalloc(n,sizeof(*pp->slots));+pp->children=xcalloc(n,sizeof(*pp->children));+pp->pfd=xcalloc(n,sizeof(*pp->pfd));+pp->err=xcalloc(n,sizeof(*pp->err));+strbuf_init(&pp->finished_children,0);++for(i=0;i<n;i++){+strbuf_init(&pp->err[i],0);+pp->pfd[i].events=POLLIN;+pp->pfd[i].fd=-1;+}+}++staticvoidrun_processes_parallel_cleanup(structparallel_processes*pp)+{+inti;+for(i=0;i<pp->max_number_processes;i++)+strbuf_release(&pp->err[i]);++free(pp->children);+free(pp->slots);+free(pp->pfd);+free(pp->err);+strbuf_release(&pp->finished_children);+}++staticvoidunblock_fd(intfd)+{+intflags=fcntl(fd,F_GETFL);+if(flags<0){+warning("Could not get file status flags, "+"output will be degraded");+return;+}+if(fcntl(fd,F_SETFL,flags|O_NONBLOCK)){+warning("Could not set file status flags, "+"output will be degraded");+return;+}+}++staticvoidrun_processes_parallel_start_new(structparallel_processes*pp)+{+inti;+/* Start new processes. */+while(!pp->all_tasks_started+&&pp->nr_processes<pp->max_number_processes){+for(i=0;i<pp->max_number_processes;i++)+if(!pp->slots[i])+break;/* found an empty slot */+if(i==pp->max_number_processes)+die("BUG: bookkeeping is hard");++if(pp->fn(pp->data,&pp->children[i],&pp->err[i])){+pp->all_tasks_started=1;+break;+}+if(start_command(&pp->children[i]))+pp->fn_err(pp->data,&pp->children[i],&pp->err[i]);++unblock_fd(pp->children[i].err);++pp->nr_processes++;+pp->slots[i]=1;+pp->pfd[i].fd=pp->children[i].err;+}+}++staticintrun_processes_parallel_buffer_stderr(structparallel_processes*pp)+{+inti;+i=poll(pp->pfd,pp->max_number_processes,100);+if(i<0){+if(errno==EINTR)+/* A signal was caught; try again */+return-1;+else{+run_processes_parallel_cleanup(pp);+die_errno("poll");+}+}++/* Buffer output from all pipes. */+for(i=0;i<pp->max_number_processes;i++){+if(!pp->slots[i])+continue;+if(pp->pfd[i].revents&POLLIN)+strbuf_read_noblock(&pp->err[i],pp->children[i].err,0);+if(pp->foreground_child==i){+fputs(pp->err[i].buf,stderr);+strbuf_reset(&pp->err[i]);+}+}+return0;+}+++staticvoidrun_processes_parallel_collect_finished(structparallel_processes*pp)+{+inti=0;+pid_tpid;+intwait_status,code;+intn=pp->max_number_processes;+/* Collect finished child processes. */+while(pp->nr_processes>0){+pid=waitpid(-1,&wait_status,WNOHANG);+if(pid==0)+return;/* no child finished */++if(pid<0){+if(errno==EINTR)+return;/* just try again next time */+if(errno==EINVAL||errno==ECHILD)+die_errno("wait");+}else{+/* Find the finished child. */+for(i=0;i<pp->max_number_processes;i++)+if(pp->slots[i]&&pid==pp->children[i].pid)+break;+if(i==pp->max_number_processes)+/*+*waitpidreturnedanotherprocessid+*whichwearenotwaitingfor.+*/+return;+}+strbuf_read_noblock(&pp->err[i],pp->children[i].err,0);++if(determine_return_value(wait_status,&code,&errno,+pp->children[i].argv[0])<0)+error("waitpid is confused (%s)",+pp->children[i].argv[0]);++pp->fn_exit(pp->data,&pp->children[i],code);++argv_array_clear(&pp->children[i].args);+argv_array_clear(&pp->children[i].env_array);++pp->nr_processes--;+pp->slots[i]=0;+pp->pfd[i].fd=-1;++if(i!=pp->foreground_child){+strbuf_addbuf(&pp->finished_children,&pp->err[i]);+strbuf_reset(&pp->err[i]);+}else{+fputs(pp->err[i].buf,stderr);+strbuf_reset(&pp->err[i]);++/* Output all other finished child processes */+fputs(pp->finished_children.buf,stderr);+strbuf_reset(&pp->finished_children);++/*+*Picknextprocesstooutputlive.+*NEEDSWORK:+*Fornowwepickitrandomlybydoingaround+*robin.Laterwemaywanttopicktheonewith+*themostoutputorthelongestorshortest+*runningprocesstime.+*/+for(i=0;i<n;i++)+if(pp->slots[(pp->foreground_child+i)%n])+break;+pp->foreground_child=(pp->foreground_child+i)%n;+fputs(pp->err[pp->foreground_child].buf,stderr);+strbuf_reset(&pp->err[pp->foreground_child]);+}+}+}++intrun_processes_parallel(intn,void*data,+get_next_taskfn,+handle_child_starting_failurefn_err,+handle_child_return_valuefn_exit)+{+structparallel_processespp;+run_processes_parallel_init(&pp,n,data,fn,fn_err,fn_exit);++while(!pp.all_tasks_started||pp.nr_processes>0){+run_processes_parallel_start_new(&pp);+if(run_processes_parallel_buffer_stderr(&pp))+continue;+run_processes_parallel_collect_finished(&pp);+}+run_processes_parallel_cleanup(&pp);++return0;+}
@@ -10,9 +10,29 @@#include"git-compat-util.h"#include"run-command.h"+#include"argv-array.h"+#include"strbuf.h"#include<string.h>#include<errno.h>+staticintnumber_callbacks;+intparallel_next(void*data,+structchild_process*cp,+structstrbuf*err)+{+structchild_process*d=data;+if(number_callbacks>=4)+return1;++argv_array_pushv(&cp->args,d->argv);+cp->stdout_to_stderr=1;+cp->no_stdin=1;+cp->err=-1;+strbuf_addf(err,"preloaded output of a child\n");+number_callbacks++;+return0;+}+intmain(intargc,char**argv){structchild_processproc=CHILD_PROCESS_INIT;
@@ -30,6 +50,10 @@ int main(int argc, char **argv)if(!strcmp(argv[1],"run-command"))exit(run_command(&proc));+if(!strcmp(argv[1],"run-command-parallel-4"))+exit(run_processes_parallel(4,&proc,parallel_next,+NULL,NULL));+fprintf(stderr,"check usage\n");return1;}
From: Stefan Beller <hidden> Date: 2016-06-15 23:06:35
split the recursion part out to its own function
Signed-off-by: Stefan Beller <redacted>
---
git-submodule.sh | 47 ++++++++++++++++++++++++++---------------------
1 file changed, 26 insertions(+), 21 deletions(-)
@@ -582,6 +582,31 @@ cmd_deinit()done}++cmd_update_recursive()+{+iftest-n"$recursive"+then+(+prefix="$prefix$sm_path/"+clear_local_git_env+cd"$sm_path"&&+evalcmd_update+)+res=$?+iftest$res-gt0+then+die_msg="$(eval_gettext"Failed to recurse into submodule path '\$displaypath'")"+iftest$res-eq1+then+err="${err};$die_msg"+else+die_with_status$res"$die_msg"+fi+fi+fi+}+## Update each submodule path to correct revision, using clone and checkout as needed#
@@ -790,27 +815,7 @@ Maybe you want to use 'update --init'?")"fifi-iftest-n"$recursive"-then-(-prefix="$prefix$sm_path/"-clear_local_git_env-cd"$sm_path"&&-evalcmd_update-)-res=$?-iftest$res-gt0-then-die_msg="$(eval_gettext"Failed to recurse into submodule path '\$displaypath'")"-iftest$res-eq1-then-err="${err};$die_msg"-continue-else-die_with_status$res"$die_msg"-fi-fi-fi+cmd_update_recursivedoneiftest-n"$err"
From: Stefan Beller <hidden> Date: 2016-06-15 23:06:35
There are no tests, which fail by this.
Signed-off-by: Stefan Beller <redacted>
---
git-submodule.sh | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
From: Stefan Beller <hidden> Date: 2016-06-15 23:06:35
We should not pass --prefix NULL into the helper. Although the helper
can deal with it, it's just messy.
Signed-off-by: Stefan Beller <redacted>
---
git-submodule.sh | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
@@ -700,7 +700,7 @@ Maybe you want to use 'update --init'?")"if!test-d"$sm_path"/.git&&!test-f"$sm_path"/.gitthen-gitsubmodule--helperclone${GIT_QUIET:+--quiet}--prefix"$prefix"--path"$sm_path"--name"$name"--url"$url""$reference""$depth"||exit+gitsubmodule--helperclone${GIT_QUIET:+--quiet}${prefix:+--prefix "$prefix"}--path"$sm_path"--name"$name"--url"$url""$reference""$depth"||exitcloned_modules="$cloned_modules;$name"subsha1=else
From: Jeff King <hidden> Date: 2016-06-15 23:06:35
On Wed, Sep 16, 2015 at 06:39:00PM -0700, Stefan Beller wrote:
+static int determine_return_value(int wait_status,
+ int *result,
+ int *error_code,
+ const char *argv0)
+{
+ if (WIFSIGNALED(wait_status)) {
+ *result = WTERMSIG(wait_status);
+ if (*result != SIGINT && *result != SIGQUIT)
+ error("%s died of signal %d", argv0, *result);
+ /*
+ * This return value is chosen so that code & 0xff
+ * mimics the exit code that a POSIX shell would report for
+ * a program that died from this signal.
+ */
+ *result += 128;
+ } else if (WIFEXITED(wait_status)) {
+ *result = WEXITSTATUS(wait_status);
+ /*
+ * Convert special exit code when execvp failed.
+ */
+ if (*result == 127) {
+ *result = -1;
+ *error_code = ENOENT;
+ }
+ } else
+ return 1;
+ return 0;
+}
Looks like we can return "0" or "1" here, and the exit code goes into
"result". But our caller:
quoted hunk
static int wait_or_whine(pid_t pid, const char *argv0)
{
int status, code = -1;
@@ -244,29 +273,10 @@ static int wait_or_whine(pid_t pid, const char *argv0) if (waiting < 0) { failed_errno = errno; error("waitpid for %s failed: %s", argv0, strerror(errno));- } else if (waiting != pid) {- error("waitpid is confused (%s)", argv0);- } else if (WIFSIGNALED(status)) {- code = WTERMSIG(status);- if (code != SIGINT && code != SIGQUIT)- error("%s died of signal %d", argv0, code);- /*- * This return value is chosen so that code & 0xff- * mimics the exit code that a POSIX shell would report for- * a program that died from this signal.- */- code += 128;- } else if (WIFEXITED(status)) {- code = WEXITSTATUS(status);- /*- * Convert special exit code when execvp failed.- */- if (code == 127) {- code = -1;- failed_errno = ENOENT;- } } else {- error("waitpid is confused (%s)", argv0);+ if (waiting != pid+ || (determine_return_value(status, &code, &failed_errno, argv0) < 0))+ error("waitpid is confused (%s)", argv0); }
...is looking for "< 0", which will never happen. Should the "1" above
have been "-1"?
I also wondered what happened to "code" and "failed_errno" in that case.
They are OK to access because wait_or_whine() has set them to defaults,
but I wonder if determine_return_value should do so in every branch (so
it is is clear that the values are always defined when it returns).
-Peff
From: Jacob Keller <hidden> Date: 2016-06-15 23:06:36
On Wed, Sep 16, 2015 at 6:38 PM, Stefan Beller [off-list ref] wrote:
It took me a while to get the idea how to realize parallelism with the
parallel run command structure now as opposed to the thread pool I proposed
earlier, but I think it will be straightforward from here.
Yea at least from a cursory review this seems significantly simpler.
I'm still trying to give a better deep dive of the code, so hopefully
I will have some more thoughts soon.
Regards,
Jake
@@ -700,7 +700,7 @@ Maybe you want to use 'update --init'?")"if!test-d"$sm_path"/.git&&!test-f"$sm_path"/.gitthen-gitsubmodule--helperclone${GIT_QUIET:+--quiet}--prefix"$prefix"--path"$sm_path"--name"$name"--url"$url""$reference""$depth"||exit+gitsubmodule--helperclone${GIT_QUIET:+--quiet}${prefix:+--prefix "$prefix"}--path"$sm_path"--name"$name"--url"$url""$reference""$depth"||exitcloned_modules="$cloned_modules;$name"subsha1=else--
@@ -607,6 +607,24 @@ cmd_update_recursive()fi}+cmd_update_clone()+{+command="git checkout $subforce -q"+die_msg="$(eval_gettext"Unable to checkout '\$sha1' in submodule path '\$displaypath'")"+say_msg="$(eval_gettext"Submodule path '\$displaypath': checked out '\$sha1'")"++gitsubmodule--helperclone${GIT_QUIET:+--quiet}${prefix:+--prefix "$prefix"}--path"$sm_path"--name"$name"--url"$url""$reference""$depth"||exit++if(clear_local_git_env;cd"$sm_path"&&$command"$sha1")+then+say"$say_msg"+else+err="${err};$die_msg"+return+fi+cmd_update_recursive+}+## Update each submodule path to correct revision, using clone and checkout as needed#
@@ -725,9 +742,8 @@ Maybe you want to use 'update --init'?")"if!test-d"$sm_path"/.git&&!test-f"$sm_path"/.gitthen-gitsubmodule--helperclone${GIT_QUIET:+--quiet}${prefix:+--prefix "$prefix"}--path"$sm_path"--name"$name"--url"$url""$reference""$depth"||exit-cloned_modules="$cloned_modules;$name"-subsha1=+cmd_update_clone+continueelsesubsha1=$(clear_local_git_env;cd"$sm_path"&&gitrev-parse--verifyHEAD)||
@@ -767,13 +783,6 @@ Maybe you want to use 'update --init'?")"die"$(eval_gettext"Unable to fetch in submodule path '\$displaypath'")"fi-# Is this something we just cloned?-case";$cloned_modules;"in-*";$name;"*)-# then there is no local change to integrate-update_module=checkout;;-esac-must_die_on_failure=case"$update_module"incheckout)--
From: Stefan Beller <hidden> Date: 2016-06-15 23:06:36
On Thu, Sep 17, 2015 at 1:31 PM, Eric Sunshine [off-list ref] wrote:
On Wed, Sep 16, 2015 at 9:39 PM, Stefan Beller [off-list ref] wrote:
quoted
git submodule update: Redirect any output to stderr
This commit message seems to be lacking an explanation of why this is
being done.
quoted
There are no tests, which fail by this.
Not sure what this means. I suppose you're trying to say that this
patch doesn't break any existing tests, but isn't that an implied goal
of all patches posted to this list?
Yes they should (but they don't yet).
What I was trying to say:
In a reroll I want to add tests to this as I was surprised
of not breaking a test by this commit (so the behavior
of this is untested which is bad)
These changes seem to be doing more than what the commit message
claims. The changed code isn't just redirecting to stderr, but is also
now respecting $GIT_QUIET.
Right, I need to redo the commit message anyways, so I'll mention that.
From: Eric Sunshine <hidden> Date: 2016-06-15 23:06:36
On Wed, Sep 16, 2015 at 9:39 PM, Stefan Beller [off-list ref] wrote:
git submodule update: Redirect any output to stderr
This commit message seems to be lacking an explanation of why this is
being done.
There are no tests, which fail by this.
Not sure what this means. I suppose you're trying to say that this
patch doesn't break any existing tests, but isn't that an implied goal
of all patches posted to this list?
These changes seem to be doing more than what the commit message
claims. The changed code isn't just redirecting to stderr, but is also
now respecting $GIT_QUIET.