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@xxxxxxxxx> 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.
