{"thread":{"id":"52612","subject":"[PATCH] fetch: emphasize failure during submodule fetch","startedAt":"2020-01-10T19:55:46Z","lastAt":"2020-01-17T10:54:43Z","messageCount":9,"participants":["Emily Shaffer","Junio C Hamano","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"389594","messageId":"20200110195533.6416-1-emilyshaffer@google.com","threadId":"52612","inReplyTo":null,"subject":"[PATCH] fetch: emphasize failure during submodule fetch","fromName":"Emily Shaffer","fromEmail":"emilyshaffer@google.com","sentAt":"2020-01-10T19:55:33Z","receivedAt":"2020-01-10T19:55:46Z","isPatch":true,"sender":{"key":"nasamuffin@google.com","avatar":"https://avatars.githubusercontent.com/u/1606826?v=4"},"body":"In cases when a submodule fetch fails when there are many submodules, the error\nfrom the lone failing submodule fetch is buried under activity on the other\nsubmodules if more than one fetch fell back on fetch-by-oid. Call out a failure\nlate so the user is aware that something went wrong.\n\nExample without this change:\n\n  $ git pull --rebase\n  remote: Counting objects: 1591, done\n  remote: Finding sources: 100% (4317/4317)\n  remote: Total 4317 (delta 1923), reused 4252 (delta 1923)\n  Receiving objects: 100% (4317/4317), 2.09 MiB | 8.15 MiB/s, done.\n  Resolving deltas: 100% (1923/1923), completed with 101 local objects.\n  From https://android.googlesource.com/platform/superproject\n  [snip ~100 lines]\n  From https://android.googlesource.com/platform/prebuilts/fullsdk/platforms/android-29\n   * branch            a97149980b7d8acf48392af591b35689f7205d9e -> FETCH_HEAD\n  From https://android.googlesource.com/platform/prebuilts/fullsdk-darwin/platform-tools\n   * branch            98f9454af8ca210818eff4f502097c471d7327b5 -> FETCH_HEAD\n  From https://android.googlesource.com/platform/prebuilts/checkstyle\n   * branch            6fb3e23f05ed186908ea9f48d6692220891363b0 -> FETCH_HEAD\n   * branch            f21d92f6339f0993a946b25fa2172c2ceb5e332b -> FETCH_HEAD\n  From https://android.googlesource.com/platform/prebuilts/androidx/studio\n   * branch            bed5e7b5866b8698bbcd1879134b03ac312a2ba8 -> FETCH_HEAD\n  From https://android.googlesource.com/platform/prebuilts/androidx/internal\n   * branch                179375220f834de5dfbee169f4c2f948d850a203 -> FETCH_HEAD\n   * branch                1dcf3ceef9a86001c693fa34b3513f0c4af26178 -> FETCH_HEAD\n   * branch                2ea3ccef4c98f5de1b74affd1dda33f5b2834a45 -> FETCH_HEAD\n   * branch                a09de09c3814c3d31cc770d5351b92d29ea624ae -> FETCH_HEAD\n   * branch                d2ae6add8b2c0e28899e4faeb2d6889ceefb0b62 -> FETCH_HEAD\n   * branch                e244e2a5f7d98f47f75d06ef57ef1c6c5701a38d -> FETCH_HEAD\n  Auto packing the repository in background for optimum performance.\n  See \"git help gc\" for manual housekeeping.\n  From https://android.googlesource.com/platform/prebuilts/androidx/external\n   * branch              c3df2fa7f3e63b8714ac8d24f86a26cc50ee4af5 -> FETCH_HEAD\n  fatal: remote error: want c5bd7796550b3742772c8fb8c73a1311013b5159 not valid\n  From https://android.googlesource.com/platform/external/noto-fonts\n   * branch            02969d3046f6944a5a211d2331d1c82736487f01 -> FETCH_HEAD\n   * branch            9ee45fcd0b8bb8621c1cdbc6de5fe7502eff7393 -> FETCH_HEAD\n  From https://android.googlesource.com/platform/external/dokka\n   * branch            03a8ed966a7b847931a0ee20327f989837aaff13 -> FETCH_HEAD\n   * branch            cb1684602b5b4e18385d890c972764c55d177704 -> FETCH_HEAD\n   * branch            fd4521e89ab0e01447dda9b42be2b9bbc000f02f -> FETCH_HEAD\n  From https://android.googlesource.com/platform/external/doclava\n   * branch            04ddf3962f0cd40c81a2e144f27f497223782457 -> FETCH_HEAD\n   * branch            44bf22680e939b21a21a365f6038d5883d5163c8 -> FETCH_HEAD\n   * branch            66f673f4a3865f3b4ab645655a6484101dbd051f -> FETCH_HEAD\n\nSigned-off-by: Emily Shaffer <emilyshaffer@google.com>\n---\nAs hinted by the snippet in the commit-message (should I remove it? I\nthink it's a poignant example, I couldn't see the fatal without grepping\neven after being told it was there) this manifested to an end user via\n'git pull'. As I was tracking down the right place to \"bump\" the error\nline, I noticed that 'git pull' is made out of run_command() calls, ever\nsince it was adopted from bash ~5 years ago. Is there interest in\nrefactoring it to use library calls instead, or do folks consider 'pull'\nto be such a thin layer over 'git fetch && git (rebase|merge)' that it's\nnot worth the effort?\n\n - Emily\n\n builtin/fetch.c | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex b4c6d921d0..0c19781cb9 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -1857,6 +1857,8 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n \t\t\t\t\t\t    verbosity < 0,\n \t\t\t\t\t\t    max_children);\n \t\targv_array_clear(&options);\n+\t\tif (result)\n+\t\t\tfprintf(stderr, _(\"Failure during submodule fetch.\\n\"));\n \t}\n \n \tstring_list_clear(&list, 0);\n-- \n2.25.0.rc1.283.g88dfdc4193-goog\n\n"},{"id":"389596","messageId":"xmqqtv53kquh.fsf@gitster-ct.c.googlers.com","threadId":"52612","inReplyTo":"20200110195533.6416-1-emilyshaffer@google.com","subject":"Re: [PATCH] fetch: emphasize failure during submodule fetch","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-01-10T20:18:30Z","receivedAt":"2020-01-10T20:18:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Emily Shaffer <emilyshaffer@google.com> writes:\n\n> In cases when a submodule fetch fails when there are many submodules, the error\n> from the lone failing submodule fetch is buried under activity on the other\n> submodules if more than one fetch fell back on fetch-by-oid. Call out a failure\n> late so the user is aware that something went wrong.\n>\n> Example without this change:\n\n>   $ git pull --rebase\n>   remote: Counting objects: 1591, done\n>   remote: Finding sources: 100% (4317/4317)\n>   remote: Total 4317 (delta 1923), reused 4252 (delta 1923)\n>   Receiving objects: 100% (4317/4317), 2.09 MiB | 8.15 MiB/s, done.\n>   Resolving deltas: 100% (1923/1923), completed with 101 local objects.\n>   From https://android.googlesource.com/platform/superproject\n>   [snip ~100 lines]\n>   From https://android.googlesource.com/platform/prebuilts/fullsdk/platforms/android-29\n>    * branch            a97149980b7d8acf48392af591b35689f7205d9e -> FETCH_HEAD\n>   From https://android.googlesource.com/platform/prebuilts/fullsdk-darwin/platform-tools\n>    * branch            98f9454af8ca210818eff4f502097c471d7327b5 -> FETCH_HEAD\n>   From https://android.googlesource.com/platform/prebuilts/checkstyle\n>    * branch            6fb3e23f05ed186908ea9f48d6692220891363b0 -> FETCH_HEAD\n>    * branch            f21d92f6339f0993a946b25fa2172c2ceb5e332b -> FETCH_HEAD\n>   From https://android.googlesource.com/platform/prebuilts/androidx/studio\n>    * branch            bed5e7b5866b8698bbcd1879134b03ac312a2ba8 -> FETCH_HEAD\n>   From https://android.googlesource.com/platform/prebuilts/androidx/internal\n>    * branch                179375220f834de5dfbee169f4c2f948d850a203 -> FETCH_HEAD\n>    * branch                1dcf3ceef9a86001c693fa34b3513f0c4af26178 -> FETCH_HEAD\n>    * branch                2ea3ccef4c98f5de1b74affd1dda33f5b2834a45 -> FETCH_HEAD\n>    * branch                a09de09c3814c3d31cc770d5351b92d29ea624ae -> FETCH_HEAD\n>    * branch                d2ae6add8b2c0e28899e4faeb2d6889ceefb0b62 -> FETCH_HEAD\n>    * branch                e244e2a5f7d98f47f75d06ef57ef1c6c5701a38d -> FETCH_HEAD\n>   Auto packing the repository in background for optimum performance.\n>   See \"git help gc\" for manual housekeeping.\n>   From https://android.googlesource.com/platform/prebuilts/androidx/external\n>    * branch              c3df2fa7f3e63b8714ac8d24f86a26cc50ee4af5 -> FETCH_HEAD\n>   fatal: remote error: want c5bd7796550b3742772c8fb8c73a1311013b5159 not valid\n>   From https://android.googlesource.com/platform/external/noto-fonts\n>    * branch            02969d3046f6944a5a211d2331d1c82736487f01 -> FETCH_HEAD\n>    * branch            9ee45fcd0b8bb8621c1cdbc6de5fe7502eff7393 -> FETCH_HEAD\n>   From https://android.googlesource.com/platform/external/dokka\n>    * branch            03a8ed966a7b847931a0ee20327f989837aaff13 -> FETCH_HEAD\n>    * branch            cb1684602b5b4e18385d890c972764c55d177704 -> FETCH_HEAD\n>    * branch            fd4521e89ab0e01447dda9b42be2b9bbc000f02f -> FETCH_HEAD\n>   From https://android.googlesource.com/platform/external/doclava\n>    * branch            04ddf3962f0cd40c81a2e144f27f497223782457 -> FETCH_HEAD\n>    * branch            44bf22680e939b21a21a365f6038d5883d5163c8 -> FETCH_HEAD\n>    * branch            66f673f4a3865f3b4ab645655a6484101dbd051f -> FETCH_HEAD\n>\n> Signed-off-by: Emily Shaffer <emilyshaffer@google.com>\n> ---\n> As hinted by the snippet in the commit-message (should I remove it? I\n> think it's a poignant example, I couldn't see the fatal without grepping\n> even after being told it was there) this manifested to an end user via\n> 'git pull'.\n\nIt indeed is too noisy, especially without showing what happens with\nthis patch.\n\nIs it clear to the users that a block of lines starting \"From $URL\"\nand ending before the next \"From $AnotherURL\" is about the same\nrepository, including error messages?\n\n> diff --git a/builtin/fetch.c b/builtin/fetch.c\n> index b4c6d921d0..0c19781cb9 100644\n> --- a/builtin/fetch.c\n> +++ b/builtin/fetch.c\n> @@ -1857,6 +1857,8 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n>  \t\t\t\t\t\t    verbosity < 0,\n>  \t\t\t\t\t\t    max_children);\n>  \t\targv_array_clear(&options);\n> +\t\tif (result)\n> +\t\t\tfprintf(stderr, _(\"Failure during submodule fetch.\\n\"));\n\nHow does a user find out which submodule had trouble with after\nseeing this message?  Or is it something you still need to find by\nscrolling back?\n\nIf the latter, I am not sure if there is much point to add a\nhalf-way solution like this.  It is a different story if \"fetch\"\nexits with success status when this happens, but I do not think the\n\"result\" that is non-zero is being lost before the function returns,\nso...\n\n>  \t}\n>  \n>  \tstring_list_clear(&list, 0);\n"},{"id":"389599","messageId":"20200110230128.GG181522@google.com","threadId":"52612","inReplyTo":"xmqqtv53kquh.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] fetch: emphasize failure during submodule fetch","fromName":"Emily Shaffer","fromEmail":"emilyshaffer@google.com","sentAt":"2020-01-10T23:01:28Z","receivedAt":"2020-01-10T23:01:35Z","isPatch":true,"sender":{"key":"nasamuffin@google.com","avatar":"https://avatars.githubusercontent.com/u/1606826?v=4"},"body":"On Fri, Jan 10, 2020 at 12:18:30PM -0800, Junio C Hamano wrote:\n> Emily Shaffer <emilyshaffer@google.com> writes:\n> \n> > In cases when a submodule fetch fails when there are many submodules, the error\n> > from the lone failing submodule fetch is buried under activity on the other\n> > submodules if more than one fetch fell back on fetch-by-oid. Call out a failure\n> > late so the user is aware that something went wrong.\n> >\n> > Example without this change:\n> \n> >   $ git pull --rebase\n> >   remote: Counting objects: 1591, done\n> >   remote: Finding sources: 100% (4317/4317)\n> >   remote: Total 4317 (delta 1923), reused 4252 (delta 1923)\n> >   Receiving objects: 100% (4317/4317), 2.09 MiB | 8.15 MiB/s, done.\n> >   Resolving deltas: 100% (1923/1923), completed with 101 local objects.\n> >   From https://android.googlesource.com/platform/superproject\n> >   [snip ~100 lines]\n> >   From https://android.googlesource.com/platform/prebuilts/fullsdk/platforms/android-29\n> >    * branch            a97149980b7d8acf48392af591b35689f7205d9e -> FETCH_HEAD\n> >   From https://android.googlesource.com/platform/prebuilts/fullsdk-darwin/platform-tools\n> >    * branch            98f9454af8ca210818eff4f502097c471d7327b5 -> FETCH_HEAD\n> >   From https://android.googlesource.com/platform/prebuilts/checkstyle\n> >    * branch            6fb3e23f05ed186908ea9f48d6692220891363b0 -> FETCH_HEAD\n> >    * branch            f21d92f6339f0993a946b25fa2172c2ceb5e332b -> FETCH_HEAD\n> >   From https://android.googlesource.com/platform/prebuilts/androidx/studio\n> >    * branch            bed5e7b5866b8698bbcd1879134b03ac312a2ba8 -> FETCH_HEAD\n> >   From https://android.googlesource.com/platform/prebuilts/androidx/internal\n> >    * branch                179375220f834de5dfbee169f4c2f948d850a203 -> FETCH_HEAD\n> >    * branch                1dcf3ceef9a86001c693fa34b3513f0c4af26178 -> FETCH_HEAD\n> >    * branch                2ea3ccef4c98f5de1b74affd1dda33f5b2834a45 -> FETCH_HEAD\n> >    * branch                a09de09c3814c3d31cc770d5351b92d29ea624ae -> FETCH_HEAD\n> >    * branch                d2ae6add8b2c0e28899e4faeb2d6889ceefb0b62 -> FETCH_HEAD\n> >    * branch                e244e2a5f7d98f47f75d06ef57ef1c6c5701a38d -> FETCH_HEAD\n> >   Auto packing the repository in background for optimum performance.\n> >   See \"git help gc\" for manual housekeeping.\n> >   From https://android.googlesource.com/platform/prebuilts/androidx/external\n> >    * branch              c3df2fa7f3e63b8714ac8d24f86a26cc50ee4af5 -> FETCH_HEAD\n> >   fatal: remote error: want c5bd7796550b3742772c8fb8c73a1311013b5159 not valid\n> >   From https://android.googlesource.com/platform/external/noto-fonts\n> >    * branch            02969d3046f6944a5a211d2331d1c82736487f01 -> FETCH_HEAD\n> >    * branch            9ee45fcd0b8bb8621c1cdbc6de5fe7502eff7393 -> FETCH_HEAD\n> >   From https://android.googlesource.com/platform/external/dokka\n> >    * branch            03a8ed966a7b847931a0ee20327f989837aaff13 -> FETCH_HEAD\n> >    * branch            cb1684602b5b4e18385d890c972764c55d177704 -> FETCH_HEAD\n> >    * branch            fd4521e89ab0e01447dda9b42be2b9bbc000f02f -> FETCH_HEAD\n> >   From https://android.googlesource.com/platform/external/doclava\n> >    * branch            04ddf3962f0cd40c81a2e144f27f497223782457 -> FETCH_HEAD\n> >    * branch            44bf22680e939b21a21a365f6038d5883d5163c8 -> FETCH_HEAD\n> >    * branch            66f673f4a3865f3b4ab645655a6484101dbd051f -> FETCH_HEAD\n> >\n> > Signed-off-by: Emily Shaffer <emilyshaffer@google.com>\n> > ---\n> > As hinted by the snippet in the commit-message (should I remove it? I\n> > think it's a poignant example, I couldn't see the fatal without grepping\n> > even after being told it was there) this manifested to an end user via\n> > 'git pull'.\n> \n> It indeed is too noisy, especially without showing what happens with\n> this patch.\n\nSure, it makes sense. I'll take it out in next round.\n\n> \n> Is it clear to the users that a block of lines starting \"From $URL\"\n> and ending before the next \"From $AnotherURL\" is about the same\n> repository, including error messages?\n\nWell, for me - and the bug reporter - the \"fatal\" line visually blends\nin with the \"From\" next to it. I think once you see the \"fatal\" line\nit's clear where it's coming from, sure.\n\nI wonder if the line order still holds with -j specified, though.\n\n> \n> > diff --git a/builtin/fetch.c b/builtin/fetch.c\n> > index b4c6d921d0..0c19781cb9 100644\n> > --- a/builtin/fetch.c\n> > +++ b/builtin/fetch.c\n> > @@ -1857,6 +1857,8 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n> >  \t\t\t\t\t\t    verbosity < 0,\n> >  \t\t\t\t\t\t    max_children);\n> >  \t\targv_array_clear(&options);\n> > +\t\tif (result)\n> > +\t\t\tfprintf(stderr, _(\"Failure during submodule fetch.\\n\"));\n> \n> How does a user find out which submodule had trouble with after\n> seeing this message?  Or is it something you still need to find by\n> scrolling back?\n\nThe handiest way is probably the latter; maybe there is some way to\nachieve the former, but my submodule fu isn't strong enough for me to\nanswer from the top of my head.\n\n> \n> If the latter, I am not sure if there is much point to add a\n> half-way solution like this.  It is a different story if \"fetch\"\n> exits with success status when this happens, but I do not think the\n> \"result\" that is non-zero is being lost before the function returns,\n> so...\n\nI agree, although I do find it irritating that there's no final\nsuccess/failure log line from 'git fetch'. I personally don't run 'echo\n$?' after every step in my Git workflow.\n\nIt's less trivial (a low bar) to try and point out the submodule(s) which\nhad an issue by this point, but I can give it a shot if you are open to\nthe change.\n\n> \n> >  \t}\n> >  \n> >  \tstring_list_clear(&list, 0);\n"},{"id":"389856","messageId":"20200116025948.136479-1-emilyshaffer@google.com","threadId":"52612","inReplyTo":"20200110195533.6416-1-emilyshaffer@google.com","subject":"[PATCH v2] fetch: emphasize failure during submodule fetch","fromName":"Emily Shaffer","fromEmail":"emilyshaffer@google.com","sentAt":"2020-01-16T02:59:48Z","receivedAt":"2020-01-16T03:01:25Z","isPatch":true,"sender":{"key":"nasamuffin@google.com","avatar":"https://avatars.githubusercontent.com/u/1606826?v=4"},"body":"In cases when a submodule fetch fails when there are many submodules, the error\nfrom the lone failing submodule fetch is buried under activity on the other\nsubmodules if more than one fetch fell back on fetch-by-oid. Call out a failure\nlate so the user is aware that something went wrong, and where.\n\nSigned-off-by: Emily Shaffer <emilyshaffer@google.com>\n---\nV1's approach was to show a generic error based on the output status of\nfetch_populated_submodules(); with a long set of submodules, this is\nonly marginally better than showing the error inline. For v2, instead\nwe're gathering a list of submodules which failed during the parallel\nprocessing.\n\nThe contents of stderr at the time fetch_finish() is called is not\navailable to us; 'err' on the input is for providing output only. So,\ngather the submodule name only.\n\n - Emily\n\n submodule.c | 22 ++++++++++++++++++----\n 1 file changed, 18 insertions(+), 4 deletions(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex 9da7181321..13bc9354bc 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -1280,10 +1280,13 @@ struct submodule_parallel_fetch {\n \t/* Pending fetches by OIDs */\n \tstruct fetch_task **oid_fetch_tasks;\n \tint oid_fetch_tasks_nr, oid_fetch_tasks_alloc;\n+\n+\tstruct strbuf submodules_with_errors;\n+\tpthread_mutex_t submodule_errors_mutex;\n };\n #define SPF_INIT {0, ARGV_ARRAY_INIT, NULL, NULL, 0, 0, 0, 0, \\\n \t\t  STRING_LIST_INIT_DUP, \\\n-\t\t  NULL, 0, 0}\n+\t\t  NULL, 0, 0, STRBUF_INIT, PTHREAD_MUTEX_INITIALIZER}\n \n static int get_fetch_recurse_config(const struct submodule *submodule,\n \t\t\t\t    struct submodule_parallel_fetch *spf)\n@@ -1547,7 +1550,10 @@ static int fetch_finish(int retvalue, struct strbuf *err,\n \tstruct string_list_item *it;\n \tstruct oid_array *commits;\n \n-\tif (retvalue)\n+\tif (!task || !task->sub)\n+\t\tBUG(\"callback cookie bogus\");\n+\n+\tif (retvalue) {\n \t\t/*\n \t\t * NEEDSWORK: This indicates that the overall fetch\n \t\t * failed, even though there may be a subsequent fetch\n@@ -1557,8 +1563,11 @@ static int fetch_finish(int retvalue, struct strbuf *err,\n \t\t */\n \t\tspf->result = 1;\n \n-\tif (!task || !task->sub)\n-\t\tBUG(\"callback cookie bogus\");\n+\t\tpthread_mutex_lock(&spf->submodule_errors_mutex);\n+\t\tstrbuf_addf(&spf->submodules_with_errors, \"\\t%s\\n\",\n+\t\t\t    task->sub->name);\n+\t\tpthread_mutex_unlock(&spf->submodule_errors_mutex);\n+\t}\n \n \t/* Is this the second time we process this submodule? */\n \tif (task->commits)\n@@ -1627,6 +1636,11 @@ int fetch_populated_submodules(struct repository *r,\n \t\t\t\t   &spf,\n \t\t\t\t   \"submodule\", \"parallel/fetch\");\n \n+\tif (spf.submodules_with_errors.len > 0)\n+\t\tfprintf(stderr, \"Errors during submodule fetch:\\n%s\",\n+\t\t\tspf.submodules_with_errors.buf);\n+\n+\n \targv_array_clear(&spf.args);\n out:\n \tfree_submodules_oids(&spf.changed_submodule_names);\n-- \n2.25.0.rc1.283.g88dfdc4193-goog\n\n"},{"id":"389906","messageId":"xmqq1rrzi7k1.fsf@gitster-ct.c.googlers.com","threadId":"52612","inReplyTo":"20200116025948.136479-1-emilyshaffer@google.com","subject":"Re: [PATCH v2] fetch: emphasize failure during submodule fetch","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-01-16T18:23:58Z","receivedAt":"2020-01-16T18:24:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Emily Shaffer <emilyshaffer@google.com> writes:\n\n> @@ -1280,10 +1280,13 @@ struct submodule_parallel_fetch {\n>  \t/* Pending fetches by OIDs */\n>  \tstruct fetch_task **oid_fetch_tasks;\n>  \tint oid_fetch_tasks_nr, oid_fetch_tasks_alloc;\n> +\n> +\tstruct strbuf submodules_with_errors;\n> +\tpthread_mutex_t submodule_errors_mutex;\n\nHmph, it is kind of surprising that we need a new mutex for this.\n\nIsn't the task_finish handler, which is what accesses the\nwith_errors field this patch adds, called by pp_collect_finished()\none at a time, is it?\n\nIt seems oid_fetch_tasks[] array is also a shared resource in this\nstructure among the parallel fetch tasks, but there is no protection\nagainst simultaneous access to it.  Am I missing what makes the new\nfield different?  Somewhat puzzled...\n\nOther than that, I think this is a vast improvement relative to the\ninitial round.  I wonder if we want to _(\"i18n/l10n\") the message,\nthough.\n\nThanks.\n\n\n>  #define SPF_INIT {0, ARGV_ARRAY_INIT, NULL, NULL, 0, 0, 0, 0, \\\n>  \t\t  STRING_LIST_INIT_DUP, \\\n> -\t\t  NULL, 0, 0}\n> +\t\t  NULL, 0, 0, STRBUF_INIT, PTHREAD_MUTEX_INITIALIZER}\n>  \n>  static int get_fetch_recurse_config(const struct submodule *submodule,\n>  \t\t\t\t    struct submodule_parallel_fetch *spf)\n> @@ -1547,7 +1550,10 @@ static int fetch_finish(int retvalue, struct strbuf *err,\n>  \tstruct string_list_item *it;\n>  \tstruct oid_array *commits;\n>  \n> -\tif (retvalue)\n> +\tif (!task || !task->sub)\n> +\t\tBUG(\"callback cookie bogus\");\n> +\n> +\tif (retvalue) {\n>  \t\t/*\n>  \t\t * NEEDSWORK: This indicates that the overall fetch\n>  \t\t * failed, even though there may be a subsequent fetch\n> @@ -1557,8 +1563,11 @@ static int fetch_finish(int retvalue, struct strbuf *err,\n>  \t\t */\n>  \t\tspf->result = 1;\n>  \n> -\tif (!task || !task->sub)\n> -\t\tBUG(\"callback cookie bogus\");\n> +\t\tpthread_mutex_lock(&spf->submodule_errors_mutex);\n> +\t\tstrbuf_addf(&spf->submodules_with_errors, \"\\t%s\\n\",\n> +\t\t\t    task->sub->name);\n> +\t\tpthread_mutex_unlock(&spf->submodule_errors_mutex);\n> +\t}\n>  \n>  \t/* Is this the second time we process this submodule? */\n>  \tif (task->commits)\n> @@ -1627,6 +1636,11 @@ int fetch_populated_submodules(struct repository *r,\n>  \t\t\t\t   &spf,\n>  \t\t\t\t   \"submodule\", \"parallel/fetch\");\n>  \n> +\tif (spf.submodules_with_errors.len > 0)\n> +\t\tfprintf(stderr, \"Errors during submodule fetch:\\n%s\",\n> +\t\t\tspf.submodules_with_errors.buf);\n> +\n> +\n>  \targv_array_clear(&spf.args);\n>  out:\n>  \tfree_submodules_oids(&spf.changed_submodule_names);\n"},{"id":"389942","messageId":"20200116215526.GK181522@google.com","threadId":"52612","inReplyTo":"xmqq1rrzi7k1.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2] fetch: emphasize failure during submodule fetch","fromName":"Emily Shaffer","fromEmail":"emilyshaffer@google.com","sentAt":"2020-01-16T21:55:26Z","receivedAt":"2020-01-16T21:55:34Z","isPatch":true,"sender":{"key":"nasamuffin@google.com","avatar":"https://avatars.githubusercontent.com/u/1606826?v=4"},"body":"On Thu, Jan 16, 2020 at 10:23:58AM -0800, Junio C Hamano wrote:\n> Emily Shaffer <emilyshaffer@google.com> writes:\n> \n> > @@ -1280,10 +1280,13 @@ struct submodule_parallel_fetch {\n> >  \t/* Pending fetches by OIDs */\n> >  \tstruct fetch_task **oid_fetch_tasks;\n> >  \tint oid_fetch_tasks_nr, oid_fetch_tasks_alloc;\n> > +\n> > +\tstruct strbuf submodules_with_errors;\n> > +\tpthread_mutex_t submodule_errors_mutex;\n> \n> Hmph, it is kind of surprising that we need a new mutex for this.\n> \n> Isn't the task_finish handler, which is what accesses the\n> with_errors field this patch adds, called by pp_collect_finished()\n> one at a time, is it?\n\nHm. It is called by pp_collect_finished() one at a time, but while other\nprocesses may still be running. So I guess that is OK - spf might still\nbe read by other tasks but this field of it won't be touched by anybody\nsimultaneously. Ok, I'm convinced.\n\n> It seems oid_fetch_tasks[] array is also a shared resource in this\n> structure among the parallel fetch tasks, but there is no protection\n> against simultaneous access to it.  Am I missing what makes the new\n> field different?  Somewhat puzzled...\n\nI think it's similar. As I understand it, it looks something like this:\n\n  loop forever:\n    can i start a new process?\n      get_next_task cb (blocking)\n      start work cb (nonblocking unless it failed to start)\n    process stderr in/out once (blocking)\n    is anybody done? (blocking)\n      task_finished cb (blocking) <- My change is in here\n        did fetch by ref fail? (blocking)\n          put fetch by OID onto the process list (blocking)\n    is everybody done?\n      break\n\nThat is, everything but the work unit itself is blocking and runs in a\nsingle threaded infinite loop. So since oid_fetch_tasks is read in\nget_next_task callback and modified in the task_finished callback, those\nareas don't need thread protection.\n\nThanks for poking me to think it through better. I'll remove the mutex\nand include a short note about why it's not needed in the commit message.\nI suppose if I wanted to try and catch more precise error information\nduring the actual work, then I would need it, but I'm not sure it's\nnecessary or trivial because of how the stdout/stderr is handled for\ncohesive printing.\n\n> Other than that, I think this is a vast improvement relative to the\n> initial round.  I wonder if we want to _(\"i18n/l10n\") the message,\n> though.\n\nSure, sorry to have missed it.\n\nThanks for the thoughtful review. Will send a reroll in a moment.\n\n - Emily\n\n> \n> \n> >  #define SPF_INIT {0, ARGV_ARRAY_INIT, NULL, NULL, 0, 0, 0, 0, \\\n> >  \t\t  STRING_LIST_INIT_DUP, \\\n> > -\t\t  NULL, 0, 0}\n> > +\t\t  NULL, 0, 0, STRBUF_INIT, PTHREAD_MUTEX_INITIALIZER}\n> >  \n> >  static int get_fetch_recurse_config(const struct submodule *submodule,\n> >  \t\t\t\t    struct submodule_parallel_fetch *spf)\n> > @@ -1547,7 +1550,10 @@ static int fetch_finish(int retvalue, struct strbuf *err,\n> >  \tstruct string_list_item *it;\n> >  \tstruct oid_array *commits;\n> >  \n> > -\tif (retvalue)\n> > +\tif (!task || !task->sub)\n> > +\t\tBUG(\"callback cookie bogus\");\n> > +\n> > +\tif (retvalue) {\n> >  \t\t/*\n> >  \t\t * NEEDSWORK: This indicates that the overall fetch\n> >  \t\t * failed, even though there may be a subsequent fetch\n> > @@ -1557,8 +1563,11 @@ static int fetch_finish(int retvalue, struct strbuf *err,\n> >  \t\t */\n> >  \t\tspf->result = 1;\n> >  \n> > -\tif (!task || !task->sub)\n> > -\t\tBUG(\"callback cookie bogus\");\n> > +\t\tpthread_mutex_lock(&spf->submodule_errors_mutex);\n> > +\t\tstrbuf_addf(&spf->submodules_with_errors, \"\\t%s\\n\",\n> > +\t\t\t    task->sub->name);\n> > +\t\tpthread_mutex_unlock(&spf->submodule_errors_mutex);\n> > +\t}\n> >  \n> >  \t/* Is this the second time we process this submodule? */\n> >  \tif (task->commits)\n> > @@ -1627,6 +1636,11 @@ int fetch_populated_submodules(struct repository *r,\n> >  \t\t\t\t   &spf,\n> >  \t\t\t\t   \"submodule\", \"parallel/fetch\");\n> >  \n> > +\tif (spf.submodules_with_errors.len > 0)\n> > +\t\tfprintf(stderr, \"Errors during submodule fetch:\\n%s\",\n> > +\t\t\tspf.submodules_with_errors.buf);\n> > +\n> > +\n> >  \targv_array_clear(&spf.args);\n> >  out:\n> >  \tfree_submodules_oids(&spf.changed_submodule_names);\n"},{"id":"389943","messageId":"20200116220437.GL181522@google.com","threadId":"52612","inReplyTo":"20200116215526.GK181522@google.com","subject":"Re: [PATCH v2] fetch: emphasize failure during submodule fetch","fromName":"Emily Shaffer","fromEmail":"emilyshaffer@google.com","sentAt":"2020-01-16T22:04:37Z","receivedAt":"2020-01-16T22:04:44Z","isPatch":true,"sender":{"key":"nasamuffin@google.com","avatar":"https://avatars.githubusercontent.com/u/1606826?v=4"},"body":"On Thu, Jan 16, 2020 at 01:55:26PM -0800, Emily Shaffer wrote:\n> On Thu, Jan 16, 2020 at 10:23:58AM -0800, Junio C Hamano wrote:\n> > Emily Shaffer <emilyshaffer@google.com> writes:\n> > \n> > > @@ -1280,10 +1280,13 @@ struct submodule_parallel_fetch {\n> > >  \t/* Pending fetches by OIDs */\n> > >  \tstruct fetch_task **oid_fetch_tasks;\n> > >  \tint oid_fetch_tasks_nr, oid_fetch_tasks_alloc;\n> > > +\n> > > +\tstruct strbuf submodules_with_errors;\n> > > +\tpthread_mutex_t submodule_errors_mutex;\n> > \n> > Hmph, it is kind of surprising that we need a new mutex for this.\n> > \n> > Isn't the task_finish handler, which is what accesses the\n> > with_errors field this patch adds, called by pp_collect_finished()\n> > one at a time, is it?\n> \n> Hm. It is called by pp_collect_finished() one at a time, but while other\n> processes may still be running. So I guess that is OK - spf might still\n> be read by other tasks but this field of it won't be touched by anybody\n> simultaneously. Ok, I'm convinced.\n> \n> > It seems oid_fetch_tasks[] array is also a shared resource in this\n> > structure among the parallel fetch tasks, but there is no protection\n> > against simultaneous access to it.  Am I missing what makes the new\n> > field different?  Somewhat puzzled...\n> \n> I think it's similar. As I understand it, it looks something like this:\n> \n>   loop forever:\n>     can i start a new process?\n>       get_next_task cb (blocking)\n>       start work cb (nonblocking unless it failed to start)\n>     process stderr in/out once (blocking)\n>     is anybody done? (blocking)\n>       task_finished cb (blocking) <- My change is in here\n>         did fetch by ref fail? (blocking)\n>           put fetch by OID onto the process list (blocking)\n>     is everybody done?\n>       break\n\nAh, as I look deeper I realize that it's a child process, not a thread,\nso this code becomes even simpler to understand. I think then I don't\nneed to worry about thread safety at all here.\n\n - Emily\n"},{"id":"389946","messageId":"20200116222012.162642-1-emilyshaffer@google.com","threadId":"52612","inReplyTo":"20200116025948.136479-1-emilyshaffer@google.com","subject":"[PATCH v3] fetch: emphasize failure during submodule fetch","fromName":"Emily Shaffer","fromEmail":"emilyshaffer@google.com","sentAt":"2020-01-16T22:20:12Z","receivedAt":"2020-01-16T22:20:19Z","isPatch":true,"sender":{"key":"nasamuffin@google.com","avatar":"https://avatars.githubusercontent.com/u/1606826?v=4"},"body":"In cases when a submodule fetch fails when there are many submodules, the error\nfrom the lone failing submodule fetch is buried under activity on the other\nsubmodules if more than one fetch fell back on fetch-by-oid. Call out a failure\nlate so the user is aware that something went wrong, and where.\n\nBecause fetch_finish() is only called synchronously by\nrun_processes_parallel, mutexing is not required around\nsubmodules_with_errors.\n\nSigned-off-by: Emily Shaffer <emilyshaffer@google.com>\n---\nSince v2, removed mutexing as run_processes_parallel() creates\nsubprocesses, not threads, and is in fact synchronous. Also added\ntranslation marker for the error message.\n\n - Emily\n\n submodule.c | 19 +++++++++++++++----\n 1 file changed, 15 insertions(+), 4 deletions(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex 9da7181321..ee74acee27 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -1280,10 +1280,12 @@ struct submodule_parallel_fetch {\n \t/* Pending fetches by OIDs */\n \tstruct fetch_task **oid_fetch_tasks;\n \tint oid_fetch_tasks_nr, oid_fetch_tasks_alloc;\n+\n+\tstruct strbuf submodules_with_errors;\n };\n #define SPF_INIT {0, ARGV_ARRAY_INIT, NULL, NULL, 0, 0, 0, 0, \\\n \t\t  STRING_LIST_INIT_DUP, \\\n-\t\t  NULL, 0, 0}\n+\t\t  NULL, 0, 0, STRBUF_INIT}\n \n static int get_fetch_recurse_config(const struct submodule *submodule,\n \t\t\t\t    struct submodule_parallel_fetch *spf)\n@@ -1547,7 +1549,10 @@ static int fetch_finish(int retvalue, struct strbuf *err,\n \tstruct string_list_item *it;\n \tstruct oid_array *commits;\n \n-\tif (retvalue)\n+\tif (!task || !task->sub)\n+\t\tBUG(\"callback cookie bogus\");\n+\n+\tif (retvalue) {\n \t\t/*\n \t\t * NEEDSWORK: This indicates that the overall fetch\n \t\t * failed, even though there may be a subsequent fetch\n@@ -1557,8 +1562,9 @@ static int fetch_finish(int retvalue, struct strbuf *err,\n \t\t */\n \t\tspf->result = 1;\n \n-\tif (!task || !task->sub)\n-\t\tBUG(\"callback cookie bogus\");\n+\t\tstrbuf_addf(&spf->submodules_with_errors, \"\\t%s\\n\",\n+\t\t\t    task->sub->name);\n+\t}\n \n \t/* Is this the second time we process this submodule? */\n \tif (task->commits)\n@@ -1627,6 +1633,11 @@ int fetch_populated_submodules(struct repository *r,\n \t\t\t\t   &spf,\n \t\t\t\t   \"submodule\", \"parallel/fetch\");\n \n+\tif (spf.submodules_with_errors.len > 0)\n+\t\tfprintf(stderr, _(\"Errors during submodule fetch:\\n%s\"),\n+\t\t\tspf.submodules_with_errors.buf);\n+\n+\n \targv_array_clear(&spf.args);\n out:\n \tfree_submodules_oids(&spf.changed_submodule_names);\n-- \n2.25.0.rc1.283.g88dfdc4193-goog\n\n"},{"id":"389966","messageId":"nycvar.QRO.7.76.6.2001171152410.46@tvgsbejvaqbjf.bet","threadId":"52612","inReplyTo":"20200116222012.162642-1-emilyshaffer@google.com","subject":"Re: [PATCH v3] fetch: emphasize failure during submodule fetch","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-01-17T10:54:29Z","receivedAt":"2020-01-17T10:54:43Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Emily,\n\nOn Thu, 16 Jan 2020, Emily Shaffer wrote:\n\n> In cases when a submodule fetch fails when there are many submodules, the error\n> from the lone failing submodule fetch is buried under activity on the other\n> submodules if more than one fetch fell back on fetch-by-oid. Call out a failure\n> late so the user is aware that something went wrong, and where.\n>\n> Because fetch_finish() is only called synchronously by\n> run_processes_parallel, mutexing is not required around\n> submodules_with_errors.\n>\n> Signed-off-by: Emily Shaffer <emilyshaffer@google.com>\n> ---\n> Since v2, removed mutexing as run_processes_parallel() creates\n> subprocesses, not threads, and is in fact synchronous. Also added\n> translation marker for the error message.\n\nExcellent. For Git for Windows' ever-green branch based on `pu`, I had to\nintroduce this:\n\n-- snip --\nSubject: [PATCH] fixup??? fetch: emphasize failure during submodule fetch\n\nThis would be the first user of `PTHREAD_MUTEX_INITIALIZER`, but as we\nhave a hard time to emulate this with critical sections in Windows,\nlet's not do that.\n\nSigned-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n---\n submodule.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex a65db54cece..ec54f4db2cd 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -1286,7 +1286,7 @@ struct submodule_parallel_fetch {\n };\n #define SPF_INIT {0, ARGV_ARRAY_INIT, NULL, NULL, 0, 0, 0, 0, \\\n \t\t  STRING_LIST_INIT_DUP, \\\n-\t\t  NULL, 0, 0, STRBUF_INIT, PTHREAD_MUTEX_INITIALIZER}\n+\t\t  NULL, 0, 0, STRBUF_INIT}\n\n static int get_fetch_recurse_config(const struct submodule *submodule,\n \t\t\t\t    struct submodule_parallel_fetch *spf)\n@@ -1614,6 +1614,7 @@ int fetch_populated_submodules(struct repository *r,\n \tspf.default_option = default_option;\n \tspf.quiet = quiet;\n \tspf.prefix = prefix;\n+\tpthread_mutex_init(&spf.submodule_errors_mutex, NULL);\n\n \tif (!r->worktree)\n \t\tgoto out;\n-- snap --\n\nI look forward to v3 hitting `pu` and making this change obsolete.\n\nThanks,\nDscho\n\n>\n>  - Emily\n>\n>  submodule.c | 19 +++++++++++++++----\n>  1 file changed, 15 insertions(+), 4 deletions(-)\n>\n> diff --git a/submodule.c b/submodule.c\n> index 9da7181321..ee74acee27 100644\n> --- a/submodule.c\n> +++ b/submodule.c\n> @@ -1280,10 +1280,12 @@ struct submodule_parallel_fetch {\n>  \t/* Pending fetches by OIDs */\n>  \tstruct fetch_task **oid_fetch_tasks;\n>  \tint oid_fetch_tasks_nr, oid_fetch_tasks_alloc;\n> +\n> +\tstruct strbuf submodules_with_errors;\n>  };\n>  #define SPF_INIT {0, ARGV_ARRAY_INIT, NULL, NULL, 0, 0, 0, 0, \\\n>  \t\t  STRING_LIST_INIT_DUP, \\\n> -\t\t  NULL, 0, 0}\n> +\t\t  NULL, 0, 0, STRBUF_INIT}\n>\n>  static int get_fetch_recurse_config(const struct submodule *submodule,\n>  \t\t\t\t    struct submodule_parallel_fetch *spf)\n> @@ -1547,7 +1549,10 @@ static int fetch_finish(int retvalue, struct strbuf *err,\n>  \tstruct string_list_item *it;\n>  \tstruct oid_array *commits;\n>\n> -\tif (retvalue)\n> +\tif (!task || !task->sub)\n> +\t\tBUG(\"callback cookie bogus\");\n> +\n> +\tif (retvalue) {\n>  \t\t/*\n>  \t\t * NEEDSWORK: This indicates that the overall fetch\n>  \t\t * failed, even though there may be a subsequent fetch\n> @@ -1557,8 +1562,9 @@ static int fetch_finish(int retvalue, struct strbuf *err,\n>  \t\t */\n>  \t\tspf->result = 1;\n>\n> -\tif (!task || !task->sub)\n> -\t\tBUG(\"callback cookie bogus\");\n> +\t\tstrbuf_addf(&spf->submodules_with_errors, \"\\t%s\\n\",\n> +\t\t\t    task->sub->name);\n> +\t}\n>\n>  \t/* Is this the second time we process this submodule? */\n>  \tif (task->commits)\n> @@ -1627,6 +1633,11 @@ int fetch_populated_submodules(struct repository *r,\n>  \t\t\t\t   &spf,\n>  \t\t\t\t   \"submodule\", \"parallel/fetch\");\n>\n> +\tif (spf.submodules_with_errors.len > 0)\n> +\t\tfprintf(stderr, _(\"Errors during submodule fetch:\\n%s\"),\n> +\t\t\tspf.submodules_with_errors.buf);\n> +\n> +\n>  \targv_array_clear(&spf.args);\n>  out:\n>  \tfree_submodules_oids(&spf.changed_submodule_names);\n> --\n> 2.25.0.rc1.283.g88dfdc4193-goog\n>\n>\n"}]}