[PATCH] gc: reject if another gc is running, unless --force is given

[PATCH] gc: reject if another gc is running, unless --force is given

[Nguyễn Thái Ngọc Duy]

This may happen when `git gc --auto` is run automatically, then the
user, to avoid wait time, switches to a new terminal, keeps working
and `git gc --auto` is started again because the first gc instance has
not clean up the repository.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 I don't know if the kill(pid, 0) trick works on Windows..

 Documentation/git-gc.txt |  6 +++++-
 builtin/gc.c             | 18 ++++++++++++++++++
 2 files changed, 23 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-gc.txt b/Documentation/git-gc.txt
index 2402ed6..e158a3b 100644
--- a/Documentation/git-gc.txt
+++ b/Documentation/git-gc.txt
@@ -9,7 +9,7 @@ git-gc - Cleanup unnecessary files and optimize the local repository
 SYNOPSIS
 --------
 [verse]
-'git gc' [--aggressive] [--auto] [--quiet] [--prune=<date> | --no-prune]
+'git gc' [--aggressive] [--auto] [--quiet] [--prune=<date> | --no-prune] [--force]
 
 DESCRIPTION
 -----------
@@ -72,6 +72,10 @@ automatic consolidation of packs.
 --quiet::
 	Suppress all progress reports.
 
+--force::
+	Force `git gc` to run even if there may be another `git gc`
+	instance running on this repository.
+
 Configuration
 -------------
 
diff --git a/builtin/gc.c b/builtin/gc.c
index 6be6c8d..07c566c 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -169,9 +169,11 @@ static int need_to_gc(void)
 
 int cmd_gc(int argc, const char **argv, const char *prefix)
 {
+	FILE *fp;
 	int aggressive = 0;
 	int auto_gc = 0;
 	int quiet = 0;
+	int force = 0;
 
 	struct option builtin_gc_options[] = {
 		OPT__QUIET(&quiet, N_("suppress progress reporting")),
@@ -180,6 +182,7 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
 			PARSE_OPT_OPTARG, NULL, (intptr_t)prune_expire },
 		OPT_BOOLEAN(0, "aggressive", &aggressive, N_("be more thorough (increased runtime)")),
 		OPT_BOOLEAN(0, "auto", &auto_gc, N_("enable auto-gc mode")),
+		OPT_BOOL(0, "force", &force, N_("force running gc even if there may be another gc running")),
 		OPT_END()
 	};
 
@@ -225,6 +228,21 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
 	} else
 		add_repack_all_option();
 
+	if (!force && (fp = fopen(git_path("gc.pid"), "r")) != NULL) {
+		uintmax_t pid;
+		if (fscanf(fp, "%"PRIuMAX, &pid) == 1 && !kill(pid, 0)) {
+			if (auto_gc)
+				return 0; /* be quiet on --auto */
+			die(_("gc is already running"));
+		}
+		fclose(fp);
+	}
+
+	if ((fp = fopen(git_path("gc.pid"), "w")) != NULL) {
+		fprintf(fp, "%"PRIuMAX"\n", (uintmax_t) getpid());
+		fclose(fp);
+	}
+
 	if (pack_refs && run_command_v_opt(pack_refs_cmd.argv, RUN_GIT_CMD))
 		return error(FAILED_RUN, pack_refs_cmd.argv[0]);
 
-- 
1.8.2.83.gc99314b

[PATCH v2] gc: reject if another gc is running, unless --force is given

[Nguyễn Thái Ngọc Duy]

This may happen when `git gc --auto` is run automatically, then the
user, to avoid wait time, switches to a new terminal, keeps working
and `git gc --auto` is started again because the first gc instance has
not clean up the repository.

This patch tries to avoid multiple gc running, especially in --auto
mode. In the worst case, gc may be delayed 12 hours if a daemon reuses
the pid stored in gc-%s.pid.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 v2 deals with the stored pid taken by another process and takes
 shared repository into account (sort of, it only makes sure gc is not
 run multiple times on the same machine, not only one gc instance across
 multiple machines)

 I changed mingw.h to add a stub uname() because I don't think MinGW
 port has that function, but that's totally untested.

 Documentation/git-gc.txt |  6 +++++-
 builtin/gc.c             | 35 +++++++++++++++++++++++++++++++++++
 compat/mingw.h           |  6 ++++++
 git-compat-util.h        |  1 +
 4 files changed, 47 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-gc.txt b/Documentation/git-gc.txt
index 2402ed6..e158a3b 100644
--- a/Documentation/git-gc.txt
+++ b/Documentation/git-gc.txt
@@ -9,7 +9,7 @@ git-gc - Cleanup unnecessary files and optimize the local repository
 SYNOPSIS
 --------
 [verse]
-'git gc' [--aggressive] [--auto] [--quiet] [--prune=<date> | --no-prune]
+'git gc' [--aggressive] [--auto] [--quiet] [--prune=<date> | --no-prune] [--force]
 
 DESCRIPTION
 -----------
@@ -72,6 +72,10 @@ automatic consolidation of packs.
 --quiet::
 	Suppress all progress reports.
 
+--force::
+	Force `git gc` to run even if there may be another `git gc`
+	instance running on this repository.
+
 Configuration
 -------------
 
diff --git a/builtin/gc.c b/builtin/gc.c
index 6be6c8d..8a7f24a 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -172,6 +172,10 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
 	int aggressive = 0;
 	int auto_gc = 0;
 	int quiet = 0;
+	int force = 0;
+	FILE *fp;
+	struct utsname utsname;
+	struct stat st;
 
 	struct option builtin_gc_options[] = {
 		OPT__QUIET(&quiet, N_("suppress progress reporting")),
@@ -180,6 +184,7 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
 			PARSE_OPT_OPTARG, NULL, (intptr_t)prune_expire },
 		OPT_BOOLEAN(0, "aggressive", &aggressive, N_("be more thorough (increased runtime)")),
 		OPT_BOOLEAN(0, "auto", &auto_gc, N_("enable auto-gc mode")),
