[PATCH] git gc --auto: defer on battery

[PATCH] git gc --auto: defer on battery

[Miklos Vajna]

This patch modifies git gc --auto so that it will not always repack when
a user is on battery.

It introduces the new gc.deferonbattery configuration variable, which
defaults to true. If it's true and the user is on battery, it will not
run git gc --auto.

Signed-off-by: Miklos Vajna <vmiklos@frugalware.org>
---

Idea is from e2fsprogs, such a repack may take a lot of time and usually
you don't have infinite time when you are on battery.. :)

If the patch looks OK, just it's too late for 1.5.5, then please let me
know and I'll resend after 1.5.5.

Thanks.

 Documentation/git-gc.txt |    4 +++
 builtin-gc.c             |   49 ++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 53 insertions(+), 0 deletions(-)

diff --git a/Documentation/git-gc.txt b/Documentation/git-gc.txt
index d424a4e..7d54148 100644
--- a/Documentation/git-gc.txt
+++ b/Documentation/git-gc.txt
@@ -104,6 +104,10 @@ The optional configuration variable 'gc.pruneExpire' controls how old
 the unreferenced loose objects have to be before they are pruned.  The
 default is "2 weeks ago".
 
+The optional configuration variable 'gc.deferonbattery' determines if
+`git gc --auto` should be disabled if the system is running on battery.
+This defaults to true.
+
 See Also
 --------
 linkgit:git-prune[1]
diff --git a/builtin-gc.c b/builtin-gc.c
index 8cef36f..7beb046 100644
--- a/builtin-gc.c
+++ b/builtin-gc.c
@@ -23,6 +23,7 @@ static const char * const builtin_gc_usage[] = {
 };
 
 static int pack_refs = 1;
+static int defer_on_battery = 1;
 static int aggressive_window = -1;
 static int gc_auto_threshold = 6700;
 static int gc_auto_pack_limit = 50;
@@ -67,6 +68,10 @@ static int gc_config(const char *var, const char *value)
 		prune_expire = xstrdup(value);
 		return 0;
 	}
+	if (!strcmp(var, "gc.deferonbattery")) {
+		defer_on_battery = git_config_bool(var, value);
+		return 0;
+	}
 	return git_default_config(var, value);
 }
 
@@ -157,6 +162,45 @@ static int too_many_packs(void)
 	return gc_auto_pack_limit <= cnt;
 }
 
