Re: [PATCH] maintenance: use systemd timers on Linux

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Mon, May 10 2021, Lénaïc Huard wrote:

> Le mercredi 5 mai 2021, 14:01:25 CEST Ævar Arnfjörð Bjarmason a écrit :
>> On Sun, May 02 2021, Eric Sunshine wrote:
>> > On Sat, May 1, 2021 at 10:59 AM Lénaïc Huard <lenaic@lhuard.fr> wrote:
>> >> +       strvec_push(&child.args, "--now");
>> >> +       strvec_pushf(&child.args, "git-maintenance@%s.timer", frequency);
>> >> +
>> >> +       if (start_command(&child))
>> >> +               die(_("failed to run systemctl"));
>> >> +       return finish_command(&child);
>> >> +}
>> >> +static int systemd_timer_write_unit_templates(const char *exec_path)
>> >> +{
>> >> +       unit = "[Unit]\n"
>> >> +              "Description=Optimize Git repositories data\n"
>> >> +              "\n"
>> >> +              "[Service]\n"
>> >> +              "Type=oneshot\n"
>> >> +              "ExecStart=\"%1$s/git\" --exec-path=\"%1$s\" for-each-repo
>> >> --config=maintenance.repo maintenance run --schedule=%%i\n"> 
>> > I see that it's in POSIX, but do we use this `%n$s` directive
>> > elsewhere in the Git source code? If not, I'd be cautious of
>> > introducing it here. Maybe it's better to just use plain `%s` twice...
>> 
>> We use it in po/, so for sprintf() on systems that don't have
>> NO_GETTEXT=Y we already test it in the wild.
>> 
>> But no, I don't think anything in the main source uses it, FWIW I have a
>> WIP series in my own fork that I've cooked for a while that uses this, I
>> haven't run into any issues with it in either GitHub's CI
>> (e.g. Windows), or on the systems I myself test on.
>> 
>> I think it would be a useful canary to just take a change like this, we
>> can always change it to the form you suggest if it doesn't work out.
>
> Based on this latest comment, I left the `%n$s` directive in the v2 of the 
> patch.
>
> Let me know if that’s still OK. Otherwise, I’d be happy to implement Eric’s 
> suggestion.
>
> Note however that this would be a “poor” canary to check if that directive is 
> supported on all the platforms on which git has been ported.
> Indeed, this code is executed only on systemd platforms, which means quite 
> recent Linux systems.
> Should this directive not be supported, I suppose it would be on more exotic 
> systems.

Indeed, although we compile it on non-Linux platforms, so we'd expect to
get complaints from smarter non-Linux compilers if fprintf() is given an
unknown formatting directive.