+		OPT_BOOL(0, "force", &force, N_("force running gc even if there may be another gc running")),
 		OPT_END()
 	};
 
@@ -225,6 +230,34 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
 	} else
 		add_repack_all_option();
 
+	if (uname(&utsname))
+		strcpy(utsname.nodename, "unknown");
+
+	if (!force &&
+	    (fp = fopen(git_path("gc-%s.pid", utsname.nodename), "r")) != NULL &&
+	    !fstat(fileno(fp), &st) &&
+	    /*
+	     * 12 hour limit is very generous as gc should never take
+	     * that long. On the other hand we don't really need a
+	     * strict limit here, running gc --auto one day late is
+	     * not a big problem. --force can be used in manual gc
+	     * after the user verifies that no gc is running.
+	     */
+	    time(NULL) - st.st_mtime <= 12 * 3600) {
+		uintmax_t pid;
+		if (fscanf(fp, "%"PRIuMAX, &pid) == 1 && !kill(pid, 0)) {
+			if (auto_gc)
+				return 0; /* be quiet on --auto */
+			die(_("gc is already running"));
+		}
+		fclose(fp);
+	}
+
+	if ((fp = fopen(git_path("gc-%s.pid", utsname.nodename), "w")) != NULL) {
+		fprintf(fp, "%"PRIuMAX"\n", (uintmax_t) getpid());
+		fclose(fp);
+	}
+
 	if (pack_refs && run_command_v_opt(pack_refs_cmd.argv, RUN_GIT_CMD))
 		return error(FAILED_RUN, pack_refs_cmd.argv[0]);
 
@@ -249,5 +282,7 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
 		warning(_("There are too many unreachable loose objects; "
 			"run 'git prune' to remove them."));
 
+	unlink(git_path("gc-%s.pid", utsname.nodename));
+
 	return 0;
 }
diff --git a/compat/mingw.h b/compat/mingw.h
index bd0a88b..2c25d2d 100644
--- a/compat/mingw.h
+++ b/compat/mingw.h
@@ -68,6 +68,10 @@ struct itimerval {
 };
 #define ITIMER_REAL 0
 
+struct utsname {
+	char nodename[128];
+};
+
 /*
  * sanitize preprocessor namespace polluted by Windows headers defining
  * macros which collide with git local versions
@@ -86,6 +90,8 @@ static inline int fchmod(int fildes, mode_t mode)
 { errno = ENOSYS; return -1; }
 static inline pid_t fork(void)
 { errno = ENOSYS; return -1; }
+static inline int uname(struct utsname *buf)
+{ errno = ENOSYS; return -1; }
 static inline unsigned int alarm(unsigned int seconds)
 { return 0; }
 static inline int fsync(int fd)
diff --git a/git-compat-util.h b/git-compat-util.h
index 115cb1d..c6a53cb 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -139,6 +139,7 @@
 #include <sys/resource.h>
 #include <sys/socket.h>
 #include <sys/ioctl.h>
+#include <sys/utsname.h>
 #include <termios.h>
 #ifndef NO_SYS_SELECT_H
 #include <sys/select.h>
-- 
1.8.2.83.gc99314b

Re: [PATCH v2] gc: reject if another gc is running, unless --force is given

[Ramkumar Ramachandra]

Nguyễn Thái Ngọc Duy wrote:
> This may happen when `git gc --auto` is run automatically, then the
> user, to avoid wait time, switches to a new terminal, keeps working
> and `git gc --auto` is started again because the first gc instance has
> not clean up the repository.
>
> This patch tries to avoid multiple gc running, especially in --auto
> mode. In the worst case, gc may be delayed 12 hours if a daemon reuses
> the pid stored in gc-%s.pid.

Definitely looks like a good solution. Thanks for this.

I'm currently on vacation, so can't apply and test: sorry.

> +       if (!force &&
> +           (fp = fopen(git_path("gc-%s.pid", utsname.nodename), "r")) != NULL &&
> +           !fstat(fileno(fp), &st) &&

It's open for a very short period of time, so lockfile (which we'd
normally use) would probably be an overkill.

> +           time(NULL) - st.st_mtime <= 12 * 3600) {

Quick question: is this kind of file-lifetime used anywhere else in git.git?

> +                       if (auto_gc)
> +                               return 0; /* be quiet on --auto */
> +                       die(_("gc is already running"));

Nice.

Re: [PATCH v2] gc: reject if another gc is running, unless --force is given

[Johannes Sixt]

Am 03.08.2013 08:21, schrieb Nguyễn Thái Ngọc Duy:
>  I changed mingw.h to add a stub uname() because I don't think MinGW
>  port has that function, but that's totally untested.

Thanks, but we don't have kill(pid, 0), either :-(

-- Hannes

Re: [PATCH v2] gc: reject if another gc is running, unless --force is given

[Duy Nguyen]

