From: Phillip Wood <phillip.wood123@gmail.com>
Date: Wed, 09 Jun 2021 14:54:29 GMT
Subject: Re: [PATCH v5 0/3] add support for systemd timers on Linux
Message-ID: <01774318-93c3-d3e9-6303-7f375008a9b8@gmail.com>
In-Reply-To: <20210608134000.663398-1-lenaic@lhuard.fr>

Hi Lénaïc

Thanks for the excellent cover letter, I found it very useful while 
reviewing these patches. I think the changes address all of my previous 
concerns, the error handling in the last patch looks good. Having read 
through the patches I don't have anything to add to Peff's comments - 
with those small memory management fixed I think this will be a good shape.

Thanks for your work on this

Phillip

On 08/06/2021 14:39, Lénaïc Huard wrote:
>[...] > The patchset contains now the following patches:
> 
> * cache.h: Introduce a generic "xdg_config_home_for(…)" function
> 
>    This patch introduces a function to compute configuration files
>    paths inside $XDG_CONFIG_HOME or ~/.config for other programs than
>    git itself.
>    It is used in the latest patch of this series to compute systemd
>    unit files location.
> 
>    The only change in this patch compared to its previous version is
>    the renaming of the first parameter of the `xdg_config_home_for(…)`
>    function from `prog` to `subdir`.
> 
> * maintenance: introduce ENABLE/DISABLE for code clarity
> 
>    I just completely dropped this patch as it turned out that replacing
>    some 0/1 values by `ENABLE`/`DISABLE` enum values wasn’t making the
>    code look nicer as initially expected.
> 
> * maintenance: `git maintenance run` learned `--scheduler=<scheduler>`
> 
>    This patch contains all the code that is related to the addition of
>    the new `--scheduler` parameter of the `git maintenance start`
>    command, independently of the systemd timers.
> 
>    The main changes in this patch compared to its previous version are:
> 
>      * Revert all the changes that were previously introduced by the
>        `ENABLE`/`DISABLE` enum values.
> 
>      * Remove the `strlcpy` in the testing framework inside the
>        `get_schedule_cmd` function.
> 
>      * `update_background_schedule` loops over all the available
>        schedulers, disables all of them except the one which is
>        enabled.
>        In this new version of the patch, it is now ensured that all the
>        schedulers deactivation are done before the activation.
>        The goal of this change is avoid a potential race condition
>        where two schedulers could be enabled at the same time.
>        This behaviour change has been reflected in the tests.
> 
>      * The local variable `builtin_maintenance_start_options` has been
>        shortened.
> 
> * maintenance: add support for systemd timers on Linux
> 
>    This patch implements the support of systemd timers on top of
>    crontab scheduler on Linux systems.
> 
>    The main changes in this patch compared to its previous version are:
> 
>      * The caching logic of `is_systemd_timer_available` has been
>        dropped.
>        I initially wanted to cache the outcome of forking and executing
>        an external command to avoid doing it several times as
>        `is_systemd_timer_available` is invoked from several places
>        (`resolve_auto_scheduler`, `validate_scheduler` and
>        `update_background_scheduler`).
>        But it’s true they’re not always all called.
>        In the case of `maintenance stop`, `resolve_auto_scheduler` and
>        `validate_scheduler` are not called.
>        In the case of `maintenance start`, the `if (enable &&
>        opts->scheduler == i)` statement inside
>        `update_background_schedule` skips the execution of
>        `is_systemd_timer_available`.
> 
>      * The `is_systemd_timer_available` has been split in two parts:
>        * `is_systemd_timer_available` is the entry point and holds the
>          platform agnostic testing framework logic.
>        * `real_is_systemd_timer_available` contains the platform
>          specific logic.
> 
>      * The error management of `systemd_timer_write_unit_templates` has
>        been reviewed.
>        The return code of `fopen`, `fputs`, `fclose`, etc. are now
>        checked.
>        If this function manages to write one file, but fails at writing
>        the second one, it will attempt to delete the first one to not
>        leave the system in an inconsistent state.
> 
>      * The error management of `systemd_timer_delete_unit_templates`
>        has also been reviewed. The error code of `unlink` is now
>        checked.
> 
> I hope I’ve addressed all your valuable feedback. Do not hesitate to
> let me know if I’ve forgotten anything.
> 
> Lénaïc Huard (3):
>    cache.h: Introduce a generic "xdg_config_home_for(…)" function
>    maintenance: `git maintenance run` learned `--scheduler=<scheduler>`
>    maintenance: add support for systemd timers on Linux
> 
>   Documentation/git-maintenance.txt |  60 ++++
>   builtin/gc.c                      | 564 ++++++++++++++++++++++++++----
>   cache.h                           |   7 +
>   path.c                            |  13 +-
>   t/t7900-maintenance.sh            | 110 +++++-
>   5 files changed, 676 insertions(+), 78 deletions(-)
>