+static int is_on_battery(void)
+{
+	FILE *fp;
+	DIR *dir;
+	char buf[256], state[256], path[256];
+	unsigned int ac = 0;
+	struct dirent* entry;
+
+	if (!defer_on_battery)
+		return 0;
+
+	if ((fp = fopen("/proc/apm", "r"))) {
+		if (fscanf(fp, "%s %s %s %x", buf, buf, buf, &ac) != 4)
+			ac = 1;
+		fclose(fp);
+		return ac != 1;
+	}
+	if((dir = opendir("/proc/acpi/ac_adapter"))) {
+		while ((entry = readdir(dir))) {
+			if (!strcmp(".", entry->d_name) || !strcmp("..",
+						entry->d_name))
+				continue;
+			snprintf(path, 255, "/proc/acpi/ac_adapter/%s/state",
+					entry->d_name);
+			if ((fp = fopen(path, "r"))) {
+				if (fscanf(fp, "%s %s", buf, state) != 2)
+					state[0] = '\0';
+				fclose(fp);
+				if (!strncmp(state, "off-line", 8)) {
+					closedir(dir);
+					return 1;
+				}
+			}
+		}
+		closedir(dir);
+	}
+	return 0;
+}
+
 static int need_to_gc(void)
 {
 	/*
@@ -176,6 +220,11 @@ static int need_to_gc(void)
 		append_option(argv_repack, "-A", MAX_ADD);
 	else if (!too_many_loose_objects())
 		return 0;
+
+	if(is_on_battery()) {
+		fprintf(stderr, "Auto packing deferred; on battery");
+		return 0;
+	}
 	return 1;
 }
 
-- 
1.5.4.5

Re: [PATCH] git gc --auto: defer on battery

[Björn Steinbrink]

On 2008.03.31 01:14:08 +0200, Miklos Vajna wrote:
> This patch modifies git gc --auto so that it will not always repack when
> a user is on battery.
> 
> It introduces the new gc.deferonbattery configuration variable, which
> defaults to true. If it's true and the user is on battery, it will not
> run git gc --auto.
> 
> Signed-off-by: Miklos Vajna <vmiklos@frugalware.org>
> ---
> 
> Idea is from e2fsprogs, such a repack may take a lot of time and usually
> you don't have infinite time when you are on battery.. :)
> 
> If the patch looks OK, just it's too late for 1.5.5, then please let me
> know and I'll resend after 1.5.5.
> 
> Thanks.
> 
>  Documentation/git-gc.txt |    4 +++
>  builtin-gc.c             |   49 ++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 53 insertions(+), 0 deletions(-)
> 
> diff --git a/Documentation/git-gc.txt b/Documentation/git-gc.txt
> index d424a4e..7d54148 100644
> --- a/Documentation/git-gc.txt
> +++ b/Documentation/git-gc.txt
> @@ -104,6 +104,10 @@ The optional configuration variable 'gc.pruneExpire' controls how old
>  the unreferenced loose objects have to be before they are pruned.  The
>  default is "2 weeks ago".
>  
> +The optional configuration variable 'gc.deferonbattery' determines if
> +`git gc --auto` should be disabled if the system is running on battery.
> +This defaults to true.
> +
>  See Also
>  --------
>  linkgit:git-prune[1]
> diff --git a/builtin-gc.c b/builtin-gc.c
> index 8cef36f..7beb046 100644
> --- a/builtin-gc.c
> +++ b/builtin-gc.c
> @@ -23,6 +23,7 @@ static const char * const builtin_gc_usage[] = {
>  };
>  
>  static int pack_refs = 1;
> +static int defer_on_battery = 1;
>  static int aggressive_window = -1;
>  static int gc_auto_threshold = 6700;
>  static int gc_auto_pack_limit = 50;
> @@ -67,6 +68,10 @@ static int gc_config(const char *var, const char *value)
>  		prune_expire = xstrdup(value);
>  		return 0;
>  	}
> +	if (!strcmp(var, "gc.deferonbattery")) {
> +		defer_on_battery = git_config_bool(var, value);
> +		return 0;
> +	}
>  	return git_default_config(var, value);
>  }
>  
> @@ -157,6 +162,45 @@ static int too_many_packs(void)
>  	return gc_auto_pack_limit <= cnt;
>  }
>  
> +static int is_on_battery(void)
> +{
> +	FILE *fp;
> +	DIR *dir;
> +	char buf[256], state[256], path[256];
> +	unsigned int ac = 0;
> +	struct dirent* entry;
> +
> +	if (!defer_on_battery)
> +		return 0;

Hm, maybe move that check into need_to_gc instead? Seems a bit weird to
lie about the status instead of just skipping the status check.

> +
> +	if ((fp = fopen("/proc/apm", "r"))) {
> +		if (fscanf(fp, "%s %s %s %x", buf, buf, buf, &ac) != 4)
> +			ac = 1;
> +		fclose(fp);
> +		return ac != 1;
> +	}
> +	if((dir = opendir("/proc/acpi/ac_adapter"))) {
> +		while ((entry = readdir(dir))) {
> +			if (!strcmp(".", entry->d_name) || !strcmp("..",
> +						entry->d_name))
> +				continue;
> +			snprintf(path, 255, "/proc/acpi/ac_adapter/%s/state",
> +					entry->d_name);
> +			if ((fp = fopen(path, "r"))) {
> +				if (fscanf(fp, "%s %s", buf, state) != 2)
> +					state[0] = '\0';
> +				fclose(fp);
> +				if (!strncmp(state, "off-line", 8)) {
> +					closedir(dir);
> +					return 1;
> +				}
> +			}
> +		}
> +		closedir(dir);
> +	}
> +	return 0;
> +}

The /proc stuff is already deprecated IIRC, the new file to check on
Linux is /sys/class/power_supply/AC/online.

Björn

> +
>  static int need_to_gc(void)
>  {
>  	/*
> @@ -176,6 +220,11 @@ static int need_to_gc(void)
>  		append_option(argv_repack, "-A", MAX_ADD);
>  	else if (!too_many_loose_objects())
>  		return 0;
> +
> +	if(is_on_battery()) {
> +		fprintf(stderr, "Auto packing deferred; on battery");
> +		return 0;
> +	}
>  	return 1;
>  }
>  

[PATCH] git gc --auto: defer on battery

[Miklos Vajna]

This patch modifies git gc --auto so that it will not always repack when
a user is on battery.

It introduces the new gc.deferonbattery configuration variable, which
defaults to true. If it's true and the user is on battery, it will not
run git gc --auto.

Signed-off-by: Miklos Vajna <vmiklos@frugalware.org>
---

On Mon, Mar 31, 2008 at 01:26:12AM +0200, Björn Steinbrink <B.Steinbrink@gmx.de> wrote:
> Hm, maybe move that check into need_to_gc instead? Seems a bit weird
> to
> lie about the status instead of just skipping the status check.

Right, I've moved the check to need_to_gc().

> The /proc stuff is already deprecated IIRC, the new file to check on
> Linux is /sys/class/power_supply/AC/online.

And that makes the patch smaller as well. :)

Something like this?

 Documentation/git-gc.txt |    4 ++++
 builtin-gc.c             |   24 ++++++++++++++++++++++++
 2 files changed, 28 insertions(+), 0 deletions(-)

diff --git a/Documentation/git-gc.txt b/Documentation/git-gc.txt
index d424a4e..7d54148 100644
--- a/Documentation/git-gc.txt
+++ b/Documentation/git-gc.txt
@@ -104,6 +104,10 @@ The optional configuration variable 'gc.pruneExpire' controls how old
 the unreferenced loose objects have to be before they are pruned.  The
 default is "2 weeks ago".
 
+The optional configuration variable 'gc.deferonbattery' determines if
+`git gc --auto` should be disabled if the system is running on battery.
+This defaults to true.
+
 See Also
 --------
 linkgit:git-prune[1]
diff --git a/builtin-gc.c b/builtin-gc.c
index 8cef36f..512a357 100644
--- a/builtin-gc.c
+++ b/builtin-gc.c
@@ -23,6 +23,7 @@ static const char * const builtin_gc_usage[] = {
 };
 
 static int pack_refs = 1;
+static int defer_on_battery = 1;
 static int aggressive_window = -1;
 static int gc_auto_threshold = 6700;
 static int gc_auto_pack_limit = 50;
@@ -67,6 +68,10 @@ static int gc_config(const char *var, const char *value)
 		prune_expire = xstrdup(value);
 		return 0;
 	}
+	if (!strcmp(var, "gc.deferonbattery")) {
+		defer_on_battery = git_config_bool(var, value);
+		return 0;
+	}
 	return git_default_config(var, value);
 }
 
@@ -157,6 +162,20 @@ static int too_many_packs(void)
 	return gc_auto_pack_limit <= cnt;
 }
 
+static int is_on_battery(void)
+{
+	FILE *fp;
+	unsigned int state = 1;
+
+	if ((fp = fopen("/sys/class/power_supply/AC/online", "r"))) {
+		if (fscanf(fp, "%d", &state) != 1)
+			state = 1;
+		fclose(fp);
+		return state != 1;
+	}
+	return 0;
+}
+
 static int need_to_gc(void)
 {
 	/*
@@ -176,6 +195,11 @@ static int need_to_gc(void)
 		append_option(argv_repack, "-A", MAX_ADD);
 	else if (!too_many_loose_objects())
 		return 0;
+
+	if(defer_on_battery && is_on_battery()) {
+		fprintf(stderr, "Auto packing deferred; on battery");
+		return 0;
+	}
 	return 1;
 }
 
-- 
1.5.4.5

Re: [PATCH] git gc --auto: defer on battery

[Björn Steinbrink]

On 2008.03.31 01:39:16 +0200, Miklos Vajna wrote:
> This patch modifies git gc --auto so that it will not always repack when
> a user is on battery.
> 
> It introduces the new gc.deferonbattery configuration variable, which
> defaults to true. If it's true and the user is on battery, it will not
> run git gc --auto.
> 
> Signed-off-by: Miklos Vajna <vmiklos@frugalware.org>
> ---
> 
> On Mon, Mar 31, 2008 at 01:26:12AM +0200, Björn Steinbrink <B.Steinbrink@gmx.de> wrote:
> > Hm, maybe move that check into need_to_gc instead? Seems a bit weird
> > to
> > lie about the status instead of just skipping the status check.
> 
> Right, I've moved the check to need_to_gc().
> 
> > The /proc stuff is already deprecated IIRC, the new file to check on
> > Linux is /sys/class/power_supply/AC/online.
> 
> And that makes the patch smaller as well. :)

Oh, oops, I didn't meant to say that you should remove the /proc/*
checks, just that they'll probably break in the future and that the new
location needs to be added. Those running older kernels should probably
not be excluded ;-)

Björn

> Something like this?
> 
>  Documentation/git-gc.txt |    4 ++++
>  builtin-gc.c             |   24 ++++++++++++++++++++++++
>  2 files changed, 28 insertions(+), 0 deletions(-)
> 
> diff --git a/Documentation/git-gc.txt b/Documentation/git-gc.txt
> index d424a4e..7d54148 100644
> --- a/Documentation/git-gc.txt
> +++ b/Documentation/git-gc.txt
> @@ -104,6 +104,10 @@ The optional configuration variable 'gc.pruneExpire' controls how old
>  the unreferenced loose objects have to be before they are pruned.  The
>  default is "2 weeks ago".
>  
> +The optional configuration variable 'gc.deferonbattery' determines if
> +`git gc --auto` should be disabled if the system is running on battery.
> +This defaults to true.
> +
>  See Also
>  --------
>  linkgit:git-prune[1]
> diff --git a/builtin-gc.c b/builtin-gc.c
> index 8cef36f..512a357 100644
> --- a/builtin-gc.c
> +++ b/builtin-gc.c
> @@ -23,6 +23,7 @@ static const char * const builtin_gc_usage[] = {
>  };
>  
>  static int pack_refs = 1;
> +static int defer_on_battery = 1;
>  static int aggressive_window = -1;
>  static int gc_auto_threshold = 6700;
>  static int gc_auto_pack_limit = 50;
> @@ -67,6 +68,10 @@ static int gc_config(const char *var, const char *value)
>  		prune_expire = xstrdup(value);
>  		return 0;
>  	}
> +	if (!strcmp(var, "gc.deferonbattery")) {
> +		defer_on_battery = git_config_bool(var, value);
> +		return 0;
> +	}
>  	return git_default_config(var, value);
>  }
>  
> @@ -157,6 +162,20 @@ static int too_many_packs(void)
>  	return gc_auto_pack_limit <= cnt;
>  }
>  
> +static int is_on_battery(void)
> +{
> +	FILE *fp;
> +	unsigned int state = 1;
> +
> +	if ((fp = fopen("/sys/class/power_supply/AC/online", "r"))) {
> +		if (fscanf(fp, "%d", &state) != 1)
> +			state = 1;
> +		fclose(fp);
> +		return state != 1;
> +	}
> +	return 0;
> +}
> +
>  static int need_to_gc(void)
>  {
>  	/*
> @@ -176,6 +195,11 @@ static int need_to_gc(void)
>  		append_option(argv_repack, "-A", MAX_ADD);
>  	else if (!too_many_loose_objects())
>  		return 0;
> +
> +	if(defer_on_battery && is_on_battery()) {
> +		fprintf(stderr, "Auto packing deferred; on battery");
> +		return 0;
> +	}
>  	return 1;
>  }
>  

Re: [PATCH] git gc --auto: defer on battery

[Linus Torvalds]

On Mon, 31 Mar 2008, Bj?rn Steinbrink wrote:
> 
> The /proc stuff is already deprecated IIRC, the new file to check on
> Linux is /sys/class/power_supply/AC/online.

I would *seriously* suggest making this soem kind of generic callback and 
not Linux-specific. 

How about making it more akin to a pre-auto-gc "hook" - run a script 
instead of hardcoding something like this!

		Linus

Re: [PATCH] git gc --auto: defer on battery

[Björn Steinbrink]

On 2008.03.30 16:46:51 -0700, Linus Torvalds wrote:
> 
> 
> On Mon, 31 Mar 2008, Bj?rn Steinbrink wrote:
> > 
> > The /proc stuff is already deprecated IIRC, the new file to check on
> > Linux is /sys/class/power_supply/AC/online.
> 
> I would *seriously* suggest making this soem kind of generic callback and 
> not Linux-specific. 

I didn't meant to make that Linux-specific, I just wanted to mention
that the /proc stuff might break rather "soon", and that the code should
also check the the sysfs stuff.

> How about making it more akin to a pre-auto-gc "hook" - run a script 
> instead of hardcoding something like this!

Sounds nice.

Björn

Re: [PATCH] git gc --auto: defer on battery

[Junio C Hamano]

Linus Torvalds <torvalds@linux-foundation.org> writes:

> On Mon, 31 Mar 2008, Bj?rn Steinbrink wrote:
>> 
>> The /proc stuff is already deprecated IIRC, the new file to check on
>> Linux is /sys/class/power_supply/AC/online.
>
> I would *seriously* suggest making this soem kind of generic callback and 
> not Linux-specific. 
>
> How about making it more akin to a pre-auto-gc "hook" - run a script 
> instead of hardcoding something like this!

That would be a sensible approach.

We also would need to make sure that Porcelain that call "gc --auto" does
not have an assumption that auto is ultra-cheap, however, as we are
talking about potentially two fork-exec in the usual "noop" case with such
a change, but we need to do that regardless.

 * git-svn has "every 1000 commits and one at the end" which should be
   Ok.

 * git-cvsimport does "repack -a -d" every 1k commits and once more at the
   end if there are many remaining loose objects.

 * "git-rebase -i" does one at the end, which should be Ok.

 * "git commit" used to have one at the end in the scripted version, but
   seems to have lost it in C rewrite.

So I think we are Ok with an additional hook.

By the way, Linus, is your MUA UTF-8 challenged?  I see Björn's name on
the To: header mangled.

Re: [PATCH] git gc --auto: defer on battery

[Linus Torvalds]

On Sun, 30 Mar 2008, Junio C Hamano wrote:
> 
> By the way, Linus, is your MUA UTF-8 challenged?  I see Björn's name on
> the To: header mangled.

My MUA is not utf-8 challenged per se, but it doesn't seem to like Björn's 
headers.

For example, I see "しらいしななこ <nanako3@bluebottle.com>" perfectly 
correctly (Content-Type: text/plain; charset=UTF-8), but Björn's emails 
tend to be (Content-Type: text/plain; charset=iso-8859-1) but then he has 
his email address in utf-8, and that seems to be what makes my MUA 
unhappy about it.

			Linus

Re: [PATCH] git gc --auto: defer on battery

[Björn Steinbrink]

On 2008.03.31 08:02:51 -0700, Linus Torvalds wrote:
> 
> 
> On Sun, 30 Mar 2008, Junio C Hamano wrote:
> > 
> > By the way, Linus, is your MUA UTF-8 challenged?  I see Björn's name on
> > the To: header mangled.
> 
> My MUA is not utf-8 challenged per se, but it doesn't seem to like Björn's 
> headers.
> 
> For example, I see "しらいしななこ <nanako3@bluebottle.com>" perfectly 
> correctly (Content-Type: text/plain; charset=UTF-8), but Björn's emails 
> tend to be (Content-Type: text/plain; charset=iso-8859-1) but then he has 
> his email address in utf-8, and that seems to be what makes my MUA 
> unhappy about it.

Hm, that's weird. My header shows my name as iso-8859-1, same as the
body. I checked the copy that I got from the list to eliminate any weird
local-copy effects.

From:	=?iso-8859-1?Q?Bj=F6rn?= Steinbrink <B.Steinbrink@gmx.de>

So maybe your MUA is iso-8859-1 challenged instead? I'll send this one
out as UTF-8.

Björn

fetchmail (Re: [PATCH] git gc --auto: defer on battery)

[Linus Torvalds]

On Mon, 31 Mar 2008, Bj?rn Steinbrink wrote:
> 
> Hm, that's weird. My header shows my name as iso-8859-1, same as the
> body. I checked the copy that I got from the list to eliminate any weird
> local-copy effects.
> 
> From:	=?iso-8859-1?Q?Bj=F6rn?= Steinbrink <B.Steinbrink@gmx.de>
> 
> So maybe your MUA is iso-8859-1 challenged instead? I'll send this one
> out as UTF-8.

Ahhah! That's it. Not my MUA, but I'm using fetchmail, and I have copied 
my .fetchmailrc file around for years. As a result, it has 'mimedecode' 
set, because pine used to be really bad at this and obviously all my 
original BK (and later git) email scripts didn't do mime decoding either.

So what is probably happening is that my fetchmail setup dropped the 
charset information for the header (this is documented by fetchmail, so 
it's not a bug, it's just part of the rules) and just turned it into the 
raw byte sequence.

I've turned off mimedecode, can you send another email to me (in private) 
to see if not doing that just fixes things?

			Linus

[PATCH] commit: resurrect "gc --auto" at the end

[Johannes Schindelin]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

As the scripted version of git-commit did, we now call gc --auto just 
before the post-commit hook.

Any errors of gc --auto should be non-fatal, so we do not catch those; the 
user should see them anyway.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---

	Junio wrote:
	>
	>  * "git commit" used to have one [call to 'gc --auto'] at the 
	>    end in the scripted version, but seems to have lost it in C
	>    rewrite.

	How about this?

 builtin-commit.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/builtin-commit.c b/builtin-commit.c
index 660a345..bec62b2 100644
--- a/builtin-commit.c
+++ b/builtin-commit.c
@@ -863,6 +863,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 	char *nl, *p;
 	unsigned char commit_sha1[20];
 	struct ref_lock *ref_lock;
+	const char *argv_gc_auto[] = { "gc", "--auto", NULL };
 
 	git_config(git_commit_config);
 
@@ -987,6 +988,8 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 		     "not exceeded, and then \"git reset HEAD\" to recover.");
 
 	rerere();
+	/* We ignore errors in 'gc --auto', since the user should see them. */
+	run_command_v_opt(argv_gc_auto, RUN_GIT_CMD);
 	run_hook(get_index_file(), "post-commit", NULL);
 	if (!quiet)
 		print_summary(prefix, commit_sha1);
-- 
1.5.5.rc2.30.gf2056

Re: [PATCH] commit: resurrect "gc --auto" at the end

[Johannes Schindelin]

Hi,

On Wed, 2 Apr 2008, Johannes Schindelin wrote:

> From: Johannes Schindelin <johannes.schindelin@gmx.de>
> 
> As the scripted version of git-commit did, we now call gc --auto just 
> before the post-commit hook.
> 
> Any errors of gc --auto should be non-fatal, so we do not catch those; the 
> user should see them anyway.
> 
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> 
> 	Junio wrote:
> 	>
> 	>  * "git commit" used to have one [call to 'gc --auto'] at the 
> 	>    end in the scripted version, but seems to have lost it in C
> 	>    rewrite.
> 
> 	How about this?

Any news on this?

Ciao,
Dscho

Re: [PATCH] commit: resurrect "gc --auto" at the end

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> On Wed, 2 Apr 2008, Johannes Schindelin wrote:
>
>> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>> 
>> As the scripted version of git-commit did, we now call gc --auto just 
>> before the post-commit hook.
>> 
>> Any errors of gc --auto should be non-fatal, so we do not catch those; the 
>> user should see them anyway.
>> 
>> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
>> ---
>> 
>> 	Junio wrote:
>> 	>
>> 	>  * "git commit" used to have one [call to 'gc --auto'] at the 
>> 	>    end in the scripted version, but seems to have lost it in C
>> 	>    rewrite.
>> 
>> 	How about this?
>
> Any news on this?

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.

With this patch, we would again have a command that runs "gc --auto" once
per every commit, wouldn't we?  Interactive use of git-commit is fine with
it, but if people drive "git commit" from their scripts in a loop, they
would be hurt.

Having said that, perhaps the overhead of "gc --auto" hook is not such a
big deal.  I dunno.

Re: [PATCH] commit: resurrect "gc --auto" at the end

[Johannes Schindelin]

Hi,

On Wed, 14 May 2008, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > On Wed, 2 Apr 2008, Johannes Schindelin wrote:
> >
> >> From: Johannes Schindelin <johannes.schindelin@gmx.de>
> >> 
> >> As the scripted version of git-commit did, we now call gc --auto just 
> >> before the post-commit hook.
> >> 
> >> Any errors of gc --auto should be non-fatal, so we do not catch those; the 
> >> user should see them anyway.
> >> 
> >> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> >> ---
> >> 
> >> 	Junio wrote:
> >> 	>
> >> 	>  * "git commit" used to have one [call to 'gc --auto'] at the 
> >> 	>    end in the scripted version, but seems to have lost it in C
> >> 	>    rewrite.
> >> 
> >> 	How about this?
> >
> > Any news on this?
> 
> 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.

Ah yes, completely forgot.  Thanks.

Ciao,
Dscho

Re: [PATCH] commit: resurrect "gc --auto" at the end

[Holger Schurig]

> With this patch, we would again have a command that runs "gc
> --auto" once per every commit, wouldn't we?

Not sure if we have timing information. E.g. look at the 
timestamp of some file that "git gc" touched and only do this if 
it's old enought. Or look at the previous commit and only 
run "gc --auto" if this is far enought away, e.g. a couple of 
hours.

[PATCH 0/4] add pre-auto-gc hook for git-gc --auto

[Miklos Vajna]

On Sun, Mar 30, 2008 at 04:46:51PM -0700, Linus Torvalds <torvalds@linux-foundation.org> wrote:
> How about making it more akin to a pre-auto-gc "hook" - run a script
> instead of hardcoding something like this!

Something like this?

Miklos Vajna (4):
  git-gc --auto: add pre-auto-gc hook
  git-gc: add a --no-verify option to bypass the pre-auto-gc hook
  Documentation/hooks: add pre-auto-gc hook
  templates: add an example pre-auto-gc hook

 Documentation/git-gc.txt     |    4 ++++
 Documentation/hooks.txt      |   10 ++++++++++
 builtin-gc.c                 |   24 ++++++++++++++++++++++++
 templates/hooks--pre-auto-gc |   29 +++++++++++++++++++++++++++++
 4 files changed, 67 insertions(+), 0 deletions(-)
 create mode 100644 templates/hooks--pre-auto-gc

[PATCH 1/4] git-gc --auto: add pre-auto-gc hook

[Miklos Vajna]

If such a hook is available and exits with a non-zero status, then
git-gc --auto won't run.

Signed-off-by: Miklos Vajna <vmiklos@frugalware.org>
---
 builtin-gc.c |   22 ++++++++++++++++++++++
 1 files changed, 22 insertions(+), 0 deletions(-)

diff --git a/builtin-gc.c b/builtin-gc.c
index 8cef36f..acd63be 100644
--- a/builtin-gc.c
+++ b/builtin-gc.c
@@ -157,6 +157,25 @@ static int too_many_packs(void)
 	return gc_auto_pack_limit <= cnt;
 }
 
