Re: [PATCH/RFC 2/2] receive-pack: hint that the user can stop "git push" at auto gc time

Junio C Hamano [off-list ref] writes:

Nguyễn Thái Ngọc Duy  [off-list ref] writes:

quoted

Housekeeping jobs like auto gc generally should not get in the way.
Users who are pushing may not want to wait until auto gc is done on
the server. Give a hint for those users that it's safe now to break
"git push" and stop waiting.

Signed-off-by: Nguyễn Thái Ngọc Duy <redacted>
---
 This bandage patch may be a good compromise between running auto gc
 and not annoying users much.
 
 If I'm not mistaken, when ^C on "git push" this way, gc will still be
 running until it needs to print something out (which it should not
 normally because of --quiet). The user won't see gc errors, but the
 user generally can't do much anyway.

If you are over local transport, I would think you would kill the
both ends.  Also, wouldn't killing "git push" before it is done
talking with the receive-pack stop it before it has a chance to
update the remote tracking refs to pretend as if it fetched from
there immediately after a push?

So, no. I do not think we should ever encourage "if this bothers
you, you can ^C it".  Making it not to bother is fine, though.

Instead of adding a boolean --break-ok that is hidden, why not
adding an exposed boolean --daemonize, and let auto-gc run in the
background?  With the recent "do not let more than one gc run at the
same time", that should give a lot more pleasant end user
experience, no?

Junio C Hamano <gitster <at> pobox.com> writes:

Instead of adding a boolean --break-ok that is hidden, why not
adding an exposed boolean --daemonize, and let auto-gc run in the
background?  With the recent "do not let more than one gc run at the
same time", that should give a lot more pleasant end user
experience, no?

That sounds quite useful to me.  Duy, are you up for generating such a patch?

Thanks,

Chris

On Fri, Feb 7, 2014 at 7:36 PM, chris [off-list ref] wrote:

Junio C Hamano <gitster <at> pobox.com> writes:

quoted

Instead of adding a boolean --break-ok that is hidden, why not
adding an exposed boolean --daemonize, and let auto-gc run in the
background?  With the recent "do not let more than one gc run at the
same time", that should give a lot more pleasant end user
experience, no?

That sounds quite useful to me.  Duy, are you up for generating such a patch?

It would not be so hard for that patch. I'm still thinking whether it
should be done if auto-gc is started on the client side too (sometimes
it does, which is equally annoying)..
-- 
Duy

Duy Nguyen <pclouds <at> gmail.com> writes:

On Fri, Feb 7, 2014 at 7:36 PM, chris <jugg <at> hotmail.com> wrote:

quoted

Junio C Hamano <gitster <at> pobox.com> writes:

quoted

Instead of adding a boolean --break-ok that is hidden, why not
adding an exposed boolean --daemonize, and let auto-gc run in the
background?  With the recent "do not let more than one gc run at the
same time", that should give a lot more pleasant end user
experience, no?

That sounds quite useful to me.  Duy, are you up for generating such a

patch?

It would not be so hard for that patch. I'm still thinking whether it
should be done if auto-gc is started on the client side too (sometimes
it does, which is equally annoying)..

That could be nice, but I'd be less concerned about that, as the client has
the ability to disable gc for itself.  Still pushing it into the background,
if considered acceptable behavior, seems reasonable.  Perhaps two separate
patches?

Chris

Signed-off-by: Nguyễn Thái Ngọc Duy <redacted>
---
 cache.h  |  1 +
 daemon.c | 30 ++++--------------------------
 setup.c  | 24 ++++++++++++++++++++++++
 3 files changed, 29 insertions(+), 26 deletions(-)

diff --git a/cache.h b/cache.h
index dc040fb..264b6f1 100644
--- a/cache.h
+++ b/cache.h

@@ -434,6 +434,7 @@ extern int set_git_dir_init(const char *git_dir, const char *real_git_dir, int);
 extern int init_db(const char *template_dir, unsigned int flags);
 
 extern void sanitize_stdfds(void);
