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(-)