On Sat, Aug 3, 2013 at 2:52 PM, Ramkumar Ramachandra <artagnon@gmail.com> wrote:
>> +           time(NULL) - st.st_mtime <= 12 * 3600) {
>
> Quick question: is this kind of file-lifetime used anywhere else in git.git?

I don't think so.
-- 
Duy

Re: [PATCH v2] gc: reject if another gc is running, unless --force is given

[Duy Nguyen]

On Sat, Aug 03, 2013 at 11:49:02AM +0200, Johannes Sixt wrote:
> Am 03.08.2013 08:21, schrieb Nguyễn Thái Ngọc Duy:
> >  I changed mingw.h to add a stub uname() because I don't think MinGW
> >  port has that function, but that's totally untested.
> 
> Thanks, but we don't have kill(pid, 0), either :-(

Yeah, I should have checked. Will this work?

-- 8< --
diff --git a/compat/mingw.c b/compat/mingw.c
index bb92c43..14d92df 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -1086,6 +1086,12 @@ int mingw_kill(pid_t pid, int sig)
 		errno = err_win_to_posix(GetLastError());
 		CloseHandle(h);
 		return -1;
+	} else if (pid > 0 && sig == 0) {
+		HANDLE h = OpenProcess(PROCESS_TERMINATE, FALSE, pid);
+		if (h) {
+			CloseHandle(h);
+			return 0;
+		}
 	}
 
 	errno = EINVAL;
-- 8< --

Re: [PATCH v2] gc: reject if another gc is running, unless --force is given

[Johannes Sixt]

Am 03.08.2013 12:01, schrieb Duy Nguyen:
> On Sat, Aug 03, 2013 at 11:49:02AM +0200, Johannes Sixt wrote:
>> Am 03.08.2013 08:21, schrieb Nguyễn Thái Ngọc Duy:
>>>  I changed mingw.h to add a stub uname() because I don't think MinGW
>>>  port has that function, but that's totally untested.
>>
>> Thanks, but we don't have kill(pid, 0), either :-(
> 
> Yeah, I should have checked. Will this work?
> 
> -- 8< --
> diff --git a/compat/mingw.c b/compat/mingw.c
> index bb92c43..14d92df 100644
> --- a/compat/mingw.c
> +++ b/compat/mingw.c
> @@ -1086,6 +1086,12 @@ int mingw_kill(pid_t pid, int sig)
>  		errno = err_win_to_posix(GetLastError());
>  		CloseHandle(h);
>  		return -1;
> +	} else if (pid > 0 && sig == 0) {
> +		HANDLE h = OpenProcess(PROCESS_TERMINATE, FALSE, pid);
> +		if (h) {
> +			CloseHandle(h);
> +			return 0;
> +		}
>  	}
>  
>  	errno = EINVAL;
> -- 8< --
> 

Looks reasonable. PROCESS_QUERY_INFORMATION instead of PROCESS_TERMINATE
should be sufficient, and errno = ESRCH; return -1; is missing.

-- Hannes

[PATCH v3] gc: reject if another gc is running, unless --force is given

[Nguyễn Thái Ngọc Duy]

This may happen when `git gc --auto` is run automatically, then the
user, to avoid wait time, switches to a new terminal, keeps working
and `git gc --auto` is started again because the first gc instance has
not clean up the repository.

This patch tries to avoid multiple gc running, especially in --auto
mode. In the worst case, gc may be delayed 12 hours if a daemon reuses
the pid stored in gc-%s.pid.

uname() and kill(..,0) are added to MinGW compatibility layer so that
it'll work on Windows. Actually uname() is stub so it won't prevent
multiple gc running on a shared repo. But that's for Windows
contributors to step in.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 - new code is taken out in a separate function for clarity
 - file locking
 - implementing kill(pid, 0) on windows (but not tested)

 Documentation/git-gc.txt |  6 ++++-
 builtin/gc.c             | 62 ++++++++++++++++++++++++++++++++++++++++++++++++
 compat/mingw.c           |  6 +++++
 compat/mingw.h           |  6 +++++
 git-compat-util.h        |  1 +
 5 files changed, 80 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-gc.txt b/Documentation/git-gc.txt
index 2402ed6..e158a3b 100644
--- a/Documentation/git-gc.txt
+++ b/Documentation/git-gc.txt
@@ -9,7 +9,7 @@ git-gc - Cleanup unnecessary files and optimize the local repository
 SYNOPSIS
 --------
 [verse]
-'git gc' [--aggressive] [--auto] [--quiet] [--prune=<date> | --no-prune]
+'git gc' [--aggressive] [--auto] [--quiet] [--prune=<date> | --no-prune] [--force]
 
 DESCRIPTION
 -----------
@@ -72,6 +72,10 @@ automatic consolidation of packs.
 --quiet::
 	Suppress all progress reports.
 
+--force::
+	Force `git gc` to run even if there may be another `git gc`
+	instance running on this repository.
+
 Configuration
 -------------
 
diff --git a/builtin/gc.c b/builtin/gc.c
index 6be6c8d..1f33908 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -167,11 +167,66 @@ static int need_to_gc(void)
 	return 1;
 }
 
+static int gc_running(int force)
+{
+	static struct lock_file lock;
+	struct utsname utsname;
+	struct stat st;
+	uintmax_t pid;
+	FILE *fp;
+	int fd, should_exit;
+
+	if (uname(&utsname))
+		strcpy(utsname.nodename, "unknown");
+
+	fd = hold_lock_file_for_update(&lock,
+			git_path("gc-%s.pid", utsname.nodename), 0);
+	if (!force) {
+		if (fd < 0)
+			return 1;
+
+		fp = fopen(git_path("gc-%s.pid", utsname.nodename), "r");
+		should_exit =
+			fp != NULL &&
+			!fstat(fileno(fp), &st) &&
+			/*
+			 * 12 hour limit is very generous as gc should
+			 * never take that long. On the other hand we
+			 * don't really need a strict limit here,
+			 * running gc --auto one day late is not a big
+			 * problem. --force can be used in manual gc
+			 * after the user verifies that no gc is
+			 * running.
+			 */
+			time(NULL) - st.st_mtime <= 12 * 3600 &&
+			fscanf(fp, "%"PRIuMAX, &pid) == 1 &&
+			!kill(pid, 0);
+		if (fp != NULL)
+			fclose(fp);
+		if (should_exit) {
+			if (fd >= 0)
+				rollback_lock_file(&lock);
+			return 1;
+		}
+	}
+
+	if (fd >= 0) {
+		struct strbuf sb = STRBUF_INIT;
+		strbuf_addf(&sb, "%"PRIuMAX"\n", (uintmax_t) getpid());
+		write_in_full(fd, sb.buf, sb.len);
+		strbuf_release(&sb);
+		commit_lock_file(&lock);
+	}
+
+	return 0;
+}
+
 int cmd_gc(int argc, const char **argv, const char *prefix)
 {
 	int aggressive = 0;
 	int auto_gc = 0;
 	int quiet = 0;
+	int force = 0;
 
 	struct option builtin_gc_options[] = {
 		OPT__QUIET(&quiet, N_("suppress progress reporting")),
@@ -180,6 +235,7 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
 			PARSE_OPT_OPTARG, NULL, (intptr_t)prune_expire },
 		OPT_BOOLEAN(0, "aggressive", &aggressive, N_("be more thorough (increased runtime)")),
 		OPT_BOOLEAN(0, "auto", &auto_gc, N_("enable auto-gc mode")),
+		OPT_BOOL(0, "force", &force, N_("force running gc even if there may be another gc running")),
 		OPT_END()
 	};
 
