{"thread":{"id":"35026","subject":"[PATCH] gc: remove gc.pid file at end of execution","startedAt":"2013-09-26T09:18:18Z","lastAt":"2013-10-16T23:11:46Z","messageCount":5,"participants":["Matthieu Moy","Jonathan Nieder","Duy Nguyen","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"228268","messageId":"1380187098-8519-1-git-send-email-Matthieu.Moy@imag.fr","threadId":"35026","inReplyTo":null,"subject":"[PATCH] gc: remove gc.pid file at end of execution","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@imag.fr","sentAt":"2013-09-26T09:18:18Z","receivedAt":"2013-09-26T09:18:18Z","isPatch":true,"sender":{"key":"git@matthieu-moy.fr","avatar":"https://avatars.githubusercontent.com/u/14709?v=4"},"body":"This file isn't really harmful, but isn't useful either, and can create\nminor annoyance for the user:\n\n* It's confusing, as the presence of a *.pid file often implies that a\n  process is currently running. A user running \"ls .git/\" and finding\n  this file may incorrectly guess that a \"git gc\" is currently running.\n\n* Leaving this file means that a \"git gc\" in an already gc-ed repo is\n  no-longer a no-op. A user running \"git gc\" in a set of repositories,\n  and then synchronizing this set (e.g. rsync -av, unison, ...) will see\n  all the gc.pid files as changed, which creates useless noise.\n\nThis patch unlinks the file after the garbage collection is done, so that\ngc.pid is actually present only during execution.\n\nFuture versions of Git may want to use the information left in the gc.pid\nfile (e.g. for policies like \"don't attempt to run a gc if one has\nalready been ran less than X hours ago\"). If so, this patch can safely be\nreverted. For now, let's not bother the users.\n\nSigned-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>\n---\n builtin/gc.c | 4 ++++\n 1 file changed, 4 insertions(+)\n\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex 891a2c2..db39963 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -316,5 +316,9 @@ 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 * The file isn't harmful, but isn't useful either.\n+\t */\n+\tunlink(git_path(\"gc.pid\"));\n \treturn 0;\n }\n-- \n1.8.4.474.g128a96c\n"},{"id":"228339","messageId":"20130928003319.GR9464@google.com","threadId":"35026","inReplyTo":"1380187098-8519-1-git-send-email-Matthieu.Moy@imag.fr","subject":"Re: [PATCH] gc: remove gc.pid file at end of execution","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-09-28T00:33:20Z","receivedAt":"2013-09-28T00:33:20Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Matthieu Moy wrote:\n\n> This file isn't really harmful, but isn't useful either, and can create\n> minor annoyance for the user:\n\nWould something like the following make sense, to ensure the gc.pid file is\nalways removed on normal exit?\n\nSigned-off-by: Jonathan Nieder <jrnieder@gmail.com>\n\ndiff --git c/builtin/gc.c i/builtin/gc.c\nindex 6e0d81a..fbbc144 100644\n--- c/builtin/gc.c\n+++ i/builtin/gc.c\n@@ -14,6 +14,7 @@\n #include \"cache.h\"\n #include \"parse-options.h\"\n #include \"run-command.h\"\n+#include \"sigchain.h\"\n #include \"argv-array.h\"\n \n #define FAILED_RUN \"failed to run %s\"\n@@ -167,6 +168,21 @@ static int need_to_gc(void)\n \treturn 1;\n }\n \n+static char *pidfile;\n+\n+static void remove_pidfile(void)\n+{\n+\tif (pidfile)\n+\t\tunlink(pidfile);\n+}\n+\n+static void remove_pidfile_on_signal(int signo)\n+{\n+\tremove_pidfile();\n+\tsigchain_pop(signo);\n+\traise(signo);\n+}\n+\n /* return NULL on success, else hostname running the gc */\n static const char *lock_repo_for_gc(int force, pid_t* ret_pid)\n {\n@@ -179,13 +195,19 @@ static const char *lock_repo_for_gc(int force, pid_t* ret_pid)\n \tFILE *fp;\n \tint fd, should_exit;\n \n+\tif (pidfile)\n+\t\t/* already locked */\n+\t\treturn NULL;\n+\n \tif (gethostname(my_host, sizeof(my_host)))\n \t\tstrcpy(my_host, \"unknown\");\n \n-\tfd = hold_lock_file_for_update(&lock, git_path(\"gc.pid\"),\n+\tpidfile = git_pathdup(\"gc.pid\");\n+\n+\tfd = hold_lock_file_for_update(&lock, pidfile,\n \t\t\t\t       LOCK_DIE_ON_ERROR);\n \tif (!force) {\n-\t\tfp = fopen(git_path(\"gc.pid\"), \"r\");\n+\t\tfp = fopen(pidfile, \"r\");\n \t\tmemset(locking_host, 0, sizeof(locking_host));\n \t\tshould_exit =\n \t\t\tfp != NULL &&\n@@ -208,6 +230,7 @@ static const char *lock_repo_for_gc(int force, pid_t* ret_pid)\n \t\tif (should_exit) {\n \t\t\tif (fd >= 0)\n \t\t\t\trollback_lock_file(&lock);\n+\t\t\tpidfile = NULL;\n \t\t\t*ret_pid = pid;\n \t\t\treturn locking_host;\n \t\t}\n@@ -219,6 +242,9 @@ static const char *lock_repo_for_gc(int force, pid_t* ret_pid)\n \tstrbuf_release(&sb);\n \tcommit_lock_file(&lock);\n \n+\tsigchain_push_common(remove_pidfile_on_signal);\n+\tatexit(remove_pidfile);\n+\n \treturn NULL;\n }\n \n"},{"id":"228344","messageId":"CACsJy8Ax1YypAnVb0FAQPsvvZixq0aJHP=ahfMaXn26KTaN1VA@mail.gmail.com","threadId":"35026","inReplyTo":"20130928003319.GR9464@google.com","subject":"Re: [PATCH] gc: remove gc.pid file at end of execution","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2013-09-28T03:42:41Z","receivedAt":"2013-09-28T03:42:41Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sat, Sep 28, 2013 at 7:33 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:\n> Matthieu Moy wrote:\n>\n>> This file isn't really harmful, but isn't useful either, and can create\n>> minor annoyance for the user:\n>\n> Would something like the following make sense, to ensure the gc.pid file is\n> always removed on normal exit?\n>\n> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>\n>\n> diff --git c/builtin/gc.c i/builtin/gc.c\n> index 6e0d81a..fbbc144 100644\n> --- c/builtin/gc.c\n> +++ i/builtin/gc.c\n> @@ -14,6 +14,7 @@\n>  #include \"cache.h\"\n>  #include \"parse-options.h\"\n>  #include \"run-command.h\"\n> +#include \"sigchain.h\"\n>  #include \"argv-array.h\"\n>\n>  #define FAILED_RUN \"failed to run %s\"\n> @@ -167,6 +168,21 @@ static int need_to_gc(void)\n>         return 1;\n>  }\n>\n> +static char *pidfile;\n> +\n> +static void remove_pidfile(void)\n> +{\n> +       if (pidfile)\n> +               unlink(pidfile);\n> +}\n> +\n> +static void remove_pidfile_on_signal(int signo)\n> +{\n> +       remove_pidfile();\n> +       sigchain_pop(signo);\n> +       raise(signo);\n> +}\n> +\n>  /* return NULL on success, else hostname running the gc */\n>  static const char *lock_repo_for_gc(int force, pid_t* ret_pid)\n>  {\n> @@ -179,13 +195,19 @@ static const char *lock_repo_for_gc(int force, pid_t* ret_pid)\n>         FILE *fp;\n>         int fd, should_exit;\n>\n> +       if (pidfile)\n> +               /* already locked */\n> +               return NULL;\n> +\n>         if (gethostname(my_host, sizeof(my_host)))\n>                 strcpy(my_host, \"unknown\");\n>\n> -       fd = hold_lock_file_for_update(&lock, git_path(\"gc.pid\"),\n> +       pidfile = git_pathdup(\"gc.pid\");\n> +\n> +       fd = hold_lock_file_for_update(&lock, pidfile,\n>                                        LOCK_DIE_ON_ERROR);\n>         if (!force) {\n> -               fp = fopen(git_path(\"gc.pid\"), \"r\");\n> +               fp = fopen(pidfile, \"r\");\n>                 memset(locking_host, 0, sizeof(locking_host));\n>                 should_exit =\n>                         fp != NULL &&\n> @@ -208,6 +230,7 @@ static const char *lock_repo_for_gc(int force, pid_t* ret_pid)\n>                 if (should_exit) {\n>                         if (fd >= 0)\n>                                 rollback_lock_file(&lock);\n> +                       pidfile = NULL;\n>                         *ret_pid = pid;\n>                         return locking_host;\n>                 }\n> @@ -219,6 +242,9 @@ static const char *lock_repo_for_gc(int force, pid_t* ret_pid)\n>         strbuf_release(&sb);\n\nIt may be a bit simpler to delay setting pidfile until we get here.\nlock.filename still contains gc.pid until commit_lock_file is called.\n\n>         commit_lock_file(&lock);\n>\n> +       sigchain_push_common(remove_pidfile_on_signal);\n> +       atexit(remove_pidfile);\n> +\n>         return NULL;\n>  }\n-- \nDuy\n"},{"id":"229092","messageId":"xmqqeh7k6glt.fsf@gitster.dls.corp.google.com","threadId":"35026","inReplyTo":"20130928003319.GR9464@google.com","subject":"Re: [PATCH] gc: remove gc.pid file at end of execution","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-10-16T21:38:54Z","receivedAt":"2013-10-16T21:38:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder <jrnieder@gmail.com> writes:\n\n> Matthieu Moy wrote:\n>\n>> This file isn't really harmful, but isn't useful either, and can create\n>> minor annoyance for the user:\n>\n> Would something like the following make sense, to ensure the gc.pid file is\n> always removed on normal exit?\n\nHas anything further happened to this discussion?\n"},{"id":"229108","messageId":"20131016231146.GP9464@google.com","threadId":"35026","inReplyTo":"xmqqeh7k6glt.fsf@gitster.dls.corp.google.com","subject":"[PATCH v2] gc: remove gc.pid file at end of execution","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-10-16T23:11:46Z","receivedAt":"2013-10-16T23:11:46Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"This file isn't really harmful, but isn't useful either, and can create\nminor annoyance for the user:\n\n* It's confusing, as the presence of a *.pid file often implies that a\n  process is currently running. A user running \"ls .git/\" and finding\n  this file may incorrectly guess that a \"git gc\" is currently running.\n\n* Leaving this file means that a \"git gc\" in an already gc-ed repo is\n  no-longer a no-op. A user running \"git gc\" in a set of repositories,\n  and then synchronizing this set (e.g. rsync -av, unison, ...) will see\n  all the gc.pid files as changed, which creates useless noise.\n\nThis patch unlinks the file after the garbage collection is done, so that\ngc.pid is actually present only during execution.\n\nFuture versions of Git may want to use the information left in the gc.pid\nfile (e.g. for policies like \"don't attempt to run a gc if one has\nalready been ran less than X hours ago\"). If so, this patch can safely be\nreverted. For now, let's not bother the users.\n\nExplained-by: Matthieu Moy <Matthieu.Moy@imag.fr>\nSigned-off-by: Jonathan Nieder <jrnieder@gmail.com>\nImproved-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\nJunio C Hamano wrote:\n\n> Has anything further happened to this discussion?\n\nHere's a patch implementing Duy's suggestion.\n\n builtin/gc.c  | 24 ++++++++++++++++++++++++\n t/t6500-gc.sh |  5 +++++\n 2 files changed, 29 insertions(+)\n\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex 891a2c2..c14190f 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -14,6 +14,7 @@\n #include \"cache.h\"\n #include \"parse-options.h\"\n #include \"run-command.h\"\n+#include \"sigchain.h\"\n #include \"argv-array.h\"\n \n #define FAILED_RUN \"failed to run %s\"\n@@ -35,6 +36,21 @@ static struct argv_array repack = ARGV_ARRAY_INIT;\n static struct argv_array prune = ARGV_ARRAY_INIT;\n static struct argv_array rerere = ARGV_ARRAY_INIT;\n \n+static char *pidfile;\n+\n+static void remove_pidfile(void)\n+{\n+\tif (pidfile)\n+\t\tunlink(pidfile);\n+}\n+\n+static void remove_pidfile_on_signal(int signo)\n+{\n+\tremove_pidfile();\n+\tsigchain_pop(signo);\n+\traise(signo);\n+}\n+\n static int gc_config(const char *var, const char *value, void *cb)\n {\n \tif (!strcmp(var, \"gc.packrefs\")) {\n@@ -179,6 +195,10 @@ static const char *lock_repo_for_gc(int force, pid_t* ret_pid)\n \tFILE *fp;\n \tint fd, should_exit;\n \n+\tif (pidfile)\n+\t\t/* already locked */\n+\t\treturn NULL;\n+\n \tif (gethostname(my_host, sizeof(my_host)))\n \t\tstrcpy(my_host, \"unknown\");\n \n@@ -219,6 +239,10 @@ static const char *lock_repo_for_gc(int force, pid_t* ret_pid)\n \tstrbuf_release(&sb);\n \tcommit_lock_file(&lock);\n \n+\tpidfile = git_pathdup(\"gc.pid\");\n+\tsigchain_push_common(remove_pidfile_on_signal);\n+\tatexit(remove_pidfile);\n+\n \treturn NULL;\n }\n \ndiff --git a/t/t6500-gc.sh b/t/t6500-gc.sh\nindex b1a6365..63194d8 100755\n--- a/t/t6500-gc.sh\n+++ b/t/t6500-gc.sh\n@@ -9,6 +9,11 @@ test_expect_success 'gc empty repository' '\n \tgit gc\n '\n \n+test_expect_success 'gc does not leave behind pid file' '\n+\tgit gc &&\n+\ttest_path_is_missing .git/gc.pid\n+'\n+\n test_expect_success 'gc --gobbledegook' '\n \ttest_expect_code 129 git gc --nonsense 2>err &&\n \ttest_i18ngrep \"[Uu]sage: git gc\" err\n-- \n1.8.4-50-g437ce60\n"}]}