| Message ID | 20210608134000.663398-1-lenaic@lhuard.fr (mailing list archive) |
|---|---|
| Headers | show |
| Series | add support for systemd timers on Linux | expand |
Lénaïc Huard <lenaic@lhuard.fr> writes: > Hello, > > I’ve reworked this submission based on the valuable feedback I’ve received. > Thanks again for it! > > The patchset contains now the following patches: > ... A summary very well written. I wish all the cover letters were written like this one. > 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 Thanks.
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(-) >
On 6/8/2021 9:39 AM, Lénaïc Huard wrote: > Hello, > > I’ve reworked this submission based on the valuable feedback I’ve received. > Thanks again for it! Hi Lénaïc! I'm replying to your series because it appears you did not see our discussion of this topic in the What's Cooking thread a couple weeks ago [1] and might miss the discussion that began today [2]. [1] https://lore.kernel.org/git/4aed0293-6a48-d370-3b72-496b7c631cb5@gmail.com/ [2] https://lore.kernel.org/git/7a4b1238-5c3b-4c08-0e9d-511f857f9c38@gmail.com/ The proposal I give in [2] is that I can forward-fix the remaining comments OR re-submit the patches with a new patch and some edits to your current patches. (You would remain author of the patches you wrote.) None of that is important if you plan to submit a v6 that responds to the remaining feedback (summarized in [1]). I'll hold off for a couple days to give you a chance to read and respond. Thanks, -Stolee
On 17/08/2021 18:22, Derrick Stolee wrote: > On 6/8/2021 9:39 AM, Lénaïc Huard wrote: >> Hello, >> >> I’ve reworked this submission based on the valuable feedback I’ve received. >> Thanks again for it! > > Hi Lénaïc! > > I'm replying to your series because it appears you did not see our discussion > of this topic in the What's Cooking thread a couple weeks ago [1] and might > miss the discussion that began today [2]. > > [1] https://lore.kernel.org/git/4aed0293-6a48-d370-3b72-496b7c631cb5@gmail.com/ > [2] https://lore.kernel.org/git/7a4b1238-5c3b-4c08-0e9d-511f857f9c38@gmail.com/ > > The proposal I give in [2] is that I can forward-fix the remaining comments > OR re-submit the patches with a new patch and some edits to your current patches. > (You would remain author of the patches you wrote.) > > None of that is important if you plan to submit a v6 that responds to the > remaining feedback (summarized in [1]). I think you mean v8, v7[1] is what is in seen at the moment. There was a suggestion at the time that a v8 would not be needed[2] Best Wishes Phillip [1] https://public-inbox.org/git/20210702142556.99864-1-lenaic@lhuard.fr/ [2] https://public-inbox.org/git/YO0O9JHtnYrk9qRm@coredump.intra.peff.net/ > I'll hold off for a couple days to give you a chance to read and respond. > > Thanks, > -Stolee >
On 8/17/2021 3:43 PM, Phillip Wood wrote: > On 17/08/2021 18:22, Derrick Stolee wrote: >> On 6/8/2021 9:39 AM, Lénaïc Huard wrote: >>> Hello, >>> >>> I’ve reworked this submission based on the valuable feedback I’ve received. >>> Thanks again for it! >> >> Hi Lénaïc! >> >> I'm replying to your series because it appears you did not see our discussion >> of this topic in the What's Cooking thread a couple weeks ago [1] and might >> miss the discussion that began today [2]. >> >> [1] https://lore.kernel.org/git/4aed0293-6a48-d370-3b72-496b7c631cb5@gmail.com/ >> [2] https://lore.kernel.org/git/7a4b1238-5c3b-4c08-0e9d-511f857f9c38@gmail.com/ >> >> The proposal I give in [2] is that I can forward-fix the remaining comments >> OR re-submit the patches with a new patch and some edits to your current patches. >> (You would remain author of the patches you wrote.) >> >> None of that is important if you plan to submit a v6 that responds to the >> remaining feedback (summarized in [1]). > > I think you mean v8, v7[1] is what is in seen at the moment. There was a suggestion at the time that a v8 would not be needed[2] I do, thanks. Sorry for picking the wrong version to start this discussion. Thanks, -Stolee
Le mardi 17 août 2021, 19:22:05 CEST Derrick Stolee a écrit : > On 6/8/2021 9:39 AM, Lénaïc Huard wrote: > > Hello, > > > > I’ve reworked this submission based on the valuable feedback I’ve > > received. > > Thanks again for it! > > Hi Lénaïc! > > I'm replying to your series because it appears you did not see our > discussion of this topic in the What's Cooking thread a couple weeks ago > [1] and might miss the discussion that began today [2]. > > [1] > https://lore.kernel.org/git/4aed0293-6a48-d370-3b72-496b7c631cb5@gmail.com/ > [2] > https://lore.kernel.org/git/7a4b1238-5c3b-4c08-0e9d-511f857f9c38@gmail.com/ > > The proposal I give in [2] is that I can forward-fix the remaining comments > OR re-submit the patches with a new patch and some edits to your current > patches. (You would remain author of the patches you wrote.) > > None of that is important if you plan to submit a v6 that responds to the > remaining feedback (summarized in [1]). > > I'll hold off for a couple days to give you a chance to read and respond. > > Thanks, > -Stolee Hello, Sorry for the silence. I just happened to be in holiday for the past few weeks and did not have access to my mails. I can catch up the discussions I missed and try to address the remaining concerns in a new re-roll. Cheers, Lénaïc.
On 8/18/2021 1:56 AM, Lénaïc Huard wrote: > Le mardi 17 août 2021, 19:22:05 CEST Derrick Stolee a écrit : >> None of that is important if you plan to submit a v6 that responds to the >> remaining feedback (summarized in [1]). >> >> I'll hold off for a couple days to give you a chance to read and respond. ... > Hello, > > Sorry for the silence. I just happened to be in holiday for the past few weeks > and did not have access to my mails. > I can catch up the discussions I missed and try to address the remaining > concerns in a new re-roll. Perfect! Welcome back! -Stolee
Derrick Stolee <stolee@gmail.com> writes: > On 8/18/2021 1:56 AM, Lénaïc Huard wrote: >> Le mardi 17 août 2021, 19:22:05 CEST Derrick Stolee a écrit : >>> None of that is important if you plan to submit a v6 that responds to the >>> remaining feedback (summarized in [1]). >>> >>> I'll hold off for a couple days to give you a chance to read and respond. > ... >> Hello, >> >> Sorry for the silence. I just happened to be in holiday for the past few weeks >> and did not have access to my mails. >> I can catch up the discussions I missed and try to address the remaining >> concerns in a new re-roll. > > Perfect! Welcome back! Thanks, both. Looking forward to see this topic reignited and see its happy ending ;-)
Hello, I’ve reworked this submission based on the valuable feedback I’ve received. Thanks again for it! 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(-)