[PATCH] Update "gc" behavior in commit, merge, am, rebase and index-pack

[PATCH] Update "gc" behavior in commit, merge, am, rebase and index-pack

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

A few attempts have been made in the past to change 'gc --auto' [1]
[2]. This is another such attempt.

Commit d4bb43e (Invoke "git gc --auto" from commit, merge, am and
rebase. - 2007-09-05) used the rule to put "gc --auto" is "where
update-ref occurs". I would argue that this is not a good condition to
run gc, because (at least current) gc is slow. We encourage commit
often and rebase to make all patches in good shape and this workflow
should not be interrupted/slowed down by random "gc --auto".

Instead, we could just inform users that "gc" should be run soon in
commonly used commands (this patch also reinstates "gc" check in
commit, which was lost at the sh->C conversion). [1] and [2] can annoy
users constantly with warnings. This patch shows the warning at most
once a day.

There are commands that are not expected to return immediately. These
are more suitable for "gc --auto". One of them is receive-pack, which
already calls "gc --auto". The other one is index-pack, which "gc
--auto" is added in by this patch.

In short, after this patch:

 - receive-pack and index-pack (or push, pull/fetch at high level) can
   do "gc --auto".
 - commmit, merge, am, rebase warns once a day, no actual gc.

[1] http://thread.gmane.org/gmane.comp.version-control.git/184848
   adds an config option to turn --auto to warning only

[2] http://thread.gmane.org/gmane.comp.version-control.git/187711
   make it warn early before actual do the house keeping

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/commit.c           |    2 ++
 builtin/gc.c               |   21 ++++++++++++++++++++-
 builtin/index-pack.c       |   16 ++++++++++++++--
 builtin/merge.c            |    2 +-
 git-am.sh                  |    2 +-
 git-rebase--interactive.sh |    2 +-
 6 files changed, 39 insertions(+), 6 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index a2ec73d..4ba5677 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1418,6 +1418,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 
 		OPT_END()
 	};
+	const char *argv_gc_auto[] = { "gc", "--check", NULL };
 
 	struct strbuf sb = STRBUF_INIT;
 	struct strbuf author_ident = STRBUF_INIT;
@@ -1589,6 +1590,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 		     "new_index file. Check that disk is not full or quota is\n"
 		     "not exceeded, and then \"git reset HEAD\" to recover."));
 
+	run_command_v_opt(argv_gc_auto, RUN_GIT_CMD);
 	rerere(0);
 	run_hook(get_index_file(), "post-commit", NULL);
 	if (amend && !no_post_rewrite) {
diff --git a/builtin/gc.c b/builtin/gc.c
index 9b4232c..a85f71c 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -171,6 +171,7 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
 {
 	int aggressive = 0;
 	int auto_gc = 0;
+	int check_gc = 0;
 	int quiet = 0;
 
 	struct option builtin_gc_options[] = {
@@ -180,12 +181,16 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
 			PARSE_OPT_OPTARG, NULL, (intptr_t)prune_expire },
 		OPT_BOOLEAN(0, "aggressive", &aggressive, "be more thorough (increased runtime)"),
 		OPT_BOOLEAN(0, "auto", &auto_gc, "enable auto-gc mode"),
+		OPT_BOOLEAN(0, "check", &check_gc, "enable auto-gc mode"),
 		OPT_END()
 	};
 
 	if (argc == 2 && !strcmp(argv[1], "-h"))
 		usage_with_options(builtin_gc_usage, builtin_gc_options);
 
+	if (auto_gc && check_gc)
+		die(_("--auto and --check are incompatible"));
+
 	argv_array_pushl(&pack_refs_cmd, "pack-refs", "--all", "--prune", NULL);
 	argv_array_pushl(&reflog, "reflog", "expire", "--all", NULL);
 	argv_array_pushl(&repack, "repack", "-d", "-l", NULL);
@@ -211,12 +216,26 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
 	if (quiet)
 		argv_array_push(&repack, "-q");
 
-	if (auto_gc) {
+	if (auto_gc || check_gc) {
 		/*
 		 * Auto-gc should be least intrusive as possible.
 		 */
 		if (!need_to_gc())
 			return 0;
+		if (check_gc) {
+			struct stat st;
+			if (stat(git_path("gc_needed"), &st))
+				open(git_path("gc_needed"), O_CREAT | O_RDWR);
+			else {
+				/* do not bother users more than once a day */
+				if (time(NULL) - st.st_mtime < 86400)
+					return 0;
+				utime(git_path("gc_needed"), NULL);
+			}
+			warning(_("This repository needs maintenance. "
+				  "Please run \"git gc\" as soon as possible."));
+			return 0;
+		}
 		if (quiet)
 			fprintf(stderr, _("Auto packing the repository for optimum performance.\n"));
 		else
diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index 83555e5..438245c 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -9,6 +9,7 @@
 #include "progress.h"
 #include "fsck.h"
 #include "exec_cmd.h"
+#include "run-command.h"
 
 static const char index_pack_usage[] =
 "git index-pack [-v] [-o <index-file>] [--keep | --keep=<msg>] [--verify] [--strict] (<pack-file> | --stdin [--fix-thin] [<pack-file>])";
@@ -63,6 +64,7 @@ static int nr_resolved_deltas;
 static int from_stdin;
 static int strict;
 static int verbose;
+static int auto_gc;
 
 static struct progress *progress;
 
@@ -968,6 +970,10 @@ static int git_index_pack_config(const char *k, const char *v, void *cb)
 			die("bad pack.indexversion=%"PRIu32, opts->version);
 		return 0;
 	}
+	if (strcmp(k, "receive.autogc") == 0) {
+		auto_gc = git_config_bool(k, v);
+		return 0;
+	}
 	return git_default_config(k, v, cb);
 }
 
@@ -1254,12 +1260,18 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix)
 	curr_index = write_idx_file(index_name, idx_objects, nr_objects, &opts, pack_sha1);
 	free(idx_objects);
 