@@ -225,6 +281,12 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
 	} else
 		add_repack_all_option();
 
+	if (gc_running(force)) {
+		if (auto_gc)
+			return 0; /* be quiet on --auto */
+		die(_("gc is already running (use --force if not)"));
+	}
+
 	if (pack_refs && run_command_v_opt(pack_refs_cmd.argv, RUN_GIT_CMD))
 		return error(FAILED_RUN, pack_refs_cmd.argv[0]);
 
diff --git a/compat/mingw.c b/compat/mingw.c
index bb92c43..22ee9ef 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -1086,6 +1086,12 @@ int mingw_kill(pid_t pid, int sig)
 		errno = err_win_to_posix(GetLastError());
 		CloseHandle(h);
 		return -1;
+	} else if (pid > 0 && sig == 0) {
+		HANDLE h = OpenProcess(PROCESS_QUERY_INFORMATION, FALSE, pid);
+		if (h) {
+			CloseHandle(h);
+			return 0;
+		}
 	}
 
 	errno = EINVAL;
diff --git a/compat/mingw.h b/compat/mingw.h
index bd0a88b..2c25d2d 100644
--- a/compat/mingw.h
+++ b/compat/mingw.h
@@ -68,6 +68,10 @@ struct itimerval {
 };
 #define ITIMER_REAL 0
 
+struct utsname {
+	char nodename[128];
+};
+
 /*
  * sanitize preprocessor namespace polluted by Windows headers defining
  * macros which collide with git local versions
@@ -86,6 +90,8 @@ static inline int fchmod(int fildes, mode_t mode)
 { errno = ENOSYS; return -1; }
 static inline pid_t fork(void)
 { errno = ENOSYS; return -1; }
+static inline int uname(struct utsname *buf)
+{ errno = ENOSYS; return -1; }
 static inline unsigned int alarm(unsigned int seconds)
 { return 0; }
 static inline int fsync(int fd)
diff --git a/git-compat-util.h b/git-compat-util.h
index 115cb1d..c6a53cb 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -139,6 +139,7 @@
 #include <sys/resource.h>
 #include <sys/socket.h>
 #include <sys/ioctl.h>
+#include <sys/utsname.h>
 #include <termios.h>
 #ifndef NO_SYS_SELECT_H
 #include <sys/select.h>
-- 
1.8.2.83.gc99314b

Re: [PATCH v3] gc: reject if another gc is running, unless --force is given

[Junio C Hamano]

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

> diff --git a/builtin/gc.c b/builtin/gc.c
> index 6be6c8d..1f33908 100644
> --- a/builtin/gc.c
> +++ b/builtin/gc.c
> @@ -167,11 +167,66 @@ static int need_to_gc(void)
>  	return 1;
>  }
>  
> +static int gc_running(int force)

Sounds like a bool asking "Is a GC running?  Yes, or no?".  Since
there is no room for "force" to enter in order to answer that
question, I have to guess that this function is somewhat misnamed.

> +{
> +	static struct lock_file lock;
> +	struct utsname utsname;
> +	struct stat st;
> +	uintmax_t pid;
> +	FILE *fp;
> +	int fd, should_exit;
> +
> +	if (uname(&utsname))
> +		strcpy(utsname.nodename, "unknown");
> +
> +	fd = hold_lock_file_for_update(&lock,
> +			git_path("gc-%s.pid", utsname.nodename), 0);
> +	if (!force) {
> +		if (fd < 0)
> +			return 1;
> +
> +		fp = fopen(git_path("gc-%s.pid", utsname.nodename), "r");

I would have imagined that you would use a lockfile gc.pid and write
nodename and pid to it (and if nodename matches, you know pid may
have a chance to actually match another instance of "gc", while
there will not way it matches if nodename is different, and do
something intelligent about it).  By letting GC that is running on
another node to be completely unnoticed, this change is closing the
door to "do something intelligent about it", like giving it the same
12 hour limit.

> +		should_exit =
> +			fp != NULL &&
> +			!fstat(fileno(fp), &st) &&
> +			/*
> +			 * 12 hour limit is very generous as gc should
> +			 * never take that long. On the other hand we
> +			 * don't really need a strict limit here,
> +			 * running gc --auto one day late is not a big
> +			 * problem. --force can be used in manual gc
> +			 * after the user verifies that no gc is
> +			 * running.
> +			 */
> +			time(NULL) - st.st_mtime <= 12 * 3600 &&
> +			fscanf(fp, "%"PRIuMAX, &pid) == 1 &&
> +			!kill(pid, 0);
> +		if (fp != NULL)
> +			fclose(fp);
> +		if (should_exit) {
> +			if (fd >= 0)
> +				rollback_lock_file(&lock);
> +			return 1;
> +		}
> +	}
> +
> +	if (fd >= 0) {
> +		struct strbuf sb = STRBUF_INIT;
> +		strbuf_addf(&sb, "%"PRIuMAX"\n", (uintmax_t) getpid());
> +		write_in_full(fd, sb.buf, sb.len);
> +		strbuf_release(&sb);
> +		commit_lock_file(&lock);
> +	}
> +
> +	return 0;
> +}

