Auto Packing message twice?

Auto Packing message twice?

[Stefan Beller]

Hi,

so I fetched from a repository today and I got the message twice:

> Auto packing the repository in background for optimum performance.
> See "git help gc" for manual housekeeping.

I'm running git version 2.1.0.rc2

Full output:

$ git fetch --all --prune
Fetching origin
Fetching mainline
remote: Counting objects: 832, done.
remote: Compressing objects: 100% (332/332), done.
remote: Total 832 (delta 593), reused 659 (delta 498)
Receiving objects: 100% (832/832), 955.44 KiB | 660.00 KiB/s, done.
Resolving deltas: 100% (593/593), done.
From git://github.com/danmar/cppcheck
   7237b01..eace67e  master     -> mainline/master
 * [new tag]         1.66       -> 1.66
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?

Thanks,
Stefan

Re: Auto Packing message twice?

[Duy Nguyen]

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". 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.
-- 
Duy

Re: Auto Packing message twice?

[Duy Nguyen]

On Sat, Aug 30, 2014 at 09:12:57PM +0700, Duy Nguyen wrote:
> 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< --