-	if (!verify)
+	if (!verify) {
 		final(pack_name, curr_pack,
 		      index_name, curr_index,
 		      keep_name, keep_msg,
 		      pack_sha1);
-	else
+		if (auto_gc) {
+			const char *argv_gc_auto[] = {
+				"gc", "--auto", "--quiet", NULL,
+			};
+			run_command_v_opt(argv_gc_auto, RUN_GIT_CMD);
+		}
+	} else
 		close(input_fd);
 	free(objects);
 	free(index_name_buf);
diff --git a/builtin/merge.c b/builtin/merge.c
index 470fc57..8b8716e 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -385,7 +385,7 @@ static void finish(struct commit *head_commit,
 		if (verbosity >= 0 && !merge_msg.len)
 			printf(_("No merge message -- not updating HEAD\n"));
 		else {
-			const char *argv_gc_auto[] = { "gc", "--auto", NULL };
+			const char *argv_gc_auto[] = { "gc", "--check", NULL };
 			update_ref(reflog_message.buf, "HEAD",
 				new_head, head, 0,
 				DIE_ON_ERR);
diff --git a/git-am.sh b/git-am.sh
index f8b7a0c..0f2dbc4 100755
--- a/git-am.sh
+++ b/git-am.sh
@@ -902,4 +902,4 @@ if test -s "$dotest"/rewritten; then
 fi
 
 rm -fr "$dotest"
-git gc --auto
+git gc --check
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 0c19b7c..89996a3 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -573,7 +573,7 @@ do_next () {
 		true # we don't care if this hook failed
 	fi &&
 	rm -rf "$state_dir" &&
-	git gc --auto &&
+	git gc --check &&
 	warn "Successfully rebased and updated $head_name."
 
 	exit
-- 
1.7.8.36.g69ee2

Re: [PATCH] Update "gc" behavior in commit, merge, am, rebase and index-pack

[Nguyen Thai Ngoc Duy]

(CCing Fernnando and complaining about send-email)

I accidentally put buccia@@repnz.net instead of one @. There were
warnings in the jungle that send-email spit out:

Use of uninitialized value $cc in string eq at
/home/pclouds/opt/git/libexec/git-core/git-send-email line 983.
Use of uninitialized value $cc in quotemeta at
/home/pclouds/opt/git/libexec/git-core/git-send-email line 983.
W: unable to extract a valid address from: Fernando Vezzosi <buccia@@repnz.net>
W: unable to extract a valid address from: Fernando Vezzosi <buccia@@repnz.net>

but it's really hard to see that. Maybe send-email should not allow me
to send in this case (unless I give --force), or make the warnings
more outstanding?

On Sat, May 12, 2012 at 3:08 PM, Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:
> A few attempts have been made in the past to change 'gc --auto' [1]
> [2]. This is another such attempt.
>
> Commit d4bb43e (Invoke "git gc --auto" from commit, merge, am and
> rebase. - 2007-09-05) used the rule to put "gc --auto" is "where
> update-ref occurs". I would argue that this is not a good condition to
> run gc, because (at least current) gc is slow. We encourage commit
> often and rebase to make all patches in good shape and this workflow
> should not be interrupted/slowed down by random "gc --auto".
>
> Instead, we could just inform users that "gc" should be run soon in
> commonly used commands (this patch also reinstates "gc" check in
> commit, which was lost at the sh->C conversion). [1] and [2] can annoy
> users constantly with warnings. This patch shows the warning at most
> once a day.
>
> There are commands that are not expected to return immediately. These
> are more suitable for "gc --auto". One of them is receive-pack, which
> already calls "gc --auto". The other one is index-pack, which "gc
> --auto" is added in by this patch.
>
> In short, after this patch:
>
>  - receive-pack and index-pack (or push, pull/fetch at high level) can
>   do "gc --auto".
>  - commmit, merge, am, rebase warns once a day, no actual gc.
>
> [1] http://thread.gmane.org/gmane.comp.version-control.git/184848
>   adds an config option to turn --auto to warning only
>
> [2] http://thread.gmane.org/gmane.comp.version-control.git/187711
>   make it warn early before actual do the house keeping
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  builtin/commit.c           |    2 ++
>  builtin/gc.c               |   21 ++++++++++++++++++++-
>  builtin/index-pack.c       |   16 ++++++++++++++--
>  builtin/merge.c            |    2 +-
>  git-am.sh                  |    2 +-
>  git-rebase--interactive.sh |    2 +-
>  6 files changed, 39 insertions(+), 6 deletions(-)
>
> diff --git a/builtin/commit.c b/builtin/commit.c
> index a2ec73d..4ba5677 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -1418,6 +1418,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
>
>                OPT_END()
>        };
> +       const char *argv_gc_auto[] = { "gc", "--check", NULL };
>
>        struct strbuf sb = STRBUF_INIT;
>        struct strbuf author_ident = STRBUF_INIT;
> @@ -1589,6 +1590,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
>                     "new_index file. Check that disk is not full or quota is\n"
>                     "not exceeded, and then \"git reset HEAD\" to recover."));
>
> +       run_command_v_opt(argv_gc_auto, RUN_GIT_CMD);
>        rerere(0);
>        run_hook(get_index_file(), "post-commit", NULL);
>        if (amend && !no_post_rewrite) {
> diff --git a/builtin/gc.c b/builtin/gc.c
> index 9b4232c..a85f71c 100644
> --- a/builtin/gc.c
> +++ b/builtin/gc.c
> @@ -171,6 +171,7 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
>  {
>        int aggressive = 0;
>        int auto_gc = 0;
> +       int check_gc = 0;
>        int quiet = 0;
>
>        struct option builtin_gc_options[] = {
> @@ -180,12 +181,16 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
>                        PARSE_OPT_OPTARG, NULL, (intptr_t)prune_expire },
>                OPT_BOOLEAN(0, "aggressive", &aggressive, "be more thorough (increased runtime)"),
>                OPT_BOOLEAN(0, "auto", &auto_gc, "enable auto-gc mode"),
> +               OPT_BOOLEAN(0, "check", &check_gc, "enable auto-gc mode"),
>                OPT_END()
>        };
>
>        if (argc == 2 && !strcmp(argv[1], "-h"))
>                usage_with_options(builtin_gc_usage, builtin_gc_options);
>
> +       if (auto_gc && check_gc)
> +               die(_("--auto and --check are incompatible"));
> +
>        argv_array_pushl(&pack_refs_cmd, "pack-refs", "--all", "--prune", NULL);
>        argv_array_pushl(&reflog, "reflog", "expire", "--all", NULL);
>        argv_array_pushl(&repack, "repack", "-d", "-l", NULL);
> @@ -211,12 +216,26 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
>        if (quiet)
>                argv_array_push(&repack, "-q");
>
> -       if (auto_gc) {
> +       if (auto_gc || check_gc) {
>                /*
>                 * Auto-gc should be least intrusive as possible.
>                 */
>                if (!need_to_gc())
>                        return 0;
> +               if (check_gc) {
> +                       struct stat st;
> +                       if (stat(git_path("gc_needed"), &st))
> +                               open(git_path("gc_needed"), O_CREAT | O_RDWR);
> +                       else {
> +                               /* do not bother users more than once a day */
> +                               if (time(NULL) - st.st_mtime < 86400)
> +                                       return 0;
> +                               utime(git_path("gc_needed"), NULL);
> +                       }
> +                       warning(_("This repository needs maintenance. "
> +                                 "Please run \"git gc\" as soon as possible."));
> +                       return 0;
> +               }
>                if (quiet)
>                        fprintf(stderr, _("Auto packing the repository for optimum performance.\n"));
>                else
> diff --git a/builtin/index-pack.c b/builtin/index-pack.c
> index 83555e5..438245c 100644
> --- a/builtin/index-pack.c
> +++ b/builtin/index-pack.c
> @@ -9,6 +9,7 @@
>  #include "progress.h"
>  #include "fsck.h"
>  #include "exec_cmd.h"
> +#include "run-command.h"
>
>  static const char index_pack_usage[] =
>  "git index-pack [-v] [-o <index-file>] [--keep | --keep=<msg>] [--verify] [--strict] (<pack-file> | --stdin [--fix-thin] [<pack-file>])";
> @@ -63,6 +64,7 @@ static int nr_resolved_deltas;
>  static int from_stdin;
>  static int strict;
>  static int verbose;
> +static int auto_gc;
>
>  static struct progress *progress;
>
> @@ -968,6 +970,10 @@ static int git_index_pack_config(const char *k, const char *v, void *cb)
>                        die("bad pack.indexversion=%"PRIu32, opts->version);
>                return 0;
>        }
> +       if (strcmp(k, "receive.autogc") == 0) {
> +               auto_gc = git_config_bool(k, v);
> +               return 0;
> +       }
>        return git_default_config(k, v, cb);
>  }
>
> @@ -1254,12 +1260,18 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix)
>        curr_index = write_idx_file(index_name, idx_objects, nr_objects, &opts, pack_sha1);
>        free(idx_objects);
>
> -       if (!verify)
> +       if (!verify) {
>                final(pack_name, curr_pack,
>                      index_name, curr_index,
>                      keep_name, keep_msg,
>                      pack_sha1);
> -       else
> +               if (auto_gc) {
> +                       const char *argv_gc_auto[] = {
> +                               "gc", "--auto", "--quiet", NULL,
> +                       };
> +                       run_command_v_opt(argv_gc_auto, RUN_GIT_CMD);
> +               }
> +       } else
>                close(input_fd);
>        free(objects);
>        free(index_name_buf);
> diff --git a/builtin/merge.c b/builtin/merge.c
> index 470fc57..8b8716e 100644
> --- a/builtin/merge.c
> +++ b/builtin/merge.c
> @@ -385,7 +385,7 @@ static void finish(struct commit *head_commit,
>                if (verbosity >= 0 && !merge_msg.len)
>                        printf(_("No merge message -- not updating HEAD\n"));
>                else {
> -                       const char *argv_gc_auto[] = { "gc", "--auto", NULL };
> +                       const char *argv_gc_auto[] = { "gc", "--check", NULL };
>                        update_ref(reflog_message.buf, "HEAD",
>                                new_head, head, 0,
>                                DIE_ON_ERR);
> diff --git a/git-am.sh b/git-am.sh
> index f8b7a0c..0f2dbc4 100755
> --- a/git-am.sh
> +++ b/git-am.sh
> @@ -902,4 +902,4 @@ if test -s "$dotest"/rewritten; then
>  fi
>
>  rm -fr "$dotest"
> -git gc --auto
> +git gc --check
> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> index 0c19b7c..89996a3 100644
> --- a/git-rebase--interactive.sh
> +++ b/git-rebase--interactive.sh
> @@ -573,7 +573,7 @@ do_next () {
>                true # we don't care if this hook failed
>        fi &&
>        rm -rf "$state_dir" &&
> -       git gc --auto &&
> +       git gc --check &&
>        warn "Successfully rebased and updated $head_name."
>
>        exit
> --
> 1.7.8.36.g69ee2
>



-- 
Duy

Re: [PATCH] Update "gc" behavior in commit, merge, am, rebase and index-pack

[Nicolas Pitre]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1179 bytes --]

On Sat, 12 May 2012, Nguyễn Thái Ngọc Duy wrote:

> A few attempts have been made in the past to change 'gc --auto' [1]
> [2]. This is another such attempt.
> 
> Commit d4bb43e (Invoke "git gc --auto" from commit, merge, am and
> rebase. - 2007-09-05) used the rule to put "gc --auto" is "where
> update-ref occurs". I would argue that this is not a good condition to
> run gc, because (at least current) gc is slow. We encourage commit
> often and rebase to make all patches in good shape and this workflow
> should not be interrupted/slowed down by random "gc --auto".
> 
> Instead, we could just inform users that "gc" should be run soon in
> commonly used commands (this patch also reinstates "gc" check in
> commit, which was lost at the sh->C conversion). [1] and [2] can annoy
> users constantly with warnings. This patch shows the warning at most
> once a day.

I agree with this.  However don't bother making this once a day.  There 
is no harm in warning every time.  Referring to the man page as git 
--auto does when it actually meets its treshold wouldn't be a bad thing 
either as incidentally that would contain the info to get rid of the 
warning.


Nicolas

Re: [PATCH] Update "gc" behavior in commit, merge, am, rebase and index-pack

[Nguyen Thai Ngoc Duy]

On Sun, May 13, 2012 at 12:36 AM, Nicolas Pitre <nico@fluxnic.net> wrote:
> On Sat, 12 May 2012, Nguyễn Thái Ngọc Duy wrote:
>
>> A few attempts have been made in the past to change 'gc --auto' [1]
>> [2]. This is another such attempt.
>>
>> Commit d4bb43e (Invoke "git gc --auto" from commit, merge, am and
>> rebase. - 2007-09-05) used the rule to put "gc --auto" is "where
>> update-ref occurs". I would argue that this is not a good condition to
>> run gc, because (at least current) gc is slow. We encourage commit
>> often and rebase to make all patches in good shape and this workflow
>> should not be interrupted/slowed down by random "gc --auto".
>>
>> Instead, we could just inform users that "gc" should be run soon in
>> commonly used commands (this patch also reinstates "gc" check in
>> commit, which was lost at the sh->C conversion). [1] and [2] can annoy
>> users constantly with warnings. This patch shows the warning at most
>> once a day.
>
> I agree with this.  However don't bother making this once a day.  There
> is no harm in warning every time.  Referring to the man page as git
> --auto does when it actually meets its treshold wouldn't be a bad thing
> either as incidentally that would contain the info to get rid of the
> warning.

I'm good with always warning too. The moment it shows up, I can just
open another terminal for "gc". Though (I guess) a new user in the
middle of the work may not want to do that as they fear "gc" may
interfere their work. Then the warning becomes a constant annoyance
(and maybe forgotten after they're used to seeing the warning).

If everybody's good with always warning, I'll make a new patch.
-- 
Duy

Re: [PATCH] Update "gc" behavior in commit, merge, am, rebase and index-pack

[Sverre Rabbelier]

On Mon, May 14, 2012 at 4:09 AM, Nguyen Thai Ngoc Duy <pclouds@gmail.com> wrote:
> I'm good with always warning too. The moment it shows up, I can just
> open another terminal for "gc". Though (I guess) a new user in the
> middle of the work may not want to do that as they fear "gc" may
> interfere their work.

If we know that it won't interfere with their work, can we just spawn
git gc as a background process?

-- 
Cheers,

Sverre Rabbelier

Re: [PATCH] Update "gc" behavior in commit, merge, am, rebase and index-pack

[Jeff King]

On Sat, May 12, 2012 at 01:36:28PM -0400, Nicolas Pitre wrote:

> > Instead, we could just inform users that "gc" should be run soon in
> > commonly used commands (this patch also reinstates "gc" check in
> > commit, which was lost at the sh->C conversion). [1] and [2] can annoy
> > users constantly with warnings. This patch shows the warning at most
> > once a day.
> 
> I agree with this.  However don't bother making this once a day.  There 
> is no harm in warning every time.

I don't mind seeing a warning every time I run "git commit" myself, but
aren't there things which run "merge" and "commit" in a loop? For
example, "git rebase -m"? Warning 20 times when 20 commits are rebased
seems excessive.

-Peff

Re: [PATCH] Update "gc" behavior in commit, merge, am, rebase and index-pack

[Jeff King]

On Sat, May 12, 2012 at 03:08:54PM +0700, Nguyen Thai Ngoc Duy wrote:

> Instead, we could just inform users that "gc" should be run soon in
> commonly used commands (this patch also reinstates "gc" check in
> commit, which was lost at the sh->C conversion). [1] and [2] can annoy
> users constantly with warnings. This patch shows the warning at most
> once a day.

Hmm. The missing "gc" from git-commit has been noticed before, but we
decided not to reinstate it[1]:

  http://thread.gmane.org/gmane.comp.version-control.git/78524/focus=78693

However, I'm not sure Junio's argument in that thread is valid:

  I had an impression that we accepted the hook which made "gc --auto"
  more expensive by forcing it to check the hook (and possibly execute
  it every time) after vetting am, svn and friends to make sure nobody
  triggered "gc --auto" once per every commit, and during that vetting
  process we noticed that "git commit" lost the "gc --auto" at the end.

The pre-auto-gc hook runs only when we need to gc (i.e., when we would
be doing something expensive, anyway). So it is still cheap to check
whether we need to gc (although if you have an option like "--check"
which does not _fix_ the situation after checking, then you may end up
running the hook repeatedly).

-Peff

[1] It also came up other times:

      http://thread.gmane.org/gmane.comp.version-control.git/101667

      http://thread.gmane.org/gmane.comp.version-control.git/114089

    but each time it seems to have been rejected on the basis of the
    first argument.

Re: [PATCH] Update "gc" behavior in commit, merge, am, rebase and index-pack

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> The pre-auto-gc hook runs only when we need to gc (i.e., when we would
> be doing something expensive, anyway). So it is still cheap to check
> whether we need to gc (although if you have an option like "--check"
> which does not _fix_ the situation after checking, then you may end up
> running the hook repeatedly).

Whether we need to gc might be cheap to check, but I think the worry about
the new hook added after commit lost auto-gc was the hook to see if the
user declines our offer to run "gc --auto" may be expensive with an extra
fork and exec.  But I am personally fine with "commit" running "gc --auto"
every time; pre-auto-gc hook is opt-in after all.

As you mentioned in your own follow-up message, however, it will make
things worse without any real benefit if it is merely "check and warn but
never gc".  Also when "gc --auto" does not make the repository better (I
vaguely recall some corner cases mentioned here in the past), repeated
invocation of it might make such a change annoying.

Re: [PATCH] Update "gc" behavior in commit, merge, am, rebase and index-pack

[Nguyen Thai Ngoc Duy]

On Mon, May 14, 2012 at 10:18 PM, Sverre Rabbelier <srabbelier@gmail.com> wrote:
> On Mon, May 14, 2012 at 4:09 AM, Nguyen Thai Ngoc Duy <pclouds@gmail.com> wrote:
>> I'm good with always warning too. The moment it shows up, I can just
>> open another terminal for "gc". Though (I guess) a new user in the
>> middle of the work may not want to do that as they fear "gc" may
>> interfere their work.
>
> If we know that it won't interfere with their work, can we just spawn
> git gc as a background process?

I thought of that for index-pack, but we would need to deal with error
messages from gc messing up with foreground command's. I don't like a
command leaving background processes (without asking me) after it
exits, so index-pack should hang on until gc finishes. Which is ok for
long time commands like index-pack, but not ok for commit, rebase...
-- 
Duy

[PATCH v2 0/5] "git gc --auto" update

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

Changes since the first patch:

 - wait time goes down to 1 hour, not too long, but also avoid flooding
 - avoid 1 fork for calling "gc --auto" if there's no actual housekeeping
 - commit learns commit.autogc config key. Scripts shoud make use of this

Nguyễn Thái Ngọc Duy (5):
  Add convenient function to do automatic garbage collection
  index-pack: perform automatic gc, share receive.gc config with
    receive-pack
  commit: reinstate "gc --auto"
  gc: add --dry-run
  Update "gc" behavior in commit, merge, am and rebase

 Documentation/config.txt   |   11 +++++++--
 Documentation/git-gc.txt   |    7 +++++-
 builtin/commit.c           |    2 +
 builtin/gc.c               |   46 +++++++++++++++++++++++++++++++++++++++++++-
 builtin/gc.h               |    9 ++++++++
 builtin/index-pack.c       |    6 +++-
 builtin/merge.c            |    8 +-----
 builtin/receive-pack.c     |   14 +-----------
 git-am.sh                  |    2 +-
 git-rebase--interactive.sh |    2 +-
 10 files changed, 80 insertions(+), 27 deletions(-)
 create mode 100644 builtin/gc.h

-- 
1.7.8.36.g69ee2

[PATCH v2 1/5] Add convenient function to do automatic garbage collection

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

This function also avoids forking most of the time by performing some
check in process.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/gc.c           |   27 +++++++++++++++++++++++++--
 builtin/gc.h           |    8 ++++++++
 builtin/merge.c        |    8 ++------
 builtin/receive-pack.c |   14 ++------------
 4 files changed, 37 insertions(+), 20 deletions(-)
 create mode 100644 builtin/gc.h

diff --git a/builtin/gc.c b/builtin/gc.c
index 9b4232c..ce60225 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -15,6 +15,7 @@
 #include "parse-options.h"
 #include "run-command.h"
 #include "argv-array.h"
+#include "gc.h"
 
 #define FAILED_RUN "failed to run %s"
 
@@ -28,6 +29,7 @@ static int aggressive_window = 250;
 static int gc_auto_threshold = 6700;
 static int gc_auto_pack_limit = 50;
 static const char *prune_expire = "2.weeks.ago";
+static int auto_gc = 1;
 
 static struct argv_array pack_refs_cmd = ARGV_ARRAY_INIT;
 static struct argv_array reflog = ARGV_ARRAY_INIT;
@@ -64,6 +66,10 @@ static int gc_config(const char *var, const char *value, void *cb)
 		}
 		return git_config_string(&prune_expire, var, value);
 	}
