Re: [PATCH v3] gc: reject if another gc is running, unless --force is given
Ramkumar Ramachandra artag...@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. -- 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 v3] gc: reject if another gc is running, unless --force is given
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. -- 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
[PATCH v3] gc: reject if another gc is running, unless --force is given
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 pclo...@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
Re: [PATCH v3] gc: reject if another gc is running, unless --force is given
Nguyễn Thái Ngọc Duy pclo...@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.
--
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 v3] gc: reject if another gc is running, unless --force is given
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. -- 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