After reading what the whole function does, I think the purpose of
this function is to take gc-lock (with optionally force).  Perhaps a
name along the lines of "lock_gc", "gc_lock", "lock_repo_for_gc",
would be more appropriate.

Re: [PATCH v3] gc: reject if another gc is running, unless --force is given

[Ramkumar Ramachandra]

Junio C Hamano wrote:
> [...]

The other comments mostly make sense.

> After reading what the whole function does, I think the purpose of
> this function is to take gc-lock (with optionally force).  Perhaps a
> name along the lines of "lock_gc", "gc_lock", "lock_repo_for_gc",
> would be more appropriate.

The whole point of this exercise is to _not_ lock up the repo during
gc, so I can do minimal commit/ worktree/ ref update operations when
it's running.  I can't expect the reflog to work, so complex
history-rewriting operations should be avoided; that's about it, I think.

Re: [PATCH v3] gc: reject if another gc is running, unless --force is given

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> Junio C Hamano wrote:
>> [...]
>
> The other comments mostly make sense.
>
>> After reading what the whole function does, I think the purpose of
>> this function is to take gc-lock (with optionally force).  Perhaps a
>> name along the lines of "lock_gc", "gc_lock", "lock_repo_for_gc",
>> would be more appropriate.
>
> The whole point of this exercise is to _not_ lock up the repo during
> gc,...

I do not think it is a misnomer to call the entity that locks other
instances of gc's "a lock on the repository for gc".  Nothing in
Duy's code suggests any other commands paying attention to this
mechanism and stalling, and I think my comments were clear enough
that I was not suggesting such a change.

So I am not sure what you are complaining.

Re: [PATCH v3] gc: reject if another gc is running, unless --force is given

[Ramkumar Ramachandra]

Junio C Hamano wrote:
>>> After reading what the whole function does, I think the purpose of
>>> this function is to take gc-lock (with optionally force).  Perhaps a
>>> name along the lines of "lock_gc", "gc_lock", "lock_repo_for_gc",
>>> would be more appropriate.
>>
>> The whole point of this exercise is to _not_ lock up the repo during
>> gc,...
>
> I do not think it is a misnomer to call the entity that locks other
> instances of gc's "a lock on the repository for gc".  Nothing in
> Duy's code suggests any other commands paying attention to this
> mechanism and stalling, and I think my comments were clear enough
> that I was not suggesting such a change.
>
> So I am not sure what you are complaining.

Not complaining; I wrote it down because of your "lock_repo_for_gc" suggestion.

[PATCH v4] gc: reject if another gc is running, unless --force is given

[Nguyễn Thái Ngọc Duy]

This may happen when `git gc --auto` is run automatically, then the
user, to avoid wait time, switches to a new terminal, keeps working
and `git gc --auto` is started again because the first gc instance has
not clean up the repository.

This patch tries to avoid multiple gc running, especially in --auto
mode. In the worst case, gc may be delayed 12 hours if a daemon reuses
the pid stored in gc.pid.

kill(pid, 0) support is added to MinGW port so it should work on
Windows too.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 this patch is getting cooler:

 - uname() is dropped in favor of gethostbyname(), which is supported
   by MinGW port.
 - host name is stored in gc.pid as junio suggested so...
 - now we can say "gc is already running on _this host_ with _this pid_..."

 Documentation/git-gc.txt |  6 ++++-
 builtin/gc.c             | 67 ++++++++++++++++++++++++++++++++++++++++++++++++
 compat/mingw.c           |  6 +++++
 3 files changed, 78 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-gc.txt b/Documentation/git-gc.txt
index 2402ed6..e158a3b 100644
--- a/Documentation/git-gc.txt
+++ b/Documentation/git-gc.txt
@@ -9,7 +9,7 @@ git-gc - Cleanup unnecessary files and optimize the local repository
 SYNOPSIS
 --------
 [verse]
-'git gc' [--aggressive] [--auto] [--quiet] [--prune=<date> | --no-prune]
+'git gc' [--aggressive] [--auto] [--quiet] [--prune=<date> | --no-prune] [--force]
 
 DESCRIPTION
 -----------
@@ -72,6 +72,10 @@ automatic consolidation of packs.
 --quiet::
 	Suppress all progress reports.
 
+--force::
+	Force `git gc` to run even if there may be another `git gc`
+	instance running on this repository.
+
 Configuration
 -------------
 
diff --git a/builtin/gc.c b/builtin/gc.c
index 6be6c8d..99682f0 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -167,11 +167,69 @@ static int need_to_gc(void)
 	return 1;
 }
 