+	if (cb && !strcmp(var, cb)) {
+		auto_gc = git_config_bool(var, value);
+		return 0;
+	}
 	return git_default_config(var, value, cb);
 }
 
@@ -162,11 +168,24 @@ static int need_to_gc(void)
 	else if (!too_many_loose_objects())
 		return 0;
 
-	if (run_hook(NULL, "pre-auto-gc", NULL))
-		return 0;
 	return 1;
 }
 
+int gc(const char *cmd, int flags)
+{
+	const char *av[] = { "gc", "--auto", NULL, NULL };
+	int ac = 2;
+
+	git_config(gc_config, (void*)cmd);
+
+	if (!auto_gc || !need_to_gc())
+		return 0;
+
+	if (flags & GC_QUIET)
+		av[ac++] = "--quiet";
+	return run_command_v_opt(av, RUN_GIT_CMD);
+}
+
 int cmd_gc(int argc, const char **argv, const char *prefix)
 {
 	int aggressive = 0;
@@ -217,6 +236,10 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
 		 */
 		if (!need_to_gc())
 			return 0;
+
+		if (run_hook(NULL, "pre-auto-gc", NULL))
+			return 0;
+
 		if (quiet)
 			fprintf(stderr, _("Auto packing the repository for optimum performance.\n"));
 		else
diff --git a/builtin/gc.h b/builtin/gc.h
new file mode 100644
index 0000000..3482e92
--- /dev/null
+++ b/builtin/gc.h
@@ -0,0 +1,8 @@
+#ifndef GC_H
+#define GC_H
+
+#define GC_QUIET  1
+
+extern int gc(const char *cmd, int flags);
+
+#endif
diff --git a/builtin/merge.c b/builtin/merge.c
index 470fc57..940259d 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -28,6 +28,7 @@
 #include "remote.h"
 #include "fmt-merge-msg.h"
 #include "gpg-interface.h"
+#include "gc.h"
 
 #define DEFAULT_TWOHEAD (1<<0)
 #define DEFAULT_OCTOPUS (1<<1)
@@ -385,15 +386,10 @@ static void finish(struct commit *head_commit,
 		if (verbosity >= 0 && !merge_msg.len)
 			printf(_("No merge message -- not updating HEAD\n"));
 		else {
-			const char *argv_gc_auto[] = { "gc", "--auto", NULL };
 			update_ref(reflog_message.buf, "HEAD",
 				new_head, head, 0,
 				DIE_ON_ERR);
-			/*
-			 * We ignore errors in 'gc --auto', since the
-			 * user should see them.
-			 */
-			run_command_v_opt(argv_gc_auto, RUN_GIT_CMD);
+			gc(NULL, 0);
 		}
 	}
 	if (new_head && show_diffstat) {
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 0afb8b2..0c1fe25 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -12,6 +12,7 @@
 #include "string-list.h"
 #include "sha1-array.h"
 #include "connected.h"
+#include "gc.h"
 
 static const char receive_pack_usage[] = "git receive-pack <git-dir>";
 
@@ -36,7 +37,6 @@ static int use_sideband;
 static int quiet;
 static int prefer_ofs_delta = 1;
 static int auto_update_server_info;
-static int auto_gc = 1;
 static const char *head_name;
 static void *head_name_to_free;
 static int sent_capabilities;
@@ -108,11 +108,6 @@ static int receive_pack_config(const char *var, const char *value, void *cb)
 		return 0;
 	}
 
-	if (strcmp(var, "receive.autogc") == 0) {
-		auto_gc = git_config_bool(var, value);
-		return 0;
-	}
-
 	return git_default_config(var, value, cb);
 }
 
