On Wed, Sep 21, 2016 at 10:24 PM, Jeff King <p...@peff.net> wrote:
> When cloning with "--recursive", we'd generally expect
> submodules to show progress reports if the main clone did,
> In older versions of git, this mostly worked out of the
> box. Since we show progress by default when stderr is a tty,
> and since the child clones inherit the parent stderr, then
> both processes would come to the same decision by default.
> If the parent clone was asked for "--quiet", we passed down
> "--quiet" to the child. However, if stderr was not a tty and
> the user specified "--progress", we did not propagate this
> to the child.
> That's a minor bug, but things got much worse when we
> switched recently to submodule--helper's update_clone
> command. With that change, the stderr of the child clones
> are always connected to a pipe, and we never output
> progress at all.
Right, that is an issue.
> Signed-off-by: Jeff King <p...@peff.net>
Acked and thanked by Stefan ;)
> I imagine there are other code paths that want similar treatment, but I
> didn't look into them. I'd assume "fetch" is one. I'm not sure if we do
> parallel checkouts, but that's another potential.
Looking for run_processes_parallel in the code,
it seems to only be used in fetch_populated_submodules
and the submodule helper.
> update_head(our_head_points_at, remote_head, reflog_msg.buf);
> + /*
> + * We want to show progress for recursive submodule clones iff
> + * we did so for the main clone. But only the transport knows
> + * the final decision for this flag, so we need to rescue the value
> + * before we free the transport.
> + */
> + submodule_progress = transport->progress;
Good point! I was aware of this bug (but I did not consider it to be impactful
or as you put it "much worse"), but I anticipated we would need some refactoring
of the transport code, e.g. have the decision via isatty(2) as a
function that we consult before we even setup the transport and then
pass it down
to the submodules as well. This seems to solve this bug elegantly.
> @@ -1108,7 +1120,7 @@ int cmd_clone(int argc, const char **argv, const char
> junk_mode = JUNK_LEAVE_REPO;
> - err = checkout();
> + err = checkout(submodule_progress);
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index 7b8ddfe..d2f9d7d 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -443,7 +443,8 @@ static int module_name(int argc, const char **argv, const
> char *prefix)
> static int clone_submodule(const char *path, const char *gitdir, const char
> - const char *depth, struct string_list *reference,
> int quiet)
> + const char *depth, struct string_list *reference,
> + int quiet, int progress)
I am not sure if having both quiet and progress is maintainable well,
but it get's the job done here, specifically if we consider this patch a bug
fix that we'd want to merge down to maint.