{"thread":{"id":"55409","subject":"[PATCH] maintenance: specify explicit stdin for crontab","startedAt":"2021-03-29T21:20:25Z","lastAt":"2021-03-30T19:38:46Z","messageCount":7,"participants":["Kevin Daudt","Martin Ågren","Derrick Stolee","Todd Zullinger"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"420514","messageId":"20210329210928.561586-1-me@ikke.info","threadId":"55409","inReplyTo":null,"subject":"[PATCH] maintenance: specify explicit stdin for crontab","fromName":"Kevin Daudt","fromEmail":"me@ikke.info","sentAt":"2021-03-29T21:09:28Z","receivedAt":"2021-03-29T21:20:25Z","isPatch":true,"sender":{"key":"me@ikke.info","avatar":"https://avatars.githubusercontent.com/u/135698?v=4"},"body":"There are multiple crontab implementations that require stdin for\nediting a crontab to be explicitly specified as '-'.\n\nBusyBox crontab just shows usage when executed without arguments.\n[man 1 crontab][0] from openbsd states:\n\n> The pseudo-filename '-' must be specified to read from standard\n> input.\n\nOther implementations might not require it, but accept '-', so\nexplicitly specify that crontab should read from stdin by passing the\n'-' pseudo-filename.\n\n[0]: https://www.unix.com/man-page/freebsd/1/crontab/\n\nSigned-off-by: Kevin Daudt <me@ikke.info>\n---\nThe `git maintenance start` command is currently broken on the default\nalpine setup. This would fix it. I've tested `echo '* * * * * echo test'\n| crontab -` on the different platforms I have access to, and it worked\nwithout issues..\n builtin/gc.c            | 1 +\n t/helper/test-crontab.c | 2 +-\n 2 files changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex ef7226d7b..dfdb5bce9 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -1904,6 +1904,7 @@ static int crontab_update_schedule(int run_maintenance, int fd, const char *cmd)\n \trewind(cron_list);\n \n \tstrvec_split(&crontab_edit.args, cmd);\n+\tstrvec_push(&crontab_edit.args, \"-\");\n \tcrontab_edit.in = -1;\n \tcrontab_edit.git_cmd = 0;\n \ndiff --git a/t/helper/test-crontab.c b/t/helper/test-crontab.c\nindex e7c0137a4..525cb318a 100644\n--- a/t/helper/test-crontab.c\n+++ b/t/helper/test-crontab.c\n@@ -17,7 +17,7 @@ int cmd__crontab(int argc, const char **argv)\n \t\tif (!from)\n \t\t\treturn 0;\n \t\tto = stdout;\n-\t} else if (argc == 2) {\n+\t} else if ((argc == 3 && !strcmp(argv[2], \"-\")) || argc == 2) {\n \t\tfrom = stdin;\n \t\tto = fopen(argv[1], \"w\");\n \t} else\n-- \n2.30.1\n\n"},{"id":"420553","messageId":"CAN0heSrSNJhy33Wi9Yq8kfnkJEyvQoadyj8joLqHtV+SYPs1sw@mail.gmail.com","threadId":"55409","inReplyTo":"20210329210928.561586-1-me@ikke.info","subject":"Re: [PATCH] maintenance: specify explicit stdin for crontab","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2021-03-30T05:41:48Z","receivedAt":"2021-03-30T05:42:36Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On Mon, 29 Mar 2021 at 23:23, Kevin Daudt <me@ikke.info> wrote:\n>\n> There are multiple crontab implementations that require stdin for\n> editing a crontab to be explicitly specified as '-'.\n\n[...]\n\n> --- a/t/helper/test-crontab.c\n> +++ b/t/helper/test-crontab.c\n> @@ -17,7 +17,7 @@ int cmd__crontab(int argc, const char **argv)\n>                 if (!from)\n>                         return 0;\n>                 to = stdout;\n> -       } else if (argc == 2) {\n> +       } else if ((argc == 3 && !strcmp(argv[2], \"-\")) || argc == 2) {\n>                 from = stdin;\n>                 to = fopen(argv[1], \"w\");\n\nWould it make sense to make this\n\n  } else if (argc == 3 && !strcmp(argv[2], \"-\")) {\n\nin order to make this test-tool as picky as possible and to only accept\nthe kind of usage we want to (well, need to) use? The tests as they\nstand would still pass, which I think argues for us not really needing\nthat \"argc == 2\".\n\nThis would be followed by\n\n  } else\n          return error(\"unknown arguments\");\n\nwhich wouldn't be super helpful if you forgot the \"-\", but helpful\nenough for an internal test-tool, I guess.\n\nSpeaking of usage and hints, there's \"Usage: ...\" in a comment at the\ntop of this file. It should probably be updated either way.\n\nMartin\n"},{"id":"420573","messageId":"25ea6f26-c829-f63f-77a1-11a28bbe7fc0@gmail.com","threadId":"55409","inReplyTo":"CAN0heSrSNJhy33Wi9Yq8kfnkJEyvQoadyj8joLqHtV+SYPs1sw@mail.gmail.com","subject":"Re: [PATCH] maintenance: specify explicit stdin for crontab","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-03-30T12:02:22Z","receivedAt":"2021-03-30T12:03:00Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 3/30/2021 1:41 AM, Martin Ågren wrote:\n> On Mon, 29 Mar 2021 at 23:23, Kevin Daudt <me@ikke.info> wrote:\n>>\n>> There are multiple crontab implementations that require stdin for\n>> editing a crontab to be explicitly specified as '-'.\n\nThank you for reporting this, especially with a patch!\n\nHowever, I'm not sure about this adding of '-' being something that\ncrontab ignores so commonly. My Ubuntu machine reports this:\n\n$ crontab -e -\ncrontab: usage error: no arguments permitted after this option\nusage:  crontab [-u user] file\n        crontab [ -u user ] [ -i ] { -e | -l | -r }\n                (default operation is replace, per 1003.2)\n        -e      (edit user's crontab)\n        -l      (list user's crontab)\n        -r      (delete user's crontab)\n        -i      (prompt before deleting user's crontab)\n\nIs there a way we could attempt writing over stdin, notice the\nfailure, then retry with the '-' option?\n\n> \n> [...]\n> \n>> --- a/t/helper/test-crontab.c\n>> +++ b/t/helper/test-crontab.c\n>> @@ -17,7 +17,7 @@ int cmd__crontab(int argc, const char **argv)\n>>                 if (!from)\n>>                         return 0;\n>>                 to = stdout;\n>> -       } else if (argc == 2) {\n>> +       } else if ((argc == 3 && !strcmp(argv[2], \"-\")) || argc == 2) {\n>>                 from = stdin;\n>>                 to = fopen(argv[1], \"w\");\n> \n> Would it make sense to make this\n> \n>   } else if (argc == 3 && !strcmp(argv[2], \"-\")) {\n> \n> in order to make this test-tool as picky as possible and to only accept\n> the kind of usage we want to (well, need to) use? The tests as they\n> stand would still pass, which I think argues for us not really needing\n> that \"argc == 2\".\n> \n> This would be followed by\n> \n>   } else\n>           return error(\"unknown arguments\");\n> \n> which wouldn't be super helpful if you forgot the \"-\", but helpful\n> enough for an internal test-tool, I guess.\n>\n> Speaking of usage and hints, there's \"Usage: ...\" in a comment at the\n> top of this file. It should probably be updated either way.\n\nI agree with Martin's review here, too.\n\nThanks,\n-Stolee\n"},{"id":"420629","messageId":"YGNcA3paBeZ8mYVP@alpha","threadId":"55409","inReplyTo":"25ea6f26-c829-f63f-77a1-11a28bbe7fc0@gmail.com","subject":"Re: [PATCH] maintenance: specify explicit stdin for crontab","fromName":"Kevin Daudt","fromEmail":"me@ikke.info","sentAt":"2021-03-30T17:12:35Z","receivedAt":"2021-03-30T17:13:15Z","isPatch":true,"sender":{"key":"me@ikke.info","avatar":"https://avatars.githubusercontent.com/u/135698?v=4"},"body":"On Tue, Mar 30, 2021 at 08:02:22AM -0400, Derrick Stolee wrote:\n> On 3/30/2021 1:41 AM, Martin Ågren wrote:\n> > On Mon, 29 Mar 2021 at 23:23, Kevin Daudt <me@ikke.info> wrote:\n> >>\n> >> There are multiple crontab implementations that require stdin for\n> >> editing a crontab to be explicitly specified as '-'.\n> \n> Thank you for reporting this, especially with a patch!\n> \n> However, I'm not sure about this adding of '-' being something that\n> crontab ignores so commonly. My Ubuntu machine reports this:\n> \n> $ crontab -e -\n> crontab: usage error: no arguments permitted after this option\n> usage:  crontab [-u user] file\n>         crontab [ -u user ] [ -i ] { -e | -l | -r }\n>                 (default operation is replace, per 1003.2)\n>         -e      (edit user's crontab)\n>         -l      (list user's crontab)\n>         -r      (delete user's crontab)\n>         -i      (prompt before deleting user's crontab)\n> \n> Is there a way we could attempt writing over stdin, notice the\n> failure, then retry with the '-' option?\n\nWe do not use -e to edit, we run `crontab` and provide the contents to\nstdin. `crontab -e` just opens the crontab in the users editor, which\nwould work with busybox as well, but that's not what's being done here.\n\n> \n> > \n> > [...]\n> > \n> >> --- a/t/helper/test-crontab.c\n> >> +++ b/t/helper/test-crontab.c\n> >> @@ -17,7 +17,7 @@ int cmd__crontab(int argc, const char **argv)\n> >>                 if (!from)\n> >>                         return 0;\n> >>                 to = stdout;\n> >> -       } else if (argc == 2) {\n> >> +       } else if ((argc == 3 && !strcmp(argv[2], \"-\")) || argc == 2) {\n> >>                 from = stdin;\n> >>                 to = fopen(argv[1], \"w\");\n> > \n> > Would it make sense to make this\n> > \n> >   } else if (argc == 3 && !strcmp(argv[2], \"-\")) {\n> > \n> > in order to make this test-tool as picky as possible and to only accept\n> > the kind of usage we want to (well, need to) use? The tests as they\n> > stand would still pass, which I think argues for us not really needing\n> > that \"argc == 2\".\n> > \n> > This would be followed by\n> > \n> >   } else\n> >           return error(\"unknown arguments\");\n> > \n> > which wouldn't be super helpful if you forgot the \"-\", but helpful\n> > enough for an internal test-tool, I guess.\n> >\n> > Speaking of usage and hints, there's \"Usage: ...\" in a comment at the\n> > top of this file. It should probably be updated either way.\n> \n> I agree with Martin's review here, too.\n\nYes, I agree too, was already contemplating that.\n\n> \n> Thanks,\n> -Stolee\n"},{"id":"420633","messageId":"20210330174333.GJ15354@pobox.com","threadId":"55409","inReplyTo":"25ea6f26-c829-f63f-77a1-11a28bbe7fc0@gmail.com","subject":"Re: [PATCH] maintenance: specify explicit stdin for crontab","fromName":"Todd Zullinger","fromEmail":"tmz@pobox.com","sentAt":"2021-03-30T17:43:33Z","receivedAt":"2021-03-30T17:44:32Z","isPatch":true,"sender":{"key":"tmz@pobox.com","avatar":"https://avatars.githubusercontent.com/u/806319?v=4"},"body":"Hi,\n\nDerrick Stolee wrote:\n> On 3/30/2021 1:41 AM, Martin Ågren wrote:\n>> On Mon, 29 Mar 2021 at 23:23, Kevin Daudt <me@ikke.info> wrote:\n>>>\n>>> There are multiple crontab implementations that require stdin for\n>>> editing a crontab to be explicitly specified as '-'.\n\nAmusingly, I wrote the exact same patch 2 weeks ago\n(including not dropping the `argc == 2` which Martin\nmentioned).  That was in response to a report in the Fedora\nbugzilla:\n\n    https://bugzilla.redhat.com/1939930\n\nI thought cronie might be rather rare with it's non-POSIX\nhandling of crontab without arguments.\n\nIn the end, the cronie folks upstream adjusted things so\nthat crontab behaves as defined by POSIX if stdin is not a\nTTY:\n\n    https://github.com/cronie-crond/cronie/commit/8b0241f\n\nThat allows cronie to behave more sensibly for interactive\nuse without breaking tools like git maintenance.  And it let\nme sidestep proposing a patch to git (or worse, maintaining\nit in the Fedora packages).\n\nBut I didn't dig in to find out whether or how many other\ncrontab implemntations had also eschewed the (rather poor)\nPOSIX-confirming behavior.  Knowing there are several among\npopular OS's makes it easy to see something like this patch\nbeing generally useful.\n\nThough, as Derrick notes below, we would break systems which\nimplement crontab strictly per the POSIX spec.  I don't know\nhow many crontab's don't accept `-`.\n\nAt the time, I checked on an older OmniOS system I had\naccess to (based on Illumos/OpenSolaris) and it did not\naccept `-`.  So my quick sample size of 3 (Fedora, CentOS,\nand OmniOS) I had a 1/3 failure rate.\n\n> Thank you for reporting this, especially with a patch!\n> \n> However, I'm not sure about this adding of '-' being something that\n> crontab ignores so commonly. My Ubuntu machine reports this:\n> \n> $ crontab -e -\n> crontab: usage error: no arguments permitted after this option\n> usage:  crontab [-u user] file\n>         crontab [ -u user ] [ -i ] { -e | -l | -r }\n>                 (default operation is replace, per 1003.2)\n>         -e      (edit user's crontab)\n>         -l      (list user's crontab)\n>         -r      (delete user's crontab)\n>         -i      (prompt before deleting user's crontab)\n> \n> Is there a way we could attempt writing over stdin, notice the\n> failure, then retry with the '-' option?\n\nYou'd skip the `-e` there, no?  Running `crontab -` in a\ncurrent ubuntu container with the cron package installed\n(what looks like vixie-cron-3.0pl1) works as expected.\n\nPerhaps a Makefile knob to allow systems with such a crontab\nto adjust the behavior would be an alternative to detecting\nand retrying?\n\nNEEDS_CRONTAB_STDIN_OPT or something like that, with\nconfig.mak.uname to override whichever default is chosen.\nWhether that's a better option really depends on how much\neffort it is to add and maintain the detection in the code\nweighed against how many systems would need to have the\ndefault changed.\n\nMildy related, I wonder whether we'll eventually see a patch\nto use systemd timers instead of cron (optionally, of\ncourse).  Fedora, for example, doesn't install crond by\ndefault anymore.  (Though, warts and all, I still prefer\ncrond myself.)\n\n>> [...]\n>> \n>>> --- a/t/helper/test-crontab.c\n>>> +++ b/t/helper/test-crontab.c\n>>> @@ -17,7 +17,7 @@ int cmd__crontab(int argc, const char **argv)\n>>>                 if (!from)\n>>>                         return 0;\n>>>                 to = stdout;\n>>> -       } else if (argc == 2) {\n>>> +       } else if ((argc == 3 && !strcmp(argv[2], \"-\")) || argc == 2) {\n>>>                 from = stdin;\n>>>                 to = fopen(argv[1], \"w\");\n>> \n>> Would it make sense to make this\n>> \n>>   } else if (argc == 3 && !strcmp(argv[2], \"-\")) {\n>> \n>> in order to make this test-tool as picky as possible and to only accept\n>> the kind of usage we want to (well, need to) use? The tests as they\n>> stand would still pass, which I think argues for us not really needing\n>> that \"argc == 2\".\n>> \n>> This would be followed by\n>> \n>>   } else\n>>           return error(\"unknown arguments\");\n>> \n>> which wouldn't be super helpful if you forgot the \"-\", but helpful\n>> enough for an internal test-tool, I guess.\n>>\n>> Speaking of usage and hints, there's \"Usage: ...\" in a comment at the\n>> top of this file. It should probably be updated either way.\n> \n> I agree with Martin's review here, too.\n\n-- \nTodd\n"},{"id":"420644","messageId":"bc4d77cd-5b2a-bdda-d447-8bf9d44c313f@gmail.com","threadId":"55409","inReplyTo":"YGNcA3paBeZ8mYVP@alpha","subject":"Re: [PATCH] maintenance: specify explicit stdin for crontab","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-03-30T19:32:39Z","receivedAt":"2021-03-30T19:33:22Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 3/30/2021 1:12 PM, Kevin Daudt wrote:\n> On Tue, Mar 30, 2021 at 08:02:22AM -0400, Derrick Stolee wrote:\n>> On 3/30/2021 1:41 AM, Martin Ågren wrote:\n>>> On Mon, 29 Mar 2021 at 23:23, Kevin Daudt <me@ikke.info> wrote:\n>>>>\n>>>> There are multiple crontab implementations that require stdin for\n>>>> editing a crontab to be explicitly specified as '-'.\n>>\n>> Thank you for reporting this, especially with a patch!\n>>\n>> However, I'm not sure about this adding of '-' being something that\n>> crontab ignores so commonly. My Ubuntu machine reports this:\n>>\n>> $ crontab -e -\n>> crontab: usage error: no arguments permitted after this option\n>> usage:  crontab [-u user] file\n>>         crontab [ -u user ] [ -i ] { -e | -l | -r }\n>>                 (default operation is replace, per 1003.2)\n>>         -e      (edit user's crontab)\n>>         -l      (list user's crontab)\n>>         -r      (delete user's crontab)\n>>         -i      (prompt before deleting user's crontab)\n>>\n>> Is there a way we could attempt writing over stdin, notice the\n>> failure, then retry with the '-' option?\n> \n> We do not use -e to edit, we run `crontab` and provide the contents to\n> stdin. `crontab -e` just opens the crontab in the users editor, which\n> would work with busybox as well, but that's not what's being done here.\n\nThank you. Of course. Muscle memory from testing crontab manually.\n\nThanks,\n-Stolee\n\n"},{"id":"420646","messageId":"f876cbad-ec12-6ee0-ede2-8f439ad4ad34@gmail.com","threadId":"55409","inReplyTo":"20210330174333.GJ15354@pobox.com","subject":"Re: [PATCH] maintenance: specify explicit stdin for crontab","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-03-30T19:38:00Z","receivedAt":"2021-03-30T19:38:46Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 3/30/2021 1:43 PM, Todd Zullinger wrote:\n> Hi,\n> \n> Derrick Stolee wrote:\n>> On 3/30/2021 1:41 AM, Martin Ågren wrote:\n>>> On Mon, 29 Mar 2021 at 23:23, Kevin Daudt <me@ikke.info> wrote:\n>>>>\n>>>> There are multiple crontab implementations that require stdin for\n>>>> editing a crontab to be explicitly specified as '-'.\n> \n> Amusingly, I wrote the exact same patch 2 weeks ago\n> (including not dropping the `argc == 2` which Martin\n> mentioned).  That was in response to a report in the Fedora\n> bugzilla:\n> \n>     https://bugzilla.redhat.com/1939930\n> \n> I thought cronie might be rather rare with it's non-POSIX\n> handling of crontab without arguments.\n\nThanks for this link! I appreciate the context.\n \n> In the end, the cronie folks upstream adjusted things so\n> that crontab behaves as defined by POSIX if stdin is not a\n> TTY:\n> \n>     https://github.com/cronie-crond/cronie/commit/8b0241f\n>> That allows cronie to behave more sensibly for interactive\n> use without breaking tools like git maintenance.  And it let\n> me sidestep proposing a patch to git (or worse, maintaining\n> it in the Fedora packages).\n\nNice!\n \n> But I didn't dig in to find out whether or how many other\n> crontab implemntations had also eschewed the (rather poor)\n> POSIX-confirming behavior.  Knowing there are several among\n> popular OS's makes it easy to see something like this patch\n> being generally useful.\n> \n> Though, as Derrick notes below, we would break systems which\n> implement crontab strictly per the POSIX spec.  I don't know\n> how many crontab's don't accept `-`.\n> \n> At the time, I checked on an older OmniOS system I had\n> access to (based on Illumos/OpenSolaris) and it did not\n> accept `-`.  So my quick sample size of 3 (Fedora, CentOS,\n> and OmniOS) I had a 1/3 failure rate.\n> \n>> Thank you for reporting this, especially with a patch!\n>>\n>> However, I'm not sure about this adding of '-' being something that\n>> crontab ignores so commonly. My Ubuntu machine reports this:\n>>\n>> $ crontab -e -\n>> crontab: usage error: no arguments permitted after this option\n>> usage:  crontab [-u user] file\n>>         crontab [ -u user ] [ -i ] { -e | -l | -r }\n>>                 (default operation is replace, per 1003.2)\n>>         -e      (edit user's crontab)\n>>         -l      (list user's crontab)\n>>         -r      (delete user's crontab)\n>>         -i      (prompt before deleting user's crontab)\n>>\n>> Is there a way we could attempt writing over stdin, notice the\n>> failure, then retry with the '-' option?\n> \n> You'd skip the `-e` there, no?  Running `crontab -` in a\n> current ubuntu container with the cron package installed\n> (what looks like vixie-cron-3.0pl1) works as expected.\n\nYes, this is my mistake. My machine supports \"crontab -\".\n\n> Perhaps a Makefile knob to allow systems with such a crontab\n> to adjust the behavior would be an alternative to detecting\n> and retrying?\n> \n> NEEDS_CRONTAB_STDIN_OPT or something like that, with\n> config.mak.uname to override whichever default is chosen.\n> Whether that's a better option really depends on how much\n> effort it is to add and maintain the detection in the code\n> weighed against how many systems would need to have the\n> default changed.\n> \n> Mildy related, I wonder whether we'll eventually see a patch\n> to use systemd timers instead of cron (optionally, of\n> course).  Fedora, for example, doesn't install crond by\n> default anymore.  (Though, warts and all, I still prefer\n> crond myself.)\n\nPerhaps the best way to approach this is to try adding '-'\nby default, and remove it and try again on a failure. If the\nusage is actually a problem, that first command should fail\nquickly. I'm not sure if we could rely upon a specific\ncategory of error codes or if we should just say \"non-zero\nexit means retry\".\n\nThanks!\n-Stolee\n"}]}