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

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 2, 2008, 04:02 UTC
Message-ID: <7vwsngaoqg.fsf@gitster.siamese.dyndns.org>
In-Reply-To: <20080402011447.GO3264@genesis.frugalware.org>

Miklos Vajna <vmiklos@frugalware.org> writes:

Show 6 quoted lines

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