Re: [PATCH] clone: pass --progress decision to recursive submodules

From: Jeff King <peff@peff.net>
Date: Sep 23, 2016, 03:31 UTC
Message-ID: <20160923033143.azkwbbh54oistg6u@sigill.intra.peff.net>
In-Reply-To: <CAGZ79kbTi+5oo325v+v8GB7isKRfqys6WRGUARHiMvieDz+png@mail.gmail.com>

On Thu, Sep 22, 2016 at 08:36:01AM -0700, Stefan Beller wrote:

> > Signed-off-by: Jeff King <peff@peff.net>
> 
> Acked and thanked by Stefan ;)

Thanks.

Show 12 quoted lines

> > +       /*
> > +        * 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.

Show 8 quoted lines

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