Re: [PATCH v2 1/1] maintenance: use systemd timers on Linux

[Eric Sunshine <sunshine@sunshineco.com>]

On Sun, May 9, 2021 at 9:20 PM Đoàn Trần Công Danh <congdanhqx@gmail.com> wrote:
> On 2021-05-09 23:32:17+0200, Lénaïc Huard <lenaic@lhuard.fr> wrote:
> > +static int systemd_timer_delete_units(const char *cmd)
> > +{
> > +     return systemd_timer_enable_unit(0, SCHEDULE_HOURLY, cmd) ||
> > +            systemd_timer_enable_unit(0, SCHEDULE_DAILY, cmd) ||
> > +            systemd_timer_enable_unit(0, SCHEDULE_WEEKLY, cmd) ||
> > +            systemd_timer_delete_unit_templates();
> > +}
>
> Argh, we're disabling those systemd timer units first, by passing 0 as
> first argument of systemd_timer_delete_units.
>
> The fact that I read that twice, and still wrote down above reply
> makes me think that above code is not self-explanatory enough.
> May we switch to something else? Let's say using enum?

This is modeled after existing scheduler functions in this file, in
which the `enable` argument is a simple 0 or 1, so changing this to an
enum just for this function would be inconsistent. Changing all the
functions to `enum` in a preparatory patch could indeed improve
readability, however, that's tangential cleanup which may be outside
the scope of this submission.

> > +     file = xfopen(filename, "w");
> > +     free(filename);
>
> I'm sure if we should use FREE_AND_NULL(filename) instead?
> Since, filename will be reused later.

Indeed, probably a good idea, as it would make catching mistakes easier.

> > +            "ExecStart=\"%1$s/git\" --exec-path=\"%1$s\" for-each-repo --config=maintenance.repo maintenance run --schedule=%%i\n"
>
> I think others have strong opinion on not using "%1$s",
> and prefer simple "%s" and using "exec_path" twice instead.

I brought it up only because I hadn't seen it in Git sources, and
wasn't sure if we'd want to start using it. Aside from Ævar, who
seemed reasonably in favor of it, nobody else chimed in, so it could
go either way, I suppose.

> > +test_systemd_analyze_verify () {
> > +     if test_have_prereq SYSTEMD_ANALYZE
> > +     then
> > +             systemd-analyze verify "$@"
> > +     else
> > +             true
>
> The "else" leg is not necessary.

This was patterned after the existing test_xmllint() function in this
file which has the `else true` leg. I wrote test_xmllint(), so I'll
take blame. But you're right, the `if` will return success if the
prerequisite is not set, so the `else` leg is indeed not needed.
(Cleaning up the test_xmllint() function is outside the scope of this
patch.)

> > +test_expect_success 'start and stop Linux/systemd maintenance' '
> > +     write_script print-args <<-\EOF &&
> > +     echo $* >>args
>
> To avoid any possible incompatibility with zillion echo implementation
> out there. printf should be prefered over echo. Not a in this test
> case, however, it costs us nothing anyway.
>
>         printf "%s\n" "$*"

This, too, is patterned after existing auxiliary scripts created by
these test functions, and you're correct that it's potentially
dangerous. A manual inspection of all the existing instances shows
that `echo $*` happens to be safe for those cases but that doesn't
excuse being sloppy about it, so the existing cases probably ought to
be cleaned up. But, again, that is outside the scope of this series.
For this particular case, though...

> > +     for frequency in hourly daily weekly
> > +     do
> > +             echo "--user enable --now git-maintenance@${frequency}.timer" >>expect || return 1
> > +     done &&
>
> And here, we can have a nicer syntax with printf:
>
>         printf "--user enable --now git-maintenance@%s.timer\n" hourly daily weekly >expect &&
>
> With printf, we don't even need "rm -f expect" above.

... you're quite correct that `printf` is the way to go both here and
in the generated `print-args` script since these arguments start with
hyphen.