* [PATCH] hooks/pre-auto-gc-battery: allow gc to run on non-laptops @ 2018-02-28 4:48 Adam Borowski 2018-02-28 18:16 ` Junio C Hamano 0 siblings, 1 reply; 6+ messages in thread From: Adam Borowski @ 2018-02-28 4:48 UTC (permalink / raw) To: git, Junio C Hamano, Miklos Vajna; +Cc: Adam Borowski Desktops and servers tend to have no power sensor, thus on_ac_power returns 255 ("unknown"). If that tool returns "unknown", there's no point in querying other sources as it already queried them, and is smarter than us (can handle multiple adapters). Reported by: Xin Li <delphij@google.com> Signed-off-by: Adam Borowski <kilobyte@angband.pl> --- contrib/hooks/pre-auto-gc-battery | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contrib/hooks/pre-auto-gc-battery b/contrib/hooks/pre-auto-gc-battery index 6a2cdebdb..7ba78c4df 100755 --- a/contrib/hooks/pre-auto-gc-battery +++ b/contrib/hooks/pre-auto-gc-battery @@ -17,7 +17,7 @@ # 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 +if test -x /sbin/on_ac_power && (/sbin/on_ac_power;test $? -ne 1) then exit 0 elif test "$(cat /sys/class/power_supply/AC/online 2>/dev/null)" = 1 -- 2.16.2 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] hooks/pre-auto-gc-battery: allow gc to run on non-laptops 2018-02-28 4:48 [PATCH] hooks/pre-auto-gc-battery: allow gc to run on non-laptops Adam Borowski @ 2018-02-28 18:16 ` Junio C Hamano 2018-02-28 21:46 ` Adam Borowski 0 siblings, 1 reply; 6+ messages in thread From: Junio C Hamano @ 2018-02-28 18:16 UTC (permalink / raw) To: Adam Borowski; +Cc: git, Miklos Vajna Adam Borowski <kilobyte@angband.pl> writes: > Desktops and servers tend to have no power sensor, thus on_ac_power returns > 255 ("unknown"). > > If that tool returns "unknown", there's no point in querying other sources > as it already queried them, and is smarter than us (can handle multiple > adapters). The explanation talks about the exit status 255 being special and serves to signal "there is no point continuing, and it is OK to assume we are not on batttery", while the code says that anything but exit status 1 can be treated as such. Which is correct? > Reported by: Xin Li <delphij@google.com> > Signed-off-by: Adam Borowski <kilobyte@angband.pl> > --- > contrib/hooks/pre-auto-gc-battery | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/contrib/hooks/pre-auto-gc-battery b/contrib/hooks/pre-auto-gc-battery > index 6a2cdebdb..7ba78c4df 100755 > --- a/contrib/hooks/pre-auto-gc-battery > +++ b/contrib/hooks/pre-auto-gc-battery > @@ -17,7 +17,7 @@ > # 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 > +if test -x /sbin/on_ac_power && (/sbin/on_ac_power;test $? -ne 1) > then > exit 0 > elif test "$(cat /sys/class/power_supply/AC/online 2>/dev/null)" = 1 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] hooks/pre-auto-gc-battery: allow gc to run on non-laptops 2018-02-28 18:16 ` Junio C Hamano @ 2018-02-28 21:46 ` Adam Borowski 2018-02-28 21:57 ` Junio C Hamano 0 siblings, 1 reply; 6+ messages in thread From: Adam Borowski @ 2018-02-28 21:46 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Miklos Vajna On Wed, Feb 28, 2018 at 10:16:21AM -0800, Junio C Hamano wrote: > Adam Borowski <kilobyte@angband.pl> writes: > > > Desktops and servers tend to have no power sensor, thus on_ac_power returns > > 255 ("unknown"). > > > > If that tool returns "unknown", there's no point in querying other sources > > as it already queried them, and is smarter than us (can handle multiple > > adapters). > > The explanation talks about the exit status 255 being special and > serves to signal "there is no point continuing, and it is OK to > assume we are not on batttery", while the code says that anything > but exit status 1 can be treated as such. Which is correct? As the man page says: # EXIT STATUS # 0 (true) System is on mains power # 1 (false) System is not on mains power # 255 (false) Power status could not be determined 0 usually means a laptop on AC power, 255 is for a typical desktop. The current code can't return 2 or any other unexpected value, but if it ever does, an unknown error should probably be treated same as 255 unknown. Thus, gc should be avoided only if the return code is 1. As for the second paragraph, I meant that on_ac_power already queried all sources this hook knows about (other than /usr/bin/pmset which is OSX only[1]), thus if the answer is "unknown", continuing to query is redundant. If that's unclear, do you have some other wording in mind? Also, it's good to trust on_ac_power, as it'll get updated whenever new quirks of power management get known: I heard allegations that some boards say "USB" instead of "Mains", which should count the same for our purposes[2]. It's not reasonable to update consumers such as git instead of a single system-provided tool. One worry is that, if on_ac_power is not installed, other sources known by this hook likewise assume that unknown means battery. And for example on Debian, powermgmt-base (which is where on_ac_power lives) is no longer installed by default. This suggests this patch needs to be extended to cover the other sources as well, but let's discuss this first. Extra commits are cheap... > > Reported by: Xin Li <delphij@google.com> > > Signed-off-by: Adam Borowski <kilobyte@angband.pl> > > --- > > contrib/hooks/pre-auto-gc-battery | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/contrib/hooks/pre-auto-gc-battery b/contrib/hooks/pre-auto-gc-battery > > index 6a2cdebdb..7ba78c4df 100755 > > --- a/contrib/hooks/pre-auto-gc-battery > > +++ b/contrib/hooks/pre-auto-gc-battery > > @@ -17,7 +17,7 @@ > > # 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 > > +if test -x /sbin/on_ac_power && (/sbin/on_ac_power;test $? -ne 1) > > then > > exit 0 > > elif test "$(cat /sys/class/power_supply/AC/online 2>/dev/null)" = 1 > [1]. I don't know if there's an implementation of on_ac_power for OSX, but if there is, it is reasonable to assume it uses or emulates pmset. [2]. Technically, that's _dc_ not ac power, but as batteries use a different interface, in the vast majority of cases USB power can be considered non-rationed. You can power it from an unplugged laptop or from a powerbank, but that's no different from "mains" that come from an unplugged UPS with no or unsupported control link. -- ⢀⣴⠾⠻⢶⣦⠀ ⣾⠁⢠⠒⠀⣿⡁ A dumb species has no way to open a tuna can. ⢿⡄⠘⠷⠚⠋⠀ A smart species invents a can opener. ⠈⠳⣄⠀⠀⠀⠀ A master species delegates. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] hooks/pre-auto-gc-battery: allow gc to run on non-laptops 2018-02-28 21:46 ` Adam Borowski @ 2018-02-28 21:57 ` Junio C Hamano 2018-02-28 22:12 ` [PATCH v2] " Adam Borowski 0 siblings, 1 reply; 6+ messages in thread From: Junio C Hamano @ 2018-02-28 21:57 UTC (permalink / raw) To: Adam Borowski; +Cc: git, Miklos Vajna Adam Borowski <kilobyte@angband.pl> writes: > 0 usually means a laptop on AC power, 255 is for a typical desktop. > The current code can't return 2 or any other unexpected value, but if it > ever does, an unknown error should probably be treated same as 255 unknown. > Thus, gc should be avoided only if the return code is 1. In short, your answer to my question is "What the code does is the more correct version between the two---the log message was lying." Then please do not talk about 255 but explain why "only if it is 1" is the right thing in the log message. That would make the result consistent. > As for the second paragraph,... That paragraph reads just fine. ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v2] hooks/pre-auto-gc-battery: allow gc to run on non-laptops 2018-02-28 21:57 ` Junio C Hamano @ 2018-02-28 22:12 ` Adam Borowski 2018-02-28 22:24 ` Junio C Hamano 0 siblings, 1 reply; 6+ messages in thread From: Adam Borowski @ 2018-02-28 22:12 UTC (permalink / raw) To: Junio C Hamano, git, Miklos Vajna; +Cc: Adam Borowski Desktops and servers tend to have no power sensor, thus on_ac_power returns 255 ("unknown"). Thus, let's take any answer other than 1 ("battery") as no contraindication to run gc. If that tool returns "unknown", there's no point in querying other sources as it already queried them, and is smarter than us (can handle multiple adapters). Reported by: Xin Li <delphij@google.com> Signed-off-by: Adam Borowski <kilobyte@angband.pl> --- v2: improved commit message contrib/hooks/pre-auto-gc-battery | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contrib/hooks/pre-auto-gc-battery b/contrib/hooks/pre-auto-gc-battery index 6a2cdebdb..7ba78c4df 100755 --- a/contrib/hooks/pre-auto-gc-battery +++ b/contrib/hooks/pre-auto-gc-battery @@ -17,7 +17,7 @@ # 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 +if test -x /sbin/on_ac_power && (/sbin/on_ac_power;test $? -ne 1) then exit 0 elif test "$(cat /sys/class/power_supply/AC/online 2>/dev/null)" = 1 -- 2.16.2 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2] hooks/pre-auto-gc-battery: allow gc to run on non-laptops 2018-02-28 22:12 ` [PATCH v2] " Adam Borowski @ 2018-02-28 22:24 ` Junio C Hamano 0 siblings, 0 replies; 6+ messages in thread From: Junio C Hamano @ 2018-02-28 22:24 UTC (permalink / raw) To: Adam Borowski; +Cc: git, Miklos Vajna Adam Borowski <kilobyte@angband.pl> writes: > Desktops and servers tend to have no power sensor, thus on_ac_power returns > 255 ("unknown"). Thus, let's take any answer other than 1 ("battery") as > no contraindication to run gc. > > If that tool returns "unknown", there's no point in querying other sources > as it already queried them, and is smarter than us (can handle multiple > adapters). > > Reported by: Xin Li <delphij@google.com> > Signed-off-by: Adam Borowski <kilobyte@angband.pl> > --- > v2: improved commit message That makes the patch and the log consistent so that people who know the area can reason about it ;-) Will queue. Thanks. > contrib/hooks/pre-auto-gc-battery | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/contrib/hooks/pre-auto-gc-battery b/contrib/hooks/pre-auto-gc-battery > index 6a2cdebdb..7ba78c4df 100755 > --- a/contrib/hooks/pre-auto-gc-battery > +++ b/contrib/hooks/pre-auto-gc-battery > @@ -17,7 +17,7 @@ > # 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 > +if test -x /sbin/on_ac_power && (/sbin/on_ac_power;test $? -ne 1) > then > exit 0 > elif test "$(cat /sys/class/power_supply/AC/online 2>/dev/null)" = 1 ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-02-28 22:24 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-02-28 4:48 [PATCH] hooks/pre-auto-gc-battery: allow gc to run on non-laptops Adam Borowski 2018-02-28 18:16 ` Junio C Hamano 2018-02-28 21:46 ` Adam Borowski 2018-02-28 21:57 ` Junio C Hamano 2018-02-28 22:12 ` [PATCH v2] " Adam Borowski 2018-02-28 22:24 ` Junio C Hamano
Code repositories for project(s) associated with this public inbox https://80x24.org/mirrors/git.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).