On Thu, Sep 22, 2016 at 08:36:01AM -0700, Stefan Beller wrote:
> > Signed-off-by: Jeff King <p...@peff.net>
> Acked and thanked by Stefan ;)
> > + /*
> > + * 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
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