{"thread":{"id":"47942","subject":"[PATCH] hooks/pre-auto-gc-battery: allow gc to run on non-laptops","startedAt":"2018-02-28T04:48:44Z","lastAt":"2018-02-28T22:24:39Z","messageCount":6,"participants":["Adam Borowski","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"340533","messageId":"20180228044807.1000-1-kilobyte@angband.pl","threadId":"47942","inReplyTo":null,"subject":"[PATCH] hooks/pre-auto-gc-battery: allow gc to run on non-laptops","fromName":"Adam Borowski","fromEmail":"kilobyte@angband.pl","sentAt":"2018-02-28T04:48:07Z","receivedAt":"2018-02-28T04:48:44Z","isPatch":true,"sender":{"key":"kilobyte@angband.pl","avatar":"https://avatars.githubusercontent.com/u/48801?v=4"},"body":"Desktops and servers tend to have no power sensor, thus on_ac_power returns\n255 (\"unknown\").\n\nIf that tool returns \"unknown\", there's no point in querying other sources\nas it already queried them, and is smarter than us (can handle multiple\nadapters).\n\nReported by: Xin Li \nSigned-off-by: Adam Borowski \n---\n contrib/hooks/pre-auto-gc-battery | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/contrib/hooks/pre-auto-gc-battery b/contrib/hooks/pre-auto-gc-battery\nindex 6a2cdebdb..7ba78c4df 100755\n--- a/contrib/hooks/pre-auto-gc-battery\n+++ b/contrib/hooks/pre-auto-gc-battery\n@@ -17,7 +17,7 @@\n # ln -sf /usr/share/git-core/contrib/hooks/pre-auto-gc-battery \\\n #\thooks/pre-auto-gc\n \n-if test -x /sbin/on_ac_power && /sbin/on_ac_power\n+if test -x /sbin/on_ac_power && (/sbin/on_ac_power;test $? -ne 1)\n then\n \texit 0\n elif test \"$(cat /sys/class/power_supply/AC/online 2>/dev/null)\" = 1\n-- \n2.16.2\n\n"},{"id":"340588","messageId":"xmqqpo4pkmiy.fsf@gitster-ct.c.googlers.com","threadId":"47942","inReplyTo":"20180228044807.1000-1-kilobyte@angband.pl","subject":"Re: [PATCH] hooks/pre-auto-gc-battery: allow gc to run on non-laptops","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-02-28T18:16:21Z","receivedAt":"2018-02-28T18:17:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Adam Borowski writes:\n\n> Desktops and servers tend to have no power sensor, thus on_ac_power returns\n> 255 (\"unknown\").\n>\n> If that tool returns \"unknown\", there's no point in querying other sources\n> as it already queried them, and is smarter than us (can handle multiple\n> adapters).\n\nThe explanation talks about the exit status 255 being special and\nserves to signal \"there is no point continuing, and it is OK to\nassume we are not on batttery\", while the code says that anything\nbut exit status 1 can be treated as such. Which is correct?\n\n> Reported by: Xin Li \n> Signed-off-by: Adam Borowski \n> ---\n> contrib/hooks/pre-auto-gc-battery | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/contrib/hooks/pre-auto-gc-battery b/contrib/hooks/pre-auto-gc-battery\n> index 6a2cdebdb..7ba78c4df 100755\n> --- a/contrib/hooks/pre-auto-gc-battery\n> +++ b/contrib/hooks/pre-auto-gc-battery\n> @@ -17,7 +17,7 @@\n> # ln -sf /usr/share/git-core/contrib/hooks/pre-auto-gc-battery \\\n> #\thooks/pre-auto-gc\n> \n> -if test -x /sbin/on_ac_power && /sbin/on_ac_power\n> +if test -x /sbin/on_ac_power && (/sbin/on_ac_power;test $? -ne 1)\n> then\n> \texit 0\n> elif test \"$(cat /sys/class/power_supply/AC/online 2>/dev/null)\" = 1\n"},{"id":"340624","messageId":"20180228214654.t4rcqmcb37q3grdh@angband.pl","threadId":"47942","inReplyTo":"xmqqpo4pkmiy.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] hooks/pre-auto-gc-battery: allow gc to run on non-laptops","fromName":"Adam Borowski","fromEmail":"kilobyte@angband.pl","sentAt":"2018-02-28T21:46:54Z","receivedAt":"2018-02-28T21:47:04Z","isPatch":true,"sender":{"key":"kilobyte@angband.pl","avatar":"https://avatars.githubusercontent.com/u/48801?v=4"},"body":"On Wed, Feb 28, 2018 at 10:16:21AM -0800, Junio C Hamano wrote:\n> Adam Borowski writes:\n> \n> > Desktops and servers tend to have no power sensor, thus on_ac_power returns\n> > 255 (\"unknown\").\n> >\n> > If that tool returns \"unknown\", there's no point in querying other sources\n> > as it already queried them, and is smarter than us (can handle multiple\n> > adapters).\n> \n> The explanation talks about the exit status 255 being special and\n> serves to signal \"there is no point continuing, and it is OK to\n> assume we are not on batttery\", while the code says that anything\n> but exit status 1 can be treated as such. Which is correct?\n\nAs the man page says:\n\n# EXIT STATUS\n# 0 (true) System is on mains power\n# 1 (false) System is not on mains power\n# 255 (false) Power status could not be determined\n\n0 usually means a laptop on AC power, 255 is for a typical desktop.\nThe current code can't return 2 or any other unexpected value, but if it\never does, an unknown error should probably be treated same as 255 unknown.\nThus, gc should be avoided only if the return code is 1.\n\nAs for the second paragraph, I meant that on_ac_power already queried all\nsources this hook knows about (other than /usr/bin/pmset which is OSX\nonly[1]), thus if the answer is \"unknown\", continuing to query is redundant.\n\nIf that's unclear, do you have some other wording in mind?\n\nAlso, it's good to trust on_ac_power, as it'll get updated whenever new\nquirks of power management get known: I heard allegations that some boards\nsay \"USB\" instead of \"Mains\", which should count the same for our\npurposes[2]. It's not reasonable to update consumers such as git instead of\na single system-provided tool.\n\nOne worry is that, if on_ac_power is not installed, other sources known by\nthis hook likewise assume that unknown means battery. And for example on\nDebian, powermgmt-base (which is where on_ac_power lives) is no longer\ninstalled by default. This suggests this patch needs to be extended to\ncover the other sources as well, but let's discuss this first. Extra\ncommits are cheap...\n\n> > Reported by: Xin Li \n> > Signed-off-by: Adam Borowski \n> > ---\n> > contrib/hooks/pre-auto-gc-battery | 2 +-\n> > 1 file changed, 1 insertion(+), 1 deletion(-)\n> >\n> > diff --git a/contrib/hooks/pre-auto-gc-battery b/contrib/hooks/pre-auto-gc-battery\n> > index 6a2cdebdb..7ba78c4df 100755\n> > --- a/contrib/hooks/pre-auto-gc-battery\n> > +++ b/contrib/hooks/pre-auto-gc-battery\n> > @@ -17,7 +17,7 @@\n> > # ln -sf /usr/share/git-core/contrib/hooks/pre-auto-gc-battery \\\n> > #\thooks/pre-auto-gc\n> > \n> > -if test -x /sbin/on_ac_power && /sbin/on_ac_power\n> > +if test -x /sbin/on_ac_power && (/sbin/on_ac_power;test $? -ne 1)\n> > then\n> > \texit 0\n> > elif test \"$(cat /sys/class/power_supply/AC/online 2>/dev/null)\" = 1\n> \n\n\n[1]. I don't know if there's an implementation of on_ac_power for OSX, but\nif there is, it is reasonable to assume it uses or emulates pmset.\n\n[2]. Technically, that's _dc_ not ac power, but as batteries use a different\ninterface, in the vast majority of cases USB power can be considered\nnon-rationed. You can power it from an unplugged laptop or from a\npowerbank, but that's no different from \"mains\" that come from an unplugged\nUPS with no or unsupported control link.\n-- \n⢀⣴⠾⠻⢶⣦⠀ \n⣾⠁⢠⠒⠀⣿⡁ A dumb species has no way to open a tuna can.\n⢿⡄⠘⠷⠚⠋⠀ A smart species invents a can opener.\n⠈⠳⣄⠀⠀⠀⠀ A master species delegates.\n"},{"id":"340626","messageId":"xmqq8tbckca1.fsf@gitster-ct.c.googlers.com","threadId":"47942","inReplyTo":"20180228214654.t4rcqmcb37q3grdh@angband.pl","subject":"Re: [PATCH] hooks/pre-auto-gc-battery: allow gc to run on non-laptops","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-02-28T21:57:42Z","receivedAt":"2018-02-28T21:57:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Adam Borowski writes:\n\n> 0 usually means a laptop on AC power, 255 is for a typical desktop.\n> The current code can't return 2 or any other unexpected value, but if it\n> ever does, an unknown error should probably be treated same as 255 unknown.\n> Thus, gc should be avoided only if the return code is 1.\n\nIn short, your answer to my question is \"What the code does is the\nmore correct version between the two---the log message was lying.\"\n\nThen please do not talk about 255 but explain why \"only if it is 1\"\nis the right thing in the log message. That would make the result\nconsistent.\n\n> As for the second paragraph,...\n\nThat paragraph reads just fine.\n"},{"id":"340629","messageId":"20180228221204.27356-1-kilobyte@angband.pl","threadId":"47942","inReplyTo":"xmqq8tbckca1.fsf@gitster-ct.c.googlers.com","subject":"[PATCH v2] hooks/pre-auto-gc-battery: allow gc to run on non-laptops","fromName":"Adam Borowski","fromEmail":"kilobyte@angband.pl","sentAt":"2018-02-28T22:12:04Z","receivedAt":"2018-02-28T22:12:19Z","isPatch":true,"sender":{"key":"kilobyte@angband.pl","avatar":"https://avatars.githubusercontent.com/u/48801?v=4"},"body":"Desktops and servers tend to have no power sensor, thus on_ac_power returns\n255 (\"unknown\"). Thus, let's take any answer other than 1 (\"battery\") as\nno contraindication to run gc.\n\nIf that tool returns \"unknown\", there's no point in querying other sources\nas it already queried them, and is smarter than us (can handle multiple\nadapters).\n\nReported by: Xin Li \nSigned-off-by: Adam Borowski \n---\nv2: improved commit message\n\n contrib/hooks/pre-auto-gc-battery | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/contrib/hooks/pre-auto-gc-battery b/contrib/hooks/pre-auto-gc-battery\nindex 6a2cdebdb..7ba78c4df 100755\n--- a/contrib/hooks/pre-auto-gc-battery\n+++ b/contrib/hooks/pre-auto-gc-battery\n@@ -17,7 +17,7 @@\n # ln -sf /usr/share/git-core/contrib/hooks/pre-auto-gc-battery \\\n #\thooks/pre-auto-gc\n \n-if test -x /sbin/on_ac_power && /sbin/on_ac_power\n+if test -x /sbin/on_ac_power && (/sbin/on_ac_power;test $? -ne 1)\n then\n \texit 0\n elif test \"$(cat /sys/class/power_supply/AC/online 2>/dev/null)\" = 1\n-- \n2.16.2\n"},{"id":"340634","messageId":"xmqq4lm0kb1b.fsf@gitster-ct.c.googlers.com","threadId":"47942","inReplyTo":"20180228221204.27356-1-kilobyte@angband.pl","subject":"Re: [PATCH v2] hooks/pre-auto-gc-battery: allow gc to run on non-laptops","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-02-28T22:24:32Z","receivedAt":"2018-02-28T22:24:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Adam Borowski writes:\n\n> Desktops and servers tend to have no power sensor, thus on_ac_power returns\n> 255 (\"unknown\"). Thus, let's take any answer other than 1 (\"battery\") as\n> no contraindication to run gc.\n>\n> If that tool returns \"unknown\", there's no point in querying other sources\n> as it already queried them, and is smarter than us (can handle multiple\n> adapters).\n>\n> Reported by: Xin Li \n> Signed-off-by: Adam Borowski \n> ---\n> v2: improved commit message\n\nThat makes the patch and the log consistent so that people who know\nthe area can reason about it ;-)\n\nWill queue. Thanks.\n\n\n> contrib/hooks/pre-auto-gc-battery | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/contrib/hooks/pre-auto-gc-battery b/contrib/hooks/pre-auto-gc-battery\n> index 6a2cdebdb..7ba78c4df 100755\n> --- a/contrib/hooks/pre-auto-gc-battery\n> +++ b/contrib/hooks/pre-auto-gc-battery\n> @@ -17,7 +17,7 @@\n> # ln -sf /usr/share/git-core/contrib/hooks/pre-auto-gc-battery \\\n> #\thooks/pre-auto-gc\n> \n> -if test -x /sbin/on_ac_power && /sbin/on_ac_power\n> +if test -x /sbin/on_ac_power && (/sbin/on_ac_power;test $? -ne 1)\n> then\n> \texit 0\n> elif test \"$(cat /sys/class/power_supply/AC/online 2>/dev/null)\" = 1\n"}]}