+/* return NULL on success, else hostname running the gc */
+static const char *lock_repo_for_gc(int force, pid_t* ret_pid)
+{
+	static struct lock_file lock;
+	static char locking_host[128];
+	char my_host[128];
+	struct strbuf sb = STRBUF_INIT;
+	struct stat st;
+	uintmax_t pid;
+	FILE *fp;
+	int fd, should_exit;
+
+	if (gethostname(my_host, sizeof(my_host)))
+		strcpy(my_host, "unknown");
+
+	fd = hold_lock_file_for_update(&lock, git_path("gc.pid"),
+				       LOCK_DIE_ON_ERROR);
+	if (!force) {
+		fp = fopen(git_path("gc.pid"), "r");
+		memset(locking_host, 0, sizeof(locking_host));
+		should_exit =
+			fp != NULL &&
+			!fstat(fileno(fp), &st) &&
+			/*
+			 * 12 hour limit is very generous as gc should
+			 * never take that long. On the other hand we
+			 * don't really need a strict limit here,
+			 * running gc --auto one day late is not a big
+			 * problem. --force can be used in manual gc
+			 * after the user verifies that no gc is
+			 * running.
+			 */
+			time(NULL) - st.st_mtime <= 12 * 3600 &&
+			fscanf(fp, "%"PRIuMAX" %127c", &pid, locking_host) == 2 &&
+			!strcmp(locking_host, my_host) &&
+			!kill(pid, 0);
+		if (fp != NULL)
+			fclose(fp);
+		if (should_exit) {
+			if (fd >= 0)
+				rollback_lock_file(&lock);
+			*ret_pid = pid;
+			return locking_host;
+		}
+	}
+
+	strbuf_addf(&sb, "%"PRIuMAX" %s",
+		    (uintmax_t) getpid(), my_host);
+	write_in_full(fd, sb.buf, sb.len);
+	strbuf_release(&sb);
+	commit_lock_file(&lock);
+
+	return NULL;
+}
+
 int cmd_gc(int argc, const char **argv, const char *prefix)
 {
 	int aggressive = 0;
 	int auto_gc = 0;
 	int quiet = 0;
+	int force = 0;
+	const char *name;
+	pid_t pid;
 
 	struct option builtin_gc_options[] = {
 		OPT__QUIET(&quiet, N_("suppress progress reporting")),
@@ -180,6 +238,7 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
 			PARSE_OPT_OPTARG, NULL, (intptr_t)prune_expire },
 		OPT_BOOLEAN(0, "aggressive", &aggressive, N_("be more thorough (increased runtime)")),
 		OPT_BOOLEAN(0, "auto", &auto_gc, N_("enable auto-gc mode")),
+		OPT_BOOL(0, "force", &force, N_("force running gc even if there may be another gc running")),
 		OPT_END()
 	};
 
@@ -225,6 +284,14 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
 	} else
 		add_repack_all_option();
 
+	name = lock_repo_for_gc(force, &pid);
+	if (name) {
+		if (auto_gc)
+			return 0; /* be quiet on --auto */
+		die(_("gc is already running on machine '%s' pid %"PRIuMAX" (use --force if not)"),
+		    name, (uintmax_t)pid);
+	}
+
 	if (pack_refs && run_command_v_opt(pack_refs_cmd.argv, RUN_GIT_CMD))
 		return error(FAILED_RUN, pack_refs_cmd.argv[0]);
 
diff --git a/compat/mingw.c b/compat/mingw.c
index bb92c43..22ee9ef 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -1086,6 +1086,12 @@ int mingw_kill(pid_t pid, int sig)
 		errno = err_win_to_posix(GetLastError());
 		CloseHandle(h);
 		return -1;
+	} else if (pid > 0 && sig == 0) {
+		HANDLE h = OpenProcess(PROCESS_QUERY_INFORMATION, FALSE, pid);
+		if (h) {
+			CloseHandle(h);
+			return 0;
+		}
 	}
 
 	errno = EINVAL;
-- 
1.8.2.83.gc99314b

Re: [PATCH v4] gc: reject if another gc is running, unless --force is given

[Junio C Hamano]

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

> diff --git a/builtin/gc.c b/builtin/gc.c
> index 6be6c8d..99682f0 100644
> --- a/builtin/gc.c
> +++ b/builtin/gc.c
> @@ -167,11 +167,69 @@ static int need_to_gc(void)
> + ...
> +	fd = hold_lock_file_for_update(&lock, git_path("gc.pid"),
> +				       LOCK_DIE_ON_ERROR);
> +	if (!force) {
> +		fp = fopen(git_path("gc.pid"), "r");
> +		memset(locking_host, 0, sizeof(locking_host));
> +		should_exit =
> +			fp != NULL &&
> +			!fstat(fileno(fp), &st) &&
> +			/*
> +			 * 12 hour limit is very generous as gc should
> +			 * never take that long. On the other hand we
> +			 * don't really need a strict limit here,
> +			 * running gc --auto one day late is not a big
> +			 * problem. --force can be used in manual gc
> +			 * after the user verifies that no gc is
> +			 * running.
> +			 */
> +			time(NULL) - st.st_mtime <= 12 * 3600 &&
> +			fscanf(fp, "%"PRIuMAX" %127c", &pid, locking_host) == 2 &&
> +			!strcmp(locking_host, my_host) &&
> +			!kill(pid, 0);

If there is a lockfile we can read, and if we can positively
determine that the process named in the lockfile is still running,
then we definitely do not want to do another "gc".  That part is
good.

If the lock is very stale, 12-hour test will kick in, and we do not
read who locked it, nor check if the locker is still alive.  By
doing so, we avoid misidentifying a new process that is unrelated to
the locker who died and left the lockfile behind, which is a good
thing.  The logic to ignore such lockfile as "unknown" applies
equally to a remote locker on a lockfile that is old.  So the logic
for an old lockfile is good, too.

When we see a recent lockfile created by a "gc" running elsewhere,
we do not set "should_exit".  Is that a good thing?  I am wondering
if the last two lines should be:

-	!strcmp(locking_host, my_host) &&
-	!kill(pid, 0);
+	(strcmp(locking_host, my_host) || !kill(pid, 0));

instead.

Thanks, looking good.

Re: [PATCH v4] gc: reject if another gc is running, unless --force is given

[Duy Nguyen]

On Fri, Aug 9, 2013 at 1:12 AM, Junio C Hamano <gitster@pobox.com> wrote:
> When we see a recent lockfile created by a "gc" running elsewhere,
> we do not set "should_exit".  Is that a good thing?  I am wondering
> if the last two lines should be:
>
> -       !strcmp(locking_host, my_host) &&
> -       !kill(pid, 0);
> +       (strcmp(locking_host, my_host) || !kill(pid, 0));
>
> instead.

Yes I think it should (we still have the 12-hour check to override
stale locks anyway). Should I send another patch or you do it yourself
(seeing that you have this chunk pasted here, you might have it saved
somewhere already)
-- 
Duy

Re: [PATCH v4] gc: reject if another gc is running, unless --force is given

[Junio C Hamano]

Duy Nguyen <pclouds@gmail.com> writes:

> On Fri, Aug 9, 2013 at 1:12 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> When we see a recent lockfile created by a "gc" running elsewhere,
>> we do not set "should_exit".  Is that a good thing?  I am wondering
>> if the last two lines should be:
>>
>> -       !strcmp(locking_host, my_host) &&
>> -       !kill(pid, 0);
>> +       (strcmp(locking_host, my_host) || !kill(pid, 0));
>>
>> instead.
>
> Yes I think it should (we still have the 12-hour check to override
> stale locks anyway). Should I send another patch or you do it yourself
> (seeing that you have this chunk pasted here, you might have it saved
> somewhere already)

The above was typed in my MUA ;-), but it is an easy update I can do
so will do so anyway.

