{"thread":{"id":"34597","subject":"[PATCH] gc: reject if another gc is running, unless --force is given","startedAt":"2013-08-03T04:20:05Z","lastAt":"2013-08-10T07:11:33Z","messageCount":20,"participants":["Nguyễn Thái Ngọc Duy","Ramkumar Ramachandra","Johannes Sixt","Duy Nguyen","Junio C Hamano","Andres Perera","Andreas Schwab"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"224484","messageId":"1375503605-32480-1-git-send-email-pclouds@gmail.com","threadId":"34597","inReplyTo":null,"subject":"[PATCH] gc: reject if another gc is running, unless --force is given","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2013-08-03T04:20:05Z","receivedAt":"2013-08-03T04:20:05Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"This may happen when `git gc --auto` is run automatically, then the\nuser, to avoid wait time, switches to a new terminal, keeps working\nand `git gc --auto` is started again because the first gc instance has\nnot clean up the repository.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n I don't know if the kill(pid, 0) trick works on Windows..\n\n Documentation/git-gc.txt | 6 +++++-\n builtin/gc.c | 18 ++++++++++++++++++\n 2 files changed, 23 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/git-gc.txt b/Documentation/git-gc.txt\nindex 2402ed6..e158a3b 100644\n--- a/Documentation/git-gc.txt\n+++ b/Documentation/git-gc.txt\n@@ -9,7 +9,7 @@ git-gc - Cleanup unnecessary files and optimize the local repository\n SYNOPSIS\n --------\n [verse]\n-'git gc' [--aggressive] [--auto] [--quiet] [--prune= | --no-prune]\n+'git gc' [--aggressive] [--auto] [--quiet] [--prune= | --no-prune] [--force]\n \n DESCRIPTION\n -----------\n@@ -72,6 +72,10 @@ automatic consolidation of packs.\n --quiet::\n \tSuppress all progress reports.\n \n+--force::\n+\tForce `git gc` to run even if there may be another `git gc`\n+\tinstance running on this repository.\n+\n Configuration\n -------------\n \ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex 6be6c8d..07c566c 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -169,9 +169,11 @@ static int need_to_gc(void)\n \n int cmd_gc(int argc, const char **argv, const char *prefix)\n {\n+\tFILE *fp;\n \tint aggressive = 0;\n \tint auto_gc = 0;\n \tint quiet = 0;\n+\tint force = 0;\n \n \tstruct option builtin_gc_options[] = {\n \t\tOPT__QUIET(&quiet, N_(\"suppress progress reporting\")),\n@@ -180,6 +182,7 @@ int cmd_gc(int argc, const char **argv, const char *prefix)\n \t\t\tPARSE_OPT_OPTARG, NULL, (intptr_t)prune_expire },\n \t\tOPT_BOOLEAN(0, \"aggressive\", &aggressive, N_(\"be more thorough (increased runtime)\")),\n \t\tOPT_BOOLEAN(0, \"auto\", &auto_gc, N_(\"enable auto-gc mode\")),\n+\t\tOPT_BOOL(0, \"force\", &force, N_(\"force running gc even if there may be another gc running\")),\n \t\tOPT_END()\n \t};\n \n@@ -225,6 +228,21 @@ int cmd_gc(int argc, const char **argv, const char *prefix)\n \t} else\n \t\tadd_repack_all_option();\n \n+\tif (!force && (fp = fopen(git_path(\"gc.pid\"), \"r\")) != NULL) {\n+\t\tuintmax_t pid;\n+\t\tif (fscanf(fp, \"%\"PRIuMAX, &pid) == 1 && !kill(pid, 0)) {\n+\t\t\tif (auto_gc)\n+\t\t\t\treturn 0; /* be quiet on --auto */\n+\t\t\tdie(_(\"gc is already running\"));\n+\t\t}\n+\t\tfclose(fp);\n+\t}\n+\n+\tif ((fp = fopen(git_path(\"gc.pid\"), \"w\")) != NULL) {\n+\t\tfprintf(fp, \"%\"PRIuMAX\"\\n\", (uintmax_t) getpid());\n+\t\tfclose(fp);\n+\t}\n+\n \tif (pack_refs && run_command_v_opt(pack_refs_cmd.argv, RUN_GIT_CMD))\n \t\treturn error(FAILED_RUN, pack_refs_cmd.argv[0]);\n \n-- \n1.8.2.83.gc99314b\n"},{"id":"224488","messageId":"1375510890-4728-1-git-send-email-pclouds@gmail.com","threadId":"34597","inReplyTo":"1375503605-32480-1-git-send-email-pclouds@gmail.com","subject":"[PATCH v2] gc: reject if another gc is running, unless --force is given","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2013-08-03T06:21:30Z","receivedAt":"2013-08-03T06:21:30Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"This may happen when `git gc --auto` is run automatically, then the\nuser, to avoid wait time, switches to a new terminal, keeps working\nand `git gc --auto` is started again because the first gc instance has\nnot clean up the repository.\n\nThis patch tries to avoid multiple gc running, especially in --auto\nmode. In the worst case, gc may be delayed 12 hours if a daemon reuses\nthe pid stored in gc-%s.pid.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n v2 deals with the stored pid taken by another process and takes\n shared repository into account (sort of, it only makes sure gc is not\n run multiple times on the same machine, not only one gc instance across\n multiple machines)\n\n I changed mingw.h to add a stub uname() because I don't think MinGW\n port has that function, but that's totally untested.\n\n Documentation/git-gc.txt | 6 +++++-\n builtin/gc.c | 35 +++++++++++++++++++++++++++++++++++\n compat/mingw.h | 6 ++++++\n git-compat-util.h | 1 +\n 4 files changed, 47 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/git-gc.txt b/Documentation/git-gc.txt\nindex 2402ed6..e158a3b 100644\n--- a/Documentation/git-gc.txt\n+++ b/Documentation/git-gc.txt\n@@ -9,7 +9,7 @@ git-gc - Cleanup unnecessary files and optimize the local repository\n SYNOPSIS\n --------\n [verse]\n-'git gc' [--aggressive] [--auto] [--quiet] [--prune= | --no-prune]\n+'git gc' [--aggressive] [--auto] [--quiet] [--prune= | --no-prune] [--force]\n \n DESCRIPTION\n -----------\n@@ -72,6 +72,10 @@ automatic consolidation of packs.\n --quiet::\n \tSuppress all progress reports.\n \n+--force::\n+\tForce `git gc` to run even if there may be another `git gc`\n+\tinstance running on this repository.\n+\n Configuration\n -------------\n \ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex 6be6c8d..8a7f24a 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -172,6 +172,10 @@ int cmd_gc(int argc, const char **argv, const char *prefix)\n \tint aggressive = 0;\n \tint auto_gc = 0;\n \tint quiet = 0;\n+\tint force = 0;\n+\tFILE *fp;\n+\tstruct utsname utsname;\n+\tstruct stat st;\n \n \tstruct option builtin_gc_options[] = {\n \t\tOPT__QUIET(&quiet, N_(\"suppress progress reporting\")),\n@@ -180,6 +184,7 @@ int cmd_gc(int argc, const char **argv, const char *prefix)\n \t\t\tPARSE_OPT_OPTARG, NULL, (intptr_t)prune_expire },\n \t\tOPT_BOOLEAN(0, \"aggressive\", &aggressive, N_(\"be more thorough (increased runtime)\")),\n \t\tOPT_BOOLEAN(0, \"auto\", &auto_gc, N_(\"enable auto-gc mode\")),\n+\t\tOPT_BOOL(0, \"force\", &force, N_(\"force running gc even if there may be another gc running\")),\n \t\tOPT_END()\n \t};\n \n@@ -225,6 +230,34 @@ int cmd_gc(int argc, const char **argv, const char *prefix)\n \t} else\n \t\tadd_repack_all_option();\n \n+\tif (uname(&utsname))\n+\t\tstrcpy(utsname.nodename, \"unknown\");\n+\n+\tif (!force &&\n+\t (fp = fopen(git_path(\"gc-%s.pid\", utsname.nodename), \"r\")) != NULL &&\n+\t !fstat(fileno(fp), &st) &&\n+\t /*\n+\t * 12 hour limit is very generous as gc should never take\n+\t * that long. On the other hand we don't really need a\n+\t * strict limit here, running gc --auto one day late is\n+\t * not a big problem. --force can be used in manual gc\n+\t * after the user verifies that no gc is running.\n+\t */\n+\t time(NULL) - st.st_mtime <= 12 * 3600) {\n+\t\tuintmax_t pid;\n+\t\tif (fscanf(fp, \"%\"PRIuMAX, &pid) == 1 && !kill(pid, 0)) {\n+\t\t\tif (auto_gc)\n+\t\t\t\treturn 0; /* be quiet on --auto */\n+\t\t\tdie(_(\"gc is already running\"));\n+\t\t}\n+\t\tfclose(fp);\n+\t}\n+\n+\tif ((fp = fopen(git_path(\"gc-%s.pid\", utsname.nodename), \"w\")) != NULL) {\n+\t\tfprintf(fp, \"%\"PRIuMAX\"\\n\", (uintmax_t) getpid());\n+\t\tfclose(fp);\n+\t}\n+\n \tif (pack_refs && run_command_v_opt(pack_refs_cmd.argv, RUN_GIT_CMD))\n \t\treturn error(FAILED_RUN, pack_refs_cmd.argv[0]);\n \n@@ -249,5 +282,7 @@ 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+\tunlink(git_path(\"gc-%s.pid\", utsname.nodename));\n+\n \treturn 0;\n }\ndiff --git a/compat/mingw.h b/compat/mingw.h\nindex bd0a88b..2c25d2d 100644\n--- a/compat/mingw.h\n+++ b/compat/mingw.h\n@@ -68,6 +68,10 @@ struct itimerval {\n };\n #define ITIMER_REAL 0\n \n+struct utsname {\n+\tchar nodename[128];\n+};\n+\n /*\n * sanitize preprocessor namespace polluted by Windows headers defining\n * macros which collide with git local versions\n@@ -86,6 +90,8 @@ static inline int fchmod(int fildes, mode_t mode)\n { errno = ENOSYS; return -1; }\n static inline pid_t fork(void)\n { errno = ENOSYS; return -1; }\n+static inline int uname(struct utsname *buf)\n+{ errno = ENOSYS; return -1; }\n static inline unsigned int alarm(unsigned int seconds)\n { return 0; }\n static inline int fsync(int fd)\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex 115cb1d..c6a53cb 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -139,6 +139,7 @@\n #include \n #include \n #include \n+#include \n #include \n #ifndef NO_SYS_SELECT_H\n #include \n-- \n1.8.2.83.gc99314b\n"},{"id":"224497","messageId":"CALkWK0k1vf_WE=pV5XTMAM6Ax6rL3sXu4qx_eyYwvWGsZjXgmA@mail.gmail.com","threadId":"34597","inReplyTo":"1375510890-4728-1-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH v2] gc: reject if another gc is running, unless --force is given","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2013-08-03T07:52:25Z","receivedAt":"2013-08-03T07:52:25Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Nguyễn Thái Ngọc Duy wrote:\n> This may happen when `git gc --auto` is run automatically, then the\n> user, to avoid wait time, switches to a new terminal, keeps working\n> and `git gc --auto` is started again because the first gc instance has\n> not clean up the repository.\n>\n> This patch tries to avoid multiple gc running, especially in --auto\n> mode. In the worst case, gc may be delayed 12 hours if a daemon reuses\n> the pid stored in gc-%s.pid.\n\nDefinitely looks like a good solution. Thanks for this.\n\nI'm currently on vacation, so can't apply and test: sorry.\n\n> + if (!force &&\n> + (fp = fopen(git_path(\"gc-%s.pid\", utsname.nodename), \"r\")) != NULL &&\n> + !fstat(fileno(fp), &st) &&\n\nIt's open for a very short period of time, so lockfile (which we'd\nnormally use) would probably be an overkill.\n\n> + time(NULL) - st.st_mtime <= 12 * 3600) {\n\nQuick question: is this kind of file-lifetime used anywhere else in git.git?\n\n> + if (auto_gc)\n> + return 0; /* be quiet on --auto */\n> + die(_(\"gc is already running\"));\n\nNice.\n"},{"id":"224498","messageId":"51FCD20E.8070406@kdbg.org","threadId":"34597","inReplyTo":"1375510890-4728-1-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH v2] gc: reject if another gc is running, unless --force is given","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2013-08-03T09:49:02Z","receivedAt":"2013-08-03T09:49:02Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 03.08.2013 08:21, schrieb Nguyễn Thái Ngọc Duy:\n> I changed mingw.h to add a stub uname() because I don't think MinGW\n> port has that function, but that's totally untested.\n\nThanks, but we don't have kill(pid, 0), either :-(\n\n-- Hannes\n"},{"id":"224500","messageId":"CACsJy8Bu55BY0iUSGWS-WgZic6mkFA1oD1+UH15_PLf3+7XEvw@mail.gmail.com","threadId":"34597","inReplyTo":"CALkWK0k1vf_WE=pV5XTMAM6Ax6rL3sXu4qx_eyYwvWGsZjXgmA@mail.gmail.com","subject":"Re: [PATCH v2] gc: reject if another gc is running, unless --force is given","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2013-08-03T10:01:11Z","receivedAt":"2013-08-03T10:01:11Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sat, Aug 3, 2013 at 2:52 PM, Ramkumar Ramachandra wrote:\n>> + time(NULL) - st.st_mtime <= 12 * 3600) {\n>\n> Quick question: is this kind of file-lifetime used anywhere else in git.git?\n\nI don't think so.\n-- \nDuy\n"},{"id":"224499","messageId":"20130803100113.GA8239@lanh","threadId":"34597","inReplyTo":"51FCD20E.8070406@kdbg.org","subject":"Re: [PATCH v2] gc: reject if another gc is running, unless --force is given","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2013-08-03T10:01:13Z","receivedAt":"2013-08-03T10:01:13Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sat, Aug 03, 2013 at 11:49:02AM +0200, Johannes Sixt wrote:\n> Am 03.08.2013 08:21, schrieb Nguyễn Thái Ngọc Duy:\n> > I changed mingw.h to add a stub uname() because I don't think MinGW\n> > port has that function, but that's totally untested.\n> \n> Thanks, but we don't have kill(pid, 0), either :-(\n\nYeah, I should have checked. Will this work?\n\n-- 8< --\ndiff --git a/compat/mingw.c b/compat/mingw.c\nindex bb92c43..14d92df 100644\n--- a/compat/mingw.c\n+++ b/compat/mingw.c\n@@ -1086,6 +1086,12 @@ int mingw_kill(pid_t pid, int sig)\n \t\terrno = err_win_to_posix(GetLastError());\n \t\tCloseHandle(h);\n \t\treturn -1;\n+\t} else if (pid > 0 && sig == 0) {\n+\t\tHANDLE h = OpenProcess(PROCESS_TERMINATE, FALSE, pid);\n+\t\tif (h) {\n+\t\t\tCloseHandle(h);\n+\t\t\treturn 0;\n+\t\t}\n \t}\n \n \terrno = EINVAL;\n-- 8< --\n"},{"id":"224504","messageId":"51FCDE25.9080805@kdbg.org","threadId":"34597","inReplyTo":"20130803100113.GA8239@lanh","subject":"Re: [PATCH v2] gc: reject if another gc is running, unless --force is given","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2013-08-03T10:40:37Z","receivedAt":"2013-08-03T10:40:37Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 03.08.2013 12:01, schrieb Duy Nguyen:\n> On Sat, Aug 03, 2013 at 11:49:02AM +0200, Johannes Sixt wrote:\n>> Am 03.08.2013 08:21, schrieb Nguyễn Thái Ngọc Duy:\n>>> I changed mingw.h to add a stub uname() because I don't think MinGW\n>>> port has that function, but that's totally untested.\n>>\n>> Thanks, but we don't have kill(pid, 0), either :-(\n> \n> Yeah, I should have checked. Will this work?\n> \n> -- 8< --\n> diff --git a/compat/mingw.c b/compat/mingw.c\n> index bb92c43..14d92df 100644\n> --- a/compat/mingw.c\n> +++ b/compat/mingw.c\n> @@ -1086,6 +1086,12 @@ int mingw_kill(pid_t pid, int sig)\n> \t\terrno = err_win_to_posix(GetLastError());\n> \t\tCloseHandle(h);\n> \t\treturn -1;\n> +\t} else if (pid > 0 && sig == 0) {\n> +\t\tHANDLE h = OpenProcess(PROCESS_TERMINATE, FALSE, pid);\n> +\t\tif (h) {\n> +\t\t\tCloseHandle(h);\n> +\t\t\treturn 0;\n> +\t\t}\n> \t}\n> \n> \terrno = EINVAL;\n> -- 8< --\n> \n\nLooks reasonable. PROCESS_QUERY_INFORMATION instead of PROCESS_TERMINATE\nshould be sufficient, and errno = ESRCH; return -1; is missing.\n\n-- Hannes\n"},{"id":"224576","messageId":"1375712354-13171-1-git-send-email-pclouds@gmail.com","threadId":"34597","inReplyTo":"1375510890-4728-1-git-send-email-pclouds@gmail.com","subject":"[PATCH v3] gc: reject if another gc is running, unless --force is given","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2013-08-05T14:19:14Z","receivedAt":"2013-08-05T14:19:14Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"This may happen when `git gc --auto` is run automatically, then the\nuser, to avoid wait time, switches to a new terminal, keeps working\nand `git gc --auto` is started again because the first gc instance has\nnot clean up the repository.\n\nThis patch tries to avoid multiple gc running, especially in --auto\nmode. In the worst case, gc may be delayed 12 hours if a daemon reuses\nthe pid stored in gc-%s.pid.\n\nuname() and kill(..,0) are added to MinGW compatibility layer so that\nit'll work on Windows. Actually uname() is stub so it won't prevent\nmultiple gc running on a shared repo. But that's for Windows\ncontributors to step in.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n - new code is taken out in a separate function for clarity\n - file locking\n - implementing kill(pid, 0) on windows (but not tested)\n\n Documentation/git-gc.txt | 6 ++++-\n builtin/gc.c | 62 ++++++++++++++++++++++++++++++++++++++++++++++++\n compat/mingw.c | 6 +++++\n compat/mingw.h | 6 +++++\n git-compat-util.h | 1 +\n 5 files changed, 80 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/git-gc.txt b/Documentation/git-gc.txt\nindex 2402ed6..e158a3b 100644\n--- a/Documentation/git-gc.txt\n+++ b/Documentation/git-gc.txt\n@@ -9,7 +9,7 @@ git-gc - Cleanup unnecessary files and optimize the local repository\n SYNOPSIS\n --------\n [verse]\n-'git gc' [--aggressive] [--auto] [--quiet] [--prune= | --no-prune]\n+'git gc' [--aggressive] [--auto] [--quiet] [--prune= | --no-prune] [--force]\n \n DESCRIPTION\n -----------\n@@ -72,6 +72,10 @@ automatic consolidation of packs.\n --quiet::\n \tSuppress all progress reports.\n \n+--force::\n+\tForce `git gc` to run even if there may be another `git gc`\n+\tinstance running on this repository.\n+\n Configuration\n -------------\n \ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex 6be6c8d..1f33908 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -167,11 +167,66 @@ static int need_to_gc(void)\n \treturn 1;\n }\n \n+static int gc_running(int force)\n+{\n+\tstatic struct lock_file lock;\n+\tstruct utsname utsname;\n+\tstruct stat st;\n+\tuintmax_t pid;\n+\tFILE *fp;\n+\tint fd, should_exit;\n+\n+\tif (uname(&utsname))\n+\t\tstrcpy(utsname.nodename, \"unknown\");\n+\n+\tfd = hold_lock_file_for_update(&lock,\n+\t\t\tgit_path(\"gc-%s.pid\", utsname.nodename), 0);\n+\tif (!force) {\n+\t\tif (fd < 0)\n+\t\t\treturn 1;\n+\n+\t\tfp = fopen(git_path(\"gc-%s.pid\", utsname.nodename), \"r\");\n+\t\tshould_exit =\n+\t\t\tfp != NULL &&\n+\t\t\t!fstat(fileno(fp), &st) &&\n+\t\t\t/*\n+\t\t\t * 12 hour limit is very generous as gc should\n+\t\t\t * never take that long. On the other hand we\n+\t\t\t * don't really need a strict limit here,\n+\t\t\t * running gc --auto one day late is not a big\n+\t\t\t * problem. --force can be used in manual gc\n+\t\t\t * after the user verifies that no gc is\n+\t\t\t * running.\n+\t\t\t */\n+\t\t\ttime(NULL) - st.st_mtime <= 12 * 3600 &&\n+\t\t\tfscanf(fp, \"%\"PRIuMAX, &pid) == 1 &&\n+\t\t\t!kill(pid, 0);\n+\t\tif (fp != NULL)\n+\t\t\tfclose(fp);\n+\t\tif (should_exit) {\n+\t\t\tif (fd >= 0)\n+\t\t\t\trollback_lock_file(&lock);\n+\t\t\treturn 1;\n+\t\t}\n+\t}\n+\n+\tif (fd >= 0) {\n+\t\tstruct strbuf sb = STRBUF_INIT;\n+\t\tstrbuf_addf(&sb, \"%\"PRIuMAX\"\\n\", (uintmax_t) getpid());\n+\t\twrite_in_full(fd, sb.buf, sb.len);\n+\t\tstrbuf_release(&sb);\n+\t\tcommit_lock_file(&lock);\n+\t}\n+\n+\treturn 0;\n+}\n+\n int cmd_gc(int argc, const char **argv, const char *prefix)\n {\n \tint aggressive = 0;\n \tint auto_gc = 0;\n \tint quiet = 0;\n+\tint force = 0;\n \n \tstruct option builtin_gc_options[] = {\n \t\tOPT__QUIET(&quiet, N_(\"suppress progress reporting\")),\n@@ -180,6 +235,7 @@ int cmd_gc(int argc, const char **argv, const char *prefix)\n \t\t\tPARSE_OPT_OPTARG, NULL, (intptr_t)prune_expire },\n \t\tOPT_BOOLEAN(0, \"aggressive\", &aggressive, N_(\"be more thorough (increased runtime)\")),\n \t\tOPT_BOOLEAN(0, \"auto\", &auto_gc, N_(\"enable auto-gc mode\")),\n+\t\tOPT_BOOL(0, \"force\", &force, N_(\"force running gc even if there may be another gc running\")),\n \t\tOPT_END()\n \t};\n \n@@ -225,6 +281,12 @@ int cmd_gc(int argc, const char **argv, const char *prefix)\n \t} else\n \t\tadd_repack_all_option();\n \n+\tif (gc_running(force)) {\n+\t\tif (auto_gc)\n+\t\t\treturn 0; /* be quiet on --auto */\n+\t\tdie(_(\"gc is already running (use --force if not)\"));\n+\t}\n+\n \tif (pack_refs && run_command_v_opt(pack_refs_cmd.argv, RUN_GIT_CMD))\n \t\treturn error(FAILED_RUN, pack_refs_cmd.argv[0]);\n \ndiff --git a/compat/mingw.c b/compat/mingw.c\nindex bb92c43..22ee9ef 100644\n--- a/compat/mingw.c\n+++ b/compat/mingw.c\n@@ -1086,6 +1086,12 @@ int mingw_kill(pid_t pid, int sig)\n \t\terrno = err_win_to_posix(GetLastError());\n \t\tCloseHandle(h);\n \t\treturn -1;\n+\t} else if (pid > 0 && sig == 0) {\n+\t\tHANDLE h = OpenProcess(PROCESS_QUERY_INFORMATION, FALSE, pid);\n+\t\tif (h) {\n+\t\t\tCloseHandle(h);\n+\t\t\treturn 0;\n+\t\t}\n \t}\n \n \terrno = EINVAL;\ndiff --git a/compat/mingw.h b/compat/mingw.h\nindex bd0a88b..2c25d2d 100644\n--- a/compat/mingw.h\n+++ b/compat/mingw.h\n@@ -68,6 +68,10 @@ struct itimerval {\n };\n #define ITIMER_REAL 0\n \n+struct utsname {\n+\tchar nodename[128];\n+};\n+\n /*\n * sanitize preprocessor namespace polluted by Windows headers defining\n * macros which collide with git local versions\n@@ -86,6 +90,8 @@ static inline int fchmod(int fildes, mode_t mode)\n { errno = ENOSYS; return -1; }\n static inline pid_t fork(void)\n { errno = ENOSYS; return -1; }\n+static inline int uname(struct utsname *buf)\n+{ errno = ENOSYS; return -1; }\n static inline unsigned int alarm(unsigned int seconds)\n { return 0; }\n static inline int fsync(int fd)\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex 115cb1d..c6a53cb 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -139,6 +139,7 @@\n #include \n #include \n #include \n+#include \n #include \n #ifndef NO_SYS_SELECT_H\n #include \n-- \n1.8.2.83.gc99314b\n"},{"id":"224612","messageId":"7vsiyoj932.fsf@alter.siamese.dyndns.org","threadId":"34597","inReplyTo":"1375712354-13171-1-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH v3] gc: reject if another gc is running, unless --force is given","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-08-05T18:17:37Z","receivedAt":"2013-08-05T18:17:37Z","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> diff --git a/builtin/gc.c b/builtin/gc.c\n> index 6be6c8d..1f33908 100644\n> --- a/builtin/gc.c\n> +++ b/builtin/gc.c\n> @@ -167,11 +167,66 @@ static int need_to_gc(void)\n> \treturn 1;\n> }\n> \n> +static int gc_running(int force)\n\nSounds like a bool asking \"Is a GC running? Yes, or no?\". Since\nthere is no room for \"force\" to enter in order to answer that\nquestion, I have to guess that this function is somewhat misnamed.\n\n> +{\n> +\tstatic struct lock_file lock;\n> +\tstruct utsname utsname;\n> +\tstruct stat st;\n> +\tuintmax_t pid;\n> +\tFILE *fp;\n> +\tint fd, should_exit;\n> +\n> +\tif (uname(&utsname))\n> +\t\tstrcpy(utsname.nodename, \"unknown\");\n> +\n> +\tfd = hold_lock_file_for_update(&lock,\n> +\t\t\tgit_path(\"gc-%s.pid\", utsname.nodename), 0);\n> +\tif (!force) {\n> +\t\tif (fd < 0)\n> +\t\t\treturn 1;\n> +\n> +\t\tfp = fopen(git_path(\"gc-%s.pid\", utsname.nodename), \"r\");\n\nI would have imagined that you would use a lockfile gc.pid and write\nnodename and pid to it (and if nodename matches, you know pid may\nhave a chance to actually match another instance of \"gc\", while\nthere will not way it matches if nodename is different, and do\nsomething intelligent about it). By letting GC that is running on\nanother node to be completely unnoticed, this change is closing the\ndoor to \"do something intelligent about it\", like giving it the same\n12 hour limit.\n\n> +\t\tshould_exit =\n> +\t\t\tfp != NULL &&\n> +\t\t\t!fstat(fileno(fp), &st) &&\n> +\t\t\t/*\n> +\t\t\t * 12 hour limit is very generous as gc should\n> +\t\t\t * never take that long. On the other hand we\n> +\t\t\t * don't really need a strict limit here,\n> +\t\t\t * running gc --auto one day late is not a big\n> +\t\t\t * problem. --force can be used in manual gc\n> +\t\t\t * after the user verifies that no gc is\n> +\t\t\t * running.\n> +\t\t\t */\n> +\t\t\ttime(NULL) - st.st_mtime <= 12 * 3600 &&\n> +\t\t\tfscanf(fp, \"%\"PRIuMAX, &pid) == 1 &&\n> +\t\t\t!kill(pid, 0);\n> +\t\tif (fp != NULL)\n> +\t\t\tfclose(fp);\n> +\t\tif (should_exit) {\n> +\t\t\tif (fd >= 0)\n> +\t\t\t\trollback_lock_file(&lock);\n> +\t\t\treturn 1;\n> +\t\t}\n> +\t}\n> +\n> +\tif (fd >= 0) {\n> +\t\tstruct strbuf sb = STRBUF_INIT;\n> +\t\tstrbuf_addf(&sb, \"%\"PRIuMAX\"\\n\", (uintmax_t) getpid());\n> +\t\twrite_in_full(fd, sb.buf, sb.len);\n> +\t\tstrbuf_release(&sb);\n> +\t\tcommit_lock_file(&lock);\n> +\t}\n> +\n> +\treturn 0;\n> +}\n\nAfter reading what the whole function does, I think the purpose of\nthis function is to take gc-lock (with optionally force). Perhaps a\nname along the lines of \"lock_gc\", \"gc_lock\", \"lock_repo_for_gc\",\nwould be more appropriate.\n"},{"id":"224613","messageId":"CALkWK0kZ=5TguAh9krAzFNuF0_sTRxcQKuZMnuQG7FQU0dJe=g@mail.gmail.com","threadId":"34597","inReplyTo":"7vsiyoj932.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v3] gc: reject if another gc is running, unless --force is given","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2013-08-05T18:37:43Z","receivedAt":"2013-08-05T18:37:43Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Junio C Hamano wrote:\n> [...]\n\nThe other comments mostly make sense.\n\n> After reading what the whole function does, I think the purpose of\n> this function is to take gc-lock (with optionally force). Perhaps a\n> name along the lines of \"lock_gc\", \"gc_lock\", \"lock_repo_for_gc\",\n> would be more appropriate.\n\nThe whole point of this exercise is to _not_ lock up the repo during\ngc, so I can do minimal commit/ worktree/ ref update operations when\nit's running. I can't expect the reflog to work, so complex\nhistory-rewriting operations should be avoided; that's about it, I think.\n"},{"id":"224651","messageId":"7vwqnzgvzi.fsf@alter.siamese.dyndns.org","threadId":"34597","inReplyTo":"CALkWK0kZ=5TguAh9krAzFNuF0_sTRxcQKuZMnuQG7FQU0dJe=g@mail.gmail.com","subject":"Re: [PATCH v3] gc: reject if another gc is running, unless --force is given","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-08-06T06:43:29Z","receivedAt":"2013-08-06T06:43:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ramkumar Ramachandra writes:\n\n> Junio C Hamano wrote:\n>> [...]\n>\n> The other comments mostly make sense.\n>\n>> After reading what the whole function does, I think the purpose of\n>> this function is to take gc-lock (with optionally force). Perhaps a\n>> name along the lines of \"lock_gc\", \"gc_lock\", \"lock_repo_for_gc\",\n>> would be more appropriate.\n>\n> The whole point of this exercise is to _not_ lock up the repo during\n> gc,...\n\nI do not think it is a misnomer to call the entity that locks other\ninstances of gc's \"a lock on the repository for gc\". Nothing in\nDuy's code suggests any other commands paying attention to this\nmechanism and stalling, and I think my comments were clear enough\nthat I was not suggesting such a change.\n\nSo I am not sure what you are complaining.\n"},{"id":"224653","messageId":"CALkWK0m=QDq2mbW9L98qOpAx67heY-Z7VW_=DT83Z1o7+Lartw@mail.gmail.com","threadId":"34597","inReplyTo":"7vwqnzgvzi.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v3] gc: reject if another gc is running, unless --force is given","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2013-08-06T06:45:49Z","receivedAt":"2013-08-06T06:45:49Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Junio C Hamano wrote:\n>>> After reading what the whole function does, I think the purpose of\n>>> this function is to take gc-lock (with optionally force). Perhaps a\n>>> name along the lines of \"lock_gc\", \"gc_lock\", \"lock_repo_for_gc\",\n>>> would be more appropriate.\n>>\n>> The whole point of this exercise is to _not_ lock up the repo during\n>> gc,...\n>\n> I do not think it is a misnomer to call the entity that locks other\n> instances of gc's \"a lock on the repository for gc\". Nothing in\n> Duy's code suggests any other commands paying attention to this\n> mechanism and stalling, and I think my comments were clear enough\n> that I was not suggesting such a change.\n>\n> So I am not sure what you are complaining.\n\nNot complaining; I wrote it down because of your \"lock_repo_for_gc\" suggestion.\n"},{"id":"224796","messageId":"1375959938-6395-1-git-send-email-pclouds@gmail.com","threadId":"34597","inReplyTo":"1375712354-13171-1-git-send-email-pclouds@gmail.com","subject":"[PATCH v4] gc: reject if another gc is running, unless --force is given","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2013-08-08T11:05:38Z","receivedAt":"2013-08-08T11:05:38Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"This may happen when `git gc --auto` is run automatically, then the\nuser, to avoid wait time, switches to a new terminal, keeps working\nand `git gc --auto` is started again because the first gc instance has\nnot clean up the repository.\n\nThis patch tries to avoid multiple gc running, especially in --auto\nmode. In the worst case, gc may be delayed 12 hours if a daemon reuses\nthe pid stored in gc.pid.\n\nkill(pid, 0) support is added to MinGW port so it should work on\nWindows too.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n this patch is getting cooler:\n\n - uname() is dropped in favor of gethostbyname(), which is supported\n by MinGW port.\n - host name is stored in gc.pid as junio suggested so...\n - now we can say \"gc is already running on _this host_ with _this pid_...\"\n\n Documentation/git-gc.txt | 6 ++++-\n builtin/gc.c | 67 ++++++++++++++++++++++++++++++++++++++++++++++++\n compat/mingw.c | 6 +++++\n 3 files changed, 78 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/git-gc.txt b/Documentation/git-gc.txt\nindex 2402ed6..e158a3b 100644\n--- a/Documentation/git-gc.txt\n+++ b/Documentation/git-gc.txt\n@@ -9,7 +9,7 @@ git-gc - Cleanup unnecessary files and optimize the local repository\n SYNOPSIS\n --------\n [verse]\n-'git gc' [--aggressive] [--auto] [--quiet] [--prune= | --no-prune]\n+'git gc' [--aggressive] [--auto] [--quiet] [--prune= | --no-prune] [--force]\n \n DESCRIPTION\n -----------\n@@ -72,6 +72,10 @@ automatic consolidation of packs.\n --quiet::\n \tSuppress all progress reports.\n \n+--force::\n+\tForce `git gc` to run even if there may be another `git gc`\n+\tinstance running on this repository.\n+\n Configuration\n -------------\n \ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex 6be6c8d..99682f0 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -167,11 +167,69 @@ static int need_to_gc(void)\n \treturn 1;\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+\tstatic struct lock_file lock;\n+\tstatic char locking_host[128];\n+\tchar my_host[128];\n+\tstruct strbuf sb = STRBUF_INIT;\n+\tstruct stat st;\n+\tuintmax_t pid;\n+\tFILE *fp;\n+\tint fd, should_exit;\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+\t\t\t\t LOCK_DIE_ON_ERROR);\n+\tif (!force) {\n+\t\tfp = fopen(git_path(\"gc.pid\"), \"r\");\n+\t\tmemset(locking_host, 0, sizeof(locking_host));\n+\t\tshould_exit =\n+\t\t\tfp != NULL &&\n+\t\t\t!fstat(fileno(fp), &st) &&\n+\t\t\t/*\n+\t\t\t * 12 hour limit is very generous as gc should\n+\t\t\t * never take that long. On the other hand we\n+\t\t\t * don't really need a strict limit here,\n+\t\t\t * running gc --auto one day late is not a big\n+\t\t\t * problem. --force can be used in manual gc\n+\t\t\t * after the user verifies that no gc is\n+\t\t\t * running.\n+\t\t\t */\n+\t\t\ttime(NULL) - st.st_mtime <= 12 * 3600 &&\n+\t\t\tfscanf(fp, \"%\"PRIuMAX\" %127c\", &pid, locking_host) == 2 &&\n+\t\t\t!strcmp(locking_host, my_host) &&\n+\t\t\t!kill(pid, 0);\n+\t\tif (fp != NULL)\n+\t\t\tfclose(fp);\n+\t\tif (should_exit) {\n+\t\t\tif (fd >= 0)\n+\t\t\t\trollback_lock_file(&lock);\n+\t\t\t*ret_pid = pid;\n+\t\t\treturn locking_host;\n+\t\t}\n+\t}\n+\n+\tstrbuf_addf(&sb, \"%\"PRIuMAX\" %s\",\n+\t\t (uintmax_t) getpid(), my_host);\n+\twrite_in_full(fd, sb.buf, sb.len);\n+\tstrbuf_release(&sb);\n+\tcommit_lock_file(&lock);\n+\n+\treturn NULL;\n+}\n+\n int cmd_gc(int argc, const char **argv, const char *prefix)\n {\n \tint aggressive = 0;\n \tint auto_gc = 0;\n \tint quiet = 0;\n+\tint force = 0;\n+\tconst char *name;\n+\tpid_t pid;\n \n \tstruct option builtin_gc_options[] = {\n \t\tOPT__QUIET(&quiet, N_(\"suppress progress reporting\")),\n@@ -180,6 +238,7 @@ int cmd_gc(int argc, const char **argv, const char *prefix)\n \t\t\tPARSE_OPT_OPTARG, NULL, (intptr_t)prune_expire },\n \t\tOPT_BOOLEAN(0, \"aggressive\", &aggressive, N_(\"be more thorough (increased runtime)\")),\n \t\tOPT_BOOLEAN(0, \"auto\", &auto_gc, N_(\"enable auto-gc mode\")),\n+\t\tOPT_BOOL(0, \"force\", &force, N_(\"force running gc even if there may be another gc running\")),\n \t\tOPT_END()\n \t};\n \n@@ -225,6 +284,14 @@ int cmd_gc(int argc, const char **argv, const char *prefix)\n \t} else\n \t\tadd_repack_all_option();\n \n+\tname = lock_repo_for_gc(force, &pid);\n+\tif (name) {\n+\t\tif (auto_gc)\n+\t\t\treturn 0; /* be quiet on --auto */\n+\t\tdie(_(\"gc is already running on machine '%s' pid %\"PRIuMAX\" (use --force if not)\"),\n+\t\t name, (uintmax_t)pid);\n+\t}\n+\n \tif (pack_refs && run_command_v_opt(pack_refs_cmd.argv, RUN_GIT_CMD))\n \t\treturn error(FAILED_RUN, pack_refs_cmd.argv[0]);\n \ndiff --git a/compat/mingw.c b/compat/mingw.c\nindex bb92c43..22ee9ef 100644\n--- a/compat/mingw.c\n+++ b/compat/mingw.c\n@@ -1086,6 +1086,12 @@ int mingw_kill(pid_t pid, int sig)\n \t\terrno = err_win_to_posix(GetLastError());\n \t\tCloseHandle(h);\n \t\treturn -1;\n+\t} else if (pid > 0 && sig == 0) {\n+\t\tHANDLE h = OpenProcess(PROCESS_QUERY_INFORMATION, FALSE, pid);\n+\t\tif (h) {\n+\t\t\tCloseHandle(h);\n+\t\t\treturn 0;\n+\t\t}\n \t}\n \n \terrno = EINVAL;\n-- \n1.8.2.83.gc99314b\n"},{"id":"224839","messageId":"7vk3jw9hlm.fsf@alter.siamese.dyndns.org","threadId":"34597","inReplyTo":"1375959938-6395-1-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH v4] gc: reject if another gc is running, unless --force is given","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-08-08T18:12:53Z","receivedAt":"2013-08-08T18:12:53Z","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> diff --git a/builtin/gc.c b/builtin/gc.c\n> index 6be6c8d..99682f0 100644\n> --- a/builtin/gc.c\n> +++ b/builtin/gc.c\n> @@ -167,11 +167,69 @@ static int need_to_gc(void)\n> + ...\n> +\tfd = hold_lock_file_for_update(&lock, git_path(\"gc.pid\"),\n> +\t\t\t\t LOCK_DIE_ON_ERROR);\n> +\tif (!force) {\n> +\t\tfp = fopen(git_path(\"gc.pid\"), \"r\");\n> +\t\tmemset(locking_host, 0, sizeof(locking_host));\n> +\t\tshould_exit =\n> +\t\t\tfp != NULL &&\n> +\t\t\t!fstat(fileno(fp), &st) &&\n> +\t\t\t/*\n> +\t\t\t * 12 hour limit is very generous as gc should\n> +\t\t\t * never take that long. On the other hand we\n> +\t\t\t * don't really need a strict limit here,\n> +\t\t\t * running gc --auto one day late is not a big\n> +\t\t\t * problem. --force can be used in manual gc\n> +\t\t\t * after the user verifies that no gc is\n> +\t\t\t * running.\n> +\t\t\t */\n> +\t\t\ttime(NULL) - st.st_mtime <= 12 * 3600 &&\n> +\t\t\tfscanf(fp, \"%\"PRIuMAX\" %127c\", &pid, locking_host) == 2 &&\n> +\t\t\t!strcmp(locking_host, my_host) &&\n> +\t\t\t!kill(pid, 0);\n\nIf there is a lockfile we can read, and if we can positively\ndetermine that the process named in the lockfile is still running,\nthen we definitely do not want to do another \"gc\". That part is\ngood.\n\nIf the lock is very stale, 12-hour test will kick in, and we do not\nread who locked it, nor check if the locker is still alive. By\ndoing so, we avoid misidentifying a new process that is unrelated to\nthe locker who died and left the lockfile behind, which is a good\nthing. The logic to ignore such lockfile as \"unknown\" applies\nequally to a remote locker on a lockfile that is old. So the logic\nfor an old lockfile is good, too.\n\nWhen we see a recent lockfile created by a \"gc\" running elsewhere,\nwe do not set \"should_exit\". Is that a good thing? I am wondering\nif the last two lines should be:\n\n-\t!strcmp(locking_host, my_host) &&\n-\t!kill(pid, 0);\n+\t(strcmp(locking_host, my_host) || !kill(pid, 0));\n\ninstead.\n\nThanks, looking good.\n"},{"id":"224898","messageId":"CACsJy8DRRLkyZid_OPSvRkvKfnd62TnLBnaueim9GrXUikPGuw@mail.gmail.com","threadId":"34597","inReplyTo":"7vk3jw9hlm.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v4] gc: reject if another gc is running, unless --force is given","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2013-08-09T12:52:49Z","receivedAt":"2013-08-09T12:52:49Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Fri, Aug 9, 2013 at 1:12 AM, Junio C Hamano wrote:\n> When we see a recent lockfile created by a \"gc\" running elsewhere,\n> we do not set \"should_exit\". Is that a good thing? I am wondering\n> if the last two lines should be:\n>\n> - !strcmp(locking_host, my_host) &&\n> - !kill(pid, 0);\n> + (strcmp(locking_host, my_host) || !kill(pid, 0));\n>\n> instead.\n\nYes I think it should (we still have the 12-hour check to override\nstale locks anyway). Should I send another patch or you do it yourself\n(seeing that you have this chunk pasted here, you might have it saved\nsomewhere already)\n-- \nDuy\n"},{"id":"224913","messageId":"7vhaey7t2f.fsf@alter.siamese.dyndns.org","threadId":"34597","inReplyTo":"CACsJy8DRRLkyZid_OPSvRkvKfnd62TnLBnaueim9GrXUikPGuw@mail.gmail.com","subject":"Re: [PATCH v4] gc: reject if another gc is running, unless --force is given","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-08-09T16:00:24Z","receivedAt":"2013-08-09T16:00:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen writes:\n\n> On Fri, Aug 9, 2013 at 1:12 AM, Junio C Hamano wrote:\n>> When we see a recent lockfile created by a \"gc\" running elsewhere,\n>> we do not set \"should_exit\". Is that a good thing? I am wondering\n>> if the last two lines should be:\n>>\n>> - !strcmp(locking_host, my_host) &&\n>> - !kill(pid, 0);\n>> + (strcmp(locking_host, my_host) || !kill(pid, 0));\n>>\n>> instead.\n>\n> Yes I think it should (we still have the 12-hour check to override\n> stale locks anyway). Should I send another patch or you do it yourself\n> (seeing that you have this chunk pasted here, you might have it saved\n> somewhere already)\n\nThe above was typed in my MUA ;-), but it is an easy update I can do\nso will do so anyway.\n\nThanks for double-checking.\n"},{"id":"224914","messageId":"CAPrKj1bO1jBsv73beA6LoeN09S-jWq8FYOP+WQ-AFwb1dn4Wsw@mail.gmail.com","threadId":"34597","inReplyTo":"1375959938-6395-1-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH v4] gc: reject if another gc is running, unless --force is given","fromName":"Andres Perera","fromEmail":"andres.p@zoho.com","sentAt":"2013-08-09T16:29:39Z","receivedAt":"2013-08-09T16:29:39Z","isPatch":true,"sender":{"key":"andres.p@zoho.com","avatar":null},"body":"On Thu, Aug 8, 2013 at 6:35 AM, Nguyễn Thái Ngọc Duy wrote:\n> This may happen when `git gc --auto` is run automatically, then the\n> user, to avoid wait time, switches to a new terminal, keeps working\n> and `git gc --auto` is started again because the first gc instance has\n> not clean up the repository.\n>\n> This patch tries to avoid multiple gc running, especially in --auto\n> mode. In the worst case, gc may be delayed 12 hours if a daemon reuses\n> the pid stored in gc.pid.\n>\n> kill(pid, 0) support is added to MinGW port so it should work on\n> Windows too.\n>\n> Signed-off-by: Nguyễn Thái Ngọc Duy \n> ---\n> this patch is getting cooler:\n>\n> - uname() is dropped in favor of gethostbyname(), which is supported\n> by MinGW port.\n> - host name is stored in gc.pid as junio suggested so...\n> - now we can say \"gc is already running on _this host_ with _this pid_...\"\n>\n> Documentation/git-gc.txt | 6 ++++-\n> builtin/gc.c | 67 ++++++++++++++++++++++++++++++++++++++++++++++++\n> compat/mingw.c | 6 +++++\n> 3 files changed, 78 insertions(+), 1 deletion(-)\n>\n> diff --git a/Documentation/git-gc.txt b/Documentation/git-gc.txt\n> index 2402ed6..e158a3b 100644\n> --- a/Documentation/git-gc.txt\n> +++ b/Documentation/git-gc.txt\n> @@ -9,7 +9,7 @@ git-gc - Cleanup unnecessary files and optimize the local repository\n> SYNOPSIS\n> --------\n> [verse]\n> -'git gc' [--aggressive] [--auto] [--quiet] [--prune= | --no-prune]\n> +'git gc' [--aggressive] [--auto] [--quiet] [--prune= | --no-prune] [--force]\n>\n> DESCRIPTION\n> -----------\n> @@ -72,6 +72,10 @@ automatic consolidation of packs.\n> --quiet::\n> Suppress all progress reports.\n>\n> +--force::\n> + Force `git gc` to run even if there may be another `git gc`\n> + instance running on this repository.\n> +\n> Configuration\n> -------------\n>\n> diff --git a/builtin/gc.c b/builtin/gc.c\n> index 6be6c8d..99682f0 100644\n> --- a/builtin/gc.c\n> +++ b/builtin/gc.c\n> @@ -167,11 +167,69 @@ static int need_to_gc(void)\n> return 1;\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> + static struct lock_file lock;\n> + static char locking_host[128];\n> + char my_host[128];\n> + struct strbuf sb = STRBUF_INIT;\n> + struct stat st;\n> + uintmax_t pid;\n\npid_t is always an signed type, therefore unintmax_t does not make\nsense as a catch all value\n\nfork() returns -1 on failure, and its return type is pid_t. i don't\nknow what fantasy unix system has an unsigned pid_t\n\n> + FILE *fp;\n> + int fd, should_exit;\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> + LOCK_DIE_ON_ERROR);\n> + if (!force) {\n> + fp = fopen(git_path(\"gc.pid\"), \"r\");\n> + memset(locking_host, 0, sizeof(locking_host));\n> + should_exit =\n> + fp != NULL &&\n> + !fstat(fileno(fp), &st) &&\n> + /*\n> + * 12 hour limit is very generous as gc should\n> + * never take that long. On the other hand we\n> + * don't really need a strict limit here,\n> + * running gc --auto one day late is not a big\n> + * problem. --force can be used in manual gc\n> + * after the user verifies that no gc is\n> + * running.\n> + */\n> + time(NULL) - st.st_mtime <= 12 * 3600 &&\n> + fscanf(fp, \"%\"PRIuMAX\" %127c\", &pid, locking_host) == 2 &&\n\nsimilar comment wrt PRIuMAX\n\n> + !strcmp(locking_host, my_host) &&\n> + !kill(pid, 0);\n> + if (fp != NULL)\n> + fclose(fp);\n> + if (should_exit) {\n> + if (fd >= 0)\n> + rollback_lock_file(&lock);\n> + *ret_pid = pid;\n> + return locking_host;\n\nwhy not exponential backoff?\n\n> + }\n> + }\n> +\n> + strbuf_addf(&sb, \"%\"PRIuMAX\" %s\",\n> + (uintmax_t) getpid(), my_host);\n> + write_in_full(fd, sb.buf, sb.len);\n> + strbuf_release(&sb);\n> + commit_lock_file(&lock);\n> +\n> + return NULL;\n> +}\n> +\n> int cmd_gc(int argc, const char **argv, const char *prefix)\n> {\n> int aggressive = 0;\n> int auto_gc = 0;\n> int quiet = 0;\n> + int force = 0;\n> + const char *name;\n> + pid_t pid;\n>\n> struct option builtin_gc_options[] = {\n> OPT__QUIET(&quiet, N_(\"suppress progress reporting\")),\n> @@ -180,6 +238,7 @@ int cmd_gc(int argc, const char **argv, const char *prefix)\n> PARSE_OPT_OPTARG, NULL, (intptr_t)prune_expire },\n> OPT_BOOLEAN(0, \"aggressive\", &aggressive, N_(\"be more thorough (increased runtime)\")),\n> OPT_BOOLEAN(0, \"auto\", &auto_gc, N_(\"enable auto-gc mode\")),\n> + OPT_BOOL(0, \"force\", &force, N_(\"force running gc even if there may be another gc running\")),\n> OPT_END()\n> };\n>\n> @@ -225,6 +284,14 @@ int cmd_gc(int argc, const char **argv, const char *prefix)\n> } else\n> add_repack_all_option();\n>\n> + name = lock_repo_for_gc(force, &pid);\n> + if (name) {\n> + if (auto_gc)\n> + return 0; /* be quiet on --auto */\n> + die(_(\"gc is already running on machine '%s' pid %\"PRIuMAX\" (use --force if not)\"),\n> + name, (uintmax_t)pid);\n> + }\n> +\n> if (pack_refs && run_command_v_opt(pack_refs_cmd.argv, RUN_GIT_CMD))\n> return error(FAILED_RUN, pack_refs_cmd.argv[0]);\n>\n> diff --git a/compat/mingw.c b/compat/mingw.c\n> index bb92c43..22ee9ef 100644\n> --- a/compat/mingw.c\n> +++ b/compat/mingw.c\n> @@ -1086,6 +1086,12 @@ int mingw_kill(pid_t pid, int sig)\n> errno = err_win_to_posix(GetLastError());\n> CloseHandle(h);\n> return -1;\n> + } else if (pid > 0 && sig == 0) {\n> + HANDLE h = OpenProcess(PROCESS_QUERY_INFORMATION, FALSE, pid);\n> + if (h) {\n> + CloseHandle(h);\n> + return 0;\n> + }\n> }\n>\n> errno = EINVAL;\n> --\n> 1.8.2.83.gc99314b\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>\n"},{"id":"224924","messageId":"7vmwoq69td.fsf@alter.siamese.dyndns.org","threadId":"34597","inReplyTo":"CAPrKj1bO1jBsv73beA6LoeN09S-jWq8FYOP+WQ-AFwb1dn4Wsw@mail.gmail.com","subject":"Re: [PATCH v4] gc: reject if another gc is running, unless --force is given","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-08-09T17:41:34Z","receivedAt":"2013-08-09T17:41:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Andres Perera writes:\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>> + static struct lock_file lock;\n>> + static char locking_host[128];\n>> + char my_host[128];\n>> + struct strbuf sb = STRBUF_INIT;\n>> + struct stat st;\n>> + uintmax_t pid;\n>\n> pid_t is always an signed type, therefore unintmax_t does not make\n> sense as a catch all value\n\nGood eyes.\n\n>> + !strcmp(locking_host, my_host) &&\n>> + !kill(pid, 0);\n>> + if (fp != NULL)\n>> + fclose(fp);\n>> + if (should_exit) {\n>> + if (fd >= 0)\n>> + rollback_lock_file(&lock);\n>> + *ret_pid = pid;\n>> + return locking_host;\n>\n> why not exponential backoff?\n\n\nIf the other guy is doing a GC, and we decide that we should exit,\nit is *not* because we want to wait until the other guy is done. It\nis because we know we do not have to do the work --- the other guy\nis doing what we were about to do, and it will do it for us anyway.\n\nSo I do not think it makes any sense to do exponential backoff if\n\"gc --auto\" is asking \"should we exit\" to this logic.\n\nAn explicit \"gc\", on the other hand, may benefit from backoff, but\nthen the user can choose to do so himself, and more importantly, the\nuser can see \"ah, another one is running so enough cruft will be\ncleaned up anyway\" and choose not to run it.\n"},{"id":"224979","messageId":"CACsJy8D8EHpPGrc8MZnpvmh1j1LDudoZ0OO-zyfuDmhwLJqNsA@mail.gmail.com","threadId":"34597","inReplyTo":"CAPrKj1bO1jBsv73beA6LoeN09S-jWq8FYOP+WQ-AFwb1dn4Wsw@mail.gmail.com","subject":"Re: [PATCH v4] gc: reject if another gc is running, unless --force is given","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2013-08-10T00:45:38Z","receivedAt":"2013-08-10T00:45:38Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Fri, Aug 9, 2013 at 11:29 PM, Andres Perera wrote:\n> On Thu, Aug 8, 2013 at 6:35 AM, Nguyễn Thái Ngọc Duy wrote:\n>> + uintmax_t pid;\n>\n> pid_t is always an signed type, therefore unintmax_t does not make\n> sense as a catch all value\n\nI only catch real process id. In practice we don't have processes with\nnegative pid_t, do we? I can't find any document about this, but at\nleast waitpid seems to treat negative pid (except -1) just as an\nindicator while the true pid is the positive counterpart.\n\n> fork() returns -1 on failure, and its return type is pid_t. i don't\n> know what fantasy unix system has an unsigned pid_t\n-- \nDuy\n"},{"id":"224996","messageId":"m238qiyq8q.fsf@linux-m68k.org","threadId":"34597","inReplyTo":"CACsJy8D8EHpPGrc8MZnpvmh1j1LDudoZ0OO-zyfuDmhwLJqNsA@mail.gmail.com","subject":"Re: [PATCH v4] gc: reject if another gc is running, unless --force is given","fromName":"Andreas Schwab","fromEmail":"schwab@linux-m68k.org","sentAt":"2013-08-10T07:11:33Z","receivedAt":"2013-08-10T07:11:33Z","isPatch":true,"sender":{"key":"schwab@linux-m68k.org","avatar":"https://avatars.githubusercontent.com/u/2175493?v=4"},"body":"Duy Nguyen writes:\n\n> On Fri, Aug 9, 2013 at 11:29 PM, Andres Perera wrote:\n>> On Thu, Aug 8, 2013 at 6:35 AM, Nguyễn Thái Ngọc Duy wrote:\n>>> + uintmax_t pid;\n>>\n>> pid_t is always an signed type, therefore unintmax_t does not make\n>> sense as a catch all value\n>\n> I only catch real process id. In practice we don't have processes with\n> negative pid_t, do we? I can't find any document about this, but at\n> least waitpid seems to treat negative pid (except -1) just as an\n> indicator while the true pid is the positive counterpart.\n\nNegative pids are used for denoting process groups in various\ninterfaces. No process can have a negative pid.\n\nAndreas.\n\n-- \nAndreas Schwab, schwab@linux-m68k.org\nGPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5\n\"And now for something completely different.\"\n"}]}