Auto Packing message twice?

DISCUSSION Stefan Beller 2 participants 3 messages Jun 15, 2016

lore

Share & tools

lore

Stefan Beller

author

2016-06-15 23:02 UTC #0 lore raw

Hi,

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

Quoted 2 lines

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

Duy Nguyen

2016-06-15 23:02 UTC #1 lore raw

Quoted 8 lines · Stefan Beller

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.

Signature · 1 line

Duy

Duy Nguyen

2016-06-15 23:02 UTC #2 lore raw

Quoted 10 lines · Duy Nguyen

Quoted 8 lines · Stefan Beller
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?

Quoted 5 lines

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.

Quoted 3 lines

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

builtin/gc.c +37 ~~-10~~

index 8d219d8..f04b5dc 100644
@@ -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< --