{"thread":{"id":"38902","subject":"[PATCH] gc: save log from daemonized gc --auto and print it next time","startedAt":"2015-03-24T12:17:28Z","lastAt":"2015-03-26T17:13:25Z","messageCount":6,"participants":["Nguyễn Thái Ngọc Duy","Junio C Hamano","Eric Sunshine","Duy Nguyen"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"258404","messageId":"1427199448-28278-1-git-send-email-pclouds@gmail.com","threadId":"38902","inReplyTo":null,"subject":"[PATCH] gc: save log from daemonized gc --auto and print it next time","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2015-03-24T12:17:28Z","receivedAt":"2015-03-24T12:17:28Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"While commit 9f673f9 (gc: config option for running --auto in\nbackground - 2014-02-08) helps reduce some complaints about 'gc\n--auto' hogging the terminal, it creates another set of problems.\n\nThe latest in this set is, as the result of daemonizing, stderr is\nclosed and all warnings are lost. This warning at the end of cmd_gc()\nis particularly important because it tells the user how to avoid \"gc\n--auto\" running repeatedly. Because stderr is closed, the user does\nnot know, naturally they complain about 'gc --auto' wasting CPU.\n\nBesides reverting 9f673f9 and looking at the problem from another\nangle, we could save the stderr in $GIT_DIR/gc.log. Next time, 'gc\n--auto' will print the saved warnings, delete gc.log and exit.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n builtin/gc.c  | 37 ++++++++++++++++++++++++++++++++++++-\n t/t6500-gc.sh | 20 ++++++++++++++++++++\n 2 files changed, 56 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex 5c634af..07769a9 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -32,6 +32,8 @@ static int aggressive_window = 250;\n static int gc_auto_threshold = 6700;\n static int gc_auto_pack_limit = 50;\n static int detach_auto = 1;\n+static struct strbuf log_filename = STRBUF_INIT;\n+static int daemonized;\n static const char *prune_expire = \"2.weeks.ago\";\n \n static struct argv_array pack_refs_cmd = ARGV_ARRAY_INIT;\n@@ -44,6 +46,15 @@ static char *pidfile;\n \n static void remove_pidfile(void)\n {\n+\tif (daemonized && log_filename.len) {\n+\t\tstruct stat st;\n+\n+\t\tclose(2);\n+\t\tif (stat(log_filename.buf, &st) ||\n+\t\t    !st.st_size ||\n+\t\t    rename(log_filename.buf, git_path(\"gc.log\")))\n+\t\t\tunlink(log_filename.buf);\n+\t}\n \tif (pidfile)\n \t\tunlink(pidfile);\n }\n@@ -324,13 +335,25 @@ int cmd_gc(int argc, const char **argv, const char *prefix)\n \t\t\tfprintf(stderr, _(\"See \\\"git help gc\\\" for manual housekeeping.\\n\"));\n \t\t}\n \t\tif (detach_auto) {\n+\t\t\tstruct strbuf sb = STRBUF_INIT;\n+\n+\t\t\tif (strbuf_read_file(&sb, git_path(\"gc.log\"), 0) > 0) {\n+\t\t\t\twarning(_(\"Last gc run reported the following, gc skipped\"));\n+\t\t\t\tfputs(sb.buf, stderr);\n+\t\t\t\tstrbuf_release(&sb);\n+\t\t\t\t/* let the next gc --auto run as usual */\n+\t\t\t\tunlink(git_path(\"gc.log\"));\n+\t\t\t\treturn 0;\n+\t\t\t}\n+\n \t\t\tif (gc_before_repack())\n \t\t\t\treturn -1;\n \t\t\t/*\n \t\t\t * failure to daemonize is ok, we'll continue\n \t\t\t * in foreground\n \t\t\t */\n-\t\t\tdaemonize();\n+\t\t\tif (!daemonize())\n+\t\t\t\tdaemonized = 1;\n \t\t}\n \t} else\n \t\tadd_repack_all_option();\n@@ -343,6 +366,18 @@ int cmd_gc(int argc, const char **argv, const char *prefix)\n \t\t    name, (uintmax_t)pid);\n \t}\n \n+\tif (daemonized) {\n+\t\tint fd;\n+\n+\t\tstrbuf_addstr(&log_filename, git_path(\"gc.log_XXXXXX\"));\n+\t\tfd = xmkstemp(log_filename.buf);\n+\t\tif (fd >= 0) {\n+\t\t\tdup2(fd, 2);\n+\t\t\tclose(fd);\n+\t\t} else\n+\t\t\tstrbuf_release(&log_filename);\n+\t}\n+\n \tif (gc_before_repack())\n \t\treturn -1;\n \ndiff --git a/t/t6500-gc.sh b/t/t6500-gc.sh\nindex 63194d8..54bc9c4 100755\n--- a/t/t6500-gc.sh\n+++ b/t/t6500-gc.sh\n@@ -30,4 +30,24 @@ test_expect_success 'gc -h with invalid configuration' '\n \ttest_i18ngrep \"[Uu]sage\" broken/usage\n '\n \n+test_expect_success !MINGW 'gc --auto and logging' '\n+\tgit init abc &&\n+\t(\n+\t\tcd abc &&\n+\t\t# These create blobs starting with the magic number \"17\"\n+\t\tfor i in 901 944; do\n+\t\t\techo $i >test && git hash-object -w test >/dev/null\n+\t\tdone &&\n+\t\tgit config gc.auto 1 &&\n+\t\tLANG=C git gc --auto &&\n+\t\tsleep 1 && # give it time to daemonize\n+\t\twhile test -f .git/gc.pid; do sleep 1; done &&\n+\t\tgrep \"too many unreachable loose objects\" .git/gc.log &&\n+\t\tLANG=C git gc --auto 2>error &&\n+\t\tgrep skipped error &&\n+\t\tgrep \"too many unreachable loose objects\" error &&\n+\t\t! test -f .git/gc.log\n+\t)\n+'\n+\n test_done\n-- \n2.3.0.rc1.137.g477eb31\n"},{"id":"258454","messageId":"xmqqy4mm3v8l.fsf@gitster.dls.corp.google.com","threadId":"38902","inReplyTo":"1427199448-28278-1-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH] gc: save log from daemonized gc --auto and print it next time","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-03-24T22:07:22Z","receivedAt":"2015-03-24T22:07:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:\n\n> While commit 9f673f9 (gc: config option for running --auto in\n> background - 2014-02-08) helps reduce some complaints about 'gc\n> --auto' hogging the terminal, it creates another set of problems.\n>\n> The latest in this set is, as the result of daemonizing, stderr is\n> closed and all warnings are lost. This warning at the end of cmd_gc()\n> is particularly important because it tells the user how to avoid \"gc\n> --auto\" running repeatedly. Because stderr is closed, the user does\n> not know, naturally they complain about 'gc --auto' wasting CPU.\n>\n> Besides reverting 9f673f9 and looking at the problem from another\n> angle, we could save the stderr in $GIT_DIR/gc.log. Next time, 'gc\n> --auto' will print the saved warnings, delete gc.log and exit.\n\nI wonder what this buys us if multiple auto-gc's are triggered\nbecause the user is running a long svn import or something similar.\n\n> diff --git a/t/t6500-gc.sh b/t/t6500-gc.sh\n> index 63194d8..54bc9c4 100755\n> --- a/t/t6500-gc.sh\n> +++ b/t/t6500-gc.sh\n> @@ -30,4 +30,24 @@ test_expect_success 'gc -h with invalid configuration' '\n>  \ttest_i18ngrep \"[Uu]sage\" broken/usage\n>  '\n>  \n> +test_expect_success !MINGW 'gc --auto and logging' '\n> +\tgit init abc &&\n> +\t(\n> +\t\tcd abc &&\n> +\t\t# These create blobs starting with the magic number \"17\"\n> +\t\tfor i in 901 944; do\n\nThere are numbers smaller than these, like 263 and 410 ;-)\n\n> +\t\t\techo $i >test && git hash-object -w test >/dev/null\n\n\"hash-object --stdin\"?\n\n> +\t\tdone &&\n> +\t\tgit config gc.auto 1 &&\n\ntest_config?\n\n> +\t\tLANG=C git gc --auto &&\n> +\t\tsleep 1 && # give it time to daemonize\n> +\t\twhile test -f .git/gc.pid; do sleep 1; done &&\n\nYuck...\n\n> +\t\tgrep \"too many unreachable loose objects\" .git/gc.log &&\n> +\t\tLANG=C git gc --auto 2>error &&\n> +\t\tgrep skipped error &&\n> +\t\tgrep \"too many unreachable loose objects\" error &&\n> +\t\t! test -f .git/gc.log\n> +\t)\n> +'\n\nFor that \"17/ has very many loose objects that are still young and\nunreachable\" issue, I wonder if the right solution is somehow to\nflag the repository and prevent \"gc --auto\" from running until the\nsituation improves.  \"I checked at this time and found too many in\n17/\"; upon finding that flag file (with a timestamp), if there are\nnew files in 17/ or if there are other reasons to do a gc (perhaps\nthere are too many packfiles to be consolidated?), then do the gc\nbut otherwise quite silently before spending too many cycles on it,\nor something along that line?\n"},{"id":"258455","messageId":"CAPig+cS8zn5XThxShZHYCPs=xUeiwHXKSueor3jX_7E0X_uhuA@mail.gmail.com","threadId":"38902","inReplyTo":"1427199448-28278-1-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH] gc: save log from daemonized gc --auto and print it next time","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-03-24T22:12:05Z","receivedAt":"2015-03-24T22:12:05Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Mar 24, 2015 at 8:17 AM, Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:\n> While commit 9f673f9 (gc: config option for running --auto in\n> background - 2014-02-08) helps reduce some complaints about 'gc\n> --auto' hogging the terminal, it creates another set of problems.\n>\n> The latest in this set is, as the result of daemonizing, stderr is\n> closed and all warnings are lost. This warning at the end of cmd_gc()\n> is particularly important because it tells the user how to avoid \"gc\n> --auto\" running repeatedly. Because stderr is closed, the user does\n> not know, naturally they complain about 'gc --auto' wasting CPU.\n>\n> Besides reverting 9f673f9 and looking at the problem from another\n> angle, we could save the stderr in $GIT_DIR/gc.log. Next time, 'gc\n> --auto' will print the saved warnings, delete gc.log and exit.\n>\n> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n> ---\n>  builtin/gc.c  | 37 ++++++++++++++++++++++++++++++++++++-\n>  t/t6500-gc.sh | 20 ++++++++++++++++++++\n>  2 files changed, 56 insertions(+), 1 deletion(-)\n>\n> diff --git a/builtin/gc.c b/builtin/gc.c\n> index 5c634af..07769a9 100644\n> --- a/builtin/gc.c\n> +++ b/builtin/gc.c\n> @@ -32,6 +32,8 @@ static int aggressive_window = 250;\n>  static int gc_auto_threshold = 6700;\n>  static int gc_auto_pack_limit = 50;\n>  static int detach_auto = 1;\n> +static struct strbuf log_filename = STRBUF_INIT;\n> +static int daemonized;\n>  static const char *prune_expire = \"2.weeks.ago\";\n>\n>  static struct argv_array pack_refs_cmd = ARGV_ARRAY_INIT;\n> @@ -44,6 +46,15 @@ static char *pidfile;\n>\n>  static void remove_pidfile(void)\n>  {\n> +       if (daemonized && log_filename.len) {\n> +               struct stat st;\n> +\n> +               close(2);\n> +               if (stat(log_filename.buf, &st) ||\n> +                   !st.st_size ||\n> +                   rename(log_filename.buf, git_path(\"gc.log\")))\n> +                       unlink(log_filename.buf);\n> +       }\n>         if (pidfile)\n>                 unlink(pidfile);\n>  }\n> @@ -324,13 +335,25 @@ int cmd_gc(int argc, const char **argv, const char *prefix)\n>                         fprintf(stderr, _(\"See \\\"git help gc\\\" for manual housekeeping.\\n\"));\n>                 }\n>                 if (detach_auto) {\n> +                       struct strbuf sb = STRBUF_INIT;\n> +\n> +                       if (strbuf_read_file(&sb, git_path(\"gc.log\"), 0) > 0) {\n> +                               warning(_(\"Last gc run reported the following, gc skipped\"));\n\nWhen I read this message, it makes me think that the previous gc was\nskipped, even though it's actually skipping the current one. Perhaps\nrephrase as \"skipping gc; last gc reported:\"?\n\n> +                               fputs(sb.buf, stderr);\n> +                               strbuf_release(&sb);\n> +                               /* let the next gc --auto run as usual */\n> +                               unlink(git_path(\"gc.log\"));\n> +                               return 0;\n> +                       }\n> +\n>                         if (gc_before_repack())\n>                                 return -1;\n>                         /*\n>                          * failure to daemonize is ok, we'll continue\n>                          * in foreground\n>                          */\n> -                       daemonize();\n> +                       if (!daemonize())\n> +                               daemonized = 1;\n>                 }\n>         } else\n>                 add_repack_all_option();\n> @@ -343,6 +366,18 @@ int cmd_gc(int argc, const char **argv, const char *prefix)\n>                     name, (uintmax_t)pid);\n>         }\n>\n> +       if (daemonized) {\n> +               int fd;\n> +\n> +               strbuf_addstr(&log_filename, git_path(\"gc.log_XXXXXX\"));\n> +               fd = xmkstemp(log_filename.buf);\n> +               if (fd >= 0) {\n> +                       dup2(fd, 2);\n> +                       close(fd);\n> +               } else\n> +                       strbuf_release(&log_filename);\n> +       }\n> +\n>         if (gc_before_repack())\n>                 return -1;\n>\n> diff --git a/t/t6500-gc.sh b/t/t6500-gc.sh\n> index 63194d8..54bc9c4 100755\n> --- a/t/t6500-gc.sh\n> +++ b/t/t6500-gc.sh\n> @@ -30,4 +30,24 @@ test_expect_success 'gc -h with invalid configuration' '\n>         test_i18ngrep \"[Uu]sage\" broken/usage\n>  '\n>\n> +test_expect_success !MINGW 'gc --auto and logging' '\n> +       git init abc &&\n> +       (\n> +               cd abc &&\n> +               # These create blobs starting with the magic number \"17\"\n> +               for i in 901 944; do\n> +                       echo $i >test && git hash-object -w test >/dev/null\n> +               done &&\n> +               git config gc.auto 1 &&\n> +               LANG=C git gc --auto &&\n> +               sleep 1 && # give it time to daemonize\n> +               while test -f .git/gc.pid; do sleep 1; done &&\n> +               grep \"too many unreachable loose objects\" .git/gc.log &&\n> +               LANG=C git gc --auto 2>error &&\n> +               grep skipped error &&\n> +               grep \"too many unreachable loose objects\" error &&\n> +               ! test -f .git/gc.log\n> +       )\n> +'\n> +\n>  test_done\n> --\n> 2.3.0.rc1.137.g477eb31\n>\n> --\n> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> the body of a message to majordomo@vger.kernel.org\n> More majordomo info at  http://vger.kernel.org/majordomo-info.html\n"},{"id":"258467","messageId":"CACsJy8Acw8u8mbW6ivoUN10462xJDZxK984Vc2E3eypTc3qJ7A@mail.gmail.com","threadId":"38902","inReplyTo":"xmqqy4mm3v8l.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] gc: save log from daemonized gc --auto and print it next time","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2015-03-25T00:58:54Z","receivedAt":"2015-03-25T00:58:54Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, Mar 25, 2015 at 5:07 AM, Junio C Hamano <gitster@pobox.com> wrote:\n>> +             LANG=C git gc --auto &&\n>> +             sleep 1 && # give it time to daemonize\n>> +             while test -f .git/gc.pid; do sleep 1; done &&\n>\n> Yuck...\n\nYeah.. it's hard to test daemon things. I'm not even sure if we should\nadd a test, but I tried anyway.\n\n>> +             grep \"too many unreachable loose objects\" .git/gc.log &&\n>> +             LANG=C git gc --auto 2>error &&\n>> +             grep skipped error &&\n>> +             grep \"too many unreachable loose objects\" error &&\n>> +             ! test -f .git/gc.log\n>> +     )\n>> +'\n>\n> For that \"17/ has very many loose objects that are still young and\n> unreachable\" issue, I wonder if the right solution is somehow to\n> flag the repository and prevent \"gc --auto\" from running until the\n> situation improves.  \"I checked at this time and found too many in\n> 17/\"; upon finding that flag file (with a timestamp), if there are\n> new files in 17/ or if there are other reasons to do a gc (perhaps\n> there are too many packfiles to be consolidated?), then do the gc\n> but otherwise quit silently before spending too many cycles on it,\n> or something along that line?\n\nThat's a separate problem that's being discussed in another thread. I\nthink Jeff's idea of storing the number of estimated loose objects may\nbe more reliable than timestamps..\n-- \nDuy\n"},{"id":"258541","messageId":"1427367263-16004-1-git-send-email-pclouds@gmail.com","threadId":"38902","inReplyTo":"1427199448-28278-1-git-send-email-pclouds@gmail.com","subject":"[PATCH v2] gc: save log from daemonized gc --auto and print it next time","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2015-03-26T10:54:23Z","receivedAt":"2015-03-26T10:54:23Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"While commit 9f673f9 (gc: config option for running --auto in\nbackground - 2014-02-08) helps reduce some complaints about 'gc\n--auto' hogging the terminal, it creates another set of problems.\n\nThe latest in this set is, as the result of daemonizing, stderr is\nclosed and all warnings are lost. This warning at the end of cmd_gc()\nis particularly important because it tells the user how to avoid \"gc\n--auto\" running repeatedly. Because stderr is closed, the user does\nnot know, naturally they complain about 'gc --auto' wasting CPU.\n\nBesides reverting 9f673f9 and looking at the problem from another\nangle, we could save the stderr in $GIT_DIR/gc.log. Next time, 'gc\n--auto' will print the saved warnings, delete gc.log and exit.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n The test is dropped. It does not feel good enough.. The rest of\n changes is minor\n \n  diff --git a/builtin/gc.c b/builtin/gc.c\n  index 07769a9..3886937 100644\n  --- a/builtin/gc.c\n  +++ b/builtin/gc.c\n  @@ -32,8 +32,6 @@ static int aggressive_window = 250;\n   static int gc_auto_threshold = 6700;\n   static int gc_auto_pack_limit = 50;\n   static int detach_auto = 1;\n  -static struct strbuf log_filename = STRBUF_INIT;\n  -static int daemonized;\n   static const char *prune_expire = \"2.weeks.ago\";\n   \n   static struct argv_array pack_refs_cmd = ARGV_ARRAY_INIT;\n  @@ -43,6 +41,8 @@ static struct argv_array prune = ARGV_ARRAY_INIT;\n   static struct argv_array rerere = ARGV_ARRAY_INIT;\n   \n   static char *pidfile;\n  +static struct strbuf log_filename = STRBUF_INIT;\n  +static int daemonized;\n   \n   static void remove_pidfile(void)\n   {\n  @@ -338,7 +338,7 @@ int cmd_gc(int argc, const char **argv, const char *prefix)\n   \t\t\tstruct strbuf sb = STRBUF_INIT;\n   \n   \t\t\tif (strbuf_read_file(&sb, git_path(\"gc.log\"), 0) > 0) {\n  -\t\t\t\twarning(_(\"Last gc run reported the following, gc skipped\"));\n  +\t\t\t\twarning(_(\"skipping gc; last gc reported:\"));\n   \t\t\t\tfputs(sb.buf, stderr);\n   \t\t\t\tstrbuf_release(&sb);\n   \t\t\t\t/* let the next gc --auto run as usual */\n  @@ -352,8 +352,7 @@ int cmd_gc(int argc, const char **argv, const char *prefix)\n   \t\t\t * failure to daemonize is ok, we'll continue\n   \t\t\t * in foreground\n   \t\t\t */\n  -\t\t\tif (!daemonize())\n  -\t\t\t\tdaemonized = 1;\n  +\t\t\tdaemonized = !daemonize();\n   \t\t}\n   \t} else\n   \t\tadd_repack_all_option();\n\n builtin/gc.c | 36 +++++++++++++++++++++++++++++++++++-\n 1 file changed, 35 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex 5c634af..3886937 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -41,9 +41,20 @@ static struct argv_array prune = ARGV_ARRAY_INIT;\n static struct argv_array rerere = ARGV_ARRAY_INIT;\n \n static char *pidfile;\n+static struct strbuf log_filename = STRBUF_INIT;\n+static int daemonized;\n \n static void remove_pidfile(void)\n {\n+\tif (daemonized && log_filename.len) {\n+\t\tstruct stat st;\n+\n+\t\tclose(2);\n+\t\tif (stat(log_filename.buf, &st) ||\n+\t\t    !st.st_size ||\n+\t\t    rename(log_filename.buf, git_path(\"gc.log\")))\n+\t\t\tunlink(log_filename.buf);\n+\t}\n \tif (pidfile)\n \t\tunlink(pidfile);\n }\n@@ -324,13 +335,24 @@ int cmd_gc(int argc, const char **argv, const char *prefix)\n \t\t\tfprintf(stderr, _(\"See \\\"git help gc\\\" for manual housekeeping.\\n\"));\n \t\t}\n \t\tif (detach_auto) {\n+\t\t\tstruct strbuf sb = STRBUF_INIT;\n+\n+\t\t\tif (strbuf_read_file(&sb, git_path(\"gc.log\"), 0) > 0) {\n+\t\t\t\twarning(_(\"skipping gc; last gc reported:\"));\n+\t\t\t\tfputs(sb.buf, stderr);\n+\t\t\t\tstrbuf_release(&sb);\n+\t\t\t\t/* let the next gc --auto run as usual */\n+\t\t\t\tunlink(git_path(\"gc.log\"));\n+\t\t\t\treturn 0;\n+\t\t\t}\n+\n \t\t\tif (gc_before_repack())\n \t\t\t\treturn -1;\n \t\t\t/*\n \t\t\t * failure to daemonize is ok, we'll continue\n \t\t\t * in foreground\n \t\t\t */\n-\t\t\tdaemonize();\n+\t\t\tdaemonized = !daemonize();\n \t\t}\n \t} else\n \t\tadd_repack_all_option();\n@@ -343,6 +365,18 @@ int cmd_gc(int argc, const char **argv, const char *prefix)\n \t\t    name, (uintmax_t)pid);\n \t}\n \n+\tif (daemonized) {\n+\t\tint fd;\n+\n+\t\tstrbuf_addstr(&log_filename, git_path(\"gc.log_XXXXXX\"));\n+\t\tfd = xmkstemp(log_filename.buf);\n+\t\tif (fd >= 0) {\n+\t\t\tdup2(fd, 2);\n+\t\t\tclose(fd);\n+\t\t} else\n+\t\t\tstrbuf_release(&log_filename);\n+\t}\n+\n \tif (gc_before_repack())\n \t\treturn -1;\n \n-- \n2.3.0.rc1.137.g477eb31\n"},{"id":"258558","messageId":"xmqqlhijznpm.fsf@gitster.dls.corp.google.com","threadId":"38902","inReplyTo":"1427367263-16004-1-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH v2] gc: save log from daemonized gc --auto and print it next time","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-03-26T17:13:25Z","receivedAt":"2015-03-26T17:13:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:\n\n> While commit 9f673f9 (gc: config option for running --auto in\n> background - 2014-02-08) helps reduce some complaints about 'gc\n> --auto' hogging the terminal, it creates another set of problems.\n>\n> The latest in this set is, as the result of daemonizing, stderr is\n> closed and all warnings are lost. This warning at the end of cmd_gc()\n> is particularly important because it tells the user how to avoid \"gc\n> --auto\" running repeatedly. Because stderr is closed, the user does\n> not know, naturally they complain about 'gc --auto' wasting CPU.\n>\n> Besides reverting 9f673f9 and looking at the problem from another\n> angle, we could save the stderr in $GIT_DIR/gc.log. Next time, 'gc\n> --auto' will print the saved warnings, delete gc.log and exit.\n>\n> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n> ---\n>  The test is dropped. It does not feel good enough.. The rest of\n>  changes is minor\n\nI still wonder what this buys us if multiple auto-gc's are triggered,\nbecause the user is running a long svn import or something similar.\n\nI cannot tell what problem this is trying to solve.\n\n (1) If the error output from the previous \"gc --auto\" indicates a\n     grave error condition that further re-running of \"gc --auto\" is\n     pointless, why is it a good idea to blindly remove the log and\n     \"let the next one run as usual\"?\n\n (2) If the problem it is trying to solve is that \"gc --auto\"\n     sometimes gives a good suggestion but that is lost when\n     daemonized, why not address that problem in a more direct way,\n     e.g. introduce a new option \"gc --probe-auto\" that only checks\n     and reports if \"gc --auto\" would spend actual cycles to do its\n     work, and then run the daemonized version of \"gc --auto\" when\n     necessary?  Either \"gc --probe-auto\" itself or the caller of\n     \"gc --probe-auto\" can give the \"you should do this to avoid\n     repeated auto-gc\" when this happens.\n\n     Also the same issue I have with (1) above applies; I do not see\n     much linkage with solving this issue with halving the frequency\n     of running \"gc --auto\" which is what this patch does.\n\nIn other words, I would understand what the change is trying to\nachieve if it were to always stop, instruct the user to take\ncorrective action, and never remove the .log file itself (naturally,\nthe corrective action would include \"remove the .log when you are\ndone\").  I do not necessarily agree that it would be a good change\nfor the overall system, but at least such a change is internally\nconsistent between its perception of a problem and its approach to\nsolve it.\n\nI would also understand, and I suspect I would prefer it if I see\nthe result, if the change were to allow \"gc --auto\" to report\nseverity of its findings (i.e. \"nothing wrong in your repository but\nyou should do X to avoid triggering me too often\" vs \"no point\nrunning me again and again\"), do something similar to what this\npatch does and after showing the message and (1) stop without\nremoving the log but give instruction when the situation is grave,\nor (2) show the message, remove the .log, and continue to go ahead\nwith \"gc --auto\" when the situation is *not* grave.\n\nBut the approach taken by this patch looks very confused about its\nown purpose to me and I cannot quite tell what it is trying to\nsolve.\n\n>   diff --git a/builtin/gc.c b/builtin/gc.c\n>   index 07769a9..3886937 100644\n>   --- a/builtin/gc.c\n>   +++ b/builtin/gc.c\n>   @@ -32,8 +32,6 @@ static int aggressive_window = 250;\n>    static int gc_auto_threshold = 6700;\n>    static int gc_auto_pack_limit = 50;\n>    static int detach_auto = 1;\n>   -static struct strbuf log_filename = STRBUF_INIT;\n>   -static int daemonized;\n>    static const char *prune_expire = \"2.weeks.ago\";\n>    \n>    static struct argv_array pack_refs_cmd = ARGV_ARRAY_INIT;\n>   @@ -43,6 +41,8 @@ static struct argv_array prune = ARGV_ARRAY_INIT;\n>    static struct argv_array rerere = ARGV_ARRAY_INIT;\n>    \n>    static char *pidfile;\n>   +static struct strbuf log_filename = STRBUF_INIT;\n>   +static int daemonized;\n>    \n>    static void remove_pidfile(void)\n>    {\n>   @@ -338,7 +338,7 @@ int cmd_gc(int argc, const char **argv, const char *prefix)\n>    \t\t\tstruct strbuf sb = STRBUF_INIT;\n>    \n>    \t\t\tif (strbuf_read_file(&sb, git_path(\"gc.log\"), 0) > 0) {\n>   -\t\t\t\twarning(_(\"Last gc run reported the following, gc skipped\"));\n>   +\t\t\t\twarning(_(\"skipping gc; last gc reported:\"));\n>    \t\t\t\tfputs(sb.buf, stderr);\n>    \t\t\t\tstrbuf_release(&sb);\n>    \t\t\t\t/* let the next gc --auto run as usual */\n>   @@ -352,8 +352,7 @@ int cmd_gc(int argc, const char **argv, const char *prefix)\n>    \t\t\t * failure to daemonize is ok, we'll continue\n>    \t\t\t * in foreground\n>    \t\t\t */\n>   -\t\t\tif (!daemonize())\n>   -\t\t\t\tdaemonized = 1;\n>   +\t\t\tdaemonized = !daemonize();\n>    \t\t}\n>    \t} else\n>    \t\tadd_repack_all_option();\n>\n>  builtin/gc.c | 36 +++++++++++++++++++++++++++++++++++-\n>  1 file changed, 35 insertions(+), 1 deletion(-)\n>\n> diff --git a/builtin/gc.c b/builtin/gc.c\n> index 5c634af..3886937 100644\n> --- a/builtin/gc.c\n> +++ b/builtin/gc.c\n> @@ -41,9 +41,20 @@ static struct argv_array prune = ARGV_ARRAY_INIT;\n>  static struct argv_array rerere = ARGV_ARRAY_INIT;\n>  \n>  static char *pidfile;\n> +static struct strbuf log_filename = STRBUF_INIT;\n> +static int daemonized;\n>  \n>  static void remove_pidfile(void)\n>  {\n> +\tif (daemonized && log_filename.len) {\n> +\t\tstruct stat st;\n> +\n> +\t\tclose(2);\n> +\t\tif (stat(log_filename.buf, &st) ||\n> +\t\t    !st.st_size ||\n> +\t\t    rename(log_filename.buf, git_path(\"gc.log\")))\n> +\t\t\tunlink(log_filename.buf);\n> +\t}\n>  \tif (pidfile)\n>  \t\tunlink(pidfile);\n>  }\n> @@ -324,13 +335,24 @@ int cmd_gc(int argc, const char **argv, const char *prefix)\n>  \t\t\tfprintf(stderr, _(\"See \\\"git help gc\\\" for manual housekeeping.\\n\"));\n>  \t\t}\n>  \t\tif (detach_auto) {\n> +\t\t\tstruct strbuf sb = STRBUF_INIT;\n> +\n> +\t\t\tif (strbuf_read_file(&sb, git_path(\"gc.log\"), 0) > 0) {\n> +\t\t\t\twarning(_(\"skipping gc; last gc reported:\"));\n> +\t\t\t\tfputs(sb.buf, stderr);\n> +\t\t\t\tstrbuf_release(&sb);\n> +\t\t\t\t/* let the next gc --auto run as usual */\n> +\t\t\t\tunlink(git_path(\"gc.log\"));\n> +\t\t\t\treturn 0;\n> +\t\t\t}\n> +\n>  \t\t\tif (gc_before_repack())\n>  \t\t\t\treturn -1;\n>  \t\t\t/*\n>  \t\t\t * failure to daemonize is ok, we'll continue\n>  \t\t\t * in foreground\n>  \t\t\t */\n> -\t\t\tdaemonize();\n> +\t\t\tdaemonized = !daemonize();\n>  \t\t}\n>  \t} else\n>  \t\tadd_repack_all_option();\n> @@ -343,6 +365,18 @@ int cmd_gc(int argc, const char **argv, const char *prefix)\n>  \t\t    name, (uintmax_t)pid);\n>  \t}\n>  \n> +\tif (daemonized) {\n> +\t\tint fd;\n> +\n> +\t\tstrbuf_addstr(&log_filename, git_path(\"gc.log_XXXXXX\"));\n> +\t\tfd = xmkstemp(log_filename.buf);\n> +\t\tif (fd >= 0) {\n> +\t\t\tdup2(fd, 2);\n> +\t\t\tclose(fd);\n> +\t\t} else\n> +\t\t\tstrbuf_release(&log_filename);\n> +\t}\n> +\n>  \tif (gc_before_repack())\n>  \t\treturn -1;\n"}]}