Hi,

Derrick Stolee wrote:

On 3/30/2021 1:41 AM, Martin Ågren wrote:

quoted

On Mon, 29 Mar 2021 at 23:23, Kevin Daudt [off-list ref] wrote:

quoted

There are multiple crontab implementations that require stdin for
editing a crontab to be explicitly specified as '-'.

Amusingly, I wrote the exact same patch 2 weeks ago
(including not dropping the `argc == 2` which Martin
mentioned).  That was in response to a report in the Fedora
bugzilla:

    https://bugzilla.redhat.com/1939930

I thought cronie might be rather rare with it's non-POSIX
handling of crontab without arguments.

In the end, the cronie folks upstream adjusted things so
that crontab behaves as defined by POSIX if stdin is not a
TTY:

    https://github.com/cronie-crond/cronie/commit/8b0241f

That allows cronie to behave more sensibly for interactive
use without breaking tools like git maintenance.  And it let
me sidestep proposing a patch to git (or worse, maintaining
it in the Fedora packages).

But I didn't dig in to find out whether or how many other
crontab implemntations had also eschewed the (rather poor)
POSIX-confirming behavior.  Knowing there are several among
popular OS's makes it easy to see something like this patch
being generally useful.

Though, as Derrick notes below, we would break systems which
implement crontab strictly per the POSIX spec.  I don't know
how many crontab's don't accept `-`.

At the time, I checked on an older OmniOS system I had
access to (based on Illumos/OpenSolaris) and it did not
accept `-`.  So my quick sample size of 3 (Fedora, CentOS,
and OmniOS) I had a 1/3 failure rate.

Thank you for reporting this, especially with a patch!

However, I'm not sure about this adding of '-' being something that
crontab ignores so commonly. My Ubuntu machine reports this:

$ crontab -e -
crontab: usage error: no arguments permitted after this option
usage:  crontab [-u user] file
        crontab [ -u user ] [ -i ] { -e | -l | -r }
                (default operation is replace, per 1003.2)
        -e      (edit user's crontab)
        -l      (list user's crontab)
        -r      (delete user's crontab)
        -i      (prompt before deleting user's crontab)

Is there a way we could attempt writing over stdin, notice the
failure, then retry with the '-' option?

You'd skip the `-e` there, no?  Running `crontab -` in a
current ubuntu container with the cron package installed
(what looks like vixie-cron-3.0pl1) works as expected.

Perhaps a Makefile knob to allow systems with such a crontab
to adjust the behavior would be an alternative to detecting
and retrying?

NEEDS_CRONTAB_STDIN_OPT or something like that, with
config.mak.uname to override whichever default is chosen.
Whether that's a better option really depends on how much
effort it is to add and maintain the detection in the code
weighed against how many systems would need to have the
default changed.

Mildy related, I wonder whether we'll eventually see a patch
to use systemd timers instead of cron (optionally, of
course).  Fedora, for example, doesn't install crond by
default anymore.  (Though, warts and all, I still prefer
crond myself.)

quoted

[...]

quoted

--- a/t/helper/test-crontab.c
+++ b/t/helper/test-crontab.c

@@ -17,7 +17,7 @@ int cmd__crontab(int argc, const char **argv)
                if (!from)
                        return 0;
                to = stdout;
-       } else if (argc == 2) {
+       } else if ((argc == 3 && !strcmp(argv[2], "-")) || argc == 2) {
                from = stdin;
                to = fopen(argv[1], "w");

Would it make sense to make this

  } else if (argc == 3 && !strcmp(argv[2], "-")) {

in order to make this test-tool as picky as possible and to only accept
the kind of usage we want to (well, need to) use? The tests as they
stand would still pass, which I think argues for us not really needing
that "argc == 2".

This would be followed by

  } else
          return error("unknown arguments");

which wouldn't be super helpful if you forgot the "-", but helpful
enough for an internal test-tool, I guess.

Speaking of usage and hints, there's "Usage: ..." in a comment at the
top of this file. It should probably be updated either way.

I agree with Martin's review here, too.

-- 
Todd