Re: [PATCH 8/8] git submodule update: Have a dedicated helper for cloning

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 21, 2015, 20:47 UTC
Message-ID: <xmqqd1w8uewx.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <1445381030-23912-9-git-send-email-sbeller@google.com>

Stefan Beller <sbeller@google.com> writes:

Show 14 quoted lines

> This introduces a new helper function in git submodule--helper
> which takes care of cloning all submodules, which we want to
> parallelize eventually.
>
> Some tests (such as empty URL, update_mode==none) are required in the
> helper to make the decision for cloning. These checks have been moved
> into the C function as well. (No need to repeat them in the shell
> script)
>
> As we can only access the stderr channel from within the parallel
> processing engine, so we need to reroute the error message for
> specified but initialized submodules to stderr. As it is an error
> message, this should have gone to stderr in the first place, so a
> bug fix along the way.

The last paragraph is hard to parse; perhaps it is slightly ungrammatical.

It would be a really good idea to split the small bit to redirect the output that should have gone to the standard error to where it should as a preparatory step before showing this patch.

I sense that this one is still a WIP/RFC, so I'll only skim it in this round (but I may come back and read it again later with finer toothed comb).

> +static int get_next_task(void **pp_task_cb,
> +			 struct child_process *cp,
> +			 struct strbuf *err,
> +			 void *pp_cb)

Will you have only one caller of the parallel run-command API in this file, or will you be adding more to allow various different operations run in parallel as more things are rewritten? I am guessing that it would be the latter, but if that is the case, perhaps the function wants to be named a bit more specificly for this first user, no? Same for start_failure and task_finished.

Show 21 quoted lines

> diff --git a/git-submodule.sh b/git-submodule.sh
> index 8b0eb9a..ea883b9 100755
> --- a/git-submodule.sh
> +++ b/git-submodule.sh
> @@ -655,17 +655,18 @@ cmd_update()
>  		cmd_init "--" "$@" || return
>  	fi
>  
> -	cloned_modules=
> -	git submodule--helper list --prefix "$wt_prefix" "$@" | {
> +	git submodule--helper update-clone ${GIT_QUIET:+--quiet} \
> +		${wt_prefix:+--prefix "$wt_prefix"} \
> +		${prefix:+--recursive_prefix "$prefix"} \
> +		${update:+--update "$update"} \
> +		${reference:+--reference "$reference"} \
> +		${depth:+--depth "$depth"} \
> +		"$@" | {
>  	err=
> -	while read mode sha1 stage sm_path
> +	while read mode sha1 stage just_cloned sm_path
>  	do

I wonder if you really want this to be upstream of a pipe. When the downstream loop needs to abort, what happens to the remainder of the "clone" part of the processing that is still ongoing in the upstream of the pipe? I would imagine that the "update-clone" network accessing phase is the more human-time consuming part, so I suspect that it would be much better to let the cloning part go and finish first (during which time the human-user can spend time for other things, like getting cup of coffee or filling expense reports) and before moving to the loop that can stop and ask the human-user for help.

The fix for the above could be trivial (do not pipe, just take the output to a temporary file, and then feed the "while read" loop from that temporary file), and I suspect it would make a big difference for usability.

Thanks.