Thanks for double-checking.

Re: [PATCH v4] gc: reject if another gc is running, unless --force is given

[Andres Perera]

On Thu, Aug 8, 2013 at 6:35 AM, Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:
> This may happen when `git gc --auto` is run automatically, then the
> user, to avoid wait time, switches to a new terminal, keeps working
> and `git gc --auto` is started again because the first gc instance has
> not clean up the repository.
>
> This patch tries to avoid multiple gc running, especially in --auto
> mode. In the worst case, gc may be delayed 12 hours if a daemon reuses
> the pid stored in gc.pid.
>
> kill(pid, 0) support is added to MinGW port so it should work on
> Windows too.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  this patch is getting cooler:
>
>  - uname() is dropped in favor of gethostbyname(), which is supported
>    by MinGW port.
>  - host name is stored in gc.pid as junio suggested so...
>  - now we can say "gc is already running on _this host_ with _this pid_..."
>
>  Documentation/git-gc.txt |  6 ++++-
>  builtin/gc.c             | 67 ++++++++++++++++++++++++++++++++++++++++++++++++
>  compat/mingw.c           |  6 +++++
>  3 files changed, 78 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/git-gc.txt b/Documentation/git-gc.txt
> index 2402ed6..e158a3b 100644
> --- a/Documentation/git-gc.txt
> +++ b/Documentation/git-gc.txt
> @@ -9,7 +9,7 @@ git-gc - Cleanup unnecessary files and optimize the local repository
>  SYNOPSIS
>  --------
>  [verse]
> -'git gc' [--aggressive] [--auto] [--quiet] [--prune=<date> | --no-prune]
> +'git gc' [--aggressive] [--auto] [--quiet] [--prune=<date> | --no-prune] [--force]
>
>  DESCRIPTION
>  -----------
> @@ -72,6 +72,10 @@ automatic consolidation of packs.
>  --quiet::
>         Suppress all progress reports.
>
> +--force::
> +       Force `git gc` to run even if there may be another `git gc`
> +       instance running on this repository.
> +
>  Configuration
>  -------------
>
> diff --git a/builtin/gc.c b/builtin/gc.c
> index 6be6c8d..99682f0 100644
> --- a/builtin/gc.c
> +++ b/builtin/gc.c
> @@ -167,11 +167,69 @@ static int need_to_gc(void)
>         return 1;
>  }
>
> +/* return NULL on success, else hostname running the gc */
> +static const char *lock_repo_for_gc(int force, pid_t* ret_pid)
> +{
> +       static struct lock_file lock;
> +       static char locking_host[128];
> +       char my_host[128];
> +       struct strbuf sb = STRBUF_INIT;
> +       struct stat st;
> +       uintmax_t pid;

pid_t is always an signed type, therefore unintmax_t does not make
sense as a catch all value

fork() returns -1 on failure, and its return type is pid_t. i don't
know what fantasy unix system has an unsigned pid_t

> +       FILE *fp;
> +       int fd, should_exit;
> +
> +       if (gethostname(my_host, sizeof(my_host)))
> +               strcpy(my_host, "unknown");
> +
> +       fd = hold_lock_file_for_update(&lock, git_path("gc.pid"),
> +                                      LOCK_DIE_ON_ERROR);
> +       if (!force) {
> +               fp = fopen(git_path("gc.pid"), "r");
> +               memset(locking_host, 0, sizeof(locking_host));
> +               should_exit =
> +                       fp != NULL &&
> +                       !fstat(fileno(fp), &st) &&
> +                       /*
> +                        * 12 hour limit is very generous as gc should
> +                        * never take that long. On the other hand we
> +                        * don't really need a strict limit here,
> +                        * running gc --auto one day late is not a big
> +                        * problem. --force can be used in manual gc
> +                        * after the user verifies that no gc is
> +                        * running.
> +                        */
> +                       time(NULL) - st.st_mtime <= 12 * 3600 &&
> +                       fscanf(fp, "%"PRIuMAX" %127c", &pid, locking_host) == 2 &&

similar comment wrt PRIuMAX

> +                       !strcmp(locking_host, my_host) &&
> +                       !kill(pid, 0);
> +               if (fp != NULL)
> +                       fclose(fp);
> +               if (should_exit) {
> +                       if (fd >= 0)
> +                               rollback_lock_file(&lock);
> +                       *ret_pid = pid;
> +                       return locking_host;

why not exponential backoff?

> +               }
> +       }
> +
> +       strbuf_addf(&sb, "%"PRIuMAX" %s",
> +                   (uintmax_t) getpid(), my_host);
> +       write_in_full(fd, sb.buf, sb.len);
> +       strbuf_release(&sb);
> +       commit_lock_file(&lock);
> +
> +       return NULL;
> +}
> +
>  int cmd_gc(int argc, const char **argv, const char *prefix)
>  {
>         int aggressive = 0;
>         int auto_gc = 0;
>         int quiet = 0;
> +       int force = 0;
> +       const char *name;
> +       pid_t pid;
>
>         struct option builtin_gc_options[] = {
>                 OPT__QUIET(&quiet, N_("suppress progress reporting")),
> @@ -180,6 +238,7 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
>                         PARSE_OPT_OPTARG, NULL, (intptr_t)prune_expire },
>                 OPT_BOOLEAN(0, "aggressive", &aggressive, N_("be more thorough (increased runtime)")),
>                 OPT_BOOLEAN(0, "auto", &auto_gc, N_("enable auto-gc mode")),
> +               OPT_BOOL(0, "force", &force, N_("force running gc even if there may be another gc running")),
>                 OPT_END()
>         };
>
> @@ -225,6 +284,14 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
>         } else
>                 add_repack_all_option();
>
> +       name = lock_repo_for_gc(force, &pid);
> +       if (name) {
> +               if (auto_gc)
> +                       return 0; /* be quiet on --auto */
> +               die(_("gc is already running on machine '%s' pid %"PRIuMAX" (use --force if not)"),
> +                   name, (uintmax_t)pid);
> +       }
> +
>         if (pack_refs && run_command_v_opt(pack_refs_cmd.argv, RUN_GIT_CMD))
>                 return error(FAILED_RUN, pack_refs_cmd.argv[0]);
>
> diff --git a/compat/mingw.c b/compat/mingw.c
> index bb92c43..22ee9ef 100644
> --- a/compat/mingw.c
> +++ b/compat/mingw.c
> @@ -1086,6 +1086,12 @@ int mingw_kill(pid_t pid, int sig)
>                 errno = err_win_to_posix(GetLastError());
>                 CloseHandle(h);
>                 return -1;
> +       } else if (pid > 0 && sig == 0) {
> +               HANDLE h = OpenProcess(PROCESS_QUERY_INFORMATION, FALSE, pid);
> +               if (h) {
> +                       CloseHandle(h);
> +                       return 0;
> +               }
>         }
>
>         errno = EINVAL;
> --
> 1.8.2.83.gc99314b
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH v4] gc: reject if another gc is running, unless --force is given

