{"thread":{"id":"39812","subject":"Since gc.autodetach=1 you can end up with auto-gc on every command with no user notification","startedAt":"2015-07-08T12:28:25Z","lastAt":"2015-09-21T16:43:15Z","messageCount":17,"participants":["Ævar Arnfjörð Bjarmason","Duy Nguyen","Nguyễn Thái Ngọc Duy","Junio C Hamano","Michael Haggerty"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"265844","messageId":"CACBZZX5hUr8TMRXYWRqdb1AQ=oOBpEFuoMwYHczPdHN82ceuTw@mail.gmail.com","threadId":"39812","inReplyTo":null,"subject":"Since gc.autodetach=1 you can end up with auto-gc on every command with no user notification","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2015-07-08T12:28:25Z","receivedAt":"2015-07-08T12:28:25Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Someone at work came to me with the problem that they were getting the\n\"Auto packing the repository in background for optimum performance\"\nnotice on every Git command that they ran.\n\nThis problem is a combination of two things:\n\n * Since Nguyễn's v1.9-rc0-2-g9f673f9 where we started running \"git\ngc\" in the background the user hasn't seen the \"There are too many\nunreachable loose objects\" message added back in v1.5.3.1-27-ga087cc9\n\n * The checkout has a lot of loose objects. So even after \"git prune\n--expire=2.week.ago\" the .git/objects/17 directory has 317 objects.\nMore than 27 in that directory trigger \"git gc --auto\".\n\nSo it's partly a UI issue. Since the repacking is happening in the\nbackground the user never sees the message suggesting that they run\n\"git prune\".\n\nBut perhaps the heuristic of \"are there more than 27 objects in\n.git/objects/17\" could be improved, but I don't know with what\nexactly.\n\nBut having something fork a gc to the background on every fetch (and\nsimilar object-modifying operations) is quite sub-optimal.\n"},{"id":"265846","messageId":"CACsJy8AKbXqsuNJL7Usb-haiynDcCDQXk56WYh5PxzGU3NvaAw@mail.gmail.com","threadId":"39812","inReplyTo":"CACBZZX5hUr8TMRXYWRqdb1AQ=oOBpEFuoMwYHczPdHN82ceuTw@mail.gmail.com","subject":"Re: Since gc.autodetach=1 you can end up with auto-gc on every command with no user notification","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2015-07-08T12:47:54Z","receivedAt":"2015-07-08T12:47:54Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, Jul 8, 2015 at 7:28 PM, Ævar Arnfjörð Bjarmason\n wrote:\n> Someone at work came to me with the problem that they were getting the\n> \"Auto packing the repository in background for optimum performance\"\n> notice on every Git command that they ran.\n>\n> This problem is a combination of two things:\n>\n> * Since Nguyễn's v1.9-rc0-2-g9f673f9 where we started running \"git\n> gc\" in the background the user hasn't seen the \"There are too many\n> unreachable loose objects\" message added back in v1.5.3.1-27-ga087cc9\n>\n> * The checkout has a lot of loose objects. So even after \"git prune\n> --expire=2.week.ago\" the .git/objects/17 directory has 317 objects.\n> More than 27 in that directory trigger \"git gc --auto\".\n>\n> So it's partly a UI issue. Since the repacking is happening in the\n> background the user never sees the message suggesting that they run\n> \"git prune\".\n>\n> But perhaps the heuristic of \"are there more than 27 objects in\n> .git/objects/17\" could be improved, but I don't know with what\n> exactly.\n>\n> But having something fork a gc to the background on every fetch (and\n> similar object-modifying operations) is quite sub-optimal.\n\nThanks for reminding me. Two related threads: [1] is about that \"17\"\nheuristics and [2] the UI thing. I need to reread them before making\nany more comments.\n\n[1] http://thread.gmane.org/gmane.comp.version-control.git/265734/focus=265756\n[2] http://thread.gmane.org/gmane.comp.version-control.git/266182\n-- \nDuy\n"},{"id":"268484","messageId":"1440209546-23442-1-git-send-email-pclouds@gmail.com","threadId":"39812","inReplyTo":"CACsJy8AKbXqsuNJL7Usb-haiynDcCDQXk56WYh5PxzGU3NvaAw@mail.gmail.com","subject":"[PATCH v3] 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-08-22T02:12:26Z","receivedAt":"2015-08-22T02:12:26Z","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\nDaemonized gc now saves stderr to $GIT_DIR/gc.log. Following gc runs\nwill not be daemonized and gc.log printed out until the user removes\ngc.log.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n Let's try again. Compared to v2 [1], this version does not delete\n gc.log automatically any more. The user needs to take action, then\n delete gc.log to bring it background again.\n\n [1] http://thread.gmane.org/gmane.comp.version-control.git/266182/focus=266320\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 bcc75d9..00a83e1 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -43,9 +43,20 @@ static struct argv_array prune_worktrees = 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@@ -330,13 +341,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+\t\t\tif (strbuf_read_file(&sb, git_path(\"gc.log\"), 0) > 0) {\n+\t\t\t\twarning(_(\"last gc run reported:\\n\"\n+\t\t\t\t\t \"%s\\n\"\n+\t\t\t\t\t \"running in foreground until %s is removed\"),\n+\t\t\t\t\tsb.buf, git_path(\"gc.log\"));\n+\t\t\t\tdetach_auto = 0;\n+\t\t\t}\n+\t\t\tstrbuf_release(&sb);\n+\t\t}\n+\t\tif (detach_auto) {\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@@ -349,6 +371,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":"268671","messageId":"xmqqtwrnz2ar.fsf@gitster.dls.corp.google.com","threadId":"39812","inReplyTo":"1440209546-23442-1-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH v3] gc: save log from daemonized gc --auto and print it next time","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-08-25T17:49:16Z","receivedAt":"2015-08-25T17:49:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy 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> Daemonized gc now saves stderr to $GIT_DIR/gc.log. Following gc runs\n> will not be daemonized and gc.log printed out until the user removes\n> gc.log.\n>\n> Signed-off-by: Nguyễn Thái Ngọc Duy \n> ---\n> Let's try again. Compared to v2 [1], this version does not delete\n> gc.log automatically any more. The user needs to take action, then\n> delete gc.log to bring it background again.\n\nSounds a bit more sensible, but I wonder if it is a good idea to\nkeep going in the first place. If the last gc run reported an\nissue, would it make it likely that we would hit the same issue?\n\nAn alternative design I have in mind is to exit \"gc --auto\" with\nsuccess without doing anything, after giving the new warning\nmessage. What would be the pros-and-cons between this patch and\nthat alternative design?\n\n\n\n> [1] http://thread.gmane.org/gmane.comp.version-control.git/266182/focus=266320\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 bcc75d9..00a83e1 100644\n> --- a/builtin/gc.c\n> +++ b/builtin/gc.c\n> @@ -43,9 +43,20 @@ static struct argv_array prune_worktrees = 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> @@ -330,13 +341,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> +\t\t\tif (strbuf_read_file(&sb, git_path(\"gc.log\"), 0) > 0) {\n> +\t\t\t\twarning(_(\"last gc run reported:\\n\"\n> +\t\t\t\t\t \"%s\\n\"\n> +\t\t\t\t\t \"running in foreground until %s is removed\"),\n> +\t\t\t\t\tsb.buf, git_path(\"gc.log\"));\n> +\t\t\t\tdetach_auto = 0;\n> +\t\t\t}\n> +\t\t\tstrbuf_release(&sb);\n> +\t\t}\n> +\t\tif (detach_auto) {\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> @@ -349,6 +371,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"},{"id":"268980","messageId":"CACsJy8Dd9OCpsytFDDwh8maBfTc1wLs+UKJMPdZngiBaJgr7pA@mail.gmail.com","threadId":"39812","inReplyTo":"xmqqtwrnz2ar.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v3] gc: save log from daemonized gc --auto and print it next time","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2015-08-31T10:17:59Z","receivedAt":"2015-08-31T10:17:59Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, Aug 26, 2015 at 12:49 AM, Junio C Hamano wrote:\n> Nguyễn Thái Ngọc Duy 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>> Daemonized gc now saves stderr to $GIT_DIR/gc.log. Following gc runs\n>> will not be daemonized and gc.log printed out until the user removes\n>> gc.log.\n>>\n>> Signed-off-by: Nguyễn Thái Ngọc Duy \n>> ---\n>> Let's try again. Compared to v2 [1], this version does not delete\n>> gc.log automatically any more. The user needs to take action, then\n>> delete gc.log to bring it background again.\n>\n> Sounds a bit more sensible, but I wonder if it is a good idea to\n> keep going in the first place. If the last gc run reported an\n> issue, would it make it likely that we would hit the same issue?\n>\n> An alternative design I have in mind is to exit \"gc --auto\" with\n> success without doing anything, after giving the new warning\n> message. What would be the pros-and-cons between this patch and\n> that alternative design?\n\nI think the alt. design is better. If anything, keep runing may\nproduce more output and probably distract the user from the real\nwarning. We only want to keep doing something again if it can gain us\nsomething. If we succeed at steps A and B, then fail at C. Doing A and\nB again next time might be worth something in general, but given the\ncontext of git-gc I don't think it is.\n-- \nDuy\n"},{"id":"269853","messageId":"1442108202-25746-1-git-send-email-pclouds@gmail.com","threadId":"39812","inReplyTo":"1440209546-23442-1-git-send-email-pclouds@gmail.com","subject":"[PATCH v4] 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-09-13T01:36:42Z","receivedAt":"2015-09-13T01:36:42Z","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\nDaemonized gc now saves stderr to $GIT_DIR/gc.log. Following gc --auto\nwill not run and gc.log printed out until the user removes gc.log.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n When gc.log exists, gc --auto now simply exits\n\n builtin/gc.c | 33 ++++++++++++++++++++++++++++++++-\n 1 file changed, 32 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex bcc75d9..2c3aaeb 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -43,9 +43,20 @@ static struct argv_array prune_worktrees = 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@@ -330,13 +341,21 @@ 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+\t\t\tif (strbuf_read_file(&sb, git_path(\"gc.log\"), 0) > 0)\n+\t\t\t\treturn error(_(\"last gc run reported:\\n\"\n+\t\t\t\t\t \"%s\\n\"\n+\t\t\t\t\t \"not running until %s is removed\"),\n+\t\t\t\t\t sb.buf, git_path(\"gc.log\"));\n+\t\t\tstrbuf_release(&sb);\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@@ -349,6 +368,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":"269943","messageId":"xmqq8u88ykv4.fsf@gitster.mtv.corp.google.com","threadId":"39812","inReplyTo":"1442108202-25746-1-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH v4] gc: save log from daemonized gc --auto and print it next time","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-09-14T17:24:47Z","receivedAt":"2015-09-14T17:24:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy 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> Daemonized gc now saves stderr to $GIT_DIR/gc.log. Following gc --auto\n> will not run and gc.log printed out until the user removes gc.log.\n>\n> Signed-off-by: Nguyễn Thái Ngọc Duy \n> ---\n> When gc.log exists, gc --auto now simply exits\n>\n> builtin/gc.c | 33 ++++++++++++++++++++++++++++++++-\n> 1 file changed, 32 insertions(+), 1 deletion(-)\n\nThanks, will queue.\n\nThis is sufficient for non-novice users, but I wonder if the error\nmessage is strong enough.\n\nA knee-jerk reaction to \"Last run reported an error so we won't run\nuntil the log is removed\" may be \"Then why don't you remove the log\nfor me automatically?\", which is a total brain-less raction [*1*],\nand \"then I'll remove the log\" is not all that better. The message\nwe want to convey is \"correct the root cause so that gc can run\nwithout having to give an error message.\"\n\nI guess it all depends on what is in gc.log; after all, this new\ncodepath is not in a good position to read the diagnosis left in\nthere and offer good pieces of advice to resolve them.\n\n\n[Footnote]\n\n*1* ... which is what a knee-jerk reaction is by definition.\n\n\n\n>\n> diff --git a/builtin/gc.c b/builtin/gc.c\n> index bcc75d9..2c3aaeb 100644\n> --- a/builtin/gc.c\n> +++ b/builtin/gc.c\n> @@ -43,9 +43,20 @@ static struct argv_array prune_worktrees = 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> @@ -330,13 +341,21 @@ 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> +\t\t\tif (strbuf_read_file(&sb, git_path(\"gc.log\"), 0) > 0)\n> +\t\t\t\treturn error(_(\"last gc run reported:\\n\"\n> +\t\t\t\t\t \"%s\\n\"\n> +\t\t\t\t\t \"not running until %s is removed\"),\n> +\t\t\t\t\t sb.buf, git_path(\"gc.log\"));\n> +\t\t\tstrbuf_release(&sb);\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> @@ -349,6 +368,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"},{"id":"269945","messageId":"xmqq1te0ykaj.fsf@gitster.mtv.corp.google.com","threadId":"39812","inReplyTo":"xmqq8u88ykv4.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v4] gc: save log from daemonized gc --auto and print it next time","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-09-14T17:37:08Z","receivedAt":"2015-09-14T17:37:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Thanks, will queue.\n\nEhh, I spoke a bit too early.\n\n>> diff --git a/builtin/gc.c b/builtin/gc.c\n>> index bcc75d9..2c3aaeb 100644\n>> --- a/builtin/gc.c\n>> +++ b/builtin/gc.c\n>> @@ -43,9 +43,20 @@ static struct argv_array prune_worktrees = 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\nUnfortuantely we cannot queue this as-is, as we let the tempfile API\nto automatically manage the pidfile since ebebeaea (gc: use tempfile\nmodule to handle gc.pid file, 2015-08-10), and you cannot piggy-back\nthe log file finalization to this function that no longer exists.\n\nBesides, it is obviously wrong to remove this log file in a function\nwhose name is remove_pidfile() ;-)\n\nAdding a new function to tempfile API that puts the file to a final\nplace if it is non-empty and otherwise remove it, and using that to\ncreate this \"gc.log\" file, would be the cleanest from the point of\nview of _this_ codepath. I however do not know if that is too\nspecific for the need of this codepath or \"leave it if non-empty,\nbut otherwise remove as it is uninteresting\" is fairly common thing\nwe would want and it is a good addition to the API set.\n\nMichael, what do you think?\n\n>> @@ -330,13 +341,21 @@ 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>> +\t\t\tif (strbuf_read_file(&sb, git_path(\"gc.log\"), 0) > 0)\n>> +\t\t\t\treturn error(_(\"last gc run reported:\\n\"\n>> +\t\t\t\t\t \"%s\\n\"\n>> +\t\t\t\t\t \"not running until %s is removed\"),\n>> +\t\t\t\t\t sb.buf, git_path(\"gc.log\"));\n>> +\t\t\tstrbuf_release(&sb);\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>> @@ -349,6 +368,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"},{"id":"270105","messageId":"55F93646.9050709@alum.mit.edu","threadId":"39812","inReplyTo":"xmqq1te0ykaj.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v4] gc: save log from daemonized gc --auto and print it next time","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-09-16T09:28:38Z","receivedAt":"2015-09-16T09:28:38Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 09/14/2015 07:37 PM, Junio C Hamano wrote:\n> Junio C Hamano writes:\n> \n>> Thanks, will queue.\n> \n> Ehh, I spoke a bit too early.\n> \n>>> diff --git a/builtin/gc.c b/builtin/gc.c\n>>> index bcc75d9..2c3aaeb 100644\n>>> --- a/builtin/gc.c\n>>> +++ b/builtin/gc.c\n>>> @@ -43,9 +43,20 @@ static struct argv_array prune_worktrees = 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> \n> Unfortuantely we cannot queue this as-is, as we let the tempfile API\n> to automatically manage the pidfile since ebebeaea (gc: use tempfile\n> module to handle gc.pid file, 2015-08-10), and you cannot piggy-back\n> the log file finalization to this function that no longer exists.\n> \n> Besides, it is obviously wrong to remove this log file in a function\n> whose name is remove_pidfile() ;-)\n> \n> Adding a new function to tempfile API that puts the file to a final\n> place if it is non-empty and otherwise remove it, and using that to\n> create this \"gc.log\" file, would be the cleanest from the point of\n> view of _this_ codepath. I however do not know if that is too\n> specific for the need of this codepath or \"leave it if non-empty,\n> but otherwise remove as it is uninteresting\" is fairly common thing\n> we would want and it is a good addition to the API set.\n> \n> Michael, what do you think?\n\nI'm not sure what behavior you want. At one point you say \"puts the file\nto a final place if it is non-empty\" but later you say \"leave it if\nnon-empty\". Should the file be written directly, or should it be written\nto a lockfile and renamed into place only when complete?\n\nTechnically I don't see a problem implementing either behavior. POSIX\nallows [1] calls to stat() and rename() from a signal handler. There is\na minor technical difficulty that commit_lock_file() allocates memory\nvia get_locked_file_path(), but this would be surmountable by\npre-allocating the memory for the locked file path and storing it in the\nlock_file object.\n\nThis doesn't seem like a common thing to want (as in, this might be the\nonly caller), but it probably makes sense to build it into the\ntempfile/lockfile API nevertheless, because implementing it externally\nwould require a lot of other code to be duplicated.\n\nAnother possibility that might work (maybe without requiring changes to\ntempfile/lockfile): don't worry about deleting the log file if it is\nempty, but make observers treat an empty log file the same as an absent one.\n\nMichael\n\n[1]\nhttp://pubs.opengroup.org/onlinepubs/9699919799/functions/V2_chap02.html#tag_15_04_03\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\n"},{"id":"270129","messageId":"xmqqmvwms6b1.fsf@gitster.mtv.corp.google.com","threadId":"39812","inReplyTo":"55F93646.9050709@alum.mit.edu","subject":"Re: [PATCH v4] gc: save log from daemonized gc --auto and print it next time","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-09-16T16:00:18Z","receivedAt":"2015-09-16T16:00:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> I'm not sure what behavior you want. At one point you say \"puts the file\n> to a final place if it is non-empty\" but later you say \"leave it if\n> non-empty\". Should the file be written directly, or should it be written\n> to a lockfile and renamed into place only when complete?\n\nI do not think we care that deeply either way, as we do not want to\nrun multiple auto-gc's at the same time in the first place. So either\none of the following is perfectly fine:\n\n * We open the final destination directly, but with O_CREAT|O_EXCL,\n which as a side effect also detects the simultanous execution\n [*1*]. We do not do any \"finalizing rename\" if we go this route.\n When we are done, close it and remove it if we did not write\n anything, or leave it there if we did write something.\n\n * We open a lockfile the usual way. When we are done, close it and\n and remove it if we did not write anything, or finalize it by\n renaming it if we did write something.\n\nI think Duy's code wants to do the latter.\n\n> This doesn't seem like a common thing to want (as in, this might be the\n> only caller), but it probably makes sense to build it into the\n> tempfile/lockfile API nevertheless, because implementing it externally\n> would require a lot of other code to be duplicated.\n>\n> Another possibility that might work (maybe without requiring changes to\n> tempfile/lockfile): don't worry about deleting the log file if it is\n> empty, but make observers treat an empty log file the same as an absent one.\n\nProbably your \"don't remove and check for emptiness\" approach would\nbe the simpler of the two, but I think we can go either way.\n\nThanks.\n\n[Footnote]\n\n*1* But one \"gc\" could be foreground gc that does not write to a log\n file and the other \"gc\" could be background that does write to a\n log file, so this cannot be the primary way to avoid double\n execution.\n"},{"id":"270198","messageId":"55FA8A8A.7000707@alum.mit.edu","threadId":"39812","inReplyTo":"xmqqmvwms6b1.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v4] gc: save log from daemonized gc --auto and print it next time","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-09-17T09:40:26Z","receivedAt":"2015-09-17T09:40:26Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 09/16/2015 06:00 PM, Junio C Hamano wrote:\n> *1* But one \"gc\" could be foreground gc that does not write to a log\n> file and the other \"gc\" could be background that does write to a\n> log file, so this cannot be the primary way to avoid double\n> execution.\n\n^^^ This was the point that was confusing me. If this is not one of the\nroles of the log file, then things are easier.\n\nIf you decide to go the route of tempfile/lockfile modifications, feel\nfree to CC me for feedback.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\n"},{"id":"270208","messageId":"CACsJy8CMFkGRCXuMiRme=B9ijq6ydWGPz0vyb8QoEzf4Ty3FDQ@mail.gmail.com","threadId":"39812","inReplyTo":"xmqqmvwms6b1.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v4] gc: save log from daemonized gc --auto and print it next time","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2015-09-17T13:08:25Z","receivedAt":"2015-09-17T13:08:25Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, Sep 16, 2015 at 11:00 PM, Junio C Hamano wrote:\n> Michael Haggerty writes:\n>\n>> I'm not sure what behavior you want. At one point you say \"puts the file\n>> to a final place if it is non-empty\" but later you say \"leave it if\n>> non-empty\". Should the file be written directly, or should it be written\n>> to a lockfile and renamed into place only when complete?\n>\n> I do not think we care that deeply either way, as we do not want to\n> run multiple auto-gc's at the same time in the first place. So either\n> one of the following is perfectly fine:\n>\n> * We open the final destination directly, but with O_CREAT|O_EXCL,\n> which as a side effect also detects the simultanous execution\n> [*1*]. We do not do any \"finalizing rename\" if we go this route.\n> When we are done, close it and remove it if we did not write\n> anything, or leave it there if we did write something.\n>\n> * We open a lockfile the usual way. When we are done, close it and\n> and remove it if we did not write anything, or finalize it by\n> renaming it if we did write something.\n>\n> I think Duy's code wants to do the latter.\n\nWe do keep another lock before coming to opening this log file. So\nonce we get here we already know nobody else will be opening the log\nfile. We can simply open it the normal way, then make sure we clean it\nup at atexit().\n\n>> This doesn't seem like a common thing to want (as in, this might be the\n>> only caller), but it probably makes sense to build it into the\n>> tempfile/lockfile API nevertheless, because implementing it externally\n>> would require a lot of other code to be duplicated.\n>>\n>> Another possibility that might work (maybe without requiring changes to\n>> tempfile/lockfile): don't worry about deleting the log file if it is\n>> empty, but make observers treat an empty log file the same as an absent one.\n>\n> Probably your \"don't remove and check for emptiness\" approach would\n> be the simpler of the two, but I think we can go either way.\n\nPeople have complained to me about stray files in $GIT_DIR, so it's\nprobably best that we delete empty/useless files. Although we could\ndelete empty files at the beginning of the next gc instead of at\natexit(). Let me try it out and see which is simplest.\n-- \nDuy\n"},{"id":"270215","messageId":"xmqqlhc5ksod.fsf@gitster.mtv.corp.google.com","threadId":"39812","inReplyTo":"CACsJy8CMFkGRCXuMiRme=B9ijq6ydWGPz0vyb8QoEzf4Ty3FDQ@mail.gmail.com","subject":"Re: [PATCH v4] gc: save log from daemonized gc --auto and print it next time","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-09-17T14:48:50Z","receivedAt":"2015-09-17T14:48:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen writes:\n\n> We do keep another lock before coming to opening this log file. So\n> once we get here we already know nobody else will be opening the log\n> file. We can simply open it the normal way, then make sure we clean it\n> up at atexit().\n>\n>>> This doesn't seem like a common thing to want (as in, this might be the\n>>> only caller), but it probably makes sense to build it into the\n>>> tempfile/lockfile API nevertheless, because implementing it externally\n>>> would require a lot of other code to be duplicated.\n>>>\n>>> Another possibility that might work (maybe without requiring changes to\n>>> tempfile/lockfile): don't worry about deleting the log file if it is\n>>> empty, but make observers treat an empty log file the same as an absent one.\n>>\n>> Probably your \"don't remove and check for emptiness\" approach would\n>> be the simpler of the two, but I think we can go either way.\n>\n> People have complained to me about stray files in $GIT_DIR, so it's\n> probably best that we delete empty/useless files. Although we could\n> delete empty files at the beginning of the next gc instead of at\n> atexit(). Let me try it out and see which is simplest.\n\nIt would be nicer if we did not have to do an extra and custom\natexit() handler.\n"},{"id":"270326","messageId":"1442639603-25568-1-git-send-email-pclouds@gmail.com","threadId":"39812","inReplyTo":"1442108202-25746-1-git-send-email-pclouds@gmail.com","subject":"[PATCH v5] 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-09-19T05:13:23Z","receivedAt":"2015-09-19T05:13: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\nDaemonized gc now saves stderr to $GIT_DIR/gc.log. Following gc --auto\nwill not run and gc.log printed out until the user removes gc.log.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n This is the lock-based version. I'm sending an alternative that does\n not need atexit() with comparison between the two.\n\n builtin/gc.c | 56 +++++++++++++++++++++++++++++++++++++++++++++++++++++++-\n 1 file changed, 55 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex bcc75d9..4e738fa 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -43,6 +43,7 @@ static struct argv_array prune_worktrees = ARGV_ARRAY_INIT;\n static struct argv_array rerere = ARGV_ARRAY_INIT;\n \n static char *pidfile;\n+static struct lock_file log_lock;\n \n static void remove_pidfile(void)\n {\n@@ -57,6 +58,28 @@ static void remove_pidfile_on_signal(int signo)\n \traise(signo);\n }\n \n+static void process_log_file(void)\n+{\n+\tstruct stat st;\n+\tif (!fstat(log_lock.fd, &st) && st.st_size)\n+\t\tcommit_lock_file(&log_lock);\n+\telse\n+\t\trollback_lock_file(&log_lock);\n+}\n+\n+static void process_log_file_at_exit(void)\n+{\n+\tfflush(stderr);\n+\tprocess_log_file();\n+}\n+\n+static void process_log_file_on_signal(int signo)\n+{\n+\tprocess_log_file();\n+\tsigchain_pop(signo);\n+\traise(signo);\n+}\n+\n static void git_config_date_string(const char *key, const char **output)\n {\n \tif (git_config_get_string_const(key, output))\n@@ -253,6 +276,24 @@ static const char *lock_repo_for_gc(int force, pid_t* ret_pid)\n \treturn NULL;\n }\n \n+static int report_last_gc_error(void)\n+{\n+\tstruct strbuf sb = STRBUF_INIT;\n+\tint ret;\n+\n+\tret = strbuf_read_file(&sb, git_path(\"gc.log\"), 0);\n+\tif (ret > 0)\n+\t\treturn error(_(\"The last gc run reported the following. \"\n+\t\t\t \"Please correct the root cause\\n\"\n+\t\t\t \"and remove %s.\\n\"\n+\t\t\t \"Automatic cleanup will not be performed \"\n+\t\t\t \"until the file is removed.\\n\\n\"\n+\t\t\t \"%s\"),\n+\t\t\t git_path(\"gc.log\"), sb.buf);\n+\tstrbuf_release(&sb);\n+\treturn 0;\n+}\n+\n static int gc_before_repack(void)\n {\n \tif (pack_refs && run_command_v_opt(pack_refs_cmd.argv, RUN_GIT_CMD))\n@@ -274,6 +315,7 @@ int cmd_gc(int argc, const char **argv, const char *prefix)\n \tint force = 0;\n \tconst char *name;\n \tpid_t pid;\n+\tint daemonized = 0;\n \n \tstruct option builtin_gc_options[] = {\n \t\tOPT__QUIET(&quiet, N_(\"suppress progress reporting\")),\n@@ -330,13 +372,16 @@ 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\tif (report_last_gc_error())\n+\t\t\t\treturn -1;\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@@ -349,6 +394,15 @@ 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\thold_lock_file_for_update(&log_lock,\n+\t\t\t\t\t git_path(\"gc.log\"),\n+\t\t\t\t\t LOCK_DIE_ON_ERROR);\n+\t\tdup2(log_lock.fd, 2);\n+\t\tsigchain_push_common(process_log_file_on_signal);\n+\t\tatexit(process_log_file_at_exit);\n+\t}\n+\n \tif (gc_before_repack())\n \t\treturn -1;\n \n-- \n2.3.0.rc1.137.g477eb31\n"},{"id":"270327","messageId":"1442639665-25692-1-git-send-email-pclouds@gmail.com","threadId":"39812","inReplyTo":"1442108202-25746-1-git-send-email-pclouds@gmail.com","subject":"[Alt. PATCH v5] 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-09-19T05:14:25Z","receivedAt":"2015-09-19T05:14:25Z","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\nDaemonized gc now saves stderr to $GIT_DIR/gc.log. Following gc --auto\nwill not run and gc.log printed out until the user removes gc.log.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n The lock-based version has an advantage that the following gc runs\n will never see partial gc.log. But it requires some more hook at\n atexit() and maybe signal handler.\n \n This version avoids that, and gc.log can be kept even if gc is\n SIGKILL'd (unlikely because gc itself does not do anything that can\n upset the kernel), but then it's racy.\n\n I think I perfer the lock-based version.\n\n builtin/gc.c | 66 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-\n 1 file changed, 65 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex bcc75d9..3d42ef7 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -43,6 +43,8 @@ static struct argv_array prune_worktrees = 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@@ -253,6 +255,41 @@ static const char *lock_repo_for_gc(int force, pid_t* ret_pid)\n \treturn NULL;\n }\n \n+static int report_last_gc_error(void)\n+{\n+\tstruct strbuf sb = STRBUF_INIT;\n+\tint ret;\n+\n+\tif (!access(git_path(\"gc.pid\"), R_OK))\n+\t\t/*\n+\t\t * Either background gc is still running, or we have a stale\n+\t\t * gc.pid.\n+\t\t *\n+\t\t * If the former, it's not the right time to look at gc.log\n+\t\t * yet. We'll exit shortly after lock_repo_for_gc().\n+\t\t *\n+\t\t * If the latter, we'll run gc --auto one more time. But\n+\t\t * because gc.log is appended, the old log (if exists) won't\n+\t\t * be lost. One extra gc run is not that big a deal to avoid.\n+\t\t */\n+\t\treturn 0;\n+\n+\tret = strbuf_read_file(&sb, git_path(\"gc.log\"), 0);\n+\tif (ret > 0)\n+\t\treturn error(_(\"The last gc run reported the following. \"\n+\t\t\t \"Please correct the root cause\\n\"\n+\t\t\t \"and remove %s. Automatic cleanup will not \"\n+\t\t\t \"be performed\\n\"\n+\t\t\t \"until the file is removed.\\n\\n\"\n+\t\t\t \"%s\"),\n+\t\t\t git_path(\"gc.log\"), sb.buf);\n+\telse if (!ret)\n+\t\t/* racy, but in the worst case we waste one more gc run */\n+\t\tunlink(git_path(\"gc.log\"));\n+\tstrbuf_release(&sb);\n+\treturn 0;\n+}\n+\n static int gc_before_repack(void)\n {\n \tif (pack_refs && run_command_v_opt(pack_refs_cmd.argv, RUN_GIT_CMD))\n@@ -330,13 +367,16 @@ 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\tif (report_last_gc_error())\n+\t\t\t\treturn -1;\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@@ -349,6 +389,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_git_path(&log_filename, \"gc.log\");\n+\t\tfd = open(log_filename.buf, O_WRONLY | O_CREAT | O_APPEND, 0644);\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@@ -376,6 +428,17 @@ int cmd_gc(int argc, const char **argv, const char *prefix)\n \t\twarning(_(\"There are too many unreachable loose objects; \"\n \t\t\t\"run 'git prune' to remove them.\"));\n \n+\t/*\n+\t * Opportunistic cleanup in the good case (which most likely\n+\t * produce empty gc.log), otherwise empty gc.log will be\n+\t * deleted at the next auto gc run.\n+\t */\n+\tif (daemonized && log_filename.len) {\n+\t\tstruct stat st;\n+\n+\t\tfflush(stderr);\n+\t\tif (!stat(log_filename.buf, &st) && !st.st_size)\n+\t\t\tunlink(log_filename.buf);\n+\t}\n \treturn 0;\n }\n-- \n2.3.0.rc1.137.g477eb31\n"},{"id":"270399","messageId":"xmqqy4fzbv90.fsf@gitster.mtv.corp.google.com","threadId":"39812","inReplyTo":"1442639665-25692-1-git-send-email-pclouds@gmail.com","subject":"Re: [Alt. PATCH v5] gc: save log from daemonized gc --auto and print it next time","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-09-21T16:19:23Z","receivedAt":"2015-09-21T16:19:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy writes:\n\n> The lock-based version has an advantage that the following gc runs\n> will never see partial gc.log. But it requires some more hook at\n> atexit() and maybe signal handler.\n> \n> This version avoids that, and gc.log can be kept even if gc is\n> SIGKILL'd (unlikely because gc itself does not do anything that can\n> upset the kernel), but then it's racy.\n\nGiven that you are only interested in an non-empty output, and also\nyou want to avoid running an auto-gc when another one is already\nrunning, I wonder if that \"never see partial\" property really\nmatters.\n\nIn any case, \"an alternative could be to avoid lockfile and not\nbothering to remove an empty one\" was idea from Michael, so let's CC\nhim back in the loop.\n\n> I think I perfer the lock-based version.\n\nIt certainly does look more familiar to me, too ;-)\n"},{"id":"270400","messageId":"xmqqtwqnbu58.fsf@gitster.mtv.corp.google.com","threadId":"39812","inReplyTo":"1442639603-25568-1-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH v5] gc: save log from daemonized gc --auto and print it next time","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-09-21T16:43:15Z","receivedAt":"2015-09-21T16:43:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy 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> Daemonized gc now saves stderr to $GIT_DIR/gc.log. Following gc --auto\n> will not run and gc.log printed out until the user removes gc.log.\n>\n> Signed-off-by: Nguyễn Thái Ngọc Duy \n> ---\n> This is the lock-based version. I'm sending an alternative that does\n> not need atexit() with comparison between the two.\n\nI think Michael deserves to be CC'ed.\n\nI am wondering if we need an audit of what our signal handlers do in\nthe same light as what led to 507d7804 (pager: don't use unsafe\nfunctions in signal handlers, 2015-09-04). lockfile API already\ndoes removal in remove_lock_files_on_signal(), which may or may not\nneed further fixing. And that is why I really hate to see us adding\ncustom signal handler if we can avoid it.\n\nWhat you are gaining by having a custom signal handler here is\ntwofold, I think:\n\n * You have a chance to remove an empty log message upon signal, but\n if you follow Michael's idea of equating an empty and missing log\n file, that would not be a huge plus either way.\n\n * You can keep a non-empty log message that may be incomplete when\n you are killed yourself. If you reused the lockfile API and let\n its signal handler clean it up, you obviously will lose that, but\n you do not even know if that incomplete non-empty log message is\n something you want to base the \"stop auto-gc until an existing\n problem is resolved\" decision on. Your subprocess may have left\n that message only because you are getting killed, no?\n\nNeither looks like a compelling enough reason to have it to me.\n\nWouldn't the logic be vastly simplified and be still correct, if you\nlet the lockfile API do its usual \"unless you explicitly commit,\nlockfiles are removed upon signal or exit\"?\n\n> builtin/gc.c | 56 +++++++++++++++++++++++++++++++++++++++++++++++++++++++-\n> 1 file changed, 55 insertions(+), 1 deletion(-)\n>\n> diff --git a/builtin/gc.c b/builtin/gc.c\n> index bcc75d9..4e738fa 100644\n> --- a/builtin/gc.c\n> +++ b/builtin/gc.c\n> @@ -43,6 +43,7 @@ static struct argv_array prune_worktrees = ARGV_ARRAY_INIT;\n> static struct argv_array rerere = ARGV_ARRAY_INIT;\n> \n> static char *pidfile;\n> +static struct lock_file log_lock;\n> \n> static void remove_pidfile(void)\n> {\n> @@ -57,6 +58,28 @@ static void remove_pidfile_on_signal(int signo)\n> \traise(signo);\n> }\n> \n> +static void process_log_file(void)\n> +{\n> +\tstruct stat st;\n> +\tif (!fstat(log_lock.fd, &st) && st.st_size)\n> +\t\tcommit_lock_file(&log_lock);\n> +\telse\n> +\t\trollback_lock_file(&log_lock);\n> +}\n> +\n> +static void process_log_file_at_exit(void)\n> +{\n> +\tfflush(stderr);\n> +\tprocess_log_file();\n> +}\n> +\n> +static void process_log_file_on_signal(int signo)\n> +{\n> +\tprocess_log_file();\n> +\tsigchain_pop(signo);\n> +\traise(signo);\n> +}\n> +\n> static void git_config_date_string(const char *key, const char **output)\n> {\n> \tif (git_config_get_string_const(key, output))\n> @@ -253,6 +276,24 @@ static const char *lock_repo_for_gc(int force, pid_t* ret_pid)\n> \treturn NULL;\n> }\n> \n> +static int report_last_gc_error(void)\n> +{\n> +\tstruct strbuf sb = STRBUF_INIT;\n> +\tint ret;\n> +\n> +\tret = strbuf_read_file(&sb, git_path(\"gc.log\"), 0);\n> +\tif (ret > 0)\n> +\t\treturn error(_(\"The last gc run reported the following. \"\n> +\t\t\t \"Please correct the root cause\\n\"\n> +\t\t\t \"and remove %s.\\n\"\n> +\t\t\t \"Automatic cleanup will not be performed \"\n> +\t\t\t \"until the file is removed.\\n\\n\"\n> +\t\t\t \"%s\"),\n> +\t\t\t git_path(\"gc.log\"), sb.buf);\n> +\tstrbuf_release(&sb);\n> +\treturn 0;\n> +}\n> +\n> static int gc_before_repack(void)\n> {\n> \tif (pack_refs && run_command_v_opt(pack_refs_cmd.argv, RUN_GIT_CMD))\n> @@ -274,6 +315,7 @@ int cmd_gc(int argc, const char **argv, const char *prefix)\n> \tint force = 0;\n> \tconst char *name;\n> \tpid_t pid;\n> +\tint daemonized = 0;\n> \n> \tstruct option builtin_gc_options[] = {\n> \t\tOPT__QUIET(&quiet, N_(\"suppress progress reporting\")),\n> @@ -330,13 +372,16 @@ 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\tif (report_last_gc_error())\n> +\t\t\t\treturn -1;\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> @@ -349,6 +394,15 @@ 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\thold_lock_file_for_update(&log_lock,\n> +\t\t\t\t\t git_path(\"gc.log\"),\n> +\t\t\t\t\t LOCK_DIE_ON_ERROR);\n> +\t\tdup2(log_lock.fd, 2);\n> +\t\tsigchain_push_common(process_log_file_on_signal);\n> +\t\tatexit(process_log_file_at_exit);\n> +\t}\n> +\n> \tif (gc_before_repack())\n> \t\treturn -1;\n"}]}