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
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.


Reply via email to