On Wed, Sep 21, 2016 at 10:24 PM, Jeff King <peff@xxxxxxxx> wrote: > When cloning with "--recursive", we'd generally expect > submodules to show progress reports if the main clone did, > too. > > 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 <peff@xxxxxxxx> 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 separate outside 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. > transport_unlock_pack(transport); > transport_disconnect(transport); > > @@ -1108,7 +1120,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix) > } > > junk_mode = JUNK_LEAVE_REPO; > - err = checkout(); > + err = checkout(submodule_progress); > > strbuf_release(&reflog_msg); > strbuf_release(&branch_top); > 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 *url, > - 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.