Re: Auto Packing message twice?

From: Duy Nguyen <pclouds@gmail.com>
Date: Aug 31, 2014, 04:26 UTC
Message-ID: <20140831042615.GA7862@lanh>
In-Reply-To: <CACsJy8Ch5WW2rF4vGrM2xyFqn2-1FgPw-kO_wLtBY8-+WqM-1g@mail.gmail.com>

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< --