Re: [PATCH] gc: remove gc.pid file at end of execution
Jonathan Nieder writes: > Matthieu Moy wrote: > >> This file isn't really harmful, but isn't useful either, and can create >> minor annoyance for the user: > > Would something like the following make sense, to ensure the gc.pid file is > always removed on normal exit? Has anything further happened to this discussion? -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] gc: remove gc.pid file at end of execution
On Sat, Sep 28, 2013 at 7:33 AM, Jonathan Nieder wrote: > Matthieu Moy wrote: > >> This file isn't really harmful, but isn't useful either, and can create >> minor annoyance for the user: > > Would something like the following make sense, to ensure the gc.pid file is > always removed on normal exit? > > Signed-off-by: Jonathan Nieder > > diff --git c/builtin/gc.c i/builtin/gc.c > index 6e0d81a..fbbc144 100644 > --- c/builtin/gc.c > +++ i/builtin/gc.c > @@ -14,6 +14,7 @@ > #include "cache.h" > #include "parse-options.h" > #include "run-command.h" > +#include "sigchain.h" > #include "argv-array.h" > > #define FAILED_RUN "failed to run %s" > @@ -167,6 +168,21 @@ static int need_to_gc(void) > return 1; > } > > +static char *pidfile; > + > +static void remove_pidfile(void) > +{ > + if (pidfile) > + unlink(pidfile); > +} > + > +static void remove_pidfile_on_signal(int signo) > +{ > + remove_pidfile(); > + sigchain_pop(signo); > + raise(signo); > +} > + > /* return NULL on success, else hostname running the gc */ > static const char *lock_repo_for_gc(int force, pid_t* ret_pid) > { > @@ -179,13 +195,19 @@ static const char *lock_repo_for_gc(int force, pid_t* > ret_pid) > FILE *fp; > int fd, should_exit; > > + if (pidfile) > + /* already locked */ > + return NULL; > + > if (gethostname(my_host, sizeof(my_host))) > strcpy(my_host, "unknown"); > > - fd = hold_lock_file_for_update(&lock, git_path("gc.pid"), > + pidfile = git_pathdup("gc.pid"); > + > + fd = hold_lock_file_for_update(&lock, pidfile, >LOCK_DIE_ON_ERROR); > if (!force) { > - fp = fopen(git_path("gc.pid"), "r"); > + fp = fopen(pidfile, "r"); > memset(locking_host, 0, sizeof(locking_host)); > should_exit = > fp != NULL && > @@ -208,6 +230,7 @@ static const char *lock_repo_for_gc(int force, pid_t* > ret_pid) > if (should_exit) { > if (fd >= 0) > rollback_lock_file(&lock); > + pidfile = NULL; > *ret_pid = pid; > return locking_host; > } > @@ -219,6 +242,9 @@ static const char *lock_repo_for_gc(int force, pid_t* > ret_pid) > strbuf_release(&sb); It may be a bit simpler to delay setting pidfile until we get here. lock.filename still contains gc.pid until commit_lock_file is called. > commit_lock_file(&lock); > > + sigchain_push_common(remove_pidfile_on_signal); > + atexit(remove_pidfile); > + > return NULL; > } -- Duy -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] gc: remove gc.pid file at end of execution
Matthieu Moy wrote: > This file isn't really harmful, but isn't useful either, and can create > minor annoyance for the user: Would something like the following make sense, to ensure the gc.pid file is always removed on normal exit? Signed-off-by: Jonathan Nieder diff --git c/builtin/gc.c i/builtin/gc.c index 6e0d81a..fbbc144 100644 --- c/builtin/gc.c +++ i/builtin/gc.c @@ -14,6 +14,7 @@ #include "cache.h" #include "parse-options.h" #include "run-command.h" +#include "sigchain.h" #include "argv-array.h" #define FAILED_RUN "failed to run %s" @@ -167,6 +168,21 @@ static int need_to_gc(void) return 1; } +static char *pidfile; + +static void remove_pidfile(void) +{ + if (pidfile) + unlink(pidfile); +} + +static void remove_pidfile_on_signal(int signo) +{ + remove_pidfile(); + sigchain_pop(signo); + raise(signo); +} + /* return NULL on success, else hostname running the gc */ static const char *lock_repo_for_gc(int force, pid_t* ret_pid) { @@ -179,13 +195,19 @@ static const char *lock_repo_for_gc(int force, pid_t* ret_pid) FILE *fp; int fd, should_exit; + if (pidfile) + /* already locked */ + return NULL; + if (gethostname(my_host, sizeof(my_host))) strcpy(my_host, "unknown"); - fd = hold_lock_file_for_update(&lock, git_path("gc.pid"), + pidfile = git_pathdup("gc.pid"); + + fd = hold_lock_file_for_update(&lock, pidfile, LOCK_DIE_ON_ERROR); if (!force) { - fp = fopen(git_path("gc.pid"), "r"); + fp = fopen(pidfile, "r"); memset(locking_host, 0, sizeof(locking_host)); should_exit = fp != NULL && @@ -208,6 +230,7 @@ static const char *lock_repo_for_gc(int force, pid_t* ret_pid) if (should_exit) { if (fd >= 0) rollback_lock_file(&lock); + pidfile = NULL; *ret_pid = pid; return locking_host; } @@ -219,6 +242,9 @@ static const char *lock_repo_for_gc(int force, pid_t* ret_pid) strbuf_release(&sb); commit_lock_file(&lock); + sigchain_push_common(remove_pidfile_on_signal); + atexit(remove_pidfile); + return NULL; } -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html