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

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

[Junio C Hamano]

Miklos Vajna <vmiklos@frugalware.org> writes:

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

Hmm.  I wonder if the hook wants any parameters, even though I do not
think of any offhand.

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

I understand no_stdin, but is there a reason to do stdout_to_stderr?

> +	return run_command(&hook);
> +}

Don't we want to distinguish between the case where start_command()
failed, wait_or_whine() failed on waitpid(), the command was killed with
signal, or the command actually ran correctly and decided that you should
not run "git gc --auto" by exiting non-zero?

I think it is prudent to refrain from running "git gc --auto" in any of
the failure cases I listed above, but shouldn't the cases other than the
last one at least issue a warning?

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

[Miklos Vajna]

On Mon, Mar 31, 2008 at 09:51:12PM -0700, Junio C Hamano <gitster@pobox.com> wrote:
> > +   return run_command(&hook);
> > +}
>
> Don't we want to distinguish between the case where start_command()
> failed, wait_or_whine() failed on waitpid(), the command was killed
> with
> signal, or the command actually ran correctly and decided that you
> should
> not run "git gc --auto" by exiting non-zero?
>
> I think it is prudent to refrain from running "git gc --auto" in any
> of
> the failure cases I listed above, but shouldn't the cases other than
> the
> last one at least issue a warning?

Ok, there are 3 cases here to handle. When wait_or_whine() fails on
waitpid() it already prints an error, so that's already handled. I've
added two warnings for the other 2 cases.

Other changes:

- try to use on_ac_power when it's available, as suggested by Joey Hess

- mention in the commend of the example pre-auto-gc hook that it's
  Linux-specific, as suggested by Brian Gernhardt

- removed mentioning what the default hook does from hooks.txt, as it's
  an example and it's under contrib/

- moved the battery example to contrib/ and added a minimal example to
  templates/

- removed unnecessary stdout_to_stderr from builtin-gc.c::run_hook()

- removed unnecessary --no-verify option

I hope I haven't missed anything you suggested.

Miklos Vajna (4):
  git-gc --auto: add pre-auto-gc hook
  Documentation/hooks: add pre-auto-gc hook
  templates: add an example pre-auto-gc hook
  contrib/hooks: add an example pre-auto-gc hook

 Documentation/hooks.txt           |    7 +++++
 builtin-gc.c                      |   30 ++++++++++++++++++++++++
 contrib/hooks/pre-auto-gc-battery |   45 +++++++++++++++++++++++++++++++++++++
 templates/hooks--pre-auto-gc      |    9 +++++++
 4 files changed, 91 insertions(+), 0 deletions(-)
 create mode 100644 contrib/hooks/pre-auto-gc-battery
 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 |   30 ++++++++++++++++++++++++++++++
 1 files changed, 30 insertions(+), 0 deletions(-)

diff --git a/builtin-gc.c b/builtin-gc.c
index 8cef36f..6561639 100644
--- a/builtin-gc.c
+++ b/builtin-gc.c
@@ -157,6 +157,33 @@ static int too_many_packs(void)
 	return gc_auto_pack_limit <= cnt;
 }
 
