{"thread":{"id":"42566","subject":"[PATCH 0/2] Dealing with a lot of submodules","startedAt":"2016-06-16T02:19:50Z","lastAt":"2016-06-16T02:19:50Z","messageCount":8,"participants":["Stefan Beller","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"288835","messageId":"20160609190637.21177-1-sbeller@google.com","threadId":"42566","inReplyTo":null,"subject":"[PATCH 0/2] Dealing with a lot of submodules","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-06-09T19:06:35Z","receivedAt":"2016-06-16T02:19:50Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"We have a test repo with about 500 submodules, and I noticed some problems\nwhen cloning this repo.  This is a series that helps dealing with that repo\nin two ways:\n\n* When having 500 submodules, you have to perform 500 clones. This makes an\n  ephemeral error 500 times more likely. To cope with such errors, just try\n  again after all other clones have finished.\n\n* If a recursive clone fails for another reason (in our case a missing\n  .gitmodules file), we want to keep going to finish the clone, instead of\n  failing.\n  \nThanks,\nStefan\n\nStefan Beller (2):\n  submodule--helper: initial clone learns retry logic\n  submodule update: continue when a clone fails\n\n builtin/submodule--helper.c | 44 ++++++++++++++++++++++++++++++++++++--------\n git-submodule.sh            |  2 +-\n 2 files changed, 37 insertions(+), 9 deletions(-)\n\n-- \n2.9.0.rc2.368.gdadd65c\n"},{"id":"288836","messageId":"20160609190637.21177-2-sbeller@google.com","threadId":"42566","inReplyTo":"20160609190637.21177-1-sbeller@google.com","subject":"[PATCH 1/2] submodule--helper: initial clone learns retry logic","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-06-09T19:06:36Z","receivedAt":"2016-06-16T02:19:50Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Each submodule that is attempted to be cloned, will be retried once in\ncase of failure after all other submodules were cloned. This helps to\nmitigate ephemeral server failures and increases chances of a reliable\nclone of a repo with hundreds of submodules immensely.\n\nThe choice of the priority queue is a bit miss-leading as we use it as a\nstandard queue, but the priority queue offers the best API to use.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n builtin/submodule--helper.c | 40 +++++++++++++++++++++++++++++++++-------\n 1 file changed, 33 insertions(+), 7 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex c7deb55..efb6759 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -12,6 +12,7 @@\n #include \"remote.h\"\n #include \"refs.h\"\n #include \"connect.h\"\n+#include \"prio-queue.h\"\n \n static char *get_default_remote(void)\n {\n@@ -568,6 +569,12 @@ static int module_clone(int argc, const char **argv, const char *prefix)\n \treturn 0;\n }\n \n+static int compare_ce(const void *one, const void *two, void *cb_data)\n+{\n+\tconst struct cache_entry *ce_one = one, *ce_two = two;\n+\treturn ce_two - ce_one;\n+}\n+\n struct submodule_update_clone {\n \t/* index into 'list', the list of submodules to look into for cloning */\n \tint current;\n@@ -590,10 +597,13 @@ struct submodule_update_clone {\n \n \t/* If we want to stop as fast as possible and return an error */\n \tunsigned quickstop : 1;\n+\n+\t/* Queue of failed clones, containing the cache_entry */\n+\tstruct prio_queue failed_queue;\n };\n #define SUBMODULE_UPDATE_CLONE_INIT {0, MODULE_LIST_INIT, 0, \\\n \tSUBMODULE_UPDATE_STRATEGY_INIT, 0, -1, NULL, NULL, NULL, NULL, \\\n-\tSTRING_LIST_INIT_DUP, 0}\n+\tSTRING_LIST_INIT_DUP, 0, { compare_ce }}\n \n \n static void next_submodule_warn_missing(struct submodule_update_clone *suc,\n@@ -718,23 +728,36 @@ cleanup:\n static int update_clone_get_next_task(struct child_process *child,\n \t\t\t\t      struct strbuf *err,\n \t\t\t\t      void *suc_cb,\n-\t\t\t\t      void **void_task_cb)\n+\t\t\t\t      void **ce_task_cb)\n {\n \tstruct submodule_update_clone *suc = suc_cb;\n+\tconst struct cache_entry *ce;\n \n \tfor (; suc->current < suc->list.nr; suc->current++) {\n-\t\tconst struct cache_entry *ce = suc->list.entries[suc->current];\n+\t\tce = suc->list.entries[suc->current];\n \t\tif (prepare_to_clone_next_submodule(ce, child, suc, err)) {\n+\t\t\t*ce_task_cb = (struct cache_entry *) ce;\n \t\t\tsuc->current++;\n \t\t\treturn 1;\n \t\t}\n \t}\n+\t/*\n+\t * The loop above tried cloning each submodule once,\n+\t * now try the stragglers again.\n+\t */\n+\tce = (struct cache_entry *) prio_queue_get(&suc->failed_queue);\n+\tif (ce) {\n+\t\tif (!prepare_to_clone_next_submodule(ce, child, suc, err))\n+\t\t\tdie(\"BUG: ce was a submodule before?\");\n+\t\treturn 1;\n+\t}\n+\n \treturn 0;\n }\n \n static int update_clone_start_failure(struct strbuf *err,\n \t\t\t\t      void *suc_cb,\n-\t\t\t\t      void *void_task_cb)\n+\t\t\t\t      void *ce_task_cb)\n {\n \tstruct submodule_update_clone *suc = suc_cb;\n \tsuc->quickstop = 1;\n@@ -744,15 +767,18 @@ static int update_clone_start_failure(struct strbuf *err,\n static int update_clone_task_finished(int result,\n \t\t\t\t      struct strbuf *err,\n \t\t\t\t      void *suc_cb,\n-\t\t\t\t      void *void_task_cb)\n+\t\t\t\t      void *ce_task_cb)\n {\n \tstruct submodule_update_clone *suc = suc_cb;\n+\tstruct cache_entry *ce = ce_task_cb;\n \n \tif (!result)\n \t\treturn 0;\n \n-\tsuc->quickstop = 1;\n-\treturn 1;\n+\tstrbuf_addf(err, _(\"Failed to clone '%s'. Scheduled for retry\"),\n+\t\t\t   ce->name);\n+\tprio_queue_put(&suc->failed_queue, ce);\n+\treturn 0;\n }\n \n static int update_clone(int argc, const char **argv, const char *prefix)\n-- \n2.9.0.rc2.368.gdadd65c\n"},{"id":"288837","messageId":"20160609190637.21177-3-sbeller@google.com","threadId":"42566","inReplyTo":"20160609190637.21177-1-sbeller@google.com","subject":"[PATCH 2/2] submodule update: continue when a clone fails","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-06-09T19:06:37Z","receivedAt":"2016-06-16T02:19:50Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"In 15ffb7cde48 (2011-06-13, submodule update: continue when a checkout\nfails), we reasoned it is ok to continue, when there is not much of\na mental burden by the failure. If a recursive submodule fails to clone\nbecause a .gitmodules file is broken (e.g. :\nfatal: No url found for submodule path 'foo/bar' in .gitmodules\nFailed to recurse into submodule path 'foo', signaled by exit code 128),\nthis is one of the cases where the user is not expected to have much of\na burden afterwards, so we can also continue in that case.\n\nThis means we only want to stop for updating submodules in case of rebase,\nmerge or custom update command failures, which are all signaled with\nexit code 2.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n git-submodule.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex b39ac10..7c62b53 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -705,7 +705,7 @@ cmd_update()\n \t\t\tif test $res -gt 0\n \t\t\tthen\n \t\t\t\tdie_msg=\"$(eval_gettext \"Failed to recurse into submodule path '\\$displaypath'\")\"\n-\t\t\t\tif test $res -eq 1\n+\t\t\t\tif test $res -ne 2\n \t\t\t\tthen\n \t\t\t\t\terr=\"${err};$die_msg\"\n \t\t\t\t\tcontinue\n-- \n2.9.0.rc2.368.gdadd65c\n"},{"id":"288839","messageId":"xmqqy46egofw.fsf@gitster.mtv.corp.google.com","threadId":"42566","inReplyTo":"20160609190637.21177-2-sbeller@google.com","subject":"Re: [PATCH 1/2] submodule--helper: initial clone learns retry logic","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-06-09T19:19:31Z","receivedAt":"2016-06-16T02:19:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> +static int compare_ce(const void *one, const void *two, void *cb_data)\n> +{\n> +\tconst struct cache_entry *ce_one = one, *ce_two = two;\n> +\treturn ce_two - ce_one;\n> +}\n\nThis would work in practice, but I suspect that this is not ANSI-C\nkosher; as address comparison for ordering (not equality) is\nundefined if two pointers are not pointing into the same array or\ninto the same struct's fields.\n\nI think we have one or two other instances of such fishy pointer\ncomparison already in the codebase, so it is not a show-stopper, but\nit would be better if this can be avoided and be replaced with\nsomething that I do not have to raise eyebrows at ;-)\n"},{"id":"288842","messageId":"CAGZ79kbpurbwAB4H5_JPMsaGHmbeZA=EWjMOfQGB6apLhEXVeA@mail.gmail.com","threadId":"42566","inReplyTo":"xmqqy46egofw.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 1/2] submodule--helper: initial clone learns retry logic","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-06-09T19:47:40Z","receivedAt":"2016-06-16T02:19:50Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Jun 9, 2016 at 12:19 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Stefan Beller <sbeller@google.com> writes:\n>\n>> +static int compare_ce(const void *one, const void *two, void *cb_data)\n>> +{\n>> +     const struct cache_entry *ce_one = one, *ce_two = two;\n>> +     return ce_two - ce_one;\n>> +}\n>\n> This would work in practice, but I suspect that this is not ANSI-C\n> kosher; as address comparison for ordering (not equality) is\n> undefined if two pointers are not pointing into the same array or\n> into the same struct's fields.\n\nUh :(\n\nI thought about that for a while as I was about to send a 'simplification'\npatch for commit.c:compare_commits_by_{author,commit}_date\nThere it made sense to keep it the way it is because\n    sizeof(date) > sizeof(return value)\n\nAt the time of writing it made sense for this to be a subtractions,\nbut I think we want to make it\n\n    if (ce_one < ce_two)\n        return 1;\n    else if (ce_one > ce_two)\n        return -1;\n    else\n        return 0;\n\ninstead. But that is still unspecified, so we rather go with\n\nstatic int compare_ce(const void *one, const void *two, void *cb_data)\n{\n    const struct cache_entry *ce_one = one, *ce_two = two;\n    return strcmp(ce_one->name, ce_two->name);\n}\n\nWhen reading the prio queue code correctly, we can be sure there\nis no NULL passed to either of one or two.\n\n>\n> I think we have one or two other instances of such fishy pointer\n> comparison already in the codebase, so it is not a show-stopper, but\n> it would be better if this can be avoided and be replaced with\n> something that I do not have to raise eyebrows at ;-)\n"},{"id":"288843","messageId":"xmqqtwh2gmlv.fsf@gitster.mtv.corp.google.com","threadId":"42566","inReplyTo":"CAGZ79kbpurbwAB4H5_JPMsaGHmbeZA=EWjMOfQGB6apLhEXVeA@mail.gmail.com","subject":"Re: [PATCH 1/2] submodule--helper: initial clone learns retry logic","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-06-09T19:59:08Z","receivedAt":"2016-06-16T02:19:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> instead. But that is still unspecified, so we rather go with\n>\n> static int compare_ce(const void *one, const void *two, void *cb_data)\n> {\n>     const struct cache_entry *ce_one = one, *ce_two = two;\n>     return strcmp(ce_one->name, ce_two->name);\n> }\n\nAs I said below, I do not think it is worth addressing by making the\ncode's behaviour on real systems worse.  As long as what you have as\nthe key into priority queue is a pointer to cache_entry, you cannot\nmake it better from that point of view.\n\n>> I think we have one or two other instances of such fishy pointer\n>> comparison already in the codebase, so it is not a show-stopper, but\n>> it would be better if this can be avoided and be replaced with\n>> something that I do not have to raise eyebrows at ;-)\n\nIf you are using priority queue, it probably would make more sense\nto use the \"priority\" part properly---the paths whose ce happens to\nbe lower in the address space are not inherently more important and\ndeserve to be processed sooner.  From \"let's retry failed ones after\nfinishing\", I expected that the queue was either fifo (simplest), or\na prio queue whose priority function would give lower priority for\nentries with least number of previous attempts (more involved but\nprobably more fair).\n\nSo a proper \"fix\" probably is either (1) not to use prio queue as\nyou are not doing anything with priority anyway, or (2) use\nsomething other than ce itself as entries in prio queue, so that the\npriority function can be computation that is more meaningful than\n\"happens to sit in the lower part of the address space\".\n"},{"id":"288845","messageId":"xmqqlh2egko4.fsf@gitster.mtv.corp.google.com","threadId":"42566","inReplyTo":"xmqqtwh2gmlv.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 1/2] submodule--helper: initial clone learns retry logic","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-06-09T20:40:59Z","receivedAt":"2016-06-16T02:19:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Stefan Beller <sbeller@google.com> writes:\n>\n>> instead. But that is still unspecified, so we rather go with\n>>\n>> static int compare_ce(const void *one, const void *two, void *cb_data)\n>> {\n>>     const struct cache_entry *ce_one = one, *ce_two = two;\n>>     return strcmp(ce_one->name, ce_two->name);\n>> }\n>\n> As I said below, I do not think it is worth addressing by making the\n> code's behaviour on real systems worse.  As long as what you have as\n> the key into priority queue is a pointer to cache_entry, you cannot\n> make it better from that point of view.\n\n... because having to strcmp() their names would be much more\nexpensive than the pointer comparison.\n\nHowever, I think you could use a pointer into a single array as\nan element of prio_queue.  I notice here:\n\n \tfor (; suc->current < suc->list.nr; suc->current++) {\n-\t\tconst struct cache_entry *ce = suc->list.entries[suc->current];\n+\t\tce = suc->list.entries[suc->current];\n \t\tif (prepare_to_clone_next_submodule(ce, child, suc, err)) {\n+\t\t\t*ce_task_cb = (struct cache_entry *) ce;\n \t\t\tsuc->current++;\n \t\t\treturn 1;\n \t\t}\n \t}\n \nthat list.entries[] can serve as such an array.  If you pass around\nthe pointer to its element instead, i.e.\n\n-\t\tce = suc->list.entries[suc->current];\n+\t\tceP = &suc->list.entries[suc->current];\n- \t\tif (prepare_to_clone_next_submodule(ce, child, suc, err)) {\n+ \t\tif (prepare_to_clone_next_submodule(*ceP, child, suc, err)) {\n+\t\t\t*ce_task_cb = (struct cache_entry *) ce;\n-\t\t\t*ce_task_cb = ceP;\n\t\t...\n\t}\n\t/*\n\t * The loop above tried cloning each submodule once,\n\t * now try the stragglers again.\n\t */\n-\tce = (struct cache_entry *) prio_queue_get(&suc->failed_queue);\n+\tceP = (struct cache_entry **) prio_queue_get(&suc->failed_queue);\n\nthen the elements you are pushing into prio-queue would not be ce\n(pointer to a cache entry), but would be a pointer of an array that\nholds many pointers to cache entries, so it becomes kosher to\ncompare them for ordering.\n\nThat would probably not add too much overhead at runtime; it may\nhave to involve a bit of code restructuring, so I do not know if it\nis worth it, though.\n"},{"id":"288861","messageId":"CAGZ79ka1=nH5iYB_jbNm8gsVYcMiZHVnKg49kEUuEMiL9PaqXQ@mail.gmail.com","threadId":"42566","inReplyTo":"xmqqlh2egko4.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 1/2] submodule--helper: initial clone learns retry logic","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-06-09T23:38:27Z","receivedAt":"2016-06-16T02:19:50Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Jun 9, 2016 at 1:40 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Junio C Hamano <gitster@pobox.com> writes:\n>\n>> Stefan Beller <sbeller@google.com> writes:\n>>\n>>> instead. But that is still unspecified, so we rather go with\n>>>\n>>> static int compare_ce(const void *one, const void *two, void *cb_data)\n>>> {\n>>>     const struct cache_entry *ce_one = one, *ce_two = two;\n>>>     return strcmp(ce_one->name, ce_two->name);\n>>> }\n>>\n>> As I said below, I do not think it is worth addressing by making the\n>> code's behaviour on real systems worse.  As long as what you have as\n>> the key into priority queue is a pointer to cache_entry, you cannot\n>> make it better from that point of view.\n>\n> ... because having to strcmp() their names would be much more\n> expensive than the pointer comparison.\n>\n> However, I think you could use a pointer into a single array as\n> an element of prio_queue.  I notice here:\n>\n>         for (; suc->current < suc->list.nr; suc->current++) {\n> -               const struct cache_entry *ce = suc->list.entries[suc->current];\n> +               ce = suc->list.entries[suc->current];\n>                 if (prepare_to_clone_next_submodule(ce, child, suc, err)) {\n> +                       *ce_task_cb = (struct cache_entry *) ce;\n>                         suc->current++;\n>                         return 1;\n>                 }\n>         }\n>\n> that list.entries[] can serve as such an array.  If you pass around\n> the pointer to its element instead, i.e.\n>\n> -               ce = suc->list.entries[suc->current];\n> +               ceP = &suc->list.entries[suc->current];\n> -               if (prepare_to_clone_next_submodule(ce, child, suc, err)) {\n> +               if (prepare_to_clone_next_submodule(*ceP, child, suc, err)) {\n> +                       *ce_task_cb = (struct cache_entry *) ce;\n> -                       *ce_task_cb = ceP;\n>                 ...\n>         }\n>         /*\n>          * The loop above tried cloning each submodule once,\n>          * now try the stragglers again.\n>          */\n> -       ce = (struct cache_entry *) prio_queue_get(&suc->failed_queue);\n> +       ceP = (struct cache_entry **) prio_queue_get(&suc->failed_queue);\n>\n> then the elements you are pushing into prio-queue would not be ce\n> (pointer to a cache entry), but would be a pointer of an array that\n> holds many pointers to cache entries, so it becomes kosher to\n> compare them for ordering.\n>\n> That would probably not add too much overhead at runtime; it may\n> have to involve a bit of code restructuring, so I do not know if it\n> is worth it, though.\n>\n\nI realized the patch above was bogus for another reason:\nIn update_clone_task_finished we never distinguish between first-failing\nsubmodules and repeated fails, such that we may end up in an infinite loop.\n\nSo I think the easiest way forward would be if we would pass around the index\nand then we can properly determine if we tried that already. (Though casting\npointer to int is not possible, so we'll pass around a pointer to an int)\n"}]}