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

["Lénaïc Huard" <lenaic@lhuard.fr>]

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.