+static int run_hook()
+{
+	const char *argv[2];
+	struct child_process hook;
+	int ret;
+
+	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;
+
+	ret = start_command(&hook);
+	if (ret) {
+		warning("Could not spawn %s", argv[0]);
+		return ret;
+	}
+	ret = finish_command(&hook);
+	if (ret == -ERR_RUN_COMMAND_WAITPID_SIGNAL)
+		warning("%s exited due to uncaught signal", argv[0]);
+	return ret;
+}
+
 static int need_to_gc(void)
 {
 	/*
@@ -176,6 +203,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.gec07.dirty

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

[Miklos Vajna]

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

diff --git a/Documentation/hooks.txt b/Documentation/hooks.txt
index 76b8d77..44fbe58 100644
--- a/Documentation/hooks.txt
+++ b/Documentation/hooks.txt
@@ -276,3 +276,10 @@ 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`. It takes no parameter, and
+exiting with non-zero status from this script causes the `git-gc --auto`
+to abort.
-- 
1.5.5.rc2.4.gec07.dirty

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

[Miklos Vajna]

it's empty by default.

Signed-off-by: Miklos Vajna <vmiklos@frugalware.org>
---
 templates/hooks--pre-auto-gc |    9 +++++++++
 1 files changed, 9 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..8a9bf37
--- /dev/null
+++ b/templates/hooks--pre-auto-gc
@@ -0,0 +1,9 @@
+#!/bin/sh
+#
+# An example hook that is 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.
+
+: Nothing
-- 
1.5.5.rc2.4.gec07.dirty

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

[Miklos Vajna]

It disables git-gc --auto when you are running Linux and you are on
battery.

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

diff --git a/contrib/hooks/pre-auto-gc-battery b/contrib/hooks/pre-auto-gc-battery
new file mode 100644
index 0000000..4077111
--- /dev/null
+++ b/contrib/hooks/pre-auto-gc-battery
@@ -0,0 +1,45 @@
+#!/bin/sh
+#
+# An example hook script to verify if you are on battery, in case you
+# are running Linux. 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.
+#
+# This hook is stored in the contrib/hooks directory. Your distribution
+# will have put this somewhere standard. You should make this script
+# executable then link to it in the repository you would like to use it
+# in.
+#
+# For example, if the hook is stored in
+# /usr/share/git-core/contrib/hooks/pre-auto-gc-battery:
+#
+# chmod a+x pre-auto-gc-battery
+# cd /path/to/your/repository.git
+# ln -sf /usr/share/git-core/contrib/hooks/pre-auto-gc-battery \
+#	hooks/pre-auto-gc-battery
+
+defer=0
+
+if [ -e /sbin/on_ac_power ]; then
+	/sbin/on_ac_power
+	if [ $? = 1 ]; then
+		defer = 1
+	fi
+elif [ -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.gec07.dirty

Re: [PATCH 0/4] add pre-auto-gc hook for git-gc --auto (try2)

[Junio C Hamano]

Miklos Vajna <vmiklos@frugalware.org> writes:

> - removed unnecessary stdout_to_stderr from builtin-gc.c::run_hook()

Wait, a, minute, please.  Not so fast.  I did not say "I think this is
wrong, please fix it".

I only asked why it is so.

Care to explain why it was thought to be necessary, and why you now think
it is unnecessary?

This comment is not just for you, but when I ask why it is so, I expect
that the contributor would do the homework and present an argument to
defend what was submit (or "it was wrong for such and such reasons so
let's do it differently like this because this is better for such and such
reasons").  The original contributor has thought about the issues much
longer and deeper than I, who only just look at the email and kibitz,
after the contribution was perfected into a patch form.  There must have
been a lot more thinking than what can be seen in the submitted material
in the contributor's head.  I want to see that in the open.  Please do not
change your mind and retract your change without explaining why.

The argument to respond to such "why is it so?" question would perhaps
look like this for this case.

(begin example #1)

Well, I just cut and pasted from existing code.  Because all these hooks
redirect their stdout to stderr, I think it is consistent to do so with
the new hook.

(example #1 ends here)

(begin example #2)

"git grep -n -e stdout_to_stderr" followed by "git blame" reveals the
following pattern:

 - builtin-checkout.c:41
   782c2d6 (Build in checkout, 2008-02-07)

 - builtin-commit:382
   f5bbc32 (Port git commit to C., 2007-11-08)

 - help.c:62
   6494998 (help: add "man.viewer" config var to use "woman" or "konqueror", 2008-03-07)

 - receive-pack.c:120,157
   f43cd49 (Change {pre,post}-receive hooks to use stdin, 2007-03-10)
   1d9e8b5 (Split back out update_hook handling in receive-pack, 2007-03-10)

 - transport.c:165,218,297
   cd547b4 (fetch/push: readd rsync support, 2007-10-01)

I think all of these are the result of cut&paste from the original one in
receive-pack.c's hook handling.

The ones in the protocol handler may not write to the standard output,
because the other end does not expect random diagnostic string in the
protocol message at the point these hooks are called.  But the other ones
could go either way.

However, because existing hooks all show their script output to stderr, it
would make sense for this new pre-auto-gc hook to do so as well from the
consistency viewpoint.  So use of stdout_to_stderr is the right thing to
do here.

(example #2 ends here)

Re: [PATCH 0/4] add pre-auto-gc hook for git-gc --auto (try2)

[Miklos Vajna]

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

On Tue, Apr 01, 2008 at 04:18:01PM -0700, Junio C Hamano <gitster@pobox.com> wrote:
> > - removed unnecessary stdout_to_stderr from builtin-gc.c::run_hook()
> 
> Wait, a, minute, please.  Not so fast.  I did not say "I think this is
> wrong, please fix it".
> 
> I only asked why it is so.
> 
> Care to explain why it was thought to be necessary, and why you now think
> it is unnecessary?

Ok, let me explain. I think it would be logical to put it to stderr, as
the other gc --auto messages are there, as well. See the fprintf() in
cmd_gc().

Though, I don't think it's that important, so I thought if you think
it's unnecessary, I would not argue for it.

So may I put it back? :)

Also, is the other parts of the series looks correct?

Thanks

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

Re: [PATCH 0/4] add pre-auto-gc hook for git-gc --auto (try2)

[Junio C Hamano]

Miklos Vajna <vmiklos@frugalware.org> writes:

> Ok, let me explain. I think it would be logical to put it to stderr, as
> the other gc --auto messages are there, as well. See the fprintf() in
> cmd_gc().
>
> Though, I don't think it's that important, so I thought if you think
> it's unnecessary, I would not argue for it.

As I made it clear already, I do _not_ think when I kibitz.  *You* think,
convinced others will say "Yeah, that's a good idea", I get convinced and
the patch is applied.

> So may I put it back? :)

I agree that it makes sense to send the output to standard error for
consistency.  Many existing hooks called from scripted versions of tools
seem to contaminate the standard output, though.  That would be a good
post 1.5.5 clean-up, perhaps.

> Also, is the other parts of the series looks correct?

I do not think we would want empty templates/hooks--pre-gc-auto file.

+#!/bin/sh
+#
+# An example hook script to verify if you are on battery, in case you
+# are running Linux. 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.
+#
+# This hook is stored in the contrib/hooks directory. Your distribution
+# will have put this somewhere standard. You should make this script

s/will/may/; s/standard/else/; s/You/If you want to use this hook, you/

+# executable then link to it in the repository you would like to use it
+# in.
+#
+# For example, if the hook is stored in
+# /usr/share/git-core/contrib/hooks/pre-auto-gc-battery:
+#
+# chmod a+x pre-auto-gc-battery
+# cd /path/to/your/repository.git
+# ln -sf /usr/share/git-core/contrib/hooks/pre-auto-gc-battery \
+#	hooks/pre-auto-gc-battery

s|\(/pre-auto-gc\)-battery|\1|;

+defer=0
+
+if [ -e /sbin/on_ac_power ]; then
+	/sbin/on_ac_power
+	if [ $? = 1 ]; then
+		defer = 1
+	fi

        if test -x /sbin/on_ac_power && /sbin/on_ac_power
        then
                exit 0

Variable assignment in Bourne shell does not allow SP around '='.

I suspect it would be cleaner to invert the logic and "exit 0" when you
know you are on AC, and fall through and err on the safer side
(i.e. decline) when you cannot tell for certain.  You can lose the
variable defer that way.

+elif [ -e /sys/class/power_supply/AC/online ]; then
+	if [ "`cat /sys/class/power_supply/AC/online`" = 0 ]; then
+		defer=1
+	fi

	elif test "$(cat /sys/class/power_supply/AC/online 2>/dev/null)" = 1
	then
        	exit 0

No need to have two separate tests.  I do not know if "= 1" is correct,
though.

+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 grep -q 'on-line' /proc/acpi/ac_adapter/AC/state 2>/dev/null
        then
        	exit 0

Likewise.

+elif [ -e /proc/apm ]; then
+	if grep -q '0$' /proc/apm; then
+		defer=1
+	fi
+fi

	elif grep -q '0x01$' /proc/apm 2>/dev/null
        then
        	exit 0

Likewise.

Re: [PATCH 0/4] add pre-auto-gc hook for git-gc --auto (try2)

[Miklos Vajna]

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

On Tue, Apr 01, 2008 at 09:02:15PM -0700, Junio C Hamano <gitster@pobox.com> wrote:
> > So may I put it back? :)
> 
> I agree that it makes sense to send the output to standard error for
> consistency.  Many existing hooks called from scripted versions of tools
> seem to contaminate the standard output, though.  That would be a good
> post 1.5.5 clean-up, perhaps.

Ok, I will put it back.

> > Also, is the other parts of the series looks correct?
> 
> I do not think we would want empty templates/hooks--pre-gc-auto file.

I did it this way as we have such an empty post-commit hook as well, so
I added it for consistency. Though it's true that for example
post-applypatch doesn't have such an empty hook under templates either.

What is your opinion here?

Possibilities I see:

1) Just don't add such an empty template for pre-auto-gc.

2) Remove post-commit as well.

3) Add missing empty templates, like post-applypatch (maybe there are
more, I haven't did a complete research).

I've did the rest of the modifications locally as you suggested just I
don't want to spam the list with the series before the empty templates
question is not clear to me :)

Thanks

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

Re: [PATCH 0/4] add pre-auto-gc hook for git-gc --auto (try2)

[Junio C Hamano]

Miklos Vajna <vmiklos@frugalware.org> writes:

> What is your opinion here?
>
> 1) Just don't add such an empty template for pre-auto-gc.

[PATCH 0/3] add pre-auto-gc hook for git-gc --auto (try3)

[Miklos Vajna]

On Wed, Apr 02, 2008 at 12:07:46PM -0700, Junio C Hamano <gitster@pobox.com> wrote:
> > What is your opinion here?
> >
> > 1) Just don't add such an empty template for pre-auto-gc.

Ok, here it is. Ah and I forgot to mention in my previous mail that yes,
checking for '1' in /sys/class/power_supply/AC/online is right, just
checked.

Miklos Vajna (3):
  git-gc --auto: add pre-auto-gc hook
  Documentation/hooks: add pre-auto-gc hook
  contrib/hooks: add an example pre-auto-gc hook

 Documentation/hooks.txt           |    7 ++++++
 builtin-gc.c                      |   31 +++++++++++++++++++++++++++++
 contrib/hooks/pre-auto-gc-battery |   39 +++++++++++++++++++++++++++++++++++++
 3 files changed, 77 insertions(+), 0 deletions(-)
 create mode 100644 contrib/hooks/pre-auto-gc-battery

[PATCH 1/3] 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 |   31 +++++++++++++++++++++++++++++++
 1 files changed, 31 insertions(+), 0 deletions(-)

diff --git a/builtin-gc.c b/builtin-gc.c
index 8cef36f..9dc7a37 100644
--- a/builtin-gc.c
+++ b/builtin-gc.c
@@ -157,6 +157,34 @@ static int too_many_packs(void)
 	return gc_auto_pack_limit <= cnt;
 }
 
+static int run_hook()
+{
+	const char *argv[2];
+	struct child_process hook;
+	int ret;
+
+	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;
+
+	ret = start_command(&hook);
+	if (ret) {
+		warning("Could not spawn %s", argv[0]);
+		return ret;
+	}
+	ret = finish_command(&hook);
+	if (ret == -ERR_RUN_COMMAND_WAITPID_SIGNAL)
+		warning("%s exited due to uncaught signal", argv[0]);
+	return ret;
+}
+
 static int need_to_gc(void)
 {
 	/*
@@ -176,6 +204,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.gec07.dirty

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

[Miklos Vajna]

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

diff --git a/Documentation/hooks.txt b/Documentation/hooks.txt
index 76b8d77..44fbe58 100644
--- a/Documentation/hooks.txt
+++ b/Documentation/hooks.txt
@@ -276,3 +276,10 @@ 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`. It takes no parameter, and
+exiting with non-zero status from this script causes the `git-gc --auto`
+to abort.
-- 
1.5.5.rc2.4.gec07.dirty

[PATCH 3/3] contrib/hooks: add an example pre-auto-gc hook

[Miklos Vajna]

It disables git-gc --auto when you are running Linux and you are on
battery.

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

diff --git a/contrib/hooks/pre-auto-gc-battery b/contrib/hooks/pre-auto-gc-battery
new file mode 100644
index 0000000..0084a1f
--- /dev/null
+++ b/contrib/hooks/pre-auto-gc-battery
@@ -0,0 +1,39 @@
+#!/bin/sh
+#
+# An example hook script to verify if you are on battery, in case you
+# are running Linux. 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.
+#
+# This hook is stored in the contrib/hooks directory. Your distribution
+# may have put this somewhere else. If you want to use this hook, you
+# should make this script executable then link to it in the repository
+# you would like to use it in.
+#
+# For example, if the hook is stored in
+# /usr/share/git-core/contrib/hooks/pre-auto-gc-battery:
+#
+# chmod a+x pre-auto-gc-battery
+# cd /path/to/your/repository.git
+# ln -sf /usr/share/git-core/contrib/hooks/pre-auto-gc-battery \
+#	hooks/pre-auto-gc
+
+if test -x /sbin/on_ac_power && /sbin/on_ac_power
+then
+	/sbin/on_ac_power
+	if [ $? = 1 ]; then
+		exit 0
+	fi
+elif test "$(cat /sys/class/power_supply/AC/online 2>/dev/null)" = 1
+then
+	exit 0
+elif grep -q 'on-line' /proc/acpi/ac_adapter/AC/state 2>/dev/null
+then
+	exit 0
+elif grep -q '0x01$' /proc/apm 2>/dev/null
+then
+	exit 0
+fi
+
+echo "Auto packing deferred; on battery"
+exit 1
-- 
1.5.5.rc2.4.gec07.dirty

Re: [PATCH 3/3] contrib/hooks: add an example pre-auto-gc hook

[Junio C Hamano]

Miklos Vajna <vmiklos@frugalware.org> writes:

> +if test -x /sbin/on_ac_power && /sbin/on_ac_power
> +then
> +	/sbin/on_ac_power
> +	if [ $? = 1 ]; then
> +		exit 0
> +	fi

Shouldn't the above be:

	if test -x /sbin/on_ac_power && /sbin/on_ac_power
        then
        	exit 0

That is, you checked if you have the command to check on-ac-power-ness,
and the command says yes, so you happily let "gc --auto" do its thing.

[PATCH 3/3] contrib/hooks: add an example pre-auto-gc hook

[Miklos Vajna]

It disables git-gc --auto when you are running Linux and you are on
battery.

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

On Wed, Apr 02, 2008 at 12:49:32PM -0700, Junio C Hamano <gitster@pobox.com> wrote:
> Miklos Vajna <vmiklos@frugalware.org> writes:
>
> > +if test -x /sbin/on_ac_power && /sbin/on_ac_power
> > +then
> > +   /sbin/on_ac_power
> > +   if [ $? = 1 ]; then
> > +           exit 0
> > +   fi
>
> Shouldn't the above be:
>
>       if test -x /sbin/on_ac_power && /sbin/on_ac_power
>         then
>               exit 0
>
> That is, you checked if you have the command to check
> on-ac-power-ness,
> and the command says yes, so you happily let "gc --auto" do its thing.

Hm, I must be crazy when I wrote this. It has three exit codes:

0 if on AC power
1 if not on AC power
255 if can't tell

So IMHO the right think to do is to check if the return value is != 1,
and if that's true, then do an exit 0.

Here is try4.

 contrib/hooks/pre-auto-gc-battery |   40 +++++++++++++++++++++++++++++++++++++
 1 files changed, 40 insertions(+), 0 deletions(-)
 create mode 100644 contrib/hooks/pre-auto-gc-battery

diff --git a/contrib/hooks/pre-auto-gc-battery b/contrib/hooks/pre-auto-gc-battery
new file mode 100644
index 0000000..c6af057
--- /dev/null
+++ b/contrib/hooks/pre-auto-gc-battery
@@ -0,0 +1,40 @@
+#!/bin/sh
+#
+# An example hook script to verify if you are on battery, in case you
+# are running Linux. 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.
+#
+# This hook is stored in the contrib/hooks directory. Your distribution
+# may have put this somewhere else. If you want to use this hook, you
+# should make this script executable then link to it in the repository
+# you would like to use it in.
+#
+# For example, if the hook is stored in
+# /usr/share/git-core/contrib/hooks/pre-auto-gc-battery:
+#
+# chmod a+x pre-auto-gc-battery
+# cd /path/to/your/repository.git
+# ln -sf /usr/share/git-core/contrib/hooks/pre-auto-gc-battery \
+#	hooks/pre-auto-gc
+
+if test -x /sbin/on_ac_power
+then
+	/sbin/on_ac_power
+	if test $? -ne 1
+	then
+		exit 0
+	fi
+elif test "$(cat /sys/class/power_supply/AC/online 2>/dev/null)" = 1
+then
+	exit 0
+elif grep -q 'on-line' /proc/acpi/ac_adapter/AC/state 2>/dev/null
+then
+	exit 0
+elif grep -q '0x01$' /proc/apm 2>/dev/null
+then
+	exit 0
+fi
+
+echo "Auto packing deferred; on battery"
+exit 1
-- 
1.5.5.rc2.4.gec07.dirty

Re: [PATCH 3/3] contrib/hooks: add an example pre-auto-gc hook

[Junio C Hamano]

Miklos Vajna <vmiklos@frugalware.org> writes:

> Hm, I must be crazy when I wrote this. It has three exit codes:
>
> 0 if on AC power
> 1 if not on AC power
> 255 if can't tell
>
> So IMHO the right think to do is to check if the return value is != 1,
> and if that's true, then do an exit 0.

If it is "can't tell", then it is "can't tell".  Don't you want to err on
the safe side?

Re: [PATCH 3/3] contrib/hooks: add an example pre-auto-gc hook

[Miklos Vajna]

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

On Wed, Apr 02, 2008 at 01:34:27PM -0700, Junio C Hamano <gitster@pobox.com> wrote:
> If it is "can't tell", then it is "can't tell".  Don't you want to err on
> the safe side?

First, I think "can't tell" almost never means "on battery" these days,
since the necessary kernel support is enabled by the distro's hotplug
scripts.

Second, that's what we do in case we fail to read for example
/sys/class/power_supply/AC/online: just silently redirect the error to
/dev/null and assume we are not on battery.

I would say it does not worth doing so, but feel free to kick me if it's
a must. :)

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

tests for pre-auto-gc hook (WAS: Re: [PATCH 3/3] contrib/hooks: add an example pre-auto-gc hook)

[Miklos Vajna]

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

Hi,

I wanted to create a test like t7503-pre-commit-hook.sh for pre-auto-gc
but actually I'm not sure how to trigger git gc --auto to do something.

Here is what I managed to do so far:

----
git init
git config gc.auto 1
for i in `seq 1 500`; do echo $i >file; git add file; done
git commit -m init
----

After this, 'git gc --auto' seem to print the 'Auto packing your
repository for optimum performance.' message, but the return code is
still 0, just like if it did not do anything.

So all what I found is that the .git/objects/pack/ directory is empty in
a new repo, and if git gc --auto did something, it won't be empty but:

1) this isn't the proper way I'm sure.

2) if I want to do it from a test, then I would need to create the repo
from scratch again and again, which is quite dirty.

Does anyone have better ideas?

Thanks

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

Re: tests for pre-auto-gc hook

[Johannes Sixt]

Miklos Vajna schrieb:
> I wanted to create a test like t7503-pre-commit-hook.sh for pre-auto-gc
> but actually I'm not sure how to trigger git gc --auto to do something.
> 
> Here is what I managed to do so far:
> 
> ----
> git init
> git config gc.auto 1
> for i in `seq 1 500`; do echo $i >file; git add file; done
> git commit -m init
> ----

Avoid 'seq'; it is not available everywhere. To avoid spawning 500
processes, you could create a pack file from the files using fastimport,
then unpack that for each test case. But you must move the pack out of
.git/objects/pack before unpacking, otherwise it won't unpack anything.

-- Hannes