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

From: Stefan Beller <sbeller@google.com>
Date: Sep 22, 2016, 15:36 UTC
Message-ID: <CAGZ79kbTi+5oo325v+v8GB7isKRfqys6WRGUARHiMvieDz+png@mail.gmail.com>
In-Reply-To: <20160922052446.iwr62hpa2meal7uj@sigill.intra.peff.net>

On Wed, Sep 21, 2016 at 10:24 PM, Jeff King <peff@peff.net> wrote:

Show 18 quoted lines

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

Show 11 quoted lines

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

Show 23 quoted lines

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