Re: Auto Packing message twice?
On Sat, Aug 30, 2014 at 09:12:57PM +0700, Duy Nguyen wrote:
Show 15 quoted lines
> On Sat, Aug 30, 2014 at 6:28 PM, Stefan Beller <stefanbeller@gmail.com> wrote:
> > Auto packing the repository in background for optimum performance.
> > See "git help gc" for manual housekeeping.
> > Auto packing the repository in background for optimum performance.
> > See "git help gc" for manual housekeeping.
> >
> > Obviously it should only report once?
> > The reporting is done from within function cmd_gc(...),
> > which makes me wonder, if it also packs twice?
>
> The problem is we print this message before checking if gc is running
> in background. Problem is we keep the lockafter forking/detaching
> because locking before forking means two processes holding the lock
> and I don't think there's a way to say "release the lock in parent
> process".
I'm wrong. It's more about the gc.pid update code, which should record the running pid (i.e. after fork), so I can't really move lock_repo_for_gc() code to the parent process. If we just peek ahead in gc.pid to see if any gc is already running, there's a chance that a new gc will start after the check, and before we update gc.pid. So still false messages.
> And we can't print this message after detaching either
> because stdout is already closed then. But this definitely needs some
> improvement to avoid confusion.
If we do not use daemonized() then it's possible. Though NO_POSIX_GOODIES is now sprinkled in more places.
-- 8< --
diff --git a/builtin/gc.c b/builtin/gc.c
index 8d219d8..f04b5dc 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -280,6 +280,7 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
int force = 0;
const char *name;
pid_t pid;
+ int forked = 0;
struct option builtin_gc_options[] = {
OPT__QUIET(&quiet, N_("suppress progress reporting")),
@@ -327,21 +328,30 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
*/
if (!need_to_gc())
return 0;
- if (!quiet) {
- if (detach_auto)
- fprintf(stderr, _("Auto packing the repository in background for optimum performance.\n"));
- else
- fprintf(stderr, _("Auto packing the repository for optimum performance.\n"));
+ if (!quiet && !detach_auto) {
+ fprintf(stderr, _("Auto packing the repository for optimum performance.\n"));
fprintf(stderr, _("See \"git help gc\" for manual housekeeping.\n"));
}
if (detach_auto) {
if (gc_before_repack())
return -1;
- /*
- * failure to daemonize is ok, we'll continue
- * in foreground
- */
- daemonize();
+#ifndef NO_POSIX_GOODIES
+ switch (fork()) {
+ case 0:
+ forked = 1;
+ break;
+
+ case -1:
+ /*
+ * failure to daemonize is ok, we'll continue
+ * in foreground
+ */
+ break;
+
+ default:
+ exit(0);
+ }
+#endif
}
} else
add_repack_all_option();
@@ -354,6 +364,23 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
name, (uintmax_t)pid);
}
+ if (auto_gc && !quiet && detach_auto) {
+ if (forked)
+ fprintf(stderr, _("Auto packing the repository in background for optimum performance.\n"));
+ else
+ fprintf(stderr, _("Auto packing the repository for optimum performance.\n"));
+ fprintf(stderr, _("See \"git help gc\" for manual housekeeping.\n"));
+ }
+#ifndef NO_POSIX_GOODIES
+ if (forked) {
+ if (setsid() == -1)
+ die_errno("setsid failed");
+ close(0);
+ close(1);
+ close(2);
+ sanitize_stdfds();
+ }
+#endif
if (gc_before_repack())
return -1;
-- 8< --
Stefan Beller