{"thread":{"id":"42856","subject":"[PATCH 0/12] push progress reporting and keepalives","startedAt":"2016-07-15T10:25:15Z","lastAt":"2016-07-20T13:28:18Z","messageCount":26,"participants":["Jeff King","Stefan Beller","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":12},"messages":[{"id":"291513","messageId":"20160715102506.GA23164@sigill.intra.peff.net","threadId":"42856","inReplyTo":null,"subject":"[PATCH 0/12] push progress reporting and keepalives","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-07-15T10:25:06Z","receivedAt":"2016-07-15T10:25:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"If you push a large number of objects, the server side may have to chew\nCPU for a long time processing the input (delta resolution, connectivity\ncheck, and whatever any hooks choose to do). During this time, you get\nno feedback that anything is happening, unless the hooks feel like\nwriting something to stderr.  For a repository the size of linux.git, a\nfull push from scratch can take several minutes.\n\nThis is annoying and confusing to the user, who wonders if the\nconnection has hung. But it can also cause problems on systems that have\nother timeouts (e.g., firewalls dropping TCP sessions, or web proxies\ndropping requests that produce no response within a certain time).\n\nThis patch series adds two new features:\n\n 1. Progress reporting for the CPU-intensive parts of receive-pack.\n\n 2. A keepalive mechanism similar to what we use in upload-pack\n    (basically sending zero-length packets on sideband 1 while the client\n    is waiting for us to speak).\n\nBoth are enabled for any client which speaks the sideband protocol.\nExisting versions of git handle the new behavior just fine (the progress\nreporting is easy, because they were expecting stderr messages anyway;\nthe keepalive works because the demuxer just relays zero bytes back to\nsend-pack).\n\nI also tested with both JGit and libgit2 clients, and both seem to\nhandle the zero-length packets just fine.\n\nThere are unfortunately no automated tests, as it's hard to simulate the\neffect. My manual testing involved inserting \"sleep\" statements into\nindex-pack (and hooks with manual sleeps), and then using \"strace\" to\nconfirm that the keepalives were sent.\n\n  [01/12]: check_everything_connected: always pass --quiet to rev-list\n  [02/12]: rev-list: add optional progress reporting\n  [03/12]: check_everything_connected: convert to argv_array\n  [04/12]: check_everything_connected: use a struct with named options\n  [05/12]: check_connected: relay errors to alternate descriptor\n  [06/12]: check_connected: add progress flag\n  [07/12]: clone: use a real progress meter for connectivity check\n  [08/12]: index-pack: add flag for showing delta-resolution progress\n  [09/12]: receive-pack: turn on index-pack resolving progress\n  [10/12]: receive-pack: relay connectivity errors to sideband\n  [11/12]: receive-pack: turn on connectivity progress\n  [12/12]: receive-pack: send keepalives during quiet periods\n\n-Peff\n"},{"id":"291514","messageId":"20160715102628.GA19271@sigill.intra.peff.net","threadId":"42856","inReplyTo":"20160715102506.GA23164@sigill.intra.peff.net","subject":"[PATCH 01/12] check_everything_connected: always pass --quiet to rev-list","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-07-15T10:26:29Z","receivedAt":"2016-07-15T10:26:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The check_everything_connected function takes a \"quiet\"\nparameter which does two things if non-zero:\n\n  1. redirect rev-list's stderr to /dev/null to avoid\n     showing errors to the user\n\n  2. pass \"--quiet\" to rev-list\n\nItem (1) is obviously useful. But item (2) is\nsurprisingly not. For rev-list, \"--quiet\" does not have\nanything to do with chattiness on stderr; it tells rev-list\nnot to bother writing the list of traversed objects to\nstdout, for efficiency.  And since we always redirect\nrev-list's stdout to /dev/null in this function, there is no\npoint in asking it to ever write anything to stdout.\n\nThe efficiency gains are modest; a best-of-five run of \"git\nrev-list --objects --all\" on linux.git dropped from 32.013s\nto 30.502s when adding \"--quiet\". That's only about 5%, but\ngiven how easy it is, it's worth doing.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n connected.c | 3 +--\n 1 file changed, 1 insertion(+), 2 deletions(-)\n\ndiff --git a/connected.c b/connected.c\nindex bf1b12e..7560a31 100644\n--- a/connected.c\n+++ b/connected.c\n@@ -56,8 +56,7 @@ static int check_everything_connected_real(sha1_iterate_fn fn,\n \targv[ac++] = \"--stdin\";\n \targv[ac++] = \"--not\";\n \targv[ac++] = \"--all\";\n-\tif (quiet)\n-\t\targv[ac++] = \"--quiet\";\n+\targv[ac++] = \"--quiet\";\n \targv[ac] = NULL;\n \n \trev_list.argv = argv;\n-- \n2.9.1.434.g748be50\n\n"},{"id":"291515","messageId":"20160715102806.GB19271@sigill.intra.peff.net","threadId":"42856","inReplyTo":"20160715102506.GA23164@sigill.intra.peff.net","subject":"[PATCH 02/12] rev-list: add optional progress reporting","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-07-15T10:28:07Z","receivedAt":"2016-07-15T10:28:23Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"It's easy to ask rev-list to do a traversal that may take\nmany seconds (e.g., by calling \"--objects --all\"). In theory\nyou can monitor its progress by the output you get to\nstdout, but this isn't always easy.\n\nSome operations, like \"--count\", don't make any output until\nthe end.\n\nAnd some callers, like check_everything_connected(), are\nusing it just for the error-checking of the traversal, and\nthrow away stdout entirely.\n\nThis patch adds a \"--progress\" option which can be used to\ngive some eye-candy for a user waiting for a long traversal.\nThis is just a rev-list option and not a regular traversal\noption, because it needs cooperation from the callbacks in\nbuiltin/rev-list.c to do the actual count.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n Documentation/rev-list-options.txt |  4 ++++\n builtin/rev-list.c                 | 17 +++++++++++++++++\n 2 files changed, 21 insertions(+)\n\ndiff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt\nindex c5bd218..f39cb6d 100644\n--- a/Documentation/rev-list-options.txt\n+++ b/Documentation/rev-list-options.txt\n@@ -274,6 +274,10 @@ ifdef::git-rev-list[]\n \tTry to speed up the traversal using the pack bitmap index (if\n \tone is available). Note that when traversing with `--objects`,\n \ttrees and blobs will not have their associated path printed.\n+\n+--progress=<header>::\n+\tShow progress reports on stderr as objects are considered. The\n+\t`<header>` text will be printed with each progress update.\n endif::git-rev-list[]\n \n --\ndiff --git a/builtin/rev-list.c b/builtin/rev-list.c\nindex b82bcc3..88d95a7 100644\n--- a/builtin/rev-list.c\n+++ b/builtin/rev-list.c\n@@ -9,6 +9,7 @@\n #include \"log-tree.h\"\n #include \"graph.h\"\n #include \"bisect.h\"\n+#include \"progress.h\"\n \n static const char rev_list_usage[] =\n \"git rev-list [OPTION] <commit-id>... [ -- paths... ]\\n\"\n@@ -49,12 +50,17 @@ static const char rev_list_usage[] =\n \"    --bisect-all\"\n ;\n \n+struct progress *progress;\n+unsigned progress_counter;\n+\n static void finish_commit(struct commit *commit, void *data);\n static void show_commit(struct commit *commit, void *data)\n {\n \tstruct rev_list_info *info = data;\n \tstruct rev_info *revs = info->revs;\n \n+\tdisplay_progress(progress, ++progress_counter);\n+\n \tif (info->flags & REV_LIST_QUIET) {\n \t\tfinish_commit(commit, data);\n \t\treturn;\n@@ -190,6 +196,7 @@ static void show_object(struct object *obj, const char *name, void *cb_data)\n {\n \tstruct rev_list_info *info = cb_data;\n \tfinish_object(obj, name, cb_data);\n+\tdisplay_progress(progress, ++progress_counter);\n \tif (info->flags & REV_LIST_QUIET)\n \t\treturn;\n \tshow_object_with_name(stdout, obj, name);\n@@ -276,6 +283,7 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)\n \tint bisect_show_vars = 0;\n \tint bisect_find_all = 0;\n \tint use_bitmap_index = 0;\n+\tconst char *show_progress = NULL;\n \n \tgit_config(git_default_config, NULL);\n \tinit_revisions(&revs, prefix);\n@@ -325,6 +333,10 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)\n \t\t\ttest_bitmap_walk(&revs);\n \t\t\treturn 0;\n \t\t}\n+\t\tif (skip_prefix(arg, \"--progress=\", &arg)) {\n+\t\t\tshow_progress = arg;\n+\t\t\tcontinue;\n+\t\t}\n \t\tusage(rev_list_usage);\n \n \t}\n@@ -355,6 +367,9 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)\n \tif (bisect_list)\n \t\trevs.limited = 1;\n \n+\tif (show_progress)\n+\t\tprogress = start_progress_delay(show_progress, 0, 0, 2);\n+\n \tif (use_bitmap_index && !revs.prune) {\n \t\tif (revs.count && !revs.left_right && !revs.cherry_mark) {\n \t\t\tuint32_t commit_count;\n@@ -392,6 +407,8 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)\n \n \ttraverse_commit_list(&revs, show_commit, show_object, &info);\n \n+\tstop_progress(&progress);\n+\n \tif (revs.count) {\n \t\tif (revs.left_right && revs.cherry_mark)\n \t\t\tprintf(\"%d\\t%d\\t%d\\n\", revs.count_left, revs.count_right, revs.count_same);\n-- \n2.9.1.434.g748be50\n\n"},{"id":"291516","messageId":"20160715102832.GC19271@sigill.intra.peff.net","threadId":"42856","inReplyTo":"20160715102506.GA23164@sigill.intra.peff.net","subject":"[PATCH 03/12] check_everything_connected: convert to argv_array","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-07-15T10:28:32Z","receivedAt":"2016-07-15T10:28:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This avoids the magic \"9\" array-size which we must avoid\noverflowing, making further patches simpler.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n connected.c | 21 +++++++++------------\n 1 file changed, 9 insertions(+), 12 deletions(-)\n\ndiff --git a/connected.c b/connected.c\nindex 7560a31..a3bfc4e 100644\n--- a/connected.c\n+++ b/connected.c\n@@ -26,10 +26,9 @@ static int check_everything_connected_real(sha1_iterate_fn fn,\n \t\t\t\t\t   const char *shallow_file)\n {\n \tstruct child_process rev_list = CHILD_PROCESS_INIT;\n-\tconst char *argv[9];\n \tchar commit[41];\n \tunsigned char sha1[20];\n-\tint err = 0, ac = 0;\n+\tint err = 0;\n \tstruct packed_git *new_pack = NULL;\n \tsize_t base_len;\n \n@@ -48,18 +47,16 @@ static int check_everything_connected_real(sha1_iterate_fn fn,\n \t}\n \n \tif (shallow_file) {\n-\t\targv[ac++] = \"--shallow-file\";\n-\t\targv[ac++] = shallow_file;\n+\t\targv_array_push(&rev_list.args, \"--shallow-file\");\n+\t\targv_array_push(&rev_list.args, shallow_file);\n \t}\n-\targv[ac++] = \"rev-list\";\n-\targv[ac++] = \"--objects\";\n-\targv[ac++] = \"--stdin\";\n-\targv[ac++] = \"--not\";\n-\targv[ac++] = \"--all\";\n-\targv[ac++] = \"--quiet\";\n-\targv[ac] = NULL;\n+\targv_array_push(&rev_list.args,\"rev-list\");\n+\targv_array_push(&rev_list.args, \"--objects\");\n+\targv_array_push(&rev_list.args, \"--stdin\");\n+\targv_array_push(&rev_list.args, \"--not\");\n+\targv_array_push(&rev_list.args, \"--all\");\n+\targv_array_push(&rev_list.args, \"--quiet\");\n \n-\trev_list.argv = argv;\n \trev_list.git_cmd = 1;\n \trev_list.in = -1;\n \trev_list.no_stdout = 1;\n-- \n2.9.1.434.g748be50\n\n"},{"id":"291517","messageId":"20160715103039.GD19271@sigill.intra.peff.net","threadId":"42856","inReplyTo":"20160715102506.GA23164@sigill.intra.peff.net","subject":"[PATCH 04/12] check_everything_connected: use a struct with named options","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-07-15T10:30:40Z","receivedAt":"2016-07-15T10:30:48Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The number of variants of check_everything_connected has\ngrown over the years, so that the \"real\" function takes\nseveral possibly-zero, possibly-NULL arguments. We hid the\ncomplexity behind some wrapper functions, but this doesn't\nscale well when we want to add new options.\n\nIf we add more wrapper variants to handle the new options,\nthen we can get a combinatorial explosion when those options\nmight be used together (right now nobody wants to use both\n\"shallow\" and \"transport\" together, so we get by with just a\nfew wrappers).\n\nIf instead we add new parameters to each function, each of\nwhich can have a default value, then callers who want the\ndefaults end up with confusing invocations like:\n\n  check_everything_connected(fn, 0, data, -1, 0, NULL);\n\nwhere it is unclear which parameter is which (and every\ncaller needs updated when we add new options).\n\nInstead, let's add a struct to hold all of the optional\nparameters. This is a little more verbose for the callers\n(who have to declare the struct and fill it in), but it\nmakes their code much easier to follow, because every option\nis named as it is set (and unused options do not have to be\nmentioned at all).\n\nNote that we could also stick the iteration function and its\ncallback data into the option struct, too. But since those\nare required for each call, by avoiding doing so, we can let\nvery simple callers just pass \"NULL\" for the options and not\nworry about the struct at all.\n\nWhile we're touching each site, let's also rename the\nfunction to check_connected(). The existing name was quite\nlong, and not all of the wrappers even used the full name.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nThe diffstat claims +2 lines, but 3 of those are documentation that\nshould have existed before but didn't. :)\n\nBut the real gain comes in the later patches. They were pretty nasty\nbefore I went back and did this cleanup.\n\n builtin/clone.c        |  7 +++++--\n builtin/fetch.c        |  6 ++++--\n builtin/receive-pack.c | 13 ++++++-------\n connected.c            | 39 +++++++++++----------------------------\n connected.h            | 27 +++++++++++++++++++++------\n 5 files changed, 47 insertions(+), 45 deletions(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex 31ea247..32fe606 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -624,10 +624,13 @@ static void update_remote_refs(const struct ref *refs,\n \tconst struct ref *rm = mapped_refs;\n \n \tif (check_connectivity) {\n+\t\tstruct check_connected_options opt = CHECK_CONNECTED_INIT;\n+\n+\t\topt.transport = transport;\n+\n \t\tif (transport->progress)\n \t\t\tfprintf(stderr, _(\"Checking connectivity... \"));\n-\t\tif (check_everything_connected_with_transport(iterate_ref_map,\n-\t\t\t\t\t\t\t      0, &rm, transport))\n+\t\tif (check_connected(iterate_ref_map, &rm, &opt))\n \t\t\tdie(_(\"remote did not send all necessary objects\"));\n \t\tif (transport->progress)\n \t\t\tfprintf(stderr, _(\"done.\\n\"));\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex f896aa1..3bf895f 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -615,7 +615,7 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,\n \t\turl = xstrdup(\"foreign\");\n \n \trm = ref_map;\n-\tif (check_everything_connected(iterate_ref_map, 0, &rm)) {\n+\tif (check_connected(iterate_ref_map, &rm, NULL)) {\n \t\trc = error(_(\"%s did not send all necessary objects\\n\"), url);\n \t\tgoto abort;\n \t}\n@@ -751,6 +751,7 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,\n static int quickfetch(struct ref *ref_map)\n {\n \tstruct ref *rm = ref_map;\n+\tstruct check_connected_options opt = CHECK_CONNECTED_INIT;\n \n \t/*\n \t * If we are deepening a shallow clone we already have these\n@@ -761,7 +762,8 @@ static int quickfetch(struct ref *ref_map)\n \t */\n \tif (depth)\n \t\treturn -1;\n-\treturn check_everything_connected(iterate_ref_map, 1, &rm);\n+\topt.quiet = 1;\n+\treturn check_connected(iterate_ref_map, &rm, &opt);\n }\n \n static int fetch_refs(struct transport *transport, struct ref *ref_map)\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 15c323a..ce81920 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -737,7 +737,7 @@ static int update_shallow_ref(struct command *cmd, struct shallow_info *si)\n {\n \tstatic struct lock_file shallow_lock;\n \tstruct sha1_array extra = SHA1_ARRAY_INIT;\n-\tconst char *alt_file;\n+\tstruct check_connected_options opt = CHECK_CONNECTED_INIT;\n \tuint32_t mask = 1 << (cmd->index % 32);\n \tint i;\n \n@@ -749,9 +749,8 @@ static int update_shallow_ref(struct command *cmd, struct shallow_info *si)\n \t\t    !delayed_reachability_test(si, i))\n \t\t\tsha1_array_append(&extra, si->shallow->sha1[i]);\n \n-\tsetup_alternate_shallow(&shallow_lock, &alt_file, &extra);\n-\tif (check_shallow_connected(command_singleton_iterator,\n-\t\t\t\t    0, cmd, alt_file)) {\n+\tsetup_alternate_shallow(&shallow_lock, &opt.shallow_file, &extra);\n+\tif (check_connected(command_singleton_iterator, cmd, &opt)) {\n \t\trollback_lock_file(&shallow_lock);\n \t\tsha1_array_clear(&extra);\n \t\treturn -1;\n@@ -1160,8 +1159,8 @@ static void set_connectivity_errors(struct command *commands,\n \t\tif (shallow_update && si->shallow_ref[cmd->index])\n \t\t\t/* to be checked in update_shallow_ref() */\n \t\t\tcontinue;\n-\t\tif (!check_everything_connected(command_singleton_iterator,\n-\t\t\t\t\t\t0, &singleton))\n+\t\tif (!check_connected(command_singleton_iterator, &singleton,\n+\t\t\t\t     NULL))\n \t\t\tcontinue;\n \t\tcmd->error_string = \"missing necessary objects\";\n \t}\n@@ -1330,7 +1329,7 @@ static void execute_commands(struct command *commands,\n \n \tdata.cmds = commands;\n \tdata.si = si;\n-\tif (check_everything_connected(iterate_receive_command_list, 0, &data))\n+\tif (check_connected(iterate_receive_command_list, &data, NULL))\n \t\tset_connectivity_errors(commands, si);\n \n \treject_updates_to_hidden(commands);\ndiff --git a/connected.c b/connected.c\nindex a3bfc4e..2a51eac 100644\n--- a/connected.c\n+++ b/connected.c\n@@ -4,10 +4,6 @@\n #include \"connected.h\"\n #include \"transport.h\"\n \n-int check_everything_connected(sha1_iterate_fn fn, int quiet, void *cb_data)\n-{\n-\treturn check_everything_connected_with_transport(fn, quiet, cb_data, NULL);\n-}\n /*\n  * If we feed all the commits we want to verify to this command\n  *\n@@ -19,19 +15,22 @@ int check_everything_connected(sha1_iterate_fn fn, int quiet, void *cb_data)\n  *\n  * Returns 0 if everything is connected, non-zero otherwise.\n  */\n-static int check_everything_connected_real(sha1_iterate_fn fn,\n-\t\t\t\t\t   int quiet,\n-\t\t\t\t\t   void *cb_data,\n-\t\t\t\t\t   struct transport *transport,\n-\t\t\t\t\t   const char *shallow_file)\n+int check_connected(sha1_iterate_fn fn, void *cb_data,\n+\t\t    struct check_connected_options *opt)\n {\n \tstruct child_process rev_list = CHILD_PROCESS_INIT;\n+\tstruct check_connected_options defaults = CHECK_CONNECTED_INIT;\n \tchar commit[41];\n \tunsigned char sha1[20];\n \tint err = 0;\n \tstruct packed_git *new_pack = NULL;\n+\tstruct transport *transport;\n \tsize_t base_len;\n \n+\tif (!opt)\n+\t\topt = &defaults;\n+\ttransport = opt->transport;\n+\n \tif (fn(cb_data, sha1))\n \t\treturn err;\n \n@@ -46,9 +45,9 @@ static int check_everything_connected_real(sha1_iterate_fn fn,\n \t\tstrbuf_release(&idx_file);\n \t}\n \n-\tif (shallow_file) {\n+\tif (opt->shallow_file) {\n \t\targv_array_push(&rev_list.args, \"--shallow-file\");\n-\t\targv_array_push(&rev_list.args, shallow_file);\n+\t\targv_array_push(&rev_list.args, opt->shallow_file);\n \t}\n \targv_array_push(&rev_list.args,\"rev-list\");\n \targv_array_push(&rev_list.args, \"--objects\");\n@@ -60,7 +59,7 @@ static int check_everything_connected_real(sha1_iterate_fn fn,\n \trev_list.git_cmd = 1;\n \trev_list.in = -1;\n \trev_list.no_stdout = 1;\n-\trev_list.no_stderr = quiet;\n+\trev_list.no_stderr = opt->quiet;\n \tif (start_command(&rev_list))\n \t\treturn error(_(\"Could not run 'git rev-list'\"));\n \n@@ -94,19 +93,3 @@ static int check_everything_connected_real(sha1_iterate_fn fn,\n \tsigchain_pop(SIGPIPE);\n \treturn finish_command(&rev_list) || err;\n }\n-\n-int check_everything_connected_with_transport(sha1_iterate_fn fn,\n-\t\t\t\t\t      int quiet,\n-\t\t\t\t\t      void *cb_data,\n-\t\t\t\t\t      struct transport *transport)\n-{\n-\treturn check_everything_connected_real(fn, quiet, cb_data,\n-\t\t\t\t\t       transport, NULL);\n-}\n-\n-int check_shallow_connected(sha1_iterate_fn fn, int quiet, void *cb_data,\n-\t\t\t    const char *shallow_file)\n-{\n-\treturn check_everything_connected_real(fn, quiet, cb_data,\n-\t\t\t\t\t       NULL, shallow_file);\n-}\ndiff --git a/connected.h b/connected.h\nindex 071d408..12594ef 100644\n--- a/connected.h\n+++ b/connected.h\n@@ -10,18 +10,33 @@ struct transport;\n  */\n typedef int (*sha1_iterate_fn)(void *, unsigned char [20]);\n \n+/*\n+ * Named-arguments struct for check_connected. All arguments are\n+ * optional, and can be left to defaults as set by CHECK_CONNECTED_INIT.\n+ */\n+struct check_connected_options {\n+\t/* Avoid printing any errors to stderr. */\n+\tint quiet;\n+\n+\t/* --shallow-file to pass to rev-list sub-process */\n+\tconst char *shallow_file;\n+\n+\t/* Transport whose objects we are checking, if available. */\n+\tstruct transport *transport;\n+};\n+\n+#define CHECK_CONNECTED_INIT { 0 }\n+\n /*\n  * Make sure that our object store has all the commits necessary to\n  * connect the ancestry chain to some of our existing refs, and all\n  * the trees and blobs that these commits use.\n  *\n  * Return 0 if Ok, non zero otherwise (i.e. some missing objects)\n+ *\n+ * If \"opt\" is NULL, behaves as if CHECK_CONNECTED_INIT was passed.\n  */\n-extern int check_everything_connected(sha1_iterate_fn, int quiet, void *cb_data);\n-extern int check_shallow_connected(sha1_iterate_fn, int quiet, void *cb_data,\n-\t\t\t\t   const char *shallow_file);\n-extern int check_everything_connected_with_transport(sha1_iterate_fn, int quiet,\n-\t\t\t\t\t\t     void *cb_data,\n-\t\t\t\t\t\t     struct transport *transport);\n+int check_connected(sha1_iterate_fn fn, void *cb_data,\n+\t\t    struct check_connected_options *opt);\n \n #endif /* CONNECTED_H */\n-- \n2.9.1.434.g748be50\n\n"},{"id":"291518","messageId":"20160715103203.GE19271@sigill.intra.peff.net","threadId":"42856","inReplyTo":"20160715102506.GA23164@sigill.intra.peff.net","subject":"[PATCH 05/12] check_connected: relay errors to alternate descriptor","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-07-15T10:32:03Z","receivedAt":"2016-07-15T10:32:11Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Unless the \"quiet\" flag is given, check_connected sends any\nerrors to the stderr of the caller (because the child\nrev-list inherits that descriptor). However, server-side\ncallers may want to send these over a sideband channel\ninstead.  Let's make that possible.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n connected.c | 11 +++++++++--\n connected.h |  7 +++++++\n 2 files changed, 16 insertions(+), 2 deletions(-)\n\ndiff --git a/connected.c b/connected.c\nindex 2a51eac..5f5c8bd 100644\n--- a/connected.c\n+++ b/connected.c\n@@ -31,8 +31,11 @@ int check_connected(sha1_iterate_fn fn, void *cb_data,\n \t\topt = &defaults;\n \ttransport = opt->transport;\n \n-\tif (fn(cb_data, sha1))\n+\tif (fn(cb_data, sha1)) {\n+\t\tif (opt->err_fd)\n+\t\t\tclose(opt->err_fd);\n \t\treturn err;\n+\t}\n \n \tif (transport && transport->smart_options &&\n \t    transport->smart_options->self_contained_and_connected &&\n@@ -59,7 +62,11 @@ int check_connected(sha1_iterate_fn fn, void *cb_data,\n \trev_list.git_cmd = 1;\n \trev_list.in = -1;\n \trev_list.no_stdout = 1;\n-\trev_list.no_stderr = opt->quiet;\n+\tif (opt->err_fd)\n+\t\trev_list.err = opt->err_fd;\n+\telse\n+\t\trev_list.no_stderr = opt->quiet;\n+\n \tif (start_command(&rev_list))\n \t\treturn error(_(\"Could not run 'git rev-list'\"));\n \ndiff --git a/connected.h b/connected.h\nindex 12594ef..5d88e26 100644\n--- a/connected.h\n+++ b/connected.h\n@@ -23,6 +23,13 @@ struct check_connected_options {\n \n \t/* Transport whose objects we are checking, if available. */\n \tstruct transport *transport;\n+\n+\t/*\n+\t * If non-zero, send error messages to this descriptor rather\n+\t * than stderr. The descriptor is closed before check_connected\n+\t * returns.\n+\t */\n+\tint err_fd;\n };\n \n #define CHECK_CONNECTED_INIT { 0 }\n-- \n2.9.1.434.g748be50\n\n"},{"id":"291519","messageId":"20160715103228.GF19271@sigill.intra.peff.net","threadId":"42856","inReplyTo":"20160715102506.GA23164@sigill.intra.peff.net","subject":"[PATCH 06/12] check_connected: add progress flag","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-07-15T10:32:28Z","receivedAt":"2016-07-15T10:32:34Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Connectivity checks have to traverse the entire object graph\nin the worst case (e.g., a full clone or a full push). For\nlarge repositories like linux.git, this can take 30-60\nseconds, during which time git may produce little or no\noutput.\n\nLet's add the option of showing progress, which is taken\ncare of by rev-list.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n connected.c | 3 +++\n connected.h | 3 +++\n 2 files changed, 6 insertions(+)\n\ndiff --git a/connected.c b/connected.c\nindex 5f5c8bd..8e3e4b1 100644\n--- a/connected.c\n+++ b/connected.c\n@@ -58,6 +58,9 @@ int check_connected(sha1_iterate_fn fn, void *cb_data,\n \targv_array_push(&rev_list.args, \"--not\");\n \targv_array_push(&rev_list.args, \"--all\");\n \targv_array_push(&rev_list.args, \"--quiet\");\n+\tif (opt->progress)\n+\t\targv_array_pushf(&rev_list.args, \"--progress=%s\",\n+\t\t\t\t _(\"Checking connectivity\"));\n \n \trev_list.git_cmd = 1;\n \trev_list.in = -1;\ndiff --git a/connected.h b/connected.h\nindex 5d88e26..afa48cc 100644\n--- a/connected.h\n+++ b/connected.h\n@@ -30,6 +30,9 @@ struct check_connected_options {\n \t * returns.\n \t */\n \tint err_fd;\n+\n+\t/* If non-zero, show progress as we traverse the objects. */\n+\tint progress;\n };\n \n #define CHECK_CONNECTED_INIT { 0 }\n-- \n2.9.1.434.g748be50\n\n"},{"id":"291520","messageId":"20160715103318.GG19271@sigill.intra.peff.net","threadId":"42856","inReplyTo":"20160715102506.GA23164@sigill.intra.peff.net","subject":"[PATCH 07/12] clone: use a real progress meter for connectivity check","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-07-15T10:33:18Z","receivedAt":"2016-07-15T10:33:25Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Because the initial connectivity check for a cloned\nrepository can be slow, 0781aa4 (clone: let the user know\nwhen check_everything_connected is run, 2013-05-03) added a\n\"fake\" progress meter; we simply say \"Checking connectivity\"\nwhen it starts, and \"done\" at the end, with nothing between.\n\nSince check_connected() now knows how to do a real progress\nmeter, we can drop our fake one and use that one instead.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nObviously not related to the receive-pack bits, but we get this for\nfree because of the earlier refactoring.\n\n builtin/clone.c | 5 +----\n 1 file changed, 1 insertion(+), 4 deletions(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex 32fe606..f044a8c 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -627,13 +627,10 @@ static void update_remote_refs(const struct ref *refs,\n \t\tstruct check_connected_options opt = CHECK_CONNECTED_INIT;\n \n \t\topt.transport = transport;\n+\t\topt.progress = transport->progress;\n \n-\t\tif (transport->progress)\n-\t\t\tfprintf(stderr, _(\"Checking connectivity... \"));\n \t\tif (check_connected(iterate_ref_map, &rm, &opt))\n \t\t\tdie(_(\"remote did not send all necessary objects\"));\n-\t\tif (transport->progress)\n-\t\t\tfprintf(stderr, _(\"done.\\n\"));\n \t}\n \n \tif (refs) {\n-- \n2.9.1.434.g748be50\n\n"},{"id":"291521","messageId":"20160715103422.GH19271@sigill.intra.peff.net","threadId":"42856","inReplyTo":"20160715102506.GA23164@sigill.intra.peff.net","subject":"[PATCH 08/12] index-pack: add flag for showing delta-resolution progress","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-07-15T10:34:22Z","receivedAt":"2016-07-15T10:34:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The index-pack command has two progress meters: one for\n\"receiving objects\", and one for \"resolving deltas\". You get\nneither by default, or both with \"-v\".\n\nBut for a push through receive-pack, we would want only the\n\"resolving deltas\" phase, _not_ the \"receiving objects\"\nprogress. There are two reasons for this.\n\nOne is simply that existing clients are already printing\n\"writing objects\" progress at the same time.  Arguably\n\"receiving\" from the far end is more useful, because it\ntells you what has actually gotten there, as opposed to what\nmight be stuck in a buffer somewhere between the client and\nserver. But that would require a protocol extension to tell\nclients not to print their progress. Possible, but\ncomplexity for little gain.\n\nThe second reason is much more important. In a full-duplex\nconnection like git-over-ssh, we can print progress while\nthe pack is incoming, and it will immediately get to the\nclient. But for a half-duplex connection like git-over-http,\nwe should not say anything until we have received the full\nrequest.  Anything we write is subject to being stuck in a\nbuffer by the webserver.  Worse, we can end up in a deadlock\nif that buffer fills up.\n\nSo our best bet is to avoid writing anything that isn't a\nsmall fixed size until we've received the full pack.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/index-pack.c | 5 ++++-\n 1 file changed, 4 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/index-pack.c b/builtin/index-pack.c\nindex e8c71fc..1cba120 100644\n--- a/builtin/index-pack.c\n+++ b/builtin/index-pack.c\n@@ -77,6 +77,7 @@ static int strict;\n static int do_fsck_object;\n static struct fsck_options fsck_options = FSCK_OPTIONS_STRICT;\n static int verbose;\n+static int show_resolving_progress;\n static int show_stat;\n static int check_self_contained_and_connected;\n \n@@ -1190,7 +1191,7 @@ static void resolve_deltas(void)\n \tqsort(ref_deltas, nr_ref_deltas, sizeof(struct ref_delta_entry),\n \t      compare_ref_delta_entry);\n \n-\tif (verbose)\n+\tif (verbose || show_resolving_progress)\n \t\tprogress = start_progress(_(\"Resolving deltas\"),\n \t\t\t\t\t  nr_ref_deltas + nr_ofs_deltas);\n \n@@ -1694,6 +1695,8 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix)\n \t\t\t\tinput_len = sizeof(*hdr);\n \t\t\t} else if (!strcmp(arg, \"-v\")) {\n \t\t\t\tverbose = 1;\n+\t\t\t} else if (!strcmp(arg, \"--show-resolving-progress\")) {\n+\t\t\t\tshow_resolving_progress = 1;\n \t\t\t} else if (!strcmp(arg, \"-o\")) {\n \t\t\t\tif (index_name || (i+1) >= argc)\n \t\t\t\t\tusage(index_pack_usage);\n-- \n2.9.1.434.g748be50\n\n"},{"id":"291522","messageId":"20160715103527.GI19271@sigill.intra.peff.net","threadId":"42856","inReplyTo":"20160715102506.GA23164@sigill.intra.peff.net","subject":"[PATCH 09/12] receive-pack: turn on index-pack resolving progress","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-07-15T10:35:28Z","receivedAt":"2016-07-15T10:35:35Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we receive a large push, the server side may have to\nspend a lot of CPU processing the incoming packfile.\n\nDuring the \"receiving\" phase, we are typically network\nbound, and the client is writing its own progress to the\nuser. But during the delta resolution phase, we may spend\nminutes (e.g., for a full push of linux.git) without\nmaking any indication to the user that the connection has\nnot hung.\n\nLet's ask index-pack to produce progress output for this\nphase (unless the client asked us to be quiet, of course).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/receive-pack.c | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex ce81920..de322bc 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -1547,6 +1547,8 @@ static const char *unpack(int err_fd, struct shallow_info *si)\n \t\t\t\t (uintmax_t)getpid(),\n \t\t\t\t hostname);\n \n+\t\tif (!quiet && err_fd)\n+\t\t\targv_array_push(&child.args, \"--show-resolving-progress\");\n \t\tif (fsck_objects)\n \t\t\targv_array_pushf(&child.args, \"--strict%s\",\n \t\t\t\tfsck_msg_types.buf);\n-- \n2.9.1.434.g748be50\n\n"},{"id":"291523","messageId":"20160715103614.GJ19271@sigill.intra.peff.net","threadId":"42856","inReplyTo":"20160715102506.GA23164@sigill.intra.peff.net","subject":"[PATCH 10/12] receive-pack: relay connectivity errors to sideband","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-07-15T10:36:14Z","receivedAt":"2016-07-15T10:36:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"If the connectivity check encounters a problem when\nreceiving a push, the error output goes to receive-pack's\nstderr, whose destination depends on the protocol used\n(ssh tends to send it to the user, though without a \"remote\"\nprefix; http will generally eat it in the server's error\nlog).\n\nThe information should consistently go back to the user, as\nthere is a reasonable chance their client is buggy and\ngenerating a bad pack.\n\nWe can do so by muxing it over the sideband as we do with\nother sub-process stderr.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/receive-pack.c | 18 +++++++++++++++++-\n 1 file changed, 17 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex de322bc..d309109 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -1317,9 +1317,12 @@ static void execute_commands(struct command *commands,\n \t\t\t     const char *unpacker_error,\n \t\t\t     struct shallow_info *si)\n {\n+\tstruct check_connected_options opt = CHECK_CONNECTED_INIT;\n \tstruct command *cmd;\n \tunsigned char sha1[20];\n \tstruct iterate_data data;\n+\tstruct async muxer;\n+\tint err_fd = 0;\n \n \tif (unpacker_error) {\n \t\tfor (cmd = commands; cmd; cmd = cmd->next)\n@@ -1327,11 +1330,24 @@ static void execute_commands(struct command *commands,\n \t\treturn;\n \t}\n \n+\tif (use_sideband) {\n+\t\tmemset(&muxer, 0, sizeof(muxer));\n+\t\tmuxer.proc = copy_to_sideband;\n+\t\tmuxer.in = -1;\n+\t\tif (!start_async(&muxer))\n+\t\t\terr_fd = muxer.in;\n+\t\t/* ...else, continue without relaying sideband */\n+\t}\n+\n \tdata.cmds = commands;\n \tdata.si = si;\n-\tif (check_connected(iterate_receive_command_list, &data, NULL))\n+\topt.err_fd = err_fd;\n+\tif (check_connected(iterate_receive_command_list, &data, &opt))\n \t\tset_connectivity_errors(commands, si);\n \n+\tif (use_sideband)\n+\t\tfinish_async(&muxer);\n+\n \treject_updates_to_hidden(commands);\n \n \tif (run_receive_hook(commands, \"pre-receive\", 0)) {\n-- \n2.9.1.434.g748be50\n\n"},{"id":"291524","messageId":"20160715103635.GK19271@sigill.intra.peff.net","threadId":"42856","inReplyTo":"20160715102506.GA23164@sigill.intra.peff.net","subject":"[PATCH 11/12] receive-pack: turn on connectivity progress","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-07-15T10:36:35Z","receivedAt":"2016-07-15T10:36:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we receive a large push, the server side of the\nconnection may spend a lot of time (30s or more for a full\npush of linux.git) walking the object graph without\nproducing any output. Let's give the user some indication\nthat we're actually working.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/receive-pack.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex d309109..7db1639 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -1342,6 +1342,7 @@ static void execute_commands(struct command *commands,\n \tdata.cmds = commands;\n \tdata.si = si;\n \topt.err_fd = err_fd;\n+\topt.progress = err_fd && !quiet;\n \tif (check_connected(iterate_receive_command_list, &data, &opt))\n \t\tset_connectivity_errors(commands, si);\n \n-- \n2.9.1.434.g748be50\n\n"},{"id":"291525","messageId":"20160715104347.GL19271@sigill.intra.peff.net","threadId":"42856","inReplyTo":"20160715102506.GA23164@sigill.intra.peff.net","subject":"[PATCH 12/12] receive-pack: send keepalives during quiet periods","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-07-15T10:43:47Z","receivedAt":"2016-07-15T10:43:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"After a client has sent us the complete pack, we may spend\nsome time processing the data and running hooks. If the\nclient asked us to be quiet, receive-pack won't send any\nprogress data during the index-pack or connectivity-check\nsteps. And hooks may or may not produce their own progress\noutput. In these cases, the network connection is totally\nsilent from both ends.\n\nGit itself doesn't care about this (it will wait forever),\nbut other parts of the system (e.g., firewalls,\nload-balancers, etc) might hang up the connection. So we'd\nlike to send some sort of keepalive to let the network and\nthe client side know that we're still alive and processing.\n\nWe can use the same trick we did in 05e9515 (upload-pack:\nsend keepalive packets during pack computation, 2013-09-08).\nNamely, we will send an empty sideband data packet every `N`\nseconds that we do not relay any stderr data over the\nsideband channel. As with 05e9515, this means that we won't\nbother sending keepalives when there's actual progress data,\nbut will kick in when it has been disabled (or if there is a\nlull in the progress data).\n\nThe concept is simple, but the details are subtle enough\nthat they need discussing here.\n\nBefore the client sends us the pack, we don't want to do any\nkeepalives. We'll have sent our ref advertisement, and we're\nwaiting for them to send us the pack (and tell us that they\nsupport sidebands at all).\n\nWhile we're receiving the pack from the client (or waiting\nfor it to start), there's no need for keepalives; it's up to\nthem to keep the connection active by sending data.\nMoreover, it would be wrong for us to do so. When we are the\nserver in the smart-http protocol, we must treat our\nconnection as half-duplex. So any keepalives we send while\nreceiving the pack would potentially be buffered by the\nwebserver. Not only does this make them useless (since they\nwould not be delivered in a timely manner), but it could\nactually cause a deadlock if we fill up the buffer with\nkeepalives. (It wouldn't be wrong to send keepalives in this\nphase for a full-duplex connection like ssh; it's simply\npointless, as it is the client's responsibility to speak).\n\nAs soon as we've gotten all of the pack data, then the\nclient is waiting for us to speak, and we should start\nkeepalives immediately. From here until the end of the\nconnection, we send one any time we are not otherwise\nsending data.\n\nBut there's a catch. Receive-pack doesn't know the moment\nwe've gotten all the data. It passes the descriptor to\nindex-pack, who reads all of the data, and then starts\nresolving the deltas. We have to communicate that back.\n\nTo make this work, we instruct the sideband muxer to enable\nkeepalives in three phases:\n\n  1. In the beginning, not at all.\n\n  2. While reading from index-pack, wait for a signal\n     indicating end-of-input, and then start them.\n\n  3. Afterwards, always.\n\nThe signal from index-pack in phase 2 has to come over the\nstderr channel which the muxer is reading. We can't use an\nextra pipe because the portable run-command interface only\ngives us stderr and stdout.\n\nStdout is already used to pass the .keep filename back to\nreceive-pack. We could also send a signal there, but then we\nwould find out about it in the main thread. And the\nkeepalive needs to be done by the async muxer thread (since\nit's the one writing sideband data back to the client). And\nwe can't reliably signal the async thread from the main\nthread, because the async code sometimes uses threads and\nsometimes uses forked processes.\n\nTherefore the signal must come over the stderr channel,\nwhere it may be interspersed with other random\nhuman-readable messages from index-pack. This patch makes\nthe signal a single NUL byte.  This is easy to parse, should\nnot appear in any normal stderr output, and we don't have to\nworry about any timing issues (like seeing half the signal\nbytes in one read(), and half in a subsequent one).\n\nThis is a bit ugly, but it's simple to code and should work\nreliably.\n\nAnother option would be to stop using an async thread for\nmuxing entirely, and just poll() both stderr and stdout of\nindex-pack from the main thread. This would work for\nindex-pack (because we aren't doing anything useful in the\nmain thread while it runs anyway). But it would make the\nconnectivity check and the hook muxers much more\ncomplicated, as they need to simultaneously feed the\nsub-programs while reading their stderr.\n\nThe index-pack phase is the only one that needs this\nsignaling, so it could simply behave differently than the\nother two. That would mean having two separate\nimplementations of copy_to_sideband (and the keepalive\ncode), though. And it still doesn't get rid of the\nsignaling; it just means we can write a nicer message like\n\"END_OF_INPUT\" or something on stdout, since we don't have\nto worry about separating it from the stderr cruft.\n\nOne final note: this signaling trick is only done with\nindex-pack, not with unpack-objects. There's no point in\ndoing it for the latter, because by definition it only kicks\nin for a small number of objects, where keepalives are not\nas useful (and this conveniently lets us avoid duplicating\nthe implementation).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n Documentation/config.txt |  9 +++++++\n builtin/index-pack.c     |  5 ++++\n builtin/receive-pack.c   | 68 +++++++++++++++++++++++++++++++++++++++++++++++-\n 3 files changed, 81 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 16dc22d..08efd50 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -2473,6 +2473,15 @@ receive.fsck.skipList::\n \tcan be safely ignored such as invalid committer email addresses.\n \tNote: corrupt objects cannot be skipped with this setting.\n \n+receive.keepAlive::\n+\tAfter receiving the pack from the client, `receive-pack` may\n+\tproduce no output (if `--quiet` was specified) while processing\n+\tthe pack, causing some networks to drop the TCP connection.\n+\tWith this option set, if `receive-pack` does not transmit\n+\tany data in this phase for `receive.keepAlive` seconds, it will\n+\tsend a short keepalive packet.  The default is 5 seconds; set\n+\tto 0 to disable keepalives entirely.\n+\n receive.unpackLimit::\n \tIf the number of objects received in a push is below this\n \tlimit then the objects will be unpacked into loose object\ndiff --git a/builtin/index-pack.c b/builtin/index-pack.c\nindex 1cba120..54f2cfb 100644\n--- a/builtin/index-pack.c\n+++ b/builtin/index-pack.c\n@@ -1626,6 +1626,7 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix)\n \tstruct pack_idx_option opts;\n \tunsigned char pack_sha1[20];\n \tunsigned foreign_nr = 1;\t/* zero is a \"good\" value, assume bad */\n+\tint report_end_of_input = 0;\n \n \tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n \t\tusage(index_pack_usage);\n@@ -1697,6 +1698,8 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix)\n \t\t\t\tverbose = 1;\n \t\t\t} else if (!strcmp(arg, \"--show-resolving-progress\")) {\n \t\t\t\tshow_resolving_progress = 1;\n+\t\t\t} else if (!strcmp(arg, \"--report-end-of-input\")) {\n+\t\t\t\treport_end_of_input = 1;\n \t\t\t} else if (!strcmp(arg, \"-o\")) {\n \t\t\t\tif (index_name || (i+1) >= argc)\n \t\t\t\t\tusage(index_pack_usage);\n@@ -1754,6 +1757,8 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix)\n \t\tobj_stat = xcalloc(st_add(nr_objects, 1), sizeof(struct object_stat));\n \tofs_deltas = xcalloc(nr_objects, sizeof(struct ofs_delta_entry));\n \tparse_pack_objects(pack_sha1);\n+\tif (report_end_of_input)\n+\t\twrite_in_full(2, \"\\0\", 1);\n \tresolve_deltas();\n \tconclude_pack(fix_thin_pack, curr_pack, pack_sha1);\n \tfree(ofs_deltas);\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 7db1639..e41f55f 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -76,6 +76,13 @@ static long nonce_stamp_slop;\n static unsigned long nonce_stamp_slop_limit;\n static struct ref_transaction *transaction;\n \n+static enum {\n+\tKEEPALIVE_NEVER = 0,\n+\tKEEPALIVE_AFTER_NUL,\n+\tKEEPALIVE_ALWAYS\n+} use_keepalive;\n+static int keepalive_in_sec = 5;\n+\n static enum deny_action parse_deny_action(const char *var, const char *value)\n {\n \tif (value) {\n@@ -193,6 +200,11 @@ static int receive_pack_config(const char *var, const char *value, void *cb)\n \t\treturn 0;\n \t}\n \n+\tif (strcmp(var, \"receive.keepalive\") == 0) {\n+\t\tkeepalive_in_sec = git_config_int(var, value);\n+\t\treturn 0;\n+\t}\n+\n \treturn git_default_config(var, value, cb);\n }\n \n@@ -319,10 +331,60 @@ static void rp_error(const char *err, ...)\n static int copy_to_sideband(int in, int out, void *arg)\n {\n \tchar data[128];\n+\tint keepalive_active = 0;\n+\n+\tif (keepalive_in_sec <= 0)\n+\t\tuse_keepalive = KEEPALIVE_NEVER;\n+\tif (use_keepalive == KEEPALIVE_ALWAYS)\n+\t\tkeepalive_active = 1;\n+\n \twhile (1) {\n-\t\tssize_t sz = xread(in, data, sizeof(data));\n+\t\tssize_t sz;\n+\n+\t\tif (keepalive_active) {\n+\t\t\tstruct pollfd pfd;\n+\t\t\tint ret;\n+\n+\t\t\tpfd.fd = in;\n+\t\t\tpfd.events = POLLIN;\n+\t\t\tret = poll(&pfd, 1, 1000 * keepalive_in_sec);\n+\n+\t\t\tif (ret < 0) {\n+\t\t\t\tif (errno == EINTR)\n+\t\t\t\t\tcontinue;\n+\t\t\t\telse\n+\t\t\t\t\tbreak;\n+\t\t\t} else if (ret == 0) {\n+\t\t\t\t/* no data; send a keepalive packet */\n+\t\t\t\tstatic const char buf[] = \"0005\\1\";\n+\t\t\t\twrite_or_die(1, buf, sizeof(buf) - 1);\n+\t\t\t\tcontinue;\n+\t\t\t} /* else there is actual data to read */\n+\t\t}\n+\n+\t\tsz = xread(in, data, sizeof(data));\n \t\tif (sz <= 0)\n \t\t\tbreak;\n+\n+\t\tif (use_keepalive == KEEPALIVE_AFTER_NUL && !keepalive_active) {\n+\t\t\tconst char *p = memchr(data, '\\0', sz);\n+\t\t\tif (p) {\n+\t\t\t\t/*\n+\t\t\t\t * The NUL tells us to start sending keepalives. Make\n+\t\t\t\t * sure we send any other data we read along\n+\t\t\t\t * with it.\n+\t\t\t\t */\n+\t\t\t\tkeepalive_active = 1;\n+\t\t\t\tsend_sideband(1, 2, data, p - data, use_sideband);\n+\t\t\t\tsend_sideband(1, 2, p + 1, sz - (p - data + 1), use_sideband);\n+\t\t\t\tcontinue;\n+\t\t\t}\n+\t\t}\n+\n+\t\t/*\n+\t\t * Either we're not looking for a NUL signal, or we didn't see\n+\t\t * it yet; just pass along the data.\n+\t\t */\n \t\tsend_sideband(1, 2, data, sz, use_sideband);\n \t}\n \tclose(in);\n@@ -1566,6 +1628,8 @@ static const char *unpack(int err_fd, struct shallow_info *si)\n \n \t\tif (!quiet && err_fd)\n \t\t\targv_array_push(&child.args, \"--show-resolving-progress\");\n+\t\tif (use_sideband)\n+\t\t\targv_array_push(&child.args, \"--report-end-of-input\");\n \t\tif (fsck_objects)\n \t\t\targv_array_pushf(&child.args, \"--strict%s\",\n \t\t\t\tfsck_msg_types.buf);\n@@ -1595,6 +1659,7 @@ static const char *unpack_with_sideband(struct shallow_info *si)\n \tif (!use_sideband)\n \t\treturn unpack(0, si);\n \n+\tuse_keepalive = KEEPALIVE_AFTER_NUL;\n \tmemset(&muxer, 0, sizeof(muxer));\n \tmuxer.proc = copy_to_sideband;\n \tmuxer.in = -1;\n@@ -1782,6 +1847,7 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix)\n \t\t\tunpack_status = unpack_with_sideband(&si);\n \t\t\tupdate_shallow_info(commands, &si, &ref);\n \t\t}\n+\t\tuse_keepalive = KEEPALIVE_ALWAYS;\n \t\texecute_commands(commands, unpack_status, &si);\n \t\tif (pack_lockfile)\n \t\t\tunlink_or_warn(pack_lockfile);\n-- \n2.9.1.434.g748be50\n"},{"id":"291553","messageId":"CAGZ79kbernZHx9SUG-_vbxy-g77_3ki1uc-21LCviwrH=aXp6w@mail.gmail.com","threadId":"42856","inReplyTo":"20160715104347.GL19271@sigill.intra.peff.net","subject":"Re: [PATCH 12/12] receive-pack: send keepalives during quiet periods","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-07-15T17:24:16Z","receivedAt":"2016-07-15T17:24:22Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, Jul 15, 2016 at 3:43 AM, Jeff King <peff@peff.net> wrote:\n\n>\n> Signed-off-by: Jeff King <peff@peff.net>\n\nRead-entirely-by Stefan ;)\nThanks!\n\n> @@ -319,10 +331,60 @@ static void rp_error(const char *err, ...)\n>  static int copy_to_sideband(int in, int out, void *arg)\n>  {\n>         char data[128];\n\nWhile looking at this code, do you think it is feasible to increase the\nsize of data[] to 1024 ? (The largest that is possible when\nside-band, but no side-band-64k is given).\n\n> +       int keepalive_active = 0;\n> +\n> +       if (keepalive_in_sec <= 0)\n> +               use_keepalive = KEEPALIVE_NEVER;\n> +       if (use_keepalive == KEEPALIVE_ALWAYS)\n> +               keepalive_active = 1;\n> +\n>         while (1) {\n> -               ssize_t sz = xread(in, data, sizeof(data));\n> +               ssize_t sz;\n> +\n> +               if (keepalive_active) {\n> +                       struct pollfd pfd;\n> +                       int ret;\n> +\n> +                       pfd.fd = in;\n> +                       pfd.events = POLLIN;\n> +                       ret = poll(&pfd, 1, 1000 * keepalive_in_sec);\n> +\n> +                       if (ret < 0) {\n> +                               if (errno == EINTR)\n> +                                       continue;\n> +                               else\n> +                                       break;\n\nThe method was short and concise, this adds a lot of lines.\nRemembering d751dd11 (2016-07-10, hoist out handle_nonblock\nfunction for xread and xwrite), do you think it would be reasonable to\nput the whole poll handling into a dedicated function, maybe even reuse the\nthat function?\n\n    if (keepalive_active) {\n        if (wrapper_around_poll(&data_in) < 0) // handles EINTR internally\n            break;\n        if (!data_in)\n            send_keep_alive();\n    }\n\nI am not sure if that makes this function more legible, just food for thought.\n\n\n> +                       } else if (ret == 0) {\n> +                               /* no data; send a keepalive packet */\n> +                               static const char buf[] = \"0005\\1\";\n\nand the \\1 is the first sideband. Why do we choose that sideband?\n\n> +                               write_or_die(1, buf, sizeof(buf) - 1);\n> +                               continue;\n> +                       } /* else there is actual data to read */\n\n\"If there is data to read, we need to break the while(1), to actually\nread the data?\"\nI got confused and needed to go back and read the actual code again,\nwould it make sense to rather have a loop here?\n\n    while (1) {\n       while(keepalive_active) {\n        if (wrapper_around_poll(&data_in) < 0) // handles EINTR internally\n            break;\n        if (!data_in)\n            send_keep_alive();\n        else\n            break;\n        }\n\n               sz = xread(in, data, sizeof(data));\n                 if (sz <= 0)\n                         break;\n\n        turn_on_keepalive_on_NUL(&data);\n     }\n\n> +               }\n> +\n> +               sz = xread(in, data, sizeof(data));\n>                 if (sz <= 0)\n>                         break;\n> +\n> +               if (use_keepalive == KEEPALIVE_AFTER_NUL && !keepalive_active) {\n> +                       const char *p = memchr(data, '\\0', sz);\n> +                       if (p) {\n> +                               /*\n> +                                * The NUL tells us to start sending keepalives. Make\n> +                                * sure we send any other data we read along\n> +                                * with it.\n> +                                */\n> +                               keepalive_active = 1;\n> +                               send_sideband(1, 2, data, p - data, use_sideband);\n> +                               send_sideband(1, 2, p + 1, sz - (p - data + 1), use_sideband);\n> +                               continue;\n\nOh, I see why the turn_on_keepalive_on_NUL doesn't work as well as I thought.\nI wonder if we can use a better read function, that would stop reading at a NUL,\nand return early instead?\n"},{"id":"291556","messageId":"xmqqk2gmn5m3.fsf@gitster.mtv.corp.google.com","threadId":"42856","inReplyTo":"20160715102806.GB19271@sigill.intra.peff.net","subject":"Re: [PATCH 02/12] rev-list: add optional progress reporting","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-07-15T18:00:52Z","receivedAt":"2016-07-15T18:01:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> diff --git a/builtin/rev-list.c b/builtin/rev-list.c\n> index b82bcc3..88d95a7 100644\n> --- a/builtin/rev-list.c\n> +++ b/builtin/rev-list.c\n> @@ -9,6 +9,7 @@\n>  #include \"log-tree.h\"\n>  #include \"graph.h\"\n>  #include \"bisect.h\"\n> +#include \"progress.h\"\n>  \n>  static const char rev_list_usage[] =\n>  \"git rev-list [OPTION] <commit-id>... [ -- paths... ]\\n\"\n> @@ -49,12 +50,17 @@ static const char rev_list_usage[] =\n>  \"    --bisect-all\"\n>  ;\n>  \n> +struct progress *progress;\n> +unsigned progress_counter;\n\nAre these supposed to be file-scope static?\n\n"},{"id":"291557","messageId":"xmqqfuran50r.fsf@gitster.mtv.corp.google.com","threadId":"42856","inReplyTo":"20160715103039.GD19271@sigill.intra.peff.net","subject":"Re: [PATCH 04/12] check_everything_connected: use a struct with named options","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-07-15T18:13:40Z","receivedAt":"2016-07-15T18:13:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> The number of variants of check_everything_connected has\n> grown over the years, so that the \"real\" function takes\n> several possibly-zero, possibly-NULL arguments. We hid the\n> complexity behind some wrapper functions, but this doesn't\n> scale well when we want to add new options.\n\nI was kind of embarrassed to admit that I wasn't even aware that\nthings got this bad, so I took a look at the history to realize that\n\"grown over the years\" above is a bit misleading statement.  It is\nnot like many people over the years were doing something like this.\nThere are only two commits that brought in this pattern with poor\ndesign taste.\n\n> Instead, let's add a struct to hold all of the optional\n> parameters.\n> ...\n>  connected.c            | 39 +++++++++++----------------------------\n\nYup, that is absolutely the right thing to do.\n\nThanks.\n"},{"id":"291558","messageId":"xmqqbn1yn4r8.fsf@gitster.mtv.corp.google.com","threadId":"42856","inReplyTo":"20160715103203.GE19271@sigill.intra.peff.net","subject":"Re: [PATCH 05/12] check_connected: relay errors to alternate descriptor","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-07-15T18:19:23Z","receivedAt":"2016-07-15T18:19:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> +\t/*\n> +\t * If non-zero, send error messages to this descriptor rather\n> +\t * than stderr. The descriptor is closed before check_connected\n> +\t * returns.\n> +\t */\n> +\tint err_fd;\n\nTheoretically speaking it may be possible that a caller may want to\nwrite to fd#0 if it closed the standard input before creating the\noutput channel for multiplexing into a sideband, but I think this\ndesign strikes a good balance between the theoretical correctness\nand usability.  Using err_fd == -1 as \"no redirect\" may allow the\ncaller to redirect the errors to fd#0, but that forces normal users\nto explicitly set this field to -1.\n"},{"id":"291561","messageId":"xmqq7fcmn20h.fsf@gitster.mtv.corp.google.com","threadId":"42856","inReplyTo":"20160715104347.GL19271@sigill.intra.peff.net","subject":"Re: [PATCH 12/12] receive-pack: send keepalives during quiet periods","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-07-15T19:18:38Z","receivedAt":"2016-07-15T19:18:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> diff --git a/builtin/index-pack.c b/builtin/index-pack.c\n> index 1cba120..54f2cfb 100644\n> --- a/builtin/index-pack.c\n> +++ b/builtin/index-pack.c\n> @@ -1626,6 +1626,7 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix)\n>  \tstruct pack_idx_option opts;\n>  \tunsigned char pack_sha1[20];\n>  \tunsigned foreign_nr = 1;\t/* zero is a \"good\" value, assume bad */\n> +\tint report_end_of_input = 0;\n>  \n>  \tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n>  \t\tusage(index_pack_usage);\n> @@ -1697,6 +1698,8 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix)\n>  \t\t\t\tverbose = 1;\n>  \t\t\t} else if (!strcmp(arg, \"--show-resolving-progress\")) {\n>  \t\t\t\tshow_resolving_progress = 1;\n> +\t\t\t} else if (!strcmp(arg, \"--report-end-of-input\")) {\n> +\t\t\t\treport_end_of_input = 1;\n>  \t\t\t} else if (!strcmp(arg, \"-o\")) {\n>  \t\t\t\tif (index_name || (i+1) >= argc)\n>  \t\t\t\t\tusage(index_pack_usage);\n> @@ -1754,6 +1757,8 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix)\n>  \t\tobj_stat = xcalloc(st_add(nr_objects, 1), sizeof(struct object_stat));\n>  \tofs_deltas = xcalloc(nr_objects, sizeof(struct ofs_delta_entry));\n>  \tparse_pack_objects(pack_sha1);\n> +\tif (report_end_of_input)\n> +\t\twrite_in_full(2, \"\\0\", 1);\n>  \tresolve_deltas();\n>  \tconclude_pack(fix_thin_pack, curr_pack, pack_sha1);\n>  \tfree(ofs_deltas);\n\nI naively thought \"well, if we are teaching index-pack a new trick\nanyway, why not make it do the keepalive?\", but that would not work\nbecause a keepalive is a 0-byte payload on band#1, i.e. \"0005\\1\",\nand index-pack is not aware of the sideband framing at all.\n\nSo I agree that a-nul-in-fd#2 is the best mechansim we can make it\nwork.\n\nThanks.\n"},{"id":"291574","messageId":"20160716012303.GA30820@sigill.intra.peff.net","threadId":"42856","inReplyTo":"xmqqk2gmn5m3.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 02/12] rev-list: add optional progress reporting","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-07-16T01:23:04Z","receivedAt":"2016-07-16T01:23:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jul 15, 2016 at 11:00:52AM -0700, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > diff --git a/builtin/rev-list.c b/builtin/rev-list.c\n> > index b82bcc3..88d95a7 100644\n> > --- a/builtin/rev-list.c\n> > +++ b/builtin/rev-list.c\n> > @@ -9,6 +9,7 @@\n> >  #include \"log-tree.h\"\n> >  #include \"graph.h\"\n> >  #include \"bisect.h\"\n> > +#include \"progress.h\"\n> >  \n> >  static const char rev_list_usage[] =\n> >  \"git rev-list [OPTION] <commit-id>... [ -- paths... ]\\n\"\n> > @@ -49,12 +50,17 @@ static const char rev_list_usage[] =\n> >  \"    --bisect-all\"\n> >  ;\n> >  \n> > +struct progress *progress;\n> > +unsigned progress_counter;\n> \n> Are these supposed to be file-scope static?\n\nYep, they should be (I had originally made them part of the rev_info,\nbut then forgot to give them \"static\" when I pulled them out).\n\n-Peff\n"},{"id":"291575","messageId":"20160716012431.GB30820@sigill.intra.peff.net","threadId":"42856","inReplyTo":"xmqqfuran50r.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 04/12] check_everything_connected: use a struct with named options","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-07-16T01:24:31Z","receivedAt":"2016-07-16T01:24:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jul 15, 2016 at 11:13:40AM -0700, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > The number of variants of check_everything_connected has\n> > grown over the years, so that the \"real\" function takes\n> > several possibly-zero, possibly-NULL arguments. We hid the\n> > complexity behind some wrapper functions, but this doesn't\n> > scale well when we want to add new options.\n> \n> I was kind of embarrassed to admit that I wasn't even aware that\n> things got this bad, so I took a look at the history to realize that\n> \"grown over the years\" above is a bit misleading statement.  It is\n> not like many people over the years were doing something like this.\n> There are only two commits that brought in this pattern with poor\n> design taste.\n\nHeh. It is easy to lose sight of such things when it is not your primary\ngoal. You should see how horrible the code was that I wrote in my\noriginal iteration, before I went back and did this refactoring. I am\nembarrassed to have even written it in a rough draft. ;)\n\n-Peff\n"},{"id":"291576","messageId":"20160716012755.GC30820@sigill.intra.peff.net","threadId":"42856","inReplyTo":"xmqqbn1yn4r8.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 05/12] check_connected: relay errors to alternate descriptor","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-07-16T01:27:56Z","receivedAt":"2016-07-16T01:28:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jul 15, 2016 at 11:19:23AM -0700, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > +\t/*\n> > +\t * If non-zero, send error messages to this descriptor rather\n> > +\t * than stderr. The descriptor is closed before check_connected\n> > +\t * returns.\n> > +\t */\n> > +\tint err_fd;\n> \n> Theoretically speaking it may be possible that a caller may want to\n> write to fd#0 if it closed the standard input before creating the\n> output channel for multiplexing into a sideband, but I think this\n> design strikes a good balance between the theoretical correctness\n> and usability.  Using err_fd == -1 as \"no redirect\" may allow the\n> caller to redirect the errors to fd#0, but that forces normal users\n> to explicitly set this field to -1.\n\nYep, I had all of those thoughts while writing it, but decided that \"0\"\nwas worth it to keep the initialization simple (though since we have\nCHECK_CONNECTED_INIT, the \"-1\" would only have to appear there).\n\nWhat decided me were two things:\n\n  - directing to fd#0 really is theoretical; you'd have to close stdin,\n    and you shouldn't do that. You should redirect it from /dev/null\n    (and our sanitize_stdfds() makes sure that we're not surprised by\n    any callers)\n\n  - \"struct child_process\" uses a similar zero-initialization\n\n-Peff\n"},{"id":"291590","messageId":"20160716075621.GA10275@sigill.intra.peff.net","threadId":"42856","inReplyTo":"CAGZ79kbernZHx9SUG-_vbxy-g77_3ki1uc-21LCviwrH=aXp6w@mail.gmail.com","subject":"Re: [PATCH 12/12] receive-pack: send keepalives during quiet periods","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-07-16T07:56:21Z","receivedAt":"2016-07-16T07:56:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jul 15, 2016 at 10:24:16AM -0700, Stefan Beller wrote:\n\n> > @@ -319,10 +331,60 @@ static void rp_error(const char *err, ...)\n> >  static int copy_to_sideband(int in, int out, void *arg)\n> >  {\n> >         char data[128];\n> \n> While looking at this code, do you think it is feasible to increase the\n> size of data[] to 1024 ? (The largest that is possible when\n> side-band, but no side-band-64k is given).\n\nI don't see any reason it could not be increased. On the other hand, I\nfind it unlikely to accomplish anything. We are relaying stderr over\nsideband 2 here, so assuming that typically consists of human-readable\nlines, they're unlikely to go over 128 (and if they do, they simply get\nsplit into two packets).\n\nI suppose if your hooks dump large ascii art to the user, we could save\na few framing bytes.\n\nSo I don't mind it, but it's definitely orthogonal to this series.\n\n> The method was short and concise, this adds a lot of lines.\n> Remembering d751dd11 (2016-07-10, hoist out handle_nonblock\n> function for xread and xwrite), do you think it would be reasonable to\n> put the whole poll handling into a dedicated function, maybe even reuse the\n> that function?\n> \n>     if (keepalive_active) {\n>         if (wrapper_around_poll(&data_in) < 0) // handles EINTR internally\n>             break;\n>         if (!data_in)\n>             send_keep_alive();\n>     }\n> \n> I am not sure if that makes this function more legible, just food for thought.\n\nThat would skip the EINTR continue, though since we're looping anyway\nhere, I'm not sure it's a big deal. The biggest simplification is more\nlike this, I think:\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex e41f55f..8d792a2 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -328,6 +328,32 @@ static void rp_error(const char *err, ...)\n \tva_end(params);\n }\n \n+/* returns 1 if data is available, 0 otherwise */\n+static int keepalive_poll(int in, int out)\n+{\n+\tstruct pollfd pfd;\n+\tint ret;\n+\n+\tpfd.fd = fd;\n+\tpfd.events = POLLIN;\n+\tret = poll(&pfd, 1, 1000 * keepalive_in_sec);\n+\n+\tif (ret < 0) {\n+\t\tif (errno == EINTR)\n+\t\t\treturn 0; /* no data, we should try again */\n+\t\telse\n+\t\t\treturn 1; /* no data, but read should return real error */\n+\t}\n+\tif (ret == 0) {\n+\t\t/* no data; send a keepalive packet */\n+\t\tstatic const char buf[] = \"0005\\1\";\n+\t\twrite_or_die(out, buf, sizeof(buf) - 1);\n+\t\treturn 0;\n+\t}\n+\t/* else there is actual data to read */\n+\treturn 1;\n+}\n+\n static int copy_to_sideband(int in, int out, void *arg)\n {\n \tchar data[128];\n@@ -341,26 +367,8 @@ static int copy_to_sideband(int in, int out, void *arg)\n \twhile (1) {\n \t\tssize_t sz;\n \n-\t\tif (keepalive_active) {\n-\t\t\tstruct pollfd pfd;\n-\t\t\tint ret;\n-\n-\t\t\tpfd.fd = in;\n-\t\t\tpfd.events = POLLIN;\n-\t\t\tret = poll(&pfd, 1, 1000 * keepalive_in_sec);\n-\n-\t\t\tif (ret < 0) {\n-\t\t\t\tif (errno == EINTR)\n-\t\t\t\t\tcontinue;\n-\t\t\t\telse\n-\t\t\t\t\tbreak;\n-\t\t\t} else if (ret == 0) {\n-\t\t\t\t/* no data; send a keepalive packet */\n-\t\t\t\tstatic const char buf[] = \"0005\\1\";\n-\t\t\t\twrite_or_die(1, buf, sizeof(buf) - 1);\n-\t\t\t\tcontinue;\n-\t\t\t} /* else there is actual data to read */\n-\t\t}\n+\t\tif (keepalive_active && !keepalive_poll(in, 1))\n+\t\t\tcontinue;\n \n \t\tsz = xread(in, data, sizeof(data));\n \t\tif (sz <= 0)\n\nNote that there are actually three outcomes to poll (error, timeout, or\ndata), and this simplifies away the \"error\" case to just \"try to read\nsome data\" and assumes that the read() will cover (which is similar to\nwhat xread() does).\n\nI dunno if it really helps much, though. There's still a bunch of\nkeepalive logic in the function to handle the NUL-signal case.\n\nI guess your proposal is more to have an \"xpoll\" that handles the EINTR\nlooping, and leave that logic in place. I think that still leaves most\nof the complexity in the function.\n\n> > +                       } else if (ret == 0) {\n> > +                               /* no data; send a keepalive packet */\n> > +                               static const char buf[] = \"0005\\1\";\n> \n> and the \\1 is the first sideband. Why do we choose that sideband?\n\nWhat other sideband do you propose? :)\n\nMore seriously, this is modeled on the similar keepalive in upload-pack.\nThe only other option is sideband 2, and IIRC, clients do bad things\nwith empty band-2 packets (like printing \"remote: \" but never finishing\nthe line if we never send any actual data).\n\n> > +                               write_or_die(1, buf, sizeof(buf) - 1);\n> > +                               continue;\n> > +                       } /* else there is actual data to read */\n> \n> \"If there is data to read, we need to break the while(1), to actually\n> read the data?\"\n\nWe're not breaking the while; we're falling through to the read call.\n\n> I got confused and needed to go back and read the actual code again,\n> would it make sense to rather have a loop here?\n> \n>     while (1) {\n>        while(keepalive_active) {\n>         if (wrapper_around_poll(&data_in) < 0) // handles EINTR internally\n>             break;\n>         if (!data_in)\n>             send_keep_alive();\n>         else\n>             break;\n>         }\n\nThis doesn't do the same thing. On poll() error, you are only breaking\nout of the inner loop. My intent was to stop copy_to_sideband()\nentirely. Perhaps this is the source of confusion; you were thinking of\nthis more like the loop in xread() where we _want_ to do the final\nread() (even though we expect it to be an error!) to give the caller the\ncorrect errno value.\n\nWhereas here, I think it's fine to leave as soon as poll() complains. At\nleast that was how I wrote it.\n\nI don't think doing it your way is actually wrong, it just wasn't what I\nintended.\n\n> > +               if (use_keepalive == KEEPALIVE_AFTER_NUL && !keepalive_active) {\n> > +                       const char *p = memchr(data, '\\0', sz);\n> > +                       if (p) {\n> > +                               /*\n> > +                                * The NUL tells us to start sending keepalives. Make\n> > +                                * sure we send any other data we read along\n> > +                                * with it.\n> > +                                */\n> > +                               keepalive_active = 1;\n> > +                               send_sideband(1, 2, data, p - data, use_sideband);\n> > +                               send_sideband(1, 2, p + 1, sz - (p - data + 1), use_sideband);\n> > +                               continue;\n> \n> Oh, I see why the turn_on_keepalive_on_NUL doesn't work as well as I thought.\n> I wonder if we can use a better read function, that would stop reading at a NUL,\n> and return early instead?\n\nHow would you do that, if not by read()ing a byte at a time (which is\ninefficient)? Otherwise you have to deal with the leftovers (after the\nNUL) in your buffer. It's one of the reasons I went with a single-byte\nsignal, because otherwise it gets rather complicated to do robustly.\n\n-Peff\n"},{"id":"291706","messageId":"CAGZ79kZPbSTAv6zjJ01PdqBOZrsfhRAte_v-mbBzXuOAWNK+Tg@mail.gmail.com","threadId":"42856","inReplyTo":"20160716075621.GA10275@sigill.intra.peff.net","subject":"Re: [PATCH 12/12] receive-pack: send keepalives during quiet periods","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-07-19T05:28:25Z","receivedAt":"2016-07-19T05:28:31Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Sat, Jul 16, 2016 at 12:56 AM, Jeff King <peff@peff.net> wrote:\n>> > +               if (use_keepalive == KEEPALIVE_AFTER_NUL && !keepalive_active) {\n>> > +                       const char *p = memchr(data, '\\0', sz);\n>> > +                       if (p) {\n>> > +                               /*\n>> > +                                * The NUL tells us to start sending keepalives. Make\n>> > +                                * sure we send any other data we read along\n>> > +                                * with it.\n>> > +                                */\n>> > +                               keepalive_active = 1;\n>> > +                               send_sideband(1, 2, data, p - data, use_sideband);\n>> > +                               send_sideband(1, 2, p + 1, sz - (p - data + 1), use_sideband);\n>> > +                               continue;\n>>\n>> Oh, I see why the turn_on_keepalive_on_NUL doesn't work as well as I thought.\n>> I wonder if we can use a better read function, that would stop reading at a NUL,\n>> and return early instead?\n>\n> How would you do that, if not by read()ing a byte at a time (which is\n> inefficient)? Otherwise you have to deal with the leftovers (after the\n> NUL) in your buffer. It's one of the reasons I went with a single-byte\n> signal, because otherwise it gets rather complicated to do robustly.\n\nI do not question the concept of a single NUL byte, but rather the\nimplementation,\ni.e. if we had an xread_until_nul you would not need to have a double\nsend_sideband\nhere?\n\n>\n> -Peff\n"},{"id":"291712","messageId":"20160719100730.GA5193@sigill.intra.peff.net","threadId":"42856","inReplyTo":"CAGZ79kZPbSTAv6zjJ01PdqBOZrsfhRAte_v-mbBzXuOAWNK+Tg@mail.gmail.com","subject":"Re: [PATCH 12/12] receive-pack: send keepalives during quiet periods","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-07-19T10:07:31Z","receivedAt":"2016-07-19T10:07:39Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jul 18, 2016 at 10:28:25PM -0700, Stefan Beller wrote:\n\n> On Sat, Jul 16, 2016 at 12:56 AM, Jeff King <peff@peff.net> wrote:\n> >> > +               if (use_keepalive == KEEPALIVE_AFTER_NUL && !keepalive_active) {\n> >> > +                       const char *p = memchr(data, '\\0', sz);\n> >> > +                       if (p) {\n> >> > +                               /*\n> >> > +                                * The NUL tells us to start sending keepalives. Make\n> >> > +                                * sure we send any other data we read along\n> >> > +                                * with it.\n> >> > +                                */\n> >> > +                               keepalive_active = 1;\n> >> > +                               send_sideband(1, 2, data, p - data, use_sideband);\n> >> > +                               send_sideband(1, 2, p + 1, sz - (p - data + 1), use_sideband);\n> >> > +                               continue;\n> >>\n> >> Oh, I see why the turn_on_keepalive_on_NUL doesn't work as well as I thought.\n> >> I wonder if we can use a better read function, that would stop reading at a NUL,\n> >> and return early instead?\n> >\n> > How would you do that, if not by read()ing a byte at a time (which is\n> > inefficient)? Otherwise you have to deal with the leftovers (after the\n> > NUL) in your buffer. It's one of the reasons I went with a single-byte\n> > signal, because otherwise it gets rather complicated to do robustly.\n> \n> I do not question the concept of a single NUL byte, but rather the\n> implementation, i.e. if we had an xread_until_nul you would not need\n> to have a double send_sideband here?\n\nWhat would xread_until_nul() look like?\n\nThe only reasonable implementation I can think of is:\n\n  ssize_t xread_until_nul(int fd, char *out, size_t len)\n  {\n\tssize_t total = 0;\n\twhile (total < len) {\n\t\tssize_t ret = xread(fd, out + total, 1);\n\t\tif (ret < 0) {\n\t\t\t/* Oops, anything in out[0..total] is lost, but\n\t\t\t * we have no way of signaling both a partial\n\t\t\t * read and an error at the end; fixable by\n\t\t\t * changing the interface, but doesn't really\n\t\t\t * matter in practice for this application. */\n\t\t\treturn -1;\n\t\t}\n\t\tif (ret == 0)\n\t\t\tbreak; /* EOF, stop reading */\n\t\tif (out[total] == '\\0')\n\t\t\tbreak; /* got our NUL, stop reading */\n\t\ttotal++;\n\t}\n\treturn total;\n  }\n\nThere are two problems with this function:\n\n  1. Until we see the NUL, we're making an excessive number of read()\n     syscalls looking for it. You could make larger reads, but then what\n     do you do with the residual bytes (i.e., the ones after the NUL in\n     the buffer you read)? You'd have to somehow save it to return at\n     the next xread_until_nul(). Which implie some kind of internal\n     buffering.\n\n     The reason there are two send_sidebands is to cover the case where\n     we have some real data, then the signal byte, then some more data.\n     So instead of buffering, we just handle the data immediately.\n\n  2. How does it know when to return?\n\n     We want to send the data as soon as it is available, in as large a\n     chunk as possible. Using a single xread() as we do now, we get\n     whatever the OS has for us, up to our buffer size.\n\n     But after each 1-byte read above, we would not want to return;\n     there might be more data. So it keeps reading until NUL or we fill\n     our buffer. But that may mean the xread() blocks, even though we\n     have data that _could_ be shipped over the sideband.\n\n     To fix that, you'd have to poll() for each xread(), and return when\n     it says nothing's ready.\n\nI know that writing the function myself and then critiquing is the very\ndefinition of a strawman. :) But it's the best I could think of.  Maybe\nyou had something more clever in mind?\n\n-Peff\n"},{"id":"291726","messageId":"CAGZ79kZ7m94T5+SHjeWcY_cDLYSP9X74eDOOjfmZ1GwAb-C-7g@mail.gmail.com","threadId":"42856","inReplyTo":"20160719100730.GA5193@sigill.intra.peff.net","subject":"Re: [PATCH 12/12] receive-pack: send keepalives during quiet periods","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-07-19T16:05:14Z","receivedAt":"2016-07-19T16:05:29Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Jul 19, 2016 at 3:07 AM, Jeff King <peff@peff.net> wrote:\n> On Mon, Jul 18, 2016 at 10:28:25PM -0700, Stefan Beller wrote:\n>\n>> On Sat, Jul 16, 2016 at 12:56 AM, Jeff King <peff@peff.net> wrote:\n>> >> > +               if (use_keepalive == KEEPALIVE_AFTER_NUL && !keepalive_active) {\n>> >> > +                       const char *p = memchr(data, '\\0', sz);\n>> >> > +                       if (p) {\n>> >> > +                               /*\n>> >> > +                                * The NUL tells us to start sending keepalives. Make\n>> >> > +                                * sure we send any other data we read along\n>> >> > +                                * with it.\n>> >> > +                                */\n>> >> > +                               keepalive_active = 1;\n>> >> > +                               send_sideband(1, 2, data, p - data, use_sideband);\n>> >> > +                               send_sideband(1, 2, p + 1, sz - (p - data + 1), use_sideband);\n>> >> > +                               continue;\n>> >>\n>> >> Oh, I see why the turn_on_keepalive_on_NUL doesn't work as well as I thought.\n>> >> I wonder if we can use a better read function, that would stop reading at a NUL,\n>> >> and return early instead?\n>> >\n>> > How would you do that, if not by read()ing a byte at a time (which is\n>> > inefficient)? Otherwise you have to deal with the leftovers (after the\n>> > NUL) in your buffer. It's one of the reasons I went with a single-byte\n>> > signal, because otherwise it gets rather complicated to do robustly.\n>>\n>> I do not question the concept of a single NUL byte, but rather the\n>> implementation, i.e. if we had an xread_until_nul you would not need\n>> to have a double send_sideband here?\n>\n> What would xread_until_nul() look like?\n>\n> The only reasonable implementation I can think of is:\n>\n>   ssize_t xread_until_nul(int fd, char *out, size_t len)\n>   {\n>         ssize_t total = 0;\n>         while (total < len) {\n>                 ssize_t ret = xread(fd, out + total, 1);\n>                 if (ret < 0) {\n>                         /* Oops, anything in out[0..total] is lost, but\n>                          * we have no way of signaling both a partial\n>                          * read and an error at the end; fixable by\n>                          * changing the interface, but doesn't really\n>                          * matter in practice for this application. */\n>                         return -1;\n>                 }\n>                 if (ret == 0)\n>                         break; /* EOF, stop reading */\n>                 if (out[total] == '\\0')\n>                         break; /* got our NUL, stop reading */\n>                 total++;\n>         }\n>         return total;\n>   }\n>\n> There are two problems with this function:\n>\n>   1. Until we see the NUL, we're making an excessive number of read()\n>      syscalls looking for it. You could make larger reads, but then what\n>      do you do with the residual bytes (i.e., the ones after the NUL in\n>      the buffer you read)? You'd have to somehow save it to return at\n>      the next xread_until_nul(). Which implie some kind of internal\n>      buffering.\n>\n>      The reason there are two send_sidebands is to cover the case where\n>      we have some real data, then the signal byte, then some more data.\n>      So instead of buffering, we just handle the data immediately.\n>\n>   2. How does it know when to return?\n>\n>      We want to send the data as soon as it is available, in as large a\n>      chunk as possible. Using a single xread() as we do now, we get\n>      whatever the OS has for us, up to our buffer size.\n>\n>      But after each 1-byte read above, we would not want to return;\n>      there might be more data. So it keeps reading until NUL or we fill\n>      our buffer. But that may mean the xread() blocks, even though we\n>      have data that _could_ be shipped over the sideband.\n>\n>      To fix that, you'd have to poll() for each xread(), and return when\n>      it says nothing's ready.\n>\n> I know that writing the function myself and then critiquing is the very\n> definition of a strawman. :) But it's the best I could think of.  Maybe\n> you had something more clever in mind?\n\nActually no, I had not. I was hoping you could come up with a clever thing.\nMy original point was the perceived added complexity to a simple\nseemingly simple function (copy_to_sideband in your original patch),\nso my gut reaction was to shove the complexity away into a helper function.\nBut no matter how it is done, there is always this one function that looks\ncomplex for this problem. So I think your original approach is fine then?\n\nThanks,\nStefan\n\n\n>\n> -Peff\n"},{"id":"291805","messageId":"20160720132809.GA18980@sigill.intra.peff.net","threadId":"42856","inReplyTo":"CAGZ79kZ7m94T5+SHjeWcY_cDLYSP9X74eDOOjfmZ1GwAb-C-7g@mail.gmail.com","subject":"Re: [PATCH 12/12] receive-pack: send keepalives during quiet periods","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-07-20T13:28:09Z","receivedAt":"2016-07-20T13:28:18Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jul 19, 2016 at 09:05:14AM -0700, Stefan Beller wrote:\n\n> > I know that writing the function myself and then critiquing is the very\n> > definition of a strawman. :) But it's the best I could think of.  Maybe\n> > you had something more clever in mind?\n> \n> Actually no, I had not. I was hoping you could come up with a clever thing.\n> My original point was the perceived added complexity to a simple\n> seemingly simple function (copy_to_sideband in your original patch),\n> so my gut reaction was to shove the complexity away into a helper function.\n> But no matter how it is done, there is always this one function that looks\n> complex for this problem. So I think your original approach is fine then?\n\nYeah, I agree the original was nicer, but IMHO none of the \"like this\"\nalternatives I showed in this thread are real improvements. So I'm\ninclined to go with what was originally posted (though I'm open to\nmore input or suggestions).\n\nThe only avenue I didn't look at is actually ditching the async muxer\ncompletely for the index-pack call, and replacing it with the main\nthread doing a poll() on stdout/stderr from index-pack. That's probably\ngoing to end up as _more_ code, but it potentially makes the \"report\nmagic NUL\" a little less gross (we could signal with something more sane\non stdout). I can explore that if people want me to.\n\nIf not, then the only change to the original series is adding \"static\"\nto the rev-list progress variables that Junio noticed. A replacement for\npatch 2 is below in case that makes things easier.\n\n-- >8 --\nSubject: [PATCH] rev-list: add optional progress reporting\n\nIt's easy to ask rev-list to do a traversal that may takes\nmany seconds (e.g., by calling \"--objects --all\"). In theory\nyou can monitor its progress by the output you get to\nstdout, but this isn't always easy.\n\nSome operations, like \"--count\", don't make any output until\nthe end.\n\nAnd some callers, like check_everything_connected(), are\nusing it just for the error-checking of the traversal, and\nthrow away stdout entirely.\n\nThis patch adds a \"--progress\" option which can be used to\ngive some eye-candy for a user waiting for a long traversal.\nThis is just a rev-list option and not a regular traversal\noption, because it needs cooperation from the callbacks in\nbuiltin/rev-list.c to do the actual count.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n Documentation/rev-list-options.txt |  4 ++++\n builtin/rev-list.c                 | 17 +++++++++++++++++\n 2 files changed, 21 insertions(+)\n\ndiff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt\nindex c5bd218..f39cb6d 100644\n--- a/Documentation/rev-list-options.txt\n+++ b/Documentation/rev-list-options.txt\n@@ -274,6 +274,10 @@ ifdef::git-rev-list[]\n \tTry to speed up the traversal using the pack bitmap index (if\n \tone is available). Note that when traversing with `--objects`,\n \ttrees and blobs will not have their associated path printed.\n+\n+--progress=<header>::\n+\tShow progress reports on stderr as objects are considered. The\n+\t`<header>` text will be printed with each progress update.\n endif::git-rev-list[]\n \n --\ndiff --git a/builtin/rev-list.c b/builtin/rev-list.c\nindex b82bcc3..0ba82b1 100644\n--- a/builtin/rev-list.c\n+++ b/builtin/rev-list.c\n@@ -9,6 +9,7 @@\n #include \"log-tree.h\"\n #include \"graph.h\"\n #include \"bisect.h\"\n+#include \"progress.h\"\n \n static const char rev_list_usage[] =\n \"git rev-list [OPTION] <commit-id>... [ -- paths... ]\\n\"\n@@ -49,12 +50,17 @@ static const char rev_list_usage[] =\n \"    --bisect-all\"\n ;\n \n+static struct progress *progress;\n+static unsigned progress_counter;\n+\n static void finish_commit(struct commit *commit, void *data);\n static void show_commit(struct commit *commit, void *data)\n {\n \tstruct rev_list_info *info = data;\n \tstruct rev_info *revs = info->revs;\n \n+\tdisplay_progress(progress, ++progress_counter);\n+\n \tif (info->flags & REV_LIST_QUIET) {\n \t\tfinish_commit(commit, data);\n \t\treturn;\n@@ -190,6 +196,7 @@ static void show_object(struct object *obj, const char *name, void *cb_data)\n {\n \tstruct rev_list_info *info = cb_data;\n \tfinish_object(obj, name, cb_data);\n+\tdisplay_progress(progress, ++progress_counter);\n \tif (info->flags & REV_LIST_QUIET)\n \t\treturn;\n \tshow_object_with_name(stdout, obj, name);\n@@ -276,6 +283,7 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)\n \tint bisect_show_vars = 0;\n \tint bisect_find_all = 0;\n \tint use_bitmap_index = 0;\n+\tconst char *show_progress = NULL;\n \n \tgit_config(git_default_config, NULL);\n \tinit_revisions(&revs, prefix);\n@@ -325,6 +333,10 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)\n \t\t\ttest_bitmap_walk(&revs);\n \t\t\treturn 0;\n \t\t}\n+\t\tif (skip_prefix(arg, \"--progress=\", &arg)) {\n+\t\t\tshow_progress = arg;\n+\t\t\tcontinue;\n+\t\t}\n \t\tusage(rev_list_usage);\n \n \t}\n@@ -355,6 +367,9 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)\n \tif (bisect_list)\n \t\trevs.limited = 1;\n \n+\tif (show_progress)\n+\t\tprogress = start_progress_delay(show_progress, 0, 0, 2);\n+\n \tif (use_bitmap_index && !revs.prune) {\n \t\tif (revs.count && !revs.left_right && !revs.cherry_mark) {\n \t\t\tuint32_t commit_count;\n@@ -392,6 +407,8 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)\n \n \ttraverse_commit_list(&revs, show_commit, show_object, &info);\n \n+\tstop_progress(&progress);\n+\n \tif (revs.count) {\n \t\tif (revs.left_right && revs.cherry_mark)\n \t\t\tprintf(\"%d\\t%d\\t%d\\n\", revs.count_left, revs.count_right, revs.count_same);\n-- \n2.9.2.506.gb3c8cac\n\n"}]}