bug? git push triggers auto pack when gc.auto = 0

bug? git push triggers auto pack when gc.auto = 0

[chris]

Hi,

I have garbage collection disabled globally with gc.auto = 0.  Today while
pushing a branch remotely, I saw a message "Auto packing the repository for
optimum performance." which I've never noticed before.  Searching for that
phrase shows me that common knowledge is that 'gc.auto = 0' should disable
such from occurring.  Looking at .git/objects/pack/ in the repository show a
new pack file created at the time.  However, all loose objects still exist
in the repository, which is what I want, so it is good that no apparent data
loss occurred.

Here is the relevant command and its output:

$ git push origin next 
Counting objects: 56, done.
Delta compression using up to 4 threads.
Compressing objects: 100% (9/9), done.
Writing objects: 100% (9/9), 895 bytes | 0 bytes/s, done.
Total 9 (delta 8), reused 0 (delta 0)
Auto packing the repository for optimum performance.
To ssh://git@my.server.com/my_project
   3560275..f508080  next -> next
$ git config gc.auto
0
$ git config gc.autopacklimit
0
$ git --version
git version 1.8.5.3

So my question is, should gc.auto = 0 disable auto-packing from occurring on
git push and other non-gc commands?

Thanks,

Chris

Re: bug? git push triggers auto pack when gc.auto = 0

[Duy Nguyen]

On Tue, Feb 4, 2014 at 9:20 AM, chris <jugg@hotmail.com> wrote:
> $ git push origin next
> Counting objects: 56, done.
> Delta compression using up to 4 threads.
> Compressing objects: 100% (9/9), done.
> Writing objects: 100% (9/9), 895 bytes | 0 bytes/s, done.
> Total 9 (delta 8), reused 0 (delta 0)
> Auto packing the repository for optimum performance.

This string only appears in versions before 1.8.0. It's longer after 1.8.0.

> To ssh://git@my.server.com/my_project
>    3560275..f508080  next -> next
> $ git config gc.auto
> 0
> $ git config gc.autopacklimit
> 0
> $ git --version
> git version 1.8.5.3

but your client is after 1.8.0 so the string printed above is from the
server side. "git config gc.auto" here does not matter. Run that
command again on my.server.com.

> So my question is, should gc.auto = 0 disable auto-packing from occurring on
> git push and other non-gc commands?

Yes it should.
-- 
Duy

Re: bug? git push triggers auto pack when gc.auto = 0

[chris]

Duy Nguyen <pclouds <at> gmail.com> writes:
> On Tue, Feb 4, 2014 at 9:20 AM, chris <jugg <at> hotmail.com> wrote:
> > $ git push origin next
> > Counting objects: 56, done.
> > Delta compression using up to 4 threads.
> > Compressing objects: 100% (9/9), done.
> > Writing objects: 100% (9/9), 895 bytes | 0 bytes/s, done.
> > Total 9 (delta 8), reused 0 (delta 0)
> > Auto packing the repository for optimum performance.
> 
> This string only appears in versions before 1.8.0. It's longer after 1.8.0.
> 
> > To ssh://git <at> my.server.com/my_project
> >    3560275..f508080  next -> next
> > $ git config gc.auto
> > 0
> > $ git config gc.autopacklimit
> > 0
> > $ git --version
> > git version 1.8.5.3
> 
> but your client is after 1.8.0 so the string printed above is from the
> server side. "git config gc.auto" here does not matter. Run that
> command again on my.server.com.

Ok, so I can understand if the message is from the server.  I'll chalk up
never noticing it before to someone else always being the lucky one to
trigger it.

However, I question why I should even care about this message?  I'm going to
assume that simply it is a lengthy synchronous operation that someone felt
deserved some verbosity to why the client push action is taking longer than
it should.  Yet that makes me question why I'm being penalized for this
server side operation.  My client time should not be consumed for server
side house keeping.

An obvious fix is to disable gc on the server and implement a cron job for
the house keeping task.  However, as often the case one does not have
control over the server, so it is unfortunate that git has this server side
house keeping as a blocking operation to a client action.

> > So my question is, should gc.auto = 0 disable auto-packing from occurring on
> > git push and other non-gc commands?
> 
> Yes it should.

Thanks for the confirmation.

Regards,

Chris

Re: bug? git push triggers auto pack when gc.auto = 0

[Duy Nguyen]

