On Wed, Sep 21, 2016 at 10:24 PM, Jeff King <p...@peff.net> 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 <p...@peff.net>

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.

Reply via email to