[Junio C Hamano]

Andres Perera <andres.p@zoho.com> writes:

>> +/* return NULL on success, else hostname running the gc */
>> +static const char *lock_repo_for_gc(int force, pid_t* ret_pid)
>> +{
>> +       static struct lock_file lock;
>> +       static char locking_host[128];
>> +       char my_host[128];
>> +       struct strbuf sb = STRBUF_INIT;
>> +       struct stat st;
>> +       uintmax_t pid;
>
> pid_t is always an signed type, therefore unintmax_t does not make
> sense as a catch all value

Good eyes.

>> +                       !strcmp(locking_host, my_host) &&
>> +                       !kill(pid, 0);
>> +               if (fp != NULL)
>> +                       fclose(fp);
>> +               if (should_exit) {
>> +                       if (fd >= 0)
>> +                               rollback_lock_file(&lock);
>> +                       *ret_pid = pid;
>> +                       return locking_host;
>
> why not exponential backoff?


If the other guy is doing a GC, and we decide that we should exit,
it is *not* because we want to wait until the other guy is done.  It
is because we know we do not have to do the work --- the other guy
is doing what we were about to do, and it will do it for us anyway.

So I do not think it makes any sense to do exponential backoff if
"gc --auto" is asking "should we exit" to this logic.

An explicit "gc", on the other hand, may benefit from backoff, but
then the user can choose to do so himself, and more importantly, the
user can see "ah, another one is running so enough cruft will be
cleaned up anyway" and choose not to run it.

Re: [PATCH v4] gc: reject if another gc is running, unless --force is given

[Duy Nguyen]

On Fri, Aug 9, 2013 at 11:29 PM, Andres Perera <andres.p@zoho.com> wrote:
> On Thu, Aug 8, 2013 at 6:35 AM, Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:
>> +       uintmax_t pid;
>
> pid_t is always an signed type, therefore unintmax_t does not make
> sense as a catch all value

I only catch real process id. In practice we don't have processes with
negative pid_t, do we? I can't find any document about this, but at
least waitpid seems to treat negative pid (except -1) just as an
indicator while the true pid is the positive counterpart.

> fork() returns -1 on failure, and its return type is pid_t. i don't
> know what fantasy unix system has an unsigned pid_t
-- 
Duy

Re: [PATCH v4] gc: reject if another gc is running, unless --force is given

[Andreas Schwab]

Duy Nguyen <pclouds@gmail.com> writes:

> On Fri, Aug 9, 2013 at 11:29 PM, Andres Perera <andres.p@zoho.com> wrote:
>> On Thu, Aug 8, 2013 at 6:35 AM, Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:
>>> +       uintmax_t pid;
>>
>> pid_t is always an signed type, therefore unintmax_t does not make
>> sense as a catch all value
>
> I only catch real process id. In practice we don't have processes with
> negative pid_t, do we? I can't find any document about this, but at
> least waitpid seems to treat negative pid (except -1) just as an
> indicator while the true pid is the positive counterpart.

Negative pids are used for denoting process groups in various
interfaces.  No process can have a negative pid.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."