On Tue, Feb 4, 2014 at 12:13 PM, chris <jugg@hotmail.com> wrote:
> However, I question why I should even care about this message?  I'm going to
> assume that simply it is a lengthy synchronous operation that someone felt
> deserved some verbosity to why the client push action is taking longer than
> it should.  Yet that makes me question why I'm being penalized for this
> server side operation.  My client time should not be consumed for server
> side house keeping.
>
> An obvious fix is to disable gc on the server and implement a cron job for
> the house keeping task.  However, as often the case one does not have
> control over the server, so it is unfortunate that git has this server side
> house keeping as a blocking operation to a client action.

I agree it should not block the client. I think you can Ctrl-C "git
push" at this point without losing anything (data has already been
pushed at this point) but that's not a good advice to general cases.
Maybe we can do something at the server side to not block the client..

Another thing we could do is put "remote: " in front of these strings,
even in ssh case. They seem to confuse you (and me too) that things
happened locally.
-- 
Duy

[PATCH 1/2] receive-pack: update $GIT_DIR/info before auto garbage collection

[Nguyễn Thái Ngọc Duy]

Auto gc could take a long time, and it's optional. "git push" user
should be allowed to stop the program if they don't want to wait. Move
server update step before auto gc. So we're ready to die any time
since auto gc is kicked off.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/receive-pack.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 85bba35..82e2f76 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -1208,6 +1208,8 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix)
 			report(commands, unpack_status);
 		run_receive_hook(commands, "post-receive", 1);
 		run_update_post_hook(commands);
+		if (auto_update_server_info)
+			update_server_info(0);
 		if (auto_gc) {
 			const char *argv_gc_auto[] = {
 				"gc", "--auto", "--quiet", NULL,
@@ -1215,8 +1217,6 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix)
 			int opt = RUN_GIT_CMD | RUN_COMMAND_STDOUT_TO_STDERR;
 			run_command_v_opt(argv_gc_auto, opt);
 		}
-		if (auto_update_server_info)
-			update_server_info(0);
 		clear_shallow_info(&si);
 	}
 	if (use_sideband)
-- 
1.8.5.2.240.g8478abd

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

[Nguyễn Thái Ngọc Duy]

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 <pclouds@gmail.com>
---
 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.

 builtin/gc.c           | 9 ++++++++-
 builtin/receive-pack.c | 2 +-
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/builtin/gc.c b/builtin/gc.c
index c19545d..592271a 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -253,6 +253,7 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
 	int auto_gc = 0;
 	int quiet = 0;
 	int force = 0;
+	int break_ok = 0;
 	const char *name;
 	pid_t pid;
 
@@ -263,6 +264,8 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
 			PARSE_OPT_OPTARG, NULL, (intptr_t)prune_expire },
 		OPT_BOOL(0, "aggressive", &aggressive, N_("be more thorough (increased runtime)")),
 		OPT_BOOL(0, "auto", &auto_gc, N_("enable auto-gc mode")),
+		OPT_HIDDEN_BOOL(0, "break-ok", &break_ok,
+				"hint that it is ok to stop the program"),
 		OPT_BOOL(0, "force", &force, N_("force running gc even if there may be another gc running")),
 		OPT_END()
 	};
@@ -301,7 +304,11 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
 		 */
 		if (!need_to_gc())
 			return 0;