+static int run_hook()
+{
+	const char *argv[2];
+	struct child_process hook;
+
+	argv[0] = git_path("hooks/pre-auto-gc");
+	argv[1] = NULL;
+
+	if (access(argv[0], X_OK) < 0)
+		return 0;
+
+	memset(&hook, 0, sizeof(hook));
+	hook.argv = argv;
+	hook.no_stdin = 1;
+	hook.stdout_to_stderr = 1;
+
+	return run_command(&hook);
+}
+
 static int need_to_gc(void)
 {
 	/*
@@ -176,6 +195,9 @@ static int need_to_gc(void)
 		append_option(argv_repack, "-A", MAX_ADD);
 	else if (!too_many_loose_objects())
 		return 0;
+
+	if (run_hook())
+		return 0;
 	return 1;
 }
 
-- 
1.5.5.rc2.4.g283c6

[PATCH 2/4] git-gc: add a --no-verify option to bypass the pre-auto-gc hook

[Miklos Vajna]

Signed-off-by: Miklos Vajna <vmiklos@frugalware.org>
---
 Documentation/git-gc.txt |    4 ++++
 builtin-gc.c             |    4 +++-
 2 files changed, 7 insertions(+), 1 deletions(-)

diff --git a/Documentation/git-gc.txt b/Documentation/git-gc.txt
index d424a4e..396da5c 100644
--- a/Documentation/git-gc.txt
+++ b/Documentation/git-gc.txt
@@ -62,6 +62,10 @@ automatic consolidation of packs.
 --quiet::
 	Suppress all progress reports.
 
+--no-verify::
+	This option bypasses the pre-auto-gc hook.
+	See also link:hooks.html[hooks].
+
 Configuration
 -------------
 
diff --git a/builtin-gc.c b/builtin-gc.c
index acd63be..1eca6b2 100644
--- a/builtin-gc.c
+++ b/builtin-gc.c
@@ -27,6 +27,7 @@ static int aggressive_window = -1;
 static int gc_auto_threshold = 6700;
 static int gc_auto_pack_limit = 50;
 static char *prune_expire = "2.weeks.ago";
+static int no_verify;
 
 #define MAX_ADD 10
 static const char *argv_pack_refs[] = {"pack-refs", "--all", "--prune", NULL};
@@ -196,7 +197,7 @@ static int need_to_gc(void)
 	else if (!too_many_loose_objects())
 		return 0;
 
-	if (run_hook())
+	if (!no_verify && run_hook())
 		return 0;
 	return 1;
 }
@@ -214,6 +215,7 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
 		OPT_BOOLEAN(0, "aggressive", &aggressive, "be more thorough (increased runtime)"),
 		OPT_BOOLEAN(0, "auto", &auto_gc, "enable auto-gc mode"),
 		OPT_BOOLEAN('q', "quiet", &quiet, "suppress progress reports"),
+		OPT_BOOLEAN('n', "no-verify", &no_verify, "bypass pre-auto-gc hook"),
 		OPT_END()
 	};
 
-- 
1.5.5.rc2.4.g283c6

[PATCH 3/4] Documentation/hooks: add pre-auto-gc hook

[Miklos Vajna]

Signed-off-by: Miklos Vajna <vmiklos@frugalware.org>
---
 Documentation/hooks.txt |   10 ++++++++++
 1 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/Documentation/hooks.txt b/Documentation/hooks.txt
index 76b8d77..04ec352 100644
--- a/Documentation/hooks.txt
+++ b/Documentation/hooks.txt
@@ -276,3 +276,13 @@ probably enable this hook.
 Both standard output and standard error output are forwarded to
 `git-send-pack` on the other end, so you can simply `echo` messages
 for the user.
+
+pre-auto-gc
+-----------
+
+This hook is invoked by `git-gc --auto`, and can be bypassed with
+`\--no-verify` option.  It takes no parameter, and exiting with non-zero
+status from this script causes the `git-gc --auto` to abort.
+
+The default 'pre-auto-gc' hook, when enabled, defers auto repacking when
+you are on battery.
-- 
1.5.5.rc2.4.g283c6

[PATCH 4/4] templates: add an example pre-auto-gc hook

[Miklos Vajna]

It disabled git-gc --auto when you are on battery.

Signed-off-by: Miklos Vajna <vmiklos@frugalware.org>
---
 templates/hooks--pre-auto-gc |   29 +++++++++++++++++++++++++++++
 1 files changed, 29 insertions(+), 0 deletions(-)
 create mode 100644 templates/hooks--pre-auto-gc

diff --git a/templates/hooks--pre-auto-gc b/templates/hooks--pre-auto-gc
new file mode 100644
index 0000000..40c4759
--- /dev/null
+++ b/templates/hooks--pre-auto-gc
@@ -0,0 +1,29 @@
+#!/bin/sh
+#
+# An example hook script to verify if you are on battery.  Called by
+# git-gc --auto with no arguments.  The hook should exit with non-zero
+# status after issuing an appropriate message if it wants to stop the
+# auto repacking.
+#
+# To enable this hook, make this file executable.
+
+defer=0
+
+if [ -e /sys/class/power_supply/AC/online ]; then
+	if [ "`cat /sys/class/power_supply/AC/online`" = 0 ]; then
+		defer=1
+	fi
+elif [ -e /proc/acpi/ac_adapter/AC/state ]; then
+	if grep -q 'off-line' /proc/acpi/ac_adapter/AC/state; then
+		defer=1
+	fi
+elif [ -e /proc/apm ]; then
+	if grep -q '0$' /proc/apm; then
+		defer=1
+	fi
+fi
+
+if [ "$defer" = 1 ]; then
+	echo "Auto packing deferred; on battery"
+	exit 1
+fi
-- 
1.5.5.rc2.4.g283c6

Re: [PATCH 4/4] templates: add an example pre-auto-gc hook

[Brian Gernhardt]

On Mar 31, 2008, at 5:37 AM, Miklos Vajna wrote:

> +# An example hook script to verify if you are on battery.  Called by
> +# git-gc --auto with no arguments.  The hook should exit with non- 
> zero
> +# status after issuing an appropriate message if it wants to stop the
> +# auto repacking.
> +#
> +# To enable this hook, make this file executable.

You probably want to mention that this example hook is Linux-specific.

~~ Brian G.

Re: [PATCH] git gc --auto: defer on battery

[Joey Hess]

[-- Attachment #1: Type: text/plain, Size: 534 bytes --]

Linus Torvalds wrote:
> I would *seriously* suggest making this soem kind of generic callback and 
> not Linux-specific. 
> 
> How about making it more akin to a pre-auto-gc "hook" - run a script 
> instead of hardcoding something like this!

FWIW, Debian (and I assume Ubuntu also) systems have a on_ac_power
script that exits 0 or 1 accordingly. It would be a good thing to point
the hook at, or even a good reference when writing your own version of
the hook since it also supports /proc/pmu and apm.

-- 
see shy jo

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] git gc --auto: defer on battery

[Johannes Schindelin]

Hi,

On Mon, 31 Mar 2008, Miklos Vajna wrote:

> +	if ((fp = fopen("/proc/apm", "r"))) {
> +		if (fscanf(fp, "%s %s %s %x", buf, buf, buf, &ac) != 4)
> +			ac = 1;
> +		fclose(fp);
> +		return ac != 1;
> +	}

If /proc/apm could be opened, should you still try to open 
/proc/acpi/ac_adapter?

And what about system dependency?  I mean, if at all, this stuff belongs 
to compat/.  Definitely not into builtin-gc.c.  And yes, that means that 
you should not call the function is_on_battery() blindly, but _only_ if 
defer_on_battery is set.

Ciao,
Dscho
> @@ -176,6 +220,11 @@ static int need_to_gc(void)
>  		append_option(argv_repack, "-A", MAX_ADD);
>  	else if (!too_many_loose_objects())
>  		return 0;
> +
> +	if(is_on_battery()) {

Style.  As can be seen 3 lines earlier, we put a space after the "if".

Ciao,
Dscho

Re: [PATCH] git gc --auto: defer on battery

[Miklos Vajna]

[-- Attachment #1: Type: text/plain, Size: 849 bytes --]

On Mon, Mar 31, 2008 at 01:41:18AM +0200, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> And what about system dependency?  I mean, if at all, this stuff belongs 
> to compat/.  Definitely not into builtin-gc.c.  And yes, that means that 
> you should not call the function is_on_battery() blindly, but _only_ if 
> defer_on_battery is set.

Hm, should I just move it to there or should there be some kind of
check? Currently it just tries to open that file under /sys and if
fails, it just assumes we are not on battery. I think that's the
expected behaviour on systems not having a /sys filesystem.

As far as I see, compat/ is for functions which are available on some
systems but not an all ones. Obviously is_on_battery() won't be available
on any system. :)

The other issues (I hope) are fixed in the second patch.

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] git gc --auto: defer on battery

[Brandon Casey]

Miklos Vajna wrote:
> This patch modifies git gc --auto so that it will not always repack when
> a user is on battery.
> 
> It introduces the new gc.deferonbattery configuration variable,

Shouldn't the config option have 'auto' in the name? Or in some way convey
that this is _only_ about deferring automatic gc'ing?

-brandon

Re: [PATCH] git gc --auto: defer on battery

[Miklos Vajna]

[-- Attachment #1: Type: text/plain, Size: 564 bytes --]

On Mon, Mar 31, 2008 at 11:24:22AM -0500, Brandon Casey <casey@nrlssc.navy.mil> wrote:
> Miklos Vajna wrote:
> > This patch modifies git gc --auto so that it will not always repack when
> > a user is on battery.
> > 
> > It introduces the new gc.deferonbattery configuration variable,
> 
> Shouldn't the config option have 'auto' in the name? Or in some way convey
> that this is _only_ about deferring automatic gc'ing?

That makes sense. Though this patch isn't OK, see my other patch series
in this thread (the pre-auto-gc hook has no config name).

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH] git gc --auto: defer on battery

[Brandon Casey]

Miklos Vajna wrote:
> On Mon, Mar 31, 2008 at 11:24:22AM -0500, Brandon Casey <casey@nrlssc.navy.mil> wrote:
>> Miklos Vajna wrote:
>>> This patch modifies git gc --auto so that it will not always repack when
>>> a user is on battery.
>>>
>>> It introduces the new gc.deferonbattery configuration variable,
>> Shouldn't the config option have 'auto' in the name? Or in some way convey
>> that this is _only_ about deferring automatic gc'ing?
> 
> That makes sense. Though this patch isn't OK, see my other patch series
> in this thread (the pre-auto-gc hook has no config name).

Ah, yes I didn't look closely at the revised series.

-brandon