On Thu, Sep 22, 2016 at 08:36:01AM -0700, Stefan Beller wrote: > > Signed-off-by: Jeff King <peff@xxxxxxxx> > > Acked and thanked by Stefan ;) Thanks. > > + /* > > + * 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 To be fair, "much worse" is relative; it's _just_ a disabled progress bar. :) I do think this counts as a real regression, though. The "do not pass down explicit --progress" bug was something probably nobody ever cared about. But losing progress with the default settings is a thing people might actually notice. > 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. Yeah, I considered passing a "no really, stderr is a tty" environment variable down (which would have avoided all of the boilerplate propagation of --progress through the various helpers). But when I realized that we do not handle explicit "--progress" either, the correct solution became more obvious. And hopefully all that propagation boilerplate will eventually go away (or at least be simplified) as the submodule code consolidates in C. > > 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. Yeah, I had a similar thought that we might need to combine these, or possibly that we could even drop "quite". But I tried to err on the side of making the minimal change, as this isn't really a code path I'm very familiar with. -Peff