-		if (!quiet)
+		if (break_ok)
+			fprintf(stderr,
+				_("Auto packing the repository for optimum performance.\n"
+				  "It is safe to stop the program with Ctrl-C.\n"));
+		else if (!quiet)
 			fprintf(stderr,
 					_("Auto packing the repository for optimum performance. You may also\n"
 					"run \"git gc\" manually. See "
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 82e2f76..68d16e0 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -1212,7 +1212,7 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix)
 			update_server_info(0);
 		if (auto_gc) {
 			const char *argv_gc_auto[] = {
-				"gc", "--auto", "--quiet", NULL,
+				"gc", "--auto", "--quiet", "--break-ok", NULL,
 			};
 			int opt = RUN_GIT_CMD | RUN_COMMAND_STDOUT_TO_STDERR;
 			run_command_v_opt(argv_gc_auto, opt);
-- 
1.8.5.2.240.g8478abd

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

[Junio C Hamano]

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

> 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 <pclouds@gmail.com>
> ---
>  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.

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

[Junio C Hamano]

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

> Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
>
>> 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 <pclouds@gmail.com>
>> ---
>>  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?

Re: [PATCH/RFC 2/2] receive-pack: hint that the user can stop

[chris]

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

Re: [PATCH/RFC 2/2] receive-pack: hint that the user can stop

[Duy Nguyen]

On Fri, Feb 7, 2014 at 7:36 PM, chris <jugg@hotmail.com> wrote:
> 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?

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

Re: [PATCH/RFC 2/2] receive-pack: hint that the user can stop

[chris]

Duy Nguyen <pclouds <at> gmail.com> writes:
> On Fri, Feb 7, 2014 at 7:36 PM, chris <jugg <at> hotmail.com> wrote:
> > 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?
> 
> 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

[PATCH v2 1/2] daemon: move daemonize() to libgit.a

[Nguyễn Thái Ngọc Duy]

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 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

[PATCH v2 2/2] gc: config option for running --auto in background

[Nguyễn Thái Ngọc Duy]

`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 <pclouds@gmail.com>
---
 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

Re: [PATCH v2 2/2] gc: config option for running --auto in background

[Erik Faye-Lund]

On Sat, Feb 8, 2014 at 8:08 AM, Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:
> `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 <pclouds@gmail.com>
> ---
>  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();

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

Re: [PATCH v2 2/2] gc: config option for running --auto in background

[Duy Nguyen]

On Mon, Feb 10, 2014 at 6:03 PM, Erik Faye-Lund <kusmabite@gmail.com> wrote:
>> `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

Re: [PATCH v2 2/2] gc: config option for running --auto in background

[Erik Faye-Lund]

On Mon, Feb 10, 2014 at 2:17 PM, Duy Nguyen <pclouds@gmail.com> wrote:
> On Mon, Feb 10, 2014 at 6:03 PM, Erik Faye-Lund <kusmabite@gmail.com> wrote:
>>> `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.

Re: [PATCH v2 2/2] gc: config option for running --auto in background

[Junio C Hamano]

Duy Nguyen <pclouds@gmail.com> writes:

> On Mon, Feb 10, 2014 at 6:03 PM, Erik Faye-Lund <kusmabite@gmail.com> wrote:
>>> `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..

Didn't you change it not to die but return nosys or something?

Re: [PATCH v2 2/2] gc: config option for running --auto in background

[Junio C Hamano]

On Mon, Feb 10, 2014 at 10:43 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> 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..
>
> Didn't you change it not to die but return nosys or something?

Ah, the problem is that it is too late to take back "... will do so in
the background" when you noticed that daemonize() did not succeed, so
you would need a way to see if we can daemonize() before actually
doing so if you want to give different messages.

"int can_daemonize(void)" could be an answer that is nicer than
NO_POSIX_GOODIES, but I am not sure if it is worth it.

Re: [PATCH v2 2/2] gc: config option for running --auto in background

[Duy Nguyen]

On Tue, Feb 11, 2014 at 2:11 AM, Junio C Hamano <gitster@pobox.com> wrote:
> On Mon, Feb 10, 2014 at 10:43 AM, Junio C Hamano <gitster@pobox.com> wrote:
>>> 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..
>>
>> Didn't you change it not to die but return nosys or something?
>
> Ah, the problem is that it is too late to take back "... will do so in
> the background" when you noticed that daemonize() did not succeed, so
> you would need a way to see if we can daemonize() before actually
> doing so if you want to give different messages.
>
> "int can_daemonize(void)" could be an answer that is nicer than
> NO_POSIX_GOODIES, but I am not sure if it is worth it.

Or we could pass the "quiet" flag to daemonize() and let it print
something in the #ifdef NO_POSIX_GOODIES part.
-- 
Duy

Re: [PATCH v2 2/2] gc: config option for running --auto in background

[Junio C Hamano]

Duy Nguyen <pclouds@gmail.com> writes:

> On Tue, Feb 11, 2014 at 2:11 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> On Mon, Feb 10, 2014 at 10:43 AM, Junio C Hamano <gitster@pobox.com> wrote:
>>>> 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..
>>>
>>> Didn't you change it not to die but return nosys or something?
>>
>> Ah, the problem is that it is too late to take back "... will do so in
>> the background" when you noticed that daemonize() did not succeed, so
>> you would need a way to see if we can daemonize() before actually
>> doing so if you want to give different messages.
>>
>> "int can_daemonize(void)" could be an answer that is nicer than
>> NO_POSIX_GOODIES, but I am not sure if it is worth it.
>
> Or we could pass the "quiet" flag to daemonize() and let it print
> something in the #ifdef NO_POSIX_GOODIES part.

Hmph...  What would that something say?  "I was asked to gc in the
background but I can't here" is not suitable for daemonize() that is
not specific to "gc".

The flow I had in mind was something along the lines of this

	if (!quiet) {
        	if (detach_auto && can_daemonize())
                	say "auto packing in the background";
		else
                       	say "auto packing"
	}
        if (detach_auto && can_daemonize())
        	daemonize();

If we had daemonize(noisy=1) and coded it this way:

	if (!quiet)
        	say "auto packing";
	if (detach_auto)
        	daemonize(!quiet);

we could do something like:

	daemonize(int noisy) {
        	if (noisy && !defined(NO_POSIX_GOODIES))
                	say ", and doing so in the background";
		... do the actual daemonizing ...
	}

but that feels ugly.

Re: [PATCH v2 1/2] daemon: move daemonize() to libgit.a

[Erik Faye-Lund]

On Sat, Feb 8, 2014 at 8:08 AM, Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> 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
> +}

Nice change.

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

Re: [PATCH v2 1/2] daemon: move daemonize() to libgit.a

[Junio C Hamano]

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

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

Negated?

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

Re: [PATCH v2 1/2] daemon: move daemonize() to libgit.a

[Duy Nguyen]

On Tue, Feb 11, 2014 at 1:46 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
>
>> 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;
>
> Negated?

Facepalm. I remember I wrote this somewhere but don't remember what
topic :( Should I resend?
-- 
Duy

Re: [PATCH v2 1/2] daemon: move daemonize() to libgit.a

[Junio C Hamano]

Duy Nguyen <pclouds@gmail.com> writes:

> On Tue, Feb 11, 2014 at 1:46 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
>>
>>> 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;
>>
>> Negated?
>
> Facepalm. I remember I wrote this somewhere but don't remember what
> topic :( Should I resend?

I've already amended it here.

Re: bug? git push triggers auto pack when gc.auto = 0

[chris]

Duy Nguyen <pclouds <at> gmail.com> writes:
> On Tue, Feb 4, 2014 at 12:13 PM, chris <jugg <at> hotmail.com> wrote:
> > However, I question why I should even care about this message?  I'm going to
> > assume that simply it is a lengthy synchronous operation that someone felt
> > deserved some verbosity to why the client push action is taking longer than
> > it should.  Yet that makes me question why I'm being penalized for this
> > server side operation.  My client time should not be consumed for server
> > side house keeping.
> >
> > An obvious fix is to disable gc on the server and implement a cron job for
> > the house keeping task.  However, as often the case one does not have
> > control over the server, so it is unfortunate that git has this server side
> > house keeping as a blocking operation to a client action.
> 
> I agree it should not block the client. I think you can Ctrl-C "git
> push" at this point without losing anything (data has already been
> pushed at this point) but that's not a good advice to general cases.
> Maybe we can do something at the server side to not block the client..

I'd like to avoid a Ctrl-C approach, but if an indication existed that
assured me the push part of the operation had completed successfully, then
that would be sufficient for when I'm impatient.

> Another thing we could do is put "remote: " in front of these strings,
> even in ssh case. They seem to confuse you (and me too) that things
> happened locally.

Yes, I would like to see more explicit clarity in what messages are coming
from the server.  That has always been a source of uncertainty for me with
any remote git command output.

Thanks for the patches and attention to this issue, I appreciate it.

Chris

Re: bug? git push triggers auto pack when gc.auto = 0

[David Kastrup]

chris <jugg@hotmail.com> writes:

> Duy Nguyen <pclouds <at> gmail.com> writes:
>> On Tue, Feb 4, 2014 at 9:20 AM, chris <jugg <at> hotmail.com> wrote:
>> > $ git push origin next
>> > Counting objects: 56, done.
>> > Delta compression using up to 4 threads.
>> > Compressing objects: 100% (9/9), done.
>> > Writing objects: 100% (9/9), 895 bytes | 0 bytes/s, done.
>> > Total 9 (delta 8), reused 0 (delta 0)
>> > Auto packing the repository for optimum performance.

> However, I question why I should even care about this message?  I'm going to
> assume that simply it is a lengthy synchronous operation that someone felt
> deserved some verbosity to why the client push action is taking longer than
> it should.  Yet that makes me question why I'm being penalized for this
> server side operation.  My client time should not be consumed for server
> side house keeping.

Your "client time" is not consumed: this is not a busy wait.  Git server
processes are synchronous: they are initiated and completed under the
control of a client.  That means that if you run a batch script
executing a number of commands in a client, it will not saturate the
server with half-finished processes and/or will refuse to honor requests
because the repository is locked.

> An obvious fix is to disable gc on the server and implement a cron job
> for the house keeping task.  However, as often the case one does not
> have control over the server, so it is unfortunate that git has this
> server side house keeping as a blocking operation to a client action.

_Any_ git operation is "blocking" the respective initiating client.

>> > So my question is, should gc.auto = 0 disable auto-packing from occurring on
>> > git push and other non-gc commands?
>> 
>> Yes it should.
>
> Thanks for the confirmation.

And indeed, there is no autopacking occuring on your site when doing git
push.  The server administrator will be rather glad that the clients'
configuration variables don't affect his server's operation.

-- 
David Kastrup

Re: bug? git push triggers auto pack when gc.auto = 0

[chris]

David Kastrup <dak <at> gnu.org> writes:
> chris <jugg <at> hotmail.com> writes:
> > Duy Nguyen <pclouds <at> gmail.com> writes:
> >> On Tue, Feb 4, 2014 at 9:20 AM, chris <jugg <at> hotmail.com> wrote:
> >> > $ git push origin next
> >> > Counting objects: 56, done.
> >> > Delta compression using up to 4 threads.
> >> > Compressing objects: 100% (9/9), done.
> >> > Writing objects: 100% (9/9), 895 bytes | 0 bytes/s, done.
> >> > Total 9 (delta 8), reused 0 (delta 0)
> >> > Auto packing the repository for optimum performance.
> 
> > However, I question why I should even care about this message?  I'm going to
> > assume that simply it is a lengthy synchronous operation that someone felt
> > deserved some verbosity to why the client push action is taking longer than
> > it should.  Yet that makes me question why I'm being penalized for this
> > server side operation.  My client time should not be consumed for server
> > side house keeping.
> 
> Your "client time" is not consumed: this is not a busy wait.  Git server
> processes are synchronous: they are initiated and completed under the
> control of a client.  That means that if you run a batch script
> executing a number of commands in a client, it will not saturate the
> server with half-finished processes and/or will refuse to honor requests
> because the repository is locked.

I'm slightly confused by your response.  You say "client time" is not
consumed, but then go on to say that git server processes are synchronous to
avoid build up from batched client requests.  I expect you took "client
time" to have some specific technical meaning, while I simply meant that the
client command did not return until the server completed its own house keeping.

But I do think we are on the same page otherwise in that the client command
is blocked until the server process completes.

That said I would naively assume that a server side house keeping operation
that does not get invoked with every client request be a nice candidate for
asynchronous handling without any need to tell the client about it.

Regards,

Chris

Re: bug? git push triggers auto pack when gc.auto = 0

[David Kastrup]

chris <jugg@hotmail.com> writes:

> David Kastrup <dak <at> gnu.org> writes:
>> chris <jugg <at> hotmail.com> writes:
>> > Duy Nguyen <pclouds <at> gmail.com> writes:
>> >> On Tue, Feb 4, 2014 at 9:20 AM, chris <jugg <at> hotmail.com> wrote:
>> >> > $ git push origin next
>> >> > Counting objects: 56, done.
>> >> > Delta compression using up to 4 threads.
>> >> > Compressing objects: 100% (9/9), done.
>> >> > Writing objects: 100% (9/9), 895 bytes | 0 bytes/s, done.
>> >> > Total 9 (delta 8), reused 0 (delta 0)
>> >> > Auto packing the repository for optimum performance.
>> 
>> > However, I question why I should even care about this message?  I'm going to
>> > assume that simply it is a lengthy synchronous operation that someone felt
>> > deserved some verbosity to why the client push action is taking longer than
>> > it should.  Yet that makes me question why I'm being penalized for this
>> > server side operation.  My client time should not be consumed for server
>> > side house keeping.
>> 
>> Your "client time" is not consumed: this is not a busy wait.  Git server
>> processes are synchronous: they are initiated and completed under the
>> control of a client.  That means that if you run a batch script
>> executing a number of commands in a client, it will not saturate the
>> server with half-finished processes and/or will refuse to honor requests
>> because the repository is locked.
>
> I'm slightly confused by your response.  You say "client time" is not
> consumed, but then go on to say that git server processes are
> synchronous to avoid build up from batched client requests.  I expect
> you took "client time" to have some specific technical meaning, while
> I simply meant that the client command did not return until the server
> completed its own house keeping.

Until the server completed the house keeping initiated under the control
of the client and on behalf of its command.

> But I do think we are on the same page otherwise in that the client
> command is blocked until the server process completes.

Sure.

> That said I would naively assume that a server side house keeping
> operation that does not get invoked with every client request be a
> nice candidate for asynchronous handling without any need to tell the
> client about it.

Except that there are _no_ asynchronously handled repository actions
executed on behalf of a client action.  If the repository owner decided
to disable demand-based garbage collection in favor of a cron job,
that's his call to make.  It makes some sense when there are frequent
and multiple accesses to the repository since it avoids getting denied
access because of somebody _else_ triggering garbage collection
predominantly when times are busiest.

Usually you are not denied access by your _own_ garbage collection since
the client waits until completion.

It would be quite bad for scripting git if you constantly had to check
after every action whether any associated garbage collection might or
might not have completed.

Note also that when pushing without a separate server process (like when
pushing into a local repository), there is no other job which could be
responsible for packing the repository rather than the one doing the
push.

-- 
David Kastrup

Re: bug? git push triggers auto pack when gc.auto = 0

[chris]

David Kastrup <dak <at> gnu.org> writes:
> chris <jugg <at> hotmail.com> writes:
> > That said I would naively assume that a server side house keeping
> > operation that does not get invoked with every client request be a
> > nice candidate for asynchronous handling without any need to tell the
> > client about it.
> 
> Except that there are _no_ asynchronously handled repository actions
> executed on behalf of a client action.  If the repository owner decided
> to disable demand-based garbage collection in favor of a cron job,
> that's his call to make.  It makes some sense when there are frequent
> and multiple accesses to the repository since it avoids getting denied
> access because of somebody _else_ triggering garbage collection
> predominantly when times are busiest.
> 
> Usually you are not denied access by your _own_ garbage collection since
> the client waits until completion.
> 
> It would be quite bad for scripting git if you constantly had to check
> after every action whether any associated garbage collection might or
> might not have completed.

I can't comment for every use case, but I find it strange that a client
script should need to care whether the server is currently garbage
collecting or not.  If such a detail must be exposed to a client, then I'd
put forth that there is a deeper issue here.  But any details there are
moving well beyond the scope I'm able to comment on.

That said, I think I understand you that it currently does matter in the
sense that a client can't perform other actions while garbage collection is
running.

> Note also that when pushing without a separate server process (like when
> pushing into a local repository), there is no other job which could be
> responsible for packing the repository rather than the one doing the
> push.

Ok, given your full response, I understand how this is being conceptualized
now, thanks.  However, if you look at it purely from a user's perspective
who is manually invoking these commands for the command's primary purpose,
the current behavior is annoying.

If we assume Git is right in implementing that no server async actions are
executed on behalf of a client action, then this falls under the category of
an ill-behaved server in my opinion.  Anything a server does that is not
directly related to fulfilling the requested client action is now considered
bad behavior as it blocks the client from continuing whatever it needs to
get on with.  I see such implementation in Git as favoring server's needs
over clients.

Regards,

Chris

Re: bug? git push triggers auto pack when gc.auto = 0

[David Kastrup]

chris <jugg@hotmail.com> writes:

> Ok, given your full response, I understand how this is being
> conceptualized now, thanks.  However, if you look at it purely from a
> user's perspective who is manually invoking these commands for the
> command's primary purpose, the current behavior is annoying.
>
> If we assume Git is right in implementing that no server async actions
> are executed on behalf of a client action, then this falls under the
> category of an ill-behaved server in my opinion.  Anything a server
> does that is not directly related to fulfilling the requested client
> action is now considered bad behavior as it blocks the client from
> continuing whatever it needs to get on with.  I see such
> implementation in Git as favoring server's needs over clients.

There are no "server's needs" at all.  Git only reacts to client
requests.  It is in the clients' own interest when garbage collection is
periodically done since it improves response time.

It's arguable that it would be nicer to use an incremental compaction
process that hides the periodic costs by distributing them over the
request totality.  That replaces the periodic "why does it have to
garbage collect when _I_ am using it" annoyance with "why is this
generally slow".  There is no net benefit to that approach safe for

a) avoiding complaints of "smart" people who have discovered that they
can speed up git by disabling garbage collection, but eventually find
that git is becoming slow for them but not for others.
b) avoiding these mailing list discussions.

The second benefit could likely be achieved by displaying "Server
unreachable... retrying..." instead of reporting about git gc.

-- 
David Kastrup