+extern int daemonize(void);
 
 #define alloc_nr(x) (((x)+16)*3/2)
 

diff --git a/daemon.c b/daemon.c
index 503e039..eba1255 100644
--- a/daemon.c
+++ b/daemon.c

@@ -1056,11 +1056,6 @@ static void drop_privileges(struct credentials *cred)
 	/* nothing */
 }
 
-static void daemonize(void)
-{
-	die("--detach not supported on this platform");
-}
-
 static struct credentials *prepare_credentials(const char *user_name,
     const char *group_name)
 {

@@ -1102,24 +1097,6 @@ static struct credentials *prepare_credentials(const char *user_name,
 
 	return &c;
 }
-
-static void daemonize(void)
-{
-	switch (fork()) {
-		case 0:
-			break;
-		case -1:
-			die_errno("fork failed");
-		default:
-			exit(0);
-	}
-	if (setsid() == -1)
-		die_errno("setsid failed");
-	close(0);
-	close(1);
-	close(2);
-	sanitize_stdfds();
-}
 #endif
 
 static void store_pid(const char *path)

@@ -1333,9 +1310,10 @@ int main(int argc, char **argv)
 	if (inetd_mode || serve_mode)
 		return execute();
 
-	if (detach)
-		daemonize();
-	else
+	if (detach) {
+		if (daemonize())
+			die("--detach not supported on this platform");
+	} else
 		sanitize_stdfds();
 
 	if (pid_file)

diff --git a/setup.c b/setup.c
index 6c3f85f..b09a412 100644
--- a/setup.c
+++ b/setup.c

@@ -787,3 +787,27 @@ void sanitize_stdfds(void)
 	if (fd > 2)
 		close(fd);
 }
+
+int daemonize(void)
+{
+#ifdef NO_POSIX_GOODIES
+	errno = -ENOSYS;
+	return -1;
+#else
+	switch (fork()) {
+		case 0:
+			break;
+		case -1:
+			die_errno("fork failed");
+		default:
+			exit(0);
+	}
+	if (setsid() == -1)
+		die_errno("setsid failed");
+	close(0);
+	close(1);
+	close(2);
+	sanitize_stdfds();
+	return 0;
+#endif
+}

-- 
1.8.5.2.240.g8478abd

`gc --auto` takes time and can block the user temporarily (but not any
less annoyingly). Make it run in background on systems that support
it. The only thing lost with running in background is printouts. But
gc output is not really interesting. You can keep it in foreground by
changing gc.autodetach.

Signed-off-by: Nguyễn Thái Ngọc Duy <redacted>
---
 Documentation/config.txt |  4 ++++
 builtin/gc.c             | 23 ++++++++++++++++++-----
 t/t5400-send-pack.sh     |  1 +
 3 files changed, 23 insertions(+), 5 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 5f4d793..4781773 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt

@@ -1167,6 +1167,10 @@ gc.autopacklimit::
 	--auto` consolidates them into one larger pack.  The
 	default	value is 50.  Setting this to 0 disables it.
 
+gc.autodetach::
+	Make `git gc --auto` return immediately andrun in background
+	if the system supports it. Default is true.
+
 gc.packrefs::
 	Running `git pack-refs` in a repository renders it
 	unclonable by Git versions prior to 1.5.1.2 over dumb

diff --git a/builtin/gc.c b/builtin/gc.c
index c19545d..ed5cc3c 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c

@@ -29,6 +29,7 @@ static int pack_refs = 1;
 static int aggressive_window = 250;
 static int gc_auto_threshold = 6700;
 static int gc_auto_pack_limit = 50;
+static int detach_auto = 1;
 static const char *prune_expire = "2.weeks.ago";
 
 static struct argv_array pack_refs_cmd = ARGV_ARRAY_INIT;

@@ -73,6 +74,10 @@ static int gc_config(const char *var, const char *value, void *cb)
 		gc_auto_pack_limit = git_config_int(var, value);
 		return 0;
 	}
+	if (!strcmp(var, "gc.autodetach")) {
+		detach_auto = git_config_bool(var, value);
+		return 0;
+	}
 	if (!strcmp(var, "gc.pruneexpire")) {
 		if (value && strcmp(value, "now")) {
 			unsigned long now = approxidate("now");

@@ -301,11 +306,19 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
 		 */
 		if (!need_to_gc())
 			return 0;
-		if (!quiet)
-			fprintf(stderr,
-					_("Auto packing the repository for optimum performance. You may also\n"
-					"run \"git gc\" manually. See "
-					"\"git help gc\" for more information.\n"));
+		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"));
+			fprintf(stderr, _("See \"git help gc\" for manual housekeeping.\n"));
+		}
+		if (detach_auto)
+			/*
+			 * failure to daemonize is ok, we'll continue
+			 * in foreground
+			 */
+			daemonize();
 	} else
 		add_repack_all_option();
 

diff --git a/t/t5400-send-pack.sh b/t/t5400-send-pack.sh
index 129fc88..0736bcb 100755
--- a/t/t5400-send-pack.sh
+++ b/t/t5400-send-pack.sh

@@ -164,6 +164,7 @@ test_expect_success 'receive-pack runs auto-gc in remote repo' '
 	    # Set the child to auto-pack if more than one pack exists
 	    cd child &&
 	    git config gc.autopacklimit 1 &&
+	    git config gc.autodetach false &&
 	    git branch test_auto_gc &&
 	    # And create a file that follows the temporary object naming
 	    # convention for the auto-gc to remove

-- 
1.8.5.2.240.g8478abd

On Sat, Feb 8, 2014 at 8:08 AM, Nguyễn Thái Ngọc Duy [off-list ref] wrote:

While I agree that it should be OK, shouldn't we warn the user?

On Sat, Feb 8, 2014 at 8:08 AM, Nguyễn Thái Ngọc Duy [off-list ref] wrote:

Nice change.

Just a nit: When I added the NO_POSIX_GOODIES-flag, Junio wanted the
implementations to be separate.

On Mon, Feb 10, 2014 at 6:03 PM, Erik Faye-Lund [off-list ref] wrote:

quoted

`gc --auto` takes time and can block the user temporarily (but not any
-               if (!quiet)
-                       fprintf(stderr,
-                                       _("Auto packing the repository for optimum performance. You may also\n"
-                                       "run \"git gc\" manually. See "
-                                       "\"git help gc\" for more information.\n"));
+               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"));
+                       fprintf(stderr, _("See \"git help gc\" for manual housekeeping.\n"));
+               }
+               if (detach_auto)
+                       /*
+                        * failure to daemonize is ok, we'll continue
+                        * in foreground
+                        */
+                       daemonize();

While I agree that it should be OK, shouldn't we warn the user?

If --quiet is set, we should not be printing anyway. If not, I thinkg
we could only print "auto packing in background.." when we actually
can do that, else just print the old message. It means an #ifdef
NO_POSIX_GOODIES here again though..
-- 
Duy

On Mon, Feb 10, 2014 at 2:17 PM, Duy Nguyen [off-list ref] wrote:

On Mon, Feb 10, 2014 at 6:03 PM, Erik Faye-Lund [off-list ref] wrote:

quoted

quoted

`gc --auto` takes time and can block the user temporarily (but not any
-               if (!quiet)
-                       fprintf(stderr,
-                                       _("Auto packing the repository for optimum performance. You may also\n"
-                                       "run \"git gc\" manually. See "
-                                       "\"git help gc\" for more information.\n"));
+               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"));
+                       fprintf(stderr, _("See \"git help gc\" for manual housekeeping.\n"));
+               }
+               if (detach_auto)
+                       /*
+                        * failure to daemonize is ok, we'll continue
+                        * in foreground
+                        */
+                       daemonize();

While I agree that it should be OK, shouldn't we warn the user?

If --quiet is set, we should not be printing anyway. If not, I thinkg
we could only print "auto packing in background.." when we actually
can do that, else just print the old message. It means an #ifdef
NO_POSIX_GOODIES here again though..

Yuck, it's probably better to simply silently drop the detaching, I guess.