{"thread":{"id":"44130","subject":"[PATCH] clone: pass --progress decision to recursive submodules","startedAt":"2016-09-22T05:24:56Z","lastAt":"2016-09-23T03:33:36Z","messageCount":3,"participants":["Jeff King","Stefan Beller"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"302357","messageId":"20160922052446.iwr62hpa2meal7uj@sigill.intra.peff.net","threadId":"44130","inReplyTo":null,"subject":"[PATCH] clone: pass --progress decision to recursive submodules","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-09-22T05:24:46Z","receivedAt":"2016-09-22T05:24:56Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When cloning with \"--recursive\", we'd generally expect\nsubmodules to show progress reports if the main clone did,\ntoo.\n\nIn older versions of git, this mostly worked out of the\nbox. Since we show progress by default when stderr is a tty,\nand since the child clones inherit the parent stderr, then\nboth processes would come to the same decision by default.\nIf the parent clone was asked for \"--quiet\", we passed down\n\"--quiet\" to the child. However, if stderr was not a tty and\nthe user specified \"--progress\", we did not propagate this\nto the child.\n\nThat's a minor bug, but things got much worse when we\nswitched recently to submodule--helper's update_clone\ncommand. With that change, the stderr of the child clones\nare always connected to a pipe, and we never output\nprogress at all.\n\nThis patch teaches git-submodule and git-submodule--helper\nhow to pass down an explicit \"--progress\" flag when cloning.\nThe clone command then decides to propagate that flag based\non the cloning decision made earlier (which takes into\naccount isatty(2) of the parent process, existing --progress\nor --quiet flags, etc). Since the child processes always run\nwithout a tty on stderr, we don't have to worry about\npassing an explicit \"--no-progress\"; it's the default for\nthem.\n\nThis fixes the recent loss of progress during recursive\nclones. And as a bonus, it makes:\n\n  git clone --recursive --progress ... 2>&1 | cat\n\nwork by triggering progress explicitly in the children.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nI don't usually use submodules, but I happened to be testing something,\nand my goto \"this has a lot of submodules\" repository happens to have\nsome pretty large submodules. So I had plenty of time to contemplate the\nlack of progress reporting. :)\n\nI checked with --jobs, too, and this should do the right thing; it\nqueues up the progress reports for submodules that aren't the\noutput_owner, and then dumps them all at once (so the progress meter\nwill appear to whiz by for those jobs until it catches up to the current\nstate, which is the only reasonable thing we can do, I think).\n\nI imagine there are other code paths that want similar treatment, but I\ndidn't look into them. I'd assume \"fetch\" is one. I'm not sure if we do\nparallel checkouts, but that's another potential.\n\n builtin/clone.c             | 16 ++++++++++++++--\n builtin/submodule--helper.c | 18 +++++++++++++++---\n git-submodule.sh            |  5 +++++\n 3 files changed, 34 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex 404c5e8..28ce938 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -670,7 +670,7 @@ static void update_head(const struct ref *our, const struct ref *remote,\n \t}\n }\n \n-static int checkout(void)\n+static int checkout(int submodule_progress)\n {\n \tunsigned char sha1[20];\n \tchar *head;\n@@ -734,6 +734,9 @@ static int checkout(void)\n \t\tif (max_jobs != -1)\n \t\t\targv_array_pushf(&args, \"--jobs=%d\", max_jobs);\n \n+\t\tif (submodule_progress)\n+\t\t\targv_array_push(&args, \"--progress\");\n+\n \t\terr = run_command_v_opt(args.argv, RUN_GIT_CMD);\n \t\targv_array_clear(&args);\n \t}\n@@ -841,6 +844,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \tconst char *src_ref_prefix = \"refs/heads/\";\n \tstruct remote *remote;\n \tint err = 0, complete_refs_before_fetch = 1;\n+\tint submodule_progress;\n \n \tstruct refspec *refspec;\n \tconst char *fetch_pattern;\n@@ -1099,6 +1103,14 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \n \tupdate_head(our_head_points_at, remote_head, reflog_msg.buf);\n \n+\t/*\n+\t * We want to show progress for recursive submodule clones iff\n+\t * we did so for the main clone. But only the transport knows\n+\t * the final decision for this flag, so we need to rescue the value\n+\t * before we free the transport.\n+\t */\n+\tsubmodule_progress = transport->progress;\n+\n \ttransport_unlock_pack(transport);\n \ttransport_disconnect(transport);\n \n@@ -1108,7 +1120,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \t}\n \n \tjunk_mode = JUNK_LEAVE_REPO;\n-\terr = checkout();\n+\terr = checkout(submodule_progress);\n \n \tstrbuf_release(&reflog_msg);\n \tstrbuf_release(&branch_top);\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 7b8ddfe..d2f9d7d 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -443,7 +443,8 @@ static int module_name(int argc, const char **argv, const char *prefix)\n }\n \n static int clone_submodule(const char *path, const char *gitdir, const char *url,\n-\t\t\t   const char *depth, struct string_list *reference, int quiet)\n+\t\t\t   const char *depth, struct string_list *reference,\n+\t\t\t   int quiet, int progress)\n {\n \tstruct child_process cp = CHILD_PROCESS_INIT;\n \n@@ -451,6 +452,8 @@ static int clone_submodule(const char *path, const char *gitdir, const char *url\n \targv_array_push(&cp.args, \"--no-checkout\");\n \tif (quiet)\n \t\targv_array_push(&cp.args, \"--quiet\");\n+\tif (progress)\n+\t\targv_array_push(&cp.args, \"--progress\");\n \tif (depth && *depth)\n \t\targv_array_pushl(&cp.args, \"--depth\", depth, NULL);\n \tif (reference->nr) {\n@@ -575,6 +578,7 @@ static int module_clone(int argc, const char **argv, const char *prefix)\n {\n \tconst char *name = NULL, *url = NULL, *depth = NULL;\n \tint quiet = 0;\n+\tint progress = 0;\n \tFILE *submodule_dot_git;\n \tchar *p, *path = NULL, *sm_gitdir;\n \tstruct strbuf rel_path = STRBUF_INIT;\n@@ -601,6 +605,8 @@ static int module_clone(int argc, const char **argv, const char *prefix)\n \t\t\t   N_(\"string\"),\n \t\t\t   N_(\"depth for shallow clones\")),\n \t\tOPT__QUIET(&quiet, \"Suppress output for cloning a submodule\"),\n+\t\tOPT_BOOL(0, \"progress\", &progress,\n+\t\t\t   N_(\"force cloning progress\")),\n \t\tOPT_END()\n \t};\n \n@@ -634,7 +640,8 @@ static int module_clone(int argc, const char **argv, const char *prefix)\n \n \t\tprepare_possible_alternates(name, &reference);\n \n-\t\tif (clone_submodule(path, sm_gitdir, url, depth, &reference, quiet))\n+\t\tif (clone_submodule(path, sm_gitdir, url, depth, &reference,\n+\t\t\t\t    quiet, progress))\n \t\t\tdie(_(\"clone of '%s' into submodule path '%s' failed\"),\n \t\t\t    url, path);\n \t} else {\n@@ -684,6 +691,7 @@ struct submodule_update_clone {\n \tstruct submodule_update_strategy update;\n \n \t/* configuration parameters which are passed on to the children */\n+\tint progress;\n \tint quiet;\n \tint recommend_shallow;\n \tstruct string_list references;\n@@ -702,7 +710,7 @@ struct submodule_update_clone {\n \tint failed_clones_nr, failed_clones_alloc;\n };\n #define SUBMODULE_UPDATE_CLONE_INIT {0, MODULE_LIST_INIT, 0, \\\n-\tSUBMODULE_UPDATE_STRATEGY_INIT, 0, -1, STRING_LIST_INIT_DUP, \\\n+\tSUBMODULE_UPDATE_STRATEGY_INIT, 0, 0, -1, STRING_LIST_INIT_DUP, \\\n \tNULL, NULL, NULL, \\\n \tSTRING_LIST_INIT_DUP, 0, NULL, 0, 0}\n \n@@ -804,6 +812,8 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce,\n \tchild->err = -1;\n \targv_array_push(&child->args, \"submodule--helper\");\n \targv_array_push(&child->args, \"clone\");\n+\tif (suc->progress)\n+\t\targv_array_push(&child->args, \"--progress\");\n \tif (suc->quiet)\n \t\targv_array_push(&child->args, \"--quiet\");\n \tif (suc->prefix)\n@@ -950,6 +960,8 @@ static int update_clone(int argc, const char **argv, const char *prefix)\n \t\tOPT_BOOL(0, \"recommend-shallow\", &suc.recommend_shallow,\n \t\t\t    N_(\"whether the initial clone should follow the shallow recommendation\")),\n \t\tOPT__QUIET(&suc.quiet, N_(\"don't print cloning progress\")),\n+\t\tOPT_BOOL(0, \"progress\", &suc.progress,\n+\t\t\t    N_(\"force cloning progress\")),\n \t\tOPT_END()\n \t};\n \ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex a1cc71b..a024a13 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -44,6 +44,7 @@ update=\n prefix=\n custom_name=\n depth=\n+progress=\n \n die_if_unmatched ()\n {\n@@ -498,6 +499,9 @@ cmd_update()\n \t\t-q|--quiet)\n \t\t\tGIT_QUIET=1\n \t\t\t;;\n+\t\t--progress)\n+\t\t\tprogress=\"--progress\"\n+\t\t\t;;\n \t\t-i|--init)\n \t\t\tinit=1\n \t\t\t;;\n@@ -573,6 +577,7 @@ cmd_update()\n \n \t{\n \tgit submodule--helper update-clone ${GIT_QUIET:+--quiet} \\\n+\t\t${progress:+\"$progress\"} \\\n \t\t${wt_prefix:+--prefix \"$wt_prefix\"} \\\n \t\t${prefix:+--recursive-prefix \"$prefix\"} \\\n \t\t${update:+--update \"$update\"} \\\n-- \n2.10.0.482.gae5a597\n"},{"id":"302373","messageId":"CAGZ79kbTi+5oo325v+v8GB7isKRfqys6WRGUARHiMvieDz+png@mail.gmail.com","threadId":"44130","inReplyTo":"20160922052446.iwr62hpa2meal7uj@sigill.intra.peff.net","subject":"Re: [PATCH] clone: pass --progress decision to recursive submodules","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-09-22T15:36:01Z","receivedAt":"2016-09-22T15:36:08Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Sep 21, 2016 at 10:24 PM, Jeff King <peff@peff.net> wrote:\n> When cloning with \"--recursive\", we'd generally expect\n> submodules to show progress reports if the main clone did,\n> too.\n>\n> In older versions of git, this mostly worked out of the\n> box. Since we show progress by default when stderr is a tty,\n> and since the child clones inherit the parent stderr, then\n> both processes would come to the same decision by default.\n> If the parent clone was asked for \"--quiet\", we passed down\n> \"--quiet\" to the child. However, if stderr was not a tty and\n> the user specified \"--progress\", we did not propagate this\n> to the child.\n>\n> That's a minor bug, but things got much worse when we\n> switched recently to submodule--helper's update_clone\n> command. With that change, the stderr of the child clones\n> are always connected to a pipe, and we never output\n> progress at all.\n\nRight, that is an issue.\n\n>\n> Signed-off-by: Jeff King <peff@peff.net>\n\nAcked and thanked by Stefan ;)\n\n>\n> I imagine there are other code paths that want similar treatment, but I\n> didn't look into them. I'd assume \"fetch\" is one. I'm not sure if we do\n> parallel checkouts, but that's another potential.\n\nLooking for run_processes_parallel in the code,\nit seems to only be used in fetch_populated_submodules\nand the submodule helper.\n\n>\n>         update_head(our_head_points_at, remote_head, reflog_msg.buf);\n>\n> +       /*\n> +        * We want to show progress for recursive submodule clones iff\n> +        * we did so for the main clone. But only the transport knows\n> +        * the final decision for this flag, so we need to rescue the value\n> +        * before we free the transport.\n> +        */\n> +       submodule_progress = transport->progress;\n> +\n\nGood point! I was aware of this bug (but I did not consider it to be impactful\nor as you put it \"much worse\"), but I anticipated we would need some refactoring\nof the transport code, e.g. have the decision via isatty(2) as a\nseparate outside\nfunction that we consult before we even setup the transport and then\npass it down\nto the submodules as well. This seems to solve this bug elegantly.\n\n>         transport_unlock_pack(transport);\n>         transport_disconnect(transport);\n>\n> @@ -1108,7 +1120,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n>         }\n>\n>         junk_mode = JUNK_LEAVE_REPO;\n> -       err = checkout();\n> +       err = checkout(submodule_progress);\n>\n>         strbuf_release(&reflog_msg);\n>         strbuf_release(&branch_top);\n> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\n> index 7b8ddfe..d2f9d7d 100644\n> --- a/builtin/submodule--helper.c\n> +++ b/builtin/submodule--helper.c\n> @@ -443,7 +443,8 @@ static int module_name(int argc, const char **argv, const char *prefix)\n>  }\n>\n>  static int clone_submodule(const char *path, const char *gitdir, const char *url,\n> -                          const char *depth, struct string_list *reference, int quiet)\n> +                          const char *depth, struct string_list *reference,\n> +                          int quiet, int progress)\n\nI am not sure if having both quiet and progress is maintainable well,\nbut it get's the job done here, specifically if we consider this patch a bug\nfix that we'd want to merge down to maint.\n"},{"id":"302421","messageId":"20160923033143.azkwbbh54oistg6u@sigill.intra.peff.net","threadId":"44130","inReplyTo":"CAGZ79kbTi+5oo325v+v8GB7isKRfqys6WRGUARHiMvieDz+png@mail.gmail.com","subject":"Re: [PATCH] clone: pass --progress decision to recursive submodules","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-09-23T03:31:44Z","receivedAt":"2016-09-23T03:33:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Sep 22, 2016 at 08:36:01AM -0700, Stefan Beller wrote:\n\n> > Signed-off-by: Jeff King <peff@peff.net>\n> \n> Acked and thanked by Stefan ;)\n\nThanks.\n\n> > +       /*\n> > +        * We want to show progress for recursive submodule clones iff\n> > +        * we did so for the main clone. But only the transport knows\n> > +        * the final decision for this flag, so we need to rescue the value\n> > +        * before we free the transport.\n> > +        */\n> > +       submodule_progress = transport->progress;\n> > +\n> \n> Good point! I was aware of this bug (but I did not consider it to be\n> impactful or as you put it \"much worse\"), but I anticipated we would\n> need some refactoring\n\nTo be fair, \"much worse\" is relative; it's _just_ a disabled progress\nbar. :)\n\nI do think this counts as a real regression, though. The \"do not pass\ndown explicit --progress\" bug was something probably nobody ever cared\nabout. But losing progress with the default settings is a thing people\nmight actually notice.\n\n> of the transport code, e.g. have the decision via isatty(2) as a\n> separate outside function that we consult before we even setup the\n> transport and then pass it down to the submodules as well. This seems\n> to solve this bug elegantly.\n\nYeah, I considered passing a \"no really, stderr is a tty\" environment\nvariable down (which would have avoided all of the boilerplate\npropagation of --progress through the various helpers). But when I\nrealized that we do not handle explicit \"--progress\" either, the\ncorrect solution became more obvious.\n\nAnd hopefully all that propagation boilerplate will eventually go away\n(or at least be simplified) as the submodule code consolidates in C.\n\n> >  static int clone_submodule(const char *path, const char *gitdir, const char *url,\n> > -                          const char *depth, struct string_list *reference, int quiet)\n> > +                          const char *depth, struct string_list *reference,\n> > +                          int quiet, int progress)\n> \n> I am not sure if having both quiet and progress is maintainable well,\n> but it get's the job done here, specifically if we consider this patch a bug\n> fix that we'd want to merge down to maint.\n\nYeah, I had a similar thought that we might need to combine these, or\npossibly that we could even drop \"quite\". But I tried to err on the side\nof making the minimal change, as this isn't really a code path I'm very\nfamiliar with.\n\n-Peff\n"}]}