{"thread":{"id":"49380","subject":"[PATCH] reflog expire: add progress output","startedAt":"2018-09-19T14:10:30Z","lastAt":"2018-09-21T02:17:31Z","messageCount":7,"participants":["Ævar Arnfjörð Bjarmason","Duy Nguyen","Jeff King","Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"358446","messageId":"20180919141016.27930-1-avarab@gmail.com","threadId":"49380","inReplyTo":null,"subject":"[PATCH] reflog expire: add progress output","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-09-19T14:10:16Z","receivedAt":"2018-09-19T14:10:30Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Before this change the \"git reflog expire\" command didn't report any\nprogress. This is the second command (after \"pack-refs --all --prune\")\nthat the \"gc\" command will run.\n\nOn small repositories like this command won't take long to run, my\ntest system it takes just under 1 second to run on git.git, but just\naround 8 seconds on linux.git, and much longer on the\n2015-04-03-1M-git.git[1] large test repository.\n\nTaking so long means that \"gc\" will appear to hang at the beginning of\nits run. It might still do so after this change if the earlier\n\"pack-refs\" command takes a really long time to run, but that'll only\nimpact repositories with a really large set of refs to pack, and can\nbe addressed in some future change.\n\nOne thing that's bad about this change is that we might *in theory*\nprint a \"Marking unreachable commits in reflog for expiry\" message for\neach ref with a reflog. This is because the abbreviated callstack\nlooks like this:\n\n    0  mark_reachable at builtin/reflog.c:227\n    1  in unreachable at builtin/reflog.c:290\n    2  in should_expire_reflog_ent at builtin/reflog.c:317\n    3  in expire_reflog_ent at refs/files-backend.c:2956\n    4  in show_one_reflog_ent at refs/files-backend.c:1879\n    # This is the last function that has the refname (e.g. \"HEAD\") available\n    5  in files_for_each_reflog_ent at refs/files-backend.c:2025\n    6  in refs_for_each_reflog_ent at refs.c:2066\n    7  in files_reflog_expire at refs/files-backend.c:3043\n    8  in refs_reflog_expire at refs.c:2117\n    9  in reflog_expire at refs.c:2129\n    # Here's where we collect reflogs to expire, and expire each one\n    10 in cmd_reflog_expire  at builtin/reflog.c:595\n\nI.e. this progress is being reported for each expired reflog. So if\nstart_progress() were used instead of start_delayed_progress() we'd\nprint (e.g. on my git.git) hundreds of these lines.\n\nIn practice I haven't been able to make it print anything except one\nline. This is because validating the reflogs for these other\nbranches (not \"HEAD\") takes such a short amount of time.\n\nThat may just be some artifact of the repositories I've tested, but I\nsuspect It'll be true in general. As the callstack above shows, in\norder to guarantee that we don't do that we'd need to pass some\n\"progress\" variable through 10 levels of functions, many of which are\n\"for_each\" callback functions with void* cb_data.\n\n1. https://github.com/avar/2015-04-03-1M-git\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n builtin/reflog.c | 8 ++++++++\n 1 file changed, 8 insertions(+)\n\ndiff --git a/builtin/reflog.c b/builtin/reflog.c\nindex 3acef5a0ab..d3075ee75a 100644\n--- a/builtin/reflog.c\n+++ b/builtin/reflog.c\n@@ -10,6 +10,7 @@\n #include \"diff.h\"\n #include \"revision.h\"\n #include \"reachable.h\"\n+#include \"progress.h\"\n \n /* NEEDSWORK: switch to using parse_options */\n static const char reflog_expire_usage[] =\n@@ -225,14 +226,20 @@ static void mark_reachable(struct expire_reflog_policy_cb *cb)\n \tstruct commit_list *pending;\n \ttimestamp_t expire_limit = cb->mark_limit;\n \tstruct commit_list *leftover = NULL;\n+\tstruct progress *progress = NULL;\n+\tint i = 0;\n \n \tfor (pending = cb->mark_list; pending; pending = pending->next)\n \t\tpending->item->object.flags &= ~REACHABLE;\n \n \tpending = cb->mark_list;\n+\tprogress = start_delayed_progress(\n+\t\t_(\"Marking unreachable commits in reflog for expiry\"), 0);\n \twhile (pending) {\n \t\tstruct commit_list *parent;\n \t\tstruct commit *commit = pop_commit(&pending);\n+\n+\t\tdisplay_progress(progress, ++i);\n \t\tif (commit->object.flags & REACHABLE)\n \t\t\tcontinue;\n \t\tif (parse_commit(commit))\n@@ -253,6 +260,7 @@ static void mark_reachable(struct expire_reflog_policy_cb *cb)\n \t\t}\n \t}\n \tcb->mark_list = leftover;\n+\tstop_progress(&progress);\n }\n \n static int unreachable(struct expire_reflog_policy_cb *cb, struct commit *commit, struct object_id *oid)\n-- \n2.19.0.444.g18242da7ef\n\n"},{"id":"358457","messageId":"CACsJy8CX8xspbsW7Ta3aOD6LHh55ZaJ0tdrYeWDP_Vyw70NXtA@mail.gmail.com","threadId":"49380","inReplyTo":"20180919141016.27930-1-avarab@gmail.com","subject":"Re: [PATCH] reflog expire: add progress output","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-09-19T16:26:14Z","receivedAt":"2018-09-19T16:26:43Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, Sep 19, 2018 at 4:23 PM Ævar Arnfjörð Bjarmason\n<avarab@gmail.com> wrote:\n> Before this change the \"git reflog expire\" command didn't report any\n> progress.\n\nI love these progress additions you've been pushing lately :)\n\n> diff --git a/builtin/reflog.c b/builtin/reflog.c\n> index 3acef5a0ab..d3075ee75a 100644\n> --- a/builtin/reflog.c\n> +++ b/builtin/reflog.c\n> @@ -10,6 +10,7 @@\n>  #include \"diff.h\"\n>  #include \"revision.h\"\n>  #include \"reachable.h\"\n> +#include \"progress.h\"\n>\n>  /* NEEDSWORK: switch to using parse_options */\n>  static const char reflog_expire_usage[] =\n> @@ -225,14 +226,20 @@ static void mark_reachable(struct expire_reflog_policy_cb *cb)\n>         struct commit_list *pending;\n>         timestamp_t expire_limit = cb->mark_limit;\n>         struct commit_list *leftover = NULL;\n> +       struct progress *progress = NULL;\n> +       int i = 0;\n>\n>         for (pending = cb->mark_list; pending; pending = pending->next)\n>                 pending->item->object.flags &= ~REACHABLE;\n>\n>         pending = cb->mark_list;\n> +       progress = start_delayed_progress(\n> +               _(\"Marking unreachable commits in reflog for expiry\"), 0);\n\nMaybe just \"Searching expired reflog entries\" or something like that.\nIt's not technically as accurate, but I think it's easier to\nunderstand by by new people.\n\nDo we have --quiet option or something that needs to completely\nsuppress this progress thing?\n\n>         while (pending) {\n>                 struct commit_list *parent;\n>                 struct commit *commit = pop_commit(&pending);\n> +\n> +               display_progress(progress, ++i);\n\nmaybe rename it to commit_count or something and leave \"i\" for\ntemporary/short lived usage.\n\n>                 if (commit->object.flags & REACHABLE)\n>                         continue;\n>                 if (parse_commit(commit))\n> @@ -253,6 +260,7 @@ static void mark_reachable(struct expire_reflog_policy_cb *cb)\n>                 }\n>         }\n>         cb->mark_list = leftover;\n> +       stop_progress(&progress);\n>  }\n>\n>  static int unreachable(struct expire_reflog_policy_cb *cb, struct commit *commit, struct object_id *oid)\n> --\n> 2.19.0.444.g18242da7ef\n>\n-- \nDuy\n"},{"id":"358467","messageId":"87tvmljtaz.fsf@evledraar.gmail.com","threadId":"49380","inReplyTo":"CACsJy8CX8xspbsW7Ta3aOD6LHh55ZaJ0tdrYeWDP_Vyw70NXtA@mail.gmail.com","subject":"Re: [PATCH] reflog expire: add progress output","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-09-19T17:22:44Z","receivedAt":"2018-09-19T17:22:49Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Sep 19 2018, Duy Nguyen wrote:\n\n> On Wed, Sep 19, 2018 at 4:23 PM Ævar Arnfjörð Bjarmason\n> <avarab@gmail.com> wrote:\n>> Before this change the \"git reflog expire\" command didn't report any\n>> progress.\n>\n> I love these progress additions you've been pushing lately :)\n>\n>> diff --git a/builtin/reflog.c b/builtin/reflog.c\n>> index 3acef5a0ab..d3075ee75a 100644\n>> --- a/builtin/reflog.c\n>> +++ b/builtin/reflog.c\n>> @@ -10,6 +10,7 @@\n>>  #include \"diff.h\"\n>>  #include \"revision.h\"\n>>  #include \"reachable.h\"\n>> +#include \"progress.h\"\n>>\n>>  /* NEEDSWORK: switch to using parse_options */\n>>  static const char reflog_expire_usage[] =\n>> @@ -225,14 +226,20 @@ static void mark_reachable(struct expire_reflog_policy_cb *cb)\n>>         struct commit_list *pending;\n>>         timestamp_t expire_limit = cb->mark_limit;\n>>         struct commit_list *leftover = NULL;\n>> +       struct progress *progress = NULL;\n>> +       int i = 0;\n>>\n>>         for (pending = cb->mark_list; pending; pending = pending->next)\n>>                 pending->item->object.flags &= ~REACHABLE;\n>>\n>>         pending = cb->mark_list;\n>> +       progress = start_delayed_progress(\n>> +               _(\"Marking unreachable commits in reflog for expiry\"), 0);\n>\n> Maybe just \"Searching expired reflog entries\" or something like that.\n> It's not technically as accurate, but I think it's easier to\n> understand by by new people.\n\nOr \"Pruning reflog entries\"? I was aiming for some combination of a)\nmaking it clear what we're doing (pruning stuff) b) that we're doing it\non a subset of the counter of the (very large) values we're showing.\n\nSo the current one has \"a\" && \"b\", \"Searching...\" has neither, and\n\"Pruning...\" has \"a\" but not \"b\".\n\nMaybe making a && b clear isn't that important, but I'm currently\nleaning towards keeping the current one because it's not *that* long and\nmakes things clearer to the user.\n\n> Do we have --quiet option or something that needs to completely\n> suppress this progress thing?\n\nYes. I also see my commit graph process patches sitting in \"next\" broke\nthe \"git gc --quiet\" mode, and I'll need to submit something on top\n(which'll be easy), and submit a v2 on this (pending further\ncomments...).\n\nIs there a better way to test that (fake up the file descriptor check)\nin the tests other than adding getenv(\"GIT_TEST...\") to the progress.c\nlogic?\n\n>>         while (pending) {\n>>                 struct commit_list *parent;\n>>                 struct commit *commit = pop_commit(&pending);\n>> +\n>> +               display_progress(progress, ++i);\n>\n> maybe rename it to commit_count or something and leave \"i\" for\n> temporary/short lived usage.\n\nGood point. Willdo.\n\n>>                 if (commit->object.flags & REACHABLE)\n>>                         continue;\n>>                 if (parse_commit(commit))\n>> @@ -253,6 +260,7 @@ static void mark_reachable(struct expire_reflog_policy_cb *cb)\n>>                 }\n>>         }\n>>         cb->mark_list = leftover;\n>> +       stop_progress(&progress);\n>>  }\n>>\n>>  static int unreachable(struct expire_reflog_policy_cb *cb, struct commit *commit, struct object_id *oid)\n>> --\n>> 2.19.0.444.g18242da7ef\n>>\n"},{"id":"358477","messageId":"20180919190110.GA14552@sigill.intra.peff.net","threadId":"49380","inReplyTo":"87tvmljtaz.fsf@evledraar.gmail.com","subject":"Re: [PATCH] reflog expire: add progress output","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-09-19T19:01:10Z","receivedAt":"2018-09-19T19:01:14Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Sep 19, 2018 at 07:22:44PM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> > Do we have --quiet option or something that needs to completely\n> > suppress this progress thing?\n> \n> Yes. I also see my commit graph process patches sitting in \"next\" broke\n> the \"git gc --quiet\" mode, and I'll need to submit something on top\n> (which'll be easy), and submit a v2 on this (pending further\n> comments...).\n> \n> Is there a better way to test that (fake up the file descriptor check)\n> in the tests other than adding getenv(\"GIT_TEST...\") to the progress.c\n> logic?\n\nThe progress code doesn't do the isatty() check at all. The caller has\nto do it (and ideally would respect --progress/--no-progress to\noverride, along with having --quiet imply --no-progress if such an\noption exists).\n\nOnce you have all that, then you can test --progress explicitly. If you\nwant to check the isatty() handling, you can use test_terminal().\n\n-Peff\n"},{"id":"358485","messageId":"87r2hpjl8p.fsf@evledraar.gmail.com","threadId":"49380","inReplyTo":"20180919190110.GA14552@sigill.intra.peff.net","subject":"Re: [PATCH] reflog expire: add progress output","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-09-19T20:16:54Z","receivedAt":"2018-09-19T20:17:00Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Sep 19 2018, Jeff King wrote:\n\n> On Wed, Sep 19, 2018 at 07:22:44PM +0200, Ævar Arnfjörð Bjarmason wrote:\n>\n>> > Do we have --quiet option or something that needs to completely\n>> > suppress this progress thing?\n>>\n>> Yes. I also see my commit graph process patches sitting in \"next\" broke\n>> the \"git gc --quiet\" mode, and I'll need to submit something on top\n>> (which'll be easy), and submit a v2 on this (pending further\n>> comments...).\n>>\n>> Is there a better way to test that (fake up the file descriptor check)\n>> in the tests other than adding getenv(\"GIT_TEST...\") to the progress.c\n>> logic?\n>\n> The progress code doesn't do the isatty() check at all. The caller has\n> to do it (and ideally would respect --progress/--no-progress to\n> override, along with having --quiet imply --no-progress if such an\n> option exists).\n\nYeah, what I was confused by was testing this with \"git gc\", and until\nmy recent commit graph progress patches + this (which I wasn't testing\nagainst) the progress output was all from pack-objects, which checks the\n--progress option, and then proceeds to ignore all of that and just\ncheck isatty().\n\n> Once you have all that, then you can test --progress explicitly. If you\n> want to check the isatty() handling, you can use test_terminal().\n\nThanks.\n"},{"id":"358517","messageId":"20180920161928.GA13379@duynguyen.home","threadId":"49380","inReplyTo":"87tvmljtaz.fsf@evledraar.gmail.com","subject":"Re: [PATCH] reflog expire: add progress output","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-09-20T16:19:28Z","receivedAt":"2018-09-20T16:19:36Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, Sep 19, 2018 at 07:22:44PM +0200, Ævar Arnfjörð Bjarmason wrote:\n> >> @@ -225,14 +226,20 @@ static void mark_reachable(struct expire_reflog_policy_cb *cb)\n> >>         struct commit_list *pending;\n> >>         timestamp_t expire_limit = cb->mark_limit;\n> >>         struct commit_list *leftover = NULL;\n> >> +       struct progress *progress = NULL;\n> >> +       int i = 0;\n> >>\n> >>         for (pending = cb->mark_list; pending; pending = pending->next)\n> >>                 pending->item->object.flags &= ~REACHABLE;\n> >>\n> >>         pending = cb->mark_list;\n> >> +       progress = start_delayed_progress(\n> >> +               _(\"Marking unreachable commits in reflog for expiry\"), 0);\n> >\n> > Maybe just \"Searching expired reflog entries\" or something like that.\n> > It's not technically as accurate, but I think it's easier to\n> > understand by by new people.\n> \n> Or \"Pruning reflog entries\"? I was aiming for some combination of a)\n> making it clear what we're doing (pruning stuff) b) that we're doing it\n> on a subset of the counter of the (very large) values we're showing.\n> \n> So the current one has \"a\" && \"b\", \"Searching...\" has neither, and\n> \"Pruning...\" has \"a\" but not \"b\".\n> \n> Maybe making a && b clear isn't that important, but I'm currently\n> leaning towards keeping the current one because it's not *that* long and\n> makes things clearer to the user.\n\nOK\n\n> >>         while (pending) {\n> >>                 struct commit_list *parent;\n> >>                 struct commit *commit = pop_commit(&pending);\n> >> +\n> >> +               display_progress(progress, ++i);\n> >\n> > maybe rename it to commit_count or something and leave \"i\" for\n> > temporary/short lived usage.\n> \n> Good point. Willdo.\n\nActually I'm still not sure if it's valuable to show the actual commit\ncount here. Some patch like this could show \"activity\" without the\nnumber. But this is tangent.\n\n-- 8< --\nSubject: [PATCH] progress: add api for displaying a throbber\n\n---\n progress.c | 29 +++++++++++++++++++++++++----\n progress.h |  1 +\n 2 files changed, 26 insertions(+), 4 deletions(-)\n\ndiff --git a/progress.c b/progress.c\nindex 5a99c9fbf0..fada556bf0 100644\n--- a/progress.c\n+++ b/progress.c\n@@ -36,6 +36,7 @@ struct progress {\n \tunsigned delay;\n \tstruct throughput *throughput;\n \tuint64_t start_ns;\n+\tint show_throbber;\n };\n \n static volatile sig_atomic_t progress_update;\n@@ -81,11 +82,13 @@ static int is_foreground_fd(int fd)\n static int display(struct progress *progress, uint64_t n, const char *done)\n {\n \tconst char *eol, *tp;\n+\tstatic char throbber[] = \"\\\\|/-\";\n \n \tif (progress->delay && (!progress_update || --progress->delay))\n \t\treturn 0;\n \n-\tprogress->last_value = n;\n+\tif (!progress->show_throbber)\n+\t\tprogress->last_value = n;\n \ttp = (progress->throughput) ? progress->throughput->display.buf : \"\";\n \teol = done ? done : \"   \\r\";\n \tif (progress->total) {\n@@ -104,8 +107,15 @@ static int display(struct progress *progress, uint64_t n, const char *done)\n \t\t}\n \t} else if (progress_update) {\n \t\tif (is_foreground_fd(fileno(stderr)) || done) {\n-\t\t\tfprintf(stderr, \"%s: %\"PRIuMAX\"%s%s\",\n-\t\t\t\tprogress->title, (uintmax_t)n, tp, eol);\n+\t\t\tif (progress->show_throbber) {\n+\t\t\t\tprogress->last_value++;\n+\t\t\t\tfprintf(stderr, \"%s: %c%s%s\",\n+\t\t\t\t\tprogress->title,\n+\t\t\t\t\tthrobber[progress->last_value % 4],\n+\t\t\t\t\ttp, eol);\n+\t\t\t} else\n+\t\t\t\tfprintf(stderr, \"%s: %\"PRIuMAX\"%s%s\",\n+\t\t\t\t\tprogress->title, (uintmax_t)n, tp, eol);\n \t\t\tfflush(stderr);\n \t\t}\n \t\tprogress_update = 0;\n@@ -193,6 +203,14 @@ int display_progress(struct progress *progress, uint64_t n)\n \treturn progress ? display(progress, n, NULL) : 0;\n }\n \n+void display_throbber(struct progress *progress)\n+{\n+\tif (progress) {\n+\t\tprogress->show_throbber = 1;\n+\t\tdisplay(progress, 0, NULL);\n+\t}\n+}\n+\n static struct progress *start_progress_delay(const char *title, uint64_t total,\n \t\t\t\t\t     unsigned delay)\n {\n@@ -248,7 +266,10 @@ void stop_progress_msg(struct progress **p_progress, const char *msg)\n \t\t\tthroughput_string(&tp->display, tp->curr_total, rate);\n \t\t}\n \t\tprogress_update = 1;\n-\t\tbuf = xstrfmt(\", %s.\\n\", msg);\n+\t\tif (progress->show_throbber)\n+\t\t\tbuf = xstrfmt(\" %s.\\n\", msg);\n+\t\telse\n+\t\t\tbuf = xstrfmt(\", %s.\\n\", msg);\n \t\tdisplay(progress, progress->last_value, buf);\n \t\tfree(buf);\n \t}\ndiff --git a/progress.h b/progress.h\nindex 70a4d4a0d6..9d23ff242f 100644\n--- a/progress.h\n+++ b/progress.h\n@@ -5,6 +5,7 @@ struct progress;\n \n void display_throughput(struct progress *progress, uint64_t total);\n int display_progress(struct progress *progress, uint64_t n);\n+void display_throbber(struct progress *progress);\n struct progress *start_progress(const char *title, uint64_t total);\n struct progress *start_delayed_progress(const char *title, uint64_t total);\n void stop_progress(struct progress **progress);\n-- \n2.19.0.rc0.337.ge906d732e7\n\n-- 8< --\n"},{"id":"358552","messageId":"CAPig+cSP6mSKYoEMwaSZjZTj-kiQUHukjPUXVh3tv8ySVtJd6A@mail.gmail.com","threadId":"49380","inReplyTo":"20180919141016.27930-1-avarab@gmail.com","subject":"Re: [PATCH] reflog expire: add progress output","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-09-21T02:17:17Z","receivedAt":"2018-09-21T02:17:31Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Sep 19, 2018 at 10:10 AM Ævar Arnfjörð Bjarmason\n<avarab@gmail.com> wrote:\n> Before this change the \"git reflog expire\" command didn't report any\n> progress. This is the second command (after \"pack-refs --all --prune\")\n> that the \"gc\" command will run.\n> [...]\n> That may just be some artifact of the repositories I've tested, but I\n> suspect It'll be true in general. As the callstack above shows, in\n\ns/It'll/it'll/\n\n> order to guarantee that we don't do that we'd need to pass some\n> \"progress\" variable through 10 levels of functions, many of which are\n> \"for_each\" callback functions with void* cb_data.\n"}]}