@@ -973,12 +968,7 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix)
 			report(commands, unpack_status);
 		run_receive_hook(commands, post_receive_hook, 1);
 		run_update_post_hook(commands);
-		if (auto_gc) {
-			const char *argv_gc_auto[] = {
-				"gc", "--auto", "--quiet", NULL,
-			};
-			run_command_v_opt(argv_gc_auto, RUN_GIT_CMD);
-		}
+		gc("receive.autogc", GC_QUIET);
 		if (auto_update_server_info)
 			update_server_info(0);
 	}
-- 
1.7.8.36.g69ee2

[PATCH v2 2/5] index-pack: perform automatic gc, share receive.gc config with receive-pack

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

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 Documentation/config.txt |    7 ++++---
 builtin/index-pack.c     |    6 ++++--
 2 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 915cb5a..3b6d773 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1754,9 +1754,10 @@ rebase.autosquash::
 	If set to true enable '--autosquash' option by default.
 
 receive.autogc::
-	By default, git-receive-pack will run "git-gc --auto" after
-	receiving data from git-push and updating refs.  You can stop
-	it by setting this variable to false.
+	By default, git-receive-pack and git-index-pack will run
+	"git-gc --auto" after receiving data from git-push and
+	updating refs.  You can stop it by setting this variable to
+	false.
 
 receive.fsckObjects::
 	If it is set to true, git-receive-pack will check all received
diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index dc2cfe6..4e888ea 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -10,6 +10,7 @@
 #include "fsck.h"
 #include "exec_cmd.h"
 #include "thread-utils.h"
+#include "gc.h"
 
 static const char index_pack_usage[] =
 "git index-pack [-v] [-o <index-file>] [--keep | --keep=<msg>] [--verify] [--strict] (<pack-file> | --stdin [--fix-thin] [<pack-file>])";
@@ -1463,12 +1464,13 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix)
 	curr_index = write_idx_file(index_name, idx_objects, nr_objects, &opts, pack_sha1);
 	free(idx_objects);
 
-	if (!verify)
+	if (!verify) {
 		final(pack_name, curr_pack,
 		      index_name, curr_index,
 		      keep_name, keep_msg,
 		      pack_sha1);
-	else
+		gc("receive.autogc", 0);
+	} else
 		close(input_fd);
 	free(objects);
 	free(index_name_buf);
-- 
1.7.8.36.g69ee2

[PATCH v2 3/5] commit: reinstate "gc --auto"

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

This check was lost after sh->C conversion

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 Documentation/config.txt |    4 ++++
 builtin/commit.c         |    2 ++
 2 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 3b6d773..8f5dd48 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -904,6 +904,10 @@ commit.template::
 	"`~/`" is expanded to the value of `$HOME` and "`~user/`" to the
 	specified user's home directory.
 
+commit.autogc::
+	By default, git-commit will run "git gc --auto" after each
+	commit. You can stop it by setting this variable to false.
+
 credential.helper::
 	Specify an external helper to be called when a username or
 	password credential is needed; the helper may consult external
diff --git a/builtin/commit.c b/builtin/commit.c
index a2ec73d..bf7d0aa 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -28,6 +28,7 @@
 #include "submodule.h"
 #include "gpg-interface.h"
 #include "column.h"
+#include "gc.h"
 
 static const char * const builtin_commit_usage[] = {
 	"git commit [options] [--] <filepattern>...",
@@ -1589,6 +1590,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 		     "new_index file. Check that disk is not full or quota is\n"
 		     "not exceeded, and then \"git reset HEAD\" to recover."));
 
+	gc("commit.autogc", 0);
 	rerere(0);
 	run_hook(get_index_file(), "post-commit", NULL);
 	if (amend && !no_post_rewrite) {
-- 
1.7.8.36.g69ee2

[PATCH v2 4/5] gc: add --dry-run

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

"git gc --auto --dry-run" will print instead of doing
housekeeping. This option is to be called by other commands rather
than by user and will not print more than one warning every hour. gc()
function also behaves this way.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 Documentation/git-gc.txt |    7 ++++++-
 builtin/gc.c             |   19 +++++++++++++++++++
 2 files changed, 25 insertions(+), 1 deletions(-)

diff --git a/Documentation/git-gc.txt b/Documentation/git-gc.txt
index b370b02..f69f5e1 100644
--- a/Documentation/git-gc.txt
+++ b/Documentation/git-gc.txt
@@ -9,7 +9,7 @@ git-gc - Cleanup unnecessary files and optimize the local repository
 SYNOPSIS
 --------
 [verse]
-'git gc' [--aggressive] [--auto] [--quiet] [--prune=<date> | --no-prune]
+'git gc' [--aggressive] [--auto [--dry-run]] [--quiet] [--prune=<date> | --no-prune]
 
 DESCRIPTION
 -----------
@@ -60,6 +60,11 @@ are consolidated into a single pack by using the `-A` option of
 'git repack'. Setting `gc.autopacklimit` to 0 disables
 automatic consolidation of packs.
 
+--dry-run::
+	Only show warning if housekeeping is required. The warning
+	is only shown at most once every hour even if
+	"gc --auto --dry-run" is run continuously.
+
 --prune=<date>::
 	Prune loose objects older than date (default is 2 weeks ago,
 	overridable by the config variable `gc.pruneExpire`).  This
diff --git a/builtin/gc.c b/builtin/gc.c
index ce60225..f82b9ef 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -150,6 +150,8 @@ static void add_repack_all_option(void)
 
 static int need_to_gc(void)
 {
+	struct stat st;
+
 	/*
 	 * Setting gc.auto to 0 or negative can disable the
 	 * automatic gc.
@@ -168,6 +170,13 @@ static int need_to_gc(void)
 	else if (!too_many_loose_objects())
 		return 0;
 
+	if (stat(git_path("gc_needed"), &st))
+		open(git_path("gc_needed"), O_CREAT | O_RDWR, 0644);
+	else if (time(NULL) - st.st_mtime < 3600)
+		return 0;
+	else
+		utime(git_path("gc_needed"), NULL);
+
 	return 1;
 }
 
@@ -190,6 +199,7 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
 {
 	int aggressive = 0;
 	int auto_gc = 0;
+	int check_gc = 0;
 	int quiet = 0;
 
 	struct option builtin_gc_options[] = {
@@ -199,12 +209,16 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
 			PARSE_OPT_OPTARG, NULL, (intptr_t)prune_expire },
 		OPT_BOOLEAN(0, "aggressive", &aggressive, "be more thorough (increased runtime)"),
 		OPT_BOOLEAN(0, "auto", &auto_gc, "enable auto-gc mode"),
+		OPT_BOOLEAN(0, "dry-run", &check_gc, "warn if auto gc is needed"),
 		OPT_END()
 	};
 
 	if (argc == 2 && !strcmp(argv[1], "-h"))
 		usage_with_options(builtin_gc_usage, builtin_gc_options);
 
+	if (!auto_gc && check_gc)
+		die(_("--dry-run is useless without --auto"));
+
 	argv_array_pushl(&pack_refs_cmd, "pack-refs", "--all", "--prune", NULL);
 	argv_array_pushl(&reflog, "reflog", "expire", "--all", NULL);
 	argv_array_pushl(&repack, "repack", "-d", "-l", NULL);
@@ -240,6 +254,11 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
 		if (run_hook(NULL, "pre-auto-gc", NULL))
 			return 0;
 
+		if (check_gc) {
+			warning(_("This repository needs maintenance. "
+				  "Please run \"git gc\" as soon as possible."));
+			return 0;
+		}
 		if (quiet)
 			fprintf(stderr, _("Auto packing the repository for optimum performance.\n"));
 		else
-- 
1.7.8.36.g69ee2

[PATCH v2 5/5] Update "gc" behavior in commit, merge, am and rebase

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

Commit d4bb43e (Invoke "git gc --auto" from commit, merge, am and
rebase. - 2007-09-05) used the rule to put "gc --auto" is "where
update-ref occurs". I would argue that this is not a good condition to
run gc, because (at least current) gc is slow. We encourage commit
often and rebase to make all patches in good shape and this workflow
should not be slowed down by random "gc".

Instead, we could just inform users that "gc" should be run soon in
commonly used commands (commit, merge, am and rebase). The warning is
shown once every hour to avoid constantly annoy users, or flooding the
output with warnings if a command is run repeatedly (e.g. in scripts).

Commands that are not expected to return immediately, like
receive-pack or index-pack, are more suitable for "gc --auto". These
keep "gc --auto" as before.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/commit.c           |    2 +-
 builtin/gc.c               |    4 +++-
 builtin/gc.h               |    1 +
 builtin/merge.c            |    2 +-
 git-am.sh                  |    2 +-
 git-rebase--interactive.sh |    2 +-
 6 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index bf7d0aa..bfc4b4e 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1590,7 +1590,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 		     "new_index file. Check that disk is not full or quota is\n"
 		     "not exceeded, and then \"git reset HEAD\" to recover."));
 
-	gc("commit.autogc", 0);
+	gc("commit.autogc", GC_DRYRUN);
 	rerere(0);
 	run_hook(get_index_file(), "post-commit", NULL);
 	if (amend && !no_post_rewrite) {
diff --git a/builtin/gc.c b/builtin/gc.c
index f82b9ef..bff3d9c 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -182,7 +182,7 @@ static int need_to_gc(void)
 
 int gc(const char *cmd, int flags)
 {
-	const char *av[] = { "gc", "--auto", NULL, NULL };
+	const char *av[] = { "gc", "--auto", NULL, NULL, NULL };
 	int ac = 2;
 
 	git_config(gc_config, (void*)cmd);
@@ -190,6 +190,8 @@ int gc(const char *cmd, int flags)
 	if (!auto_gc || !need_to_gc())
 		return 0;
 
+	if (flags & GC_DRYRUN)
+		av[ac++] = "--dry-run";
 	if (flags & GC_QUIET)
 		av[ac++] = "--quiet";
 	return run_command_v_opt(av, RUN_GIT_CMD);
diff --git a/builtin/gc.h b/builtin/gc.h
index 3482e92..d022944 100644
--- a/builtin/gc.h
+++ b/builtin/gc.h
@@ -2,6 +2,7 @@
 #define GC_H
 
 #define GC_QUIET  1
+#define GC_DRYRUN 2
 
 extern int gc(const char *cmd, int flags);
 
diff --git a/builtin/merge.c b/builtin/merge.c
index 940259d..a0680b0 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -389,7 +389,7 @@ static void finish(struct commit *head_commit,
 			update_ref(reflog_message.buf, "HEAD",
 				new_head, head, 0,
 				DIE_ON_ERR);
-			gc(NULL, 0);
+			gc(NULL, GC_DRYRUN);
 		}
 	}
 	if (new_head && show_diffstat) {
diff --git a/git-am.sh b/git-am.sh
index f8b7a0c..71cd2fd 100755
--- a/git-am.sh
+++ b/git-am.sh
@@ -902,4 +902,4 @@ if test -s "$dotest"/rewritten; then
 fi
 
 rm -fr "$dotest"
-git gc --auto
+git gc --auto --dry-run
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 0c19b7c..6c1c054 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -573,7 +573,7 @@ do_next () {
 		true # we don't care if this hook failed
 	fi &&
 	rm -rf "$state_dir" &&
-	git gc --auto &&
+	git gc --auto --dry-run &&
 	warn "Successfully rebased and updated $head_name."
 
 	exit
-- 
1.7.8.36.g69ee2

Re: [PATCH v2 1/5] Add convenient function to do automatic garbage collection

[Junio C Hamano]

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

> This function also avoids forking most of the time by performing some
> check in process.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ...
> @@ -64,6 +66,10 @@ static int gc_config(const char *var, const char *value, void *cb)
>  		}
>  		return git_config_string(&prune_expire, var, value);
>  	}
> +	if (cb && !strcmp(var, cb)) {
> +		auto_gc = git_config_bool(var, value);
> +		return 0;
> +	}

This does not look like "add convenient function for auto-gc" nor "avoid
forking"; it is something else that is not explained.

> +int gc(const char *cmd, int flags)

Don't call this "gc".

It is hardcoded to perform a cheaper version "gc --auto" and nothing
else, and it may not even do that when configured not to.

I do not think the first parameter to this function is "cmd"
(abbreviation of "command") either.  Isn't it a configuration key to
tell if auto-gc is allowed, or something?

Other than that, it seems to be a nice change.

Re: [PATCH v2 1/5] Add convenient function to do automatic garbage collection

[Nguyen Thai Ngoc Duy]

On Sat, May 19, 2012 at 5:37 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
>
>> This function also avoids forking most of the time by performing some
>> check in process.
>>
>> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
>> ...
>> @@ -64,6 +66,10 @@ static int gc_config(const char *var, const char *value, void *cb)
>>               }
>>               return git_config_string(&prune_expire, var, value);
>>       }
>> +     if (cb && !strcmp(var, cb)) {
>> +             auto_gc = git_config_bool(var, value);
>> +             return 0;
>> +     }
>
> This does not look like "add convenient function for auto-gc" nor "avoid
> forking"; it is something else that is not explained.

It takes care of "foo.autogc" config key in commands that allows to
suppress auto gc. But yes, commit message can use some improvement.
-- 
Duy