| Message ID | 20240318153257.27451-4-mg@max.gautier.name (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | maintenance: use packaged systemd units | expand |
I'm working on updating the test in t7900-maintenance.sh, but I might be missing something here: >test_expect_success 'start and stop Linux/systemd maintenance' ' > write_script print-args <<-\EOF && > printf "%s\n" "$*" >>args > EOF > > XDG_CONFIG_HOME="$PWD" && > export XDG_CONFIG_HOME && > rm -f args && > GIT_TEST_MAINT_SCHEDULER="systemctl:./print-args" git maintenance start --scheduler=systemd-timer && Do I understand correctly that this means we're not actually running systemctl here, just printing the arguments to our file ? > # start registers the repo > git config --get --global --fixed-value maintenance.repo "$(pwd)" && > > for schedule in hourly daily weekly > do > test_path_is_file "systemd/user/git-maintenance@$schedule.timer" || return 1 > done && > test_path_is_file "systemd/user/git-maintenance@.service" && > > test_systemd_analyze_verify "systemd/user/git-maintenance@hourly.service" && > test_systemd_analyze_verify "systemd/user/git-maintenance@daily.service" && > test_systemd_analyze_verify "systemd/user/git-maintenance@weekly.service" && > > printf -- "--user enable --now git-maintenance@%s.timer\n" hourly daily weekly >expect && > test_cmp expect args && > > rm -f args && > GIT_TEST_MAINT_SCHEDULER="systemctl:./print-args" git maintenance stop && > > # stop does not unregister the repo > git config --get --global --fixed-value maintenance.repo "$(pwd)" && > > for schedule in hourly daily weekly > do > test_path_is_missing "systemd/user/git-maintenance@$schedule.timer" || return 1 > done && > test_path_is_missing "systemd/user/git-maintenance@.service" && > > printf -- "--user disable --now git-maintenance@%s.timer\n" hourly daily weekly >expect && > test_cmp expect args The rest of the systemd tests only check that the service file are in XDG_CONFIG_HOME, which should not be the case anymore. However, the test does not actually check we have enabled and started the timers as it is , right ? Should I add that ? I'm not sure how, because it does not seem like the tests run in a isolated env, so it would mess with the systemd user manager of the developper running the tests... Regarding systemd-analyze verify, do the tests have access to the source directory in a special way, or is using '../..' enough ? Thanks
On Tue, Mar 19, 2024 at 8:10 AM Max Gautier <mg@max.gautier.name> wrote: > I'm working on updating the test in t7900-maintenance.sh, but I might be > missing something here: > > >test_expect_success 'start and stop Linux/systemd maintenance' ' > > write_script print-args <<-\EOF && > > printf "%s\n" "$*" >>args > > EOF > > > > XDG_CONFIG_HOME="$PWD" && > > export XDG_CONFIG_HOME && > > rm -f args && > > GIT_TEST_MAINT_SCHEDULER="systemctl:./print-args" git maintenance start --scheduler=systemd-timer && > > Do I understand correctly that this means we're not actually running > systemctl here, just printing the arguments to our file ? That's correct. The purpose of GIT_TEST_MAINT_SCHEDULER is twofold. The primary purpose is to test as much as possible without actually mucking with the user's real scheduler-related configuration (whether it be systemd, cron, launchctl, etc.). This means that we want to verify that the expected files are created or removed by git-maintenance, that they are well-formed, and that git-maintenance is invoking the correct platform-specific scheduler-related command with correct arguments (without actually invoking that command and messing up the user's personal configuration). The secondary purpose is to allow these otherwise platform-specific tests to run on any platform. This is possible since, as noted above, we're not actually running the platform-specific scheduler-related command, but instead only capturing the command and arguments that would have been applied had git-maintenace been run "for real" outside of the test framework. > > for schedule in hourly daily weekly > > do > > test_path_is_missing "systemd/user/git-maintenance@$schedule.timer" || return 1 > > done && > > test_path_is_missing "systemd/user/git-maintenance@.service" && > > > > printf -- "--user disable --now git-maintenance@%s.timer\n" hourly daily weekly >expect && > > test_cmp expect args > > The rest of the systemd tests only check that the service file are in > XDG_CONFIG_HOME, which should not be the case anymore. > > However, the test does not actually check we have enabled and started > the timers as it is , right ? Correct. As noted above, we don't want to muck with the user's real configuration, and we certainly don't want the system-specific scheduler to actually kick off some command we're testing in the user's account while the test script is running. > Should I add that ? I'm not sure how, because it does not seem like the > tests run in a isolated env, so it would mess with the systemd user > manager of the developper running the tests... No you don't want to add that since, as you state and as stated above, it would muck with the user's own configuration which would be undesirable. > Regarding systemd-analyze verify, do the tests have access to the source > directory in a special way, or is using '../..' enough ? You can't assume that the source directory is available at "../.." since the --root option (see t/README) allows the root of the tests working-tree to reside outside of the project directory. You may be able to use "$TEST_DIRECTORY/.." to reference files in the source tree, though the documentation in t/test-lib.sh doesn't seem to state explicitly that this is intended or supported use. However, a few existing tests (t1021, t3000, t4023) already access files in the source tree in this fashion, so there is precedent.
Eric Sunshine <sunshine@sunshineco.com> writes: > You may be able to use "$TEST_DIRECTORY/.." to reference files in the > source tree, though the documentation in t/test-lib.sh doesn't seem to > state explicitly that this is intended or supported use. However, a > few existing tests (t1021, t3000, t4023) already access files in the > source tree in this fashion, so there is precedent. I think the following from t/test-lib.sh should give us sufficient assurance. # The TEST_DIRECTORY will always be the path to the "t" # directory in the git.git checkout. This is overridden by # e.g. t/lib-subtest.sh, but only because its $(pwd) is # different. Those tests still set "$TEST_DIRECTORY" to the # same path. Everything you said in your review is correct. Thanks.
Le 19 mars 2024 18:17:27 GMT+01:00, Eric Sunshine <sunshine@sunshineco.com> a écrit : >On Tue, Mar 19, 2024 at 8:10 AM Max Gautier <mg@max.gautier.name> wrote: >> I'm working on updating the test in t7900-maintenance.sh, but I might be >> missing something here: >> >> >test_expect_success 'start and stop Linux/systemd maintenance' ' >> > write_script print-args <<-\EOF && >> > printf "%s\n" "$*" >>args >> > EOF >> > >> > XDG_CONFIG_HOME="$PWD" && >> > export XDG_CONFIG_HOME && >> > rm -f args && >> > GIT_TEST_MAINT_SCHEDULER="systemctl:./print-args" git maintenance start --scheduler=systemd-timer && >> >> Do I understand correctly that this means we're not actually running >> systemctl here, just printing the arguments to our file ? > >That's correct. The purpose of GIT_TEST_MAINT_SCHEDULER is twofold. > >The primary purpose is to test as much as possible without actually >mucking with the user's real scheduler-related configuration (whether >it be systemd, cron, launchctl, etc.). This means that we want to >verify that the expected files are created or removed by >git-maintenance, that they are well-formed, and that git-maintenance >is invoking the correct platform-specific scheduler-related command >with correct arguments (without actually invoking that command and >messing up the user's personal configuration). > >The secondary purpose is to allow these otherwise platform-specific >tests to run on any platform. This is possible since, as noted above, >we're not actually running the platform-specific scheduler-related >command, but instead only capturing the command and arguments that >would have been applied had git-maintenace been run "for real" outside >of the test framework. Ok thanks, now I see why it's done this way. > >> > for schedule in hourly daily weekly >> > do >> > test_path_is_missing "systemd/user/git-maintenance@$schedule.timer" || return 1 >> > done && >> > test_path_is_missing "systemd/user/git-maintenance@.service" && >> > >> > printf -- "--user disable --now git-maintenance@%s.timer\n" hourly daily weekly >expect && >> > test_cmp expect args >> >> The rest of the systemd tests only check that the service file are in >> XDG_CONFIG_HOME, which should not be the case anymore. >> >> However, the test does not actually check we have enabled and started >> the timers as it is , right ? > >Correct. As noted above, we don't want to muck with the user's real >configuration, and we certainly don't want the system-specific >scheduler to actually kick off some command we're testing in the >user's account while the test script is running. > >> Should I add that ? I'm not sure how, because it does not seem like the >> tests run in a isolated env, so it would mess with the systemd user >> manager of the developper running the tests... > >No you don't want to add that since, as you state and as stated above, >it would muck with the user's own configuration which would be >undesirable. > That makes things easier then :) >> Regarding systemd-analyze verify, do the tests have access to the source >> directory in a special way, or is using '../..' enough ? > >You can't assume that the source directory is available at "../.." >since the --root option (see t/README) allows the root of the tests >working-tree to reside outside of the project directory. > >You may be able to use "$TEST_DIRECTORY/.." to reference files in the >source tree, though the documentation in t/test-lib.sh doesn't seem to >state explicitly that this is intended or supported use. However, a >few existing tests (t1021, t3000, t4023) already access files in the >source tree in this fashion, so there is precedent. I'll look into those. Thanks for all the info !
On Mon, Mar 18, 2024 at 04:31:17PM +0100, Max Gautier wrote: > - Remove the code writing the units > - Simplify calling systemctl > - systemctl does not error out when disabling units which aren't > enabled, nor when enabling already enabled units > - call systemctl only once, with all the units. > - Add clean-up code for leftover units in $XDG_CONFIG_HOME created by > previous versions of git, taking care of preserving actual user > override (by checking the start of the file). We very much try to avoid bulleted-list-style commit messages. Once you have a list of changes it very often indicates that there are multiple semi-related things going on in the same commit. At that point we prefer to split the commit up into multiple commits so that each of the bullet items can be explained separately. Also, as mentioned before, commit messages should explain what the problem is and how that problem is solved. Patrick > Signed-off-by: Max Gautier <mg@max.gautier.name> > --- > builtin/gc.c | 293 ++++++++------------------------------------------- > 1 file changed, 45 insertions(+), 248 deletions(-) > > diff --git a/builtin/gc.c b/builtin/gc.c > index cb80ced6cb..981db8e297 100644 > --- a/builtin/gc.c > +++ b/builtin/gc.c > @@ -2308,276 +2308,73 @@ static char *xdg_config_home_systemd(const char *filename) > return xdg_config_home_for("systemd/user", filename); > } > > -#define SYSTEMD_UNIT_FORMAT "git-maintenance@%s.%s" > - > -static int systemd_timer_delete_timer_file(enum schedule_priority priority) > -{ > - int ret = 0; > - const char *frequency = get_frequency(priority); > - char *local_timer_name = xstrfmt(SYSTEMD_UNIT_FORMAT, frequency, "timer"); > - char *filename = xdg_config_home_systemd(local_timer_name); > - > - if (unlink(filename) && !is_missing_file_error(errno)) > - ret = error_errno(_("failed to delete '%s'"), filename); > - > - free(filename); > - free(local_timer_name); > - return ret; > -} > - > -static int systemd_timer_delete_service_template(void) > -{ > - int ret = 0; > - char *local_service_name = xstrfmt(SYSTEMD_UNIT_FORMAT, "", "service"); > - char *filename = xdg_config_home_systemd(local_service_name); > - if (unlink(filename) && !is_missing_file_error(errno)) > - ret = error_errno(_("failed to delete '%s'"), filename); > - > - free(filename); > - free(local_service_name); > - return ret; > -} > - > -/* > - * Write the schedule information into a git-maintenance@<schedule>.timer > - * file using a custom minute. This timer file cannot use the templating > - * system, so we generate a specific file for each. > - */ > -static int systemd_timer_write_timer_file(enum schedule_priority schedule, > - int minute) > -{ > - int res = -1; > - char *filename; > - FILE *file; > - const char *unit; > - char *schedule_pattern = NULL; > - const char *frequency = get_frequency(schedule); > - char *local_timer_name = xstrfmt(SYSTEMD_UNIT_FORMAT, frequency, "timer"); > - > - filename = xdg_config_home_systemd(local_timer_name); > - > - if (safe_create_leading_directories(filename)) { > - error(_("failed to create directories for '%s'"), filename); > - goto error; > - } > - file = fopen_or_warn(filename, "w"); > - if (!file) > - goto error; > - > - switch (schedule) { > - case SCHEDULE_HOURLY: > - schedule_pattern = xstrfmt("*-*-* 1..23:%02d:00", minute); > - break; > - > - case SCHEDULE_DAILY: > - schedule_pattern = xstrfmt("Tue..Sun *-*-* 0:%02d:00", minute); > - break; > - > - case SCHEDULE_WEEKLY: > - schedule_pattern = xstrfmt("Mon 0:%02d:00", minute); > - break; > - > - default: > - BUG("Unhandled schedule_priority"); > - } > - > - unit = "# This file was created and is maintained by Git.\n" > - "# Any edits made in this file might be replaced in the future\n" > - "# by a Git command.\n" > - "\n" > - "[Unit]\n" > - "Description=Optimize Git repositories data\n" > - "\n" > - "[Timer]\n" > - "OnCalendar=%s\n" > - "Persistent=true\n" > - "\n" > - "[Install]\n" > - "WantedBy=timers.target\n"; > - if (fprintf(file, unit, schedule_pattern) < 0) { > - error(_("failed to write to '%s'"), filename); > - fclose(file); > - goto error; > - } > - if (fclose(file) == EOF) { > - error_errno(_("failed to flush '%s'"), filename); > - goto error; > - } > - > - res = 0; > - > -error: > - free(schedule_pattern); > - free(local_timer_name); > - free(filename); > - return res; > -} > - > -/* > - * No matter the schedule, we use the same service and can make use of the > - * templating system. When installing git-maintenance@<schedule>.timer, > - * systemd will notice that git-maintenance@.service exists as a template > - * and will use this file and insert the <schedule> into the template at > - * the position of "%i". > - */ > -static int systemd_timer_write_service_template(const char *exec_path) > -{ > - int res = -1; > - char *filename; > - FILE *file; > - const char *unit; > - char *local_service_name = xstrfmt(SYSTEMD_UNIT_FORMAT, "", "service"); > - > - filename = xdg_config_home_systemd(local_service_name); > - if (safe_create_leading_directories(filename)) { > - error(_("failed to create directories for '%s'"), filename); > - goto error; > - } > - file = fopen_or_warn(filename, "w"); > - if (!file) > - goto error; > - > - unit = "# This file was created and is maintained by Git.\n" > - "# Any edits made in this file might be replaced in the future\n" > - "# by a Git command.\n" > - "\n" > - "[Unit]\n" > - "Description=Optimize Git repositories data\n" > - "\n" > - "[Service]\n" > - "Type=oneshot\n" > - "ExecStart=\"%s/git\" --exec-path=\"%s\" for-each-repo --config=maintenance.repo maintenance run --schedule=%%i\n" > - "LockPersonality=yes\n" > - "MemoryDenyWriteExecute=yes\n" > - "NoNewPrivileges=yes\n" > - "RestrictAddressFamilies=AF_UNIX AF_INET AF_INET6 AF_VSOCK\n" > - "RestrictNamespaces=yes\n" > - "RestrictRealtime=yes\n" > - "RestrictSUIDSGID=yes\n" > - "SystemCallArchitectures=native\n" > - "SystemCallFilter=@system-service\n"; > - if (fprintf(file, unit, exec_path, exec_path) < 0) { > - error(_("failed to write to '%s'"), filename); > - fclose(file); > - goto error; > - } > - if (fclose(file) == EOF) { > - error_errno(_("failed to flush '%s'"), filename); > - goto error; > - } > - > - res = 0; > - > -error: > - free(local_service_name); > - free(filename); > - return res; > -} > - > -static int systemd_timer_enable_unit(int enable, > - enum schedule_priority schedule, > - int minute) > +static int systemd_set_units_state(int enable) > { > const char *cmd = "systemctl"; > struct child_process child = CHILD_PROCESS_INIT; > - const char *frequency = get_frequency(schedule); > - > - /* > - * Disabling the systemd unit while it is already disabled makes > - * systemctl print an error. > - * Let's ignore it since it means we already are in the expected state: > - * the unit is disabled. > - * > - * On the other hand, enabling a systemd unit which is already enabled > - * produces no error. > - */ > - if (!enable) > - child.no_stderr = 1; > - else if (systemd_timer_write_timer_file(schedule, minute)) > - return -1; > > get_schedule_cmd(&cmd, NULL); > strvec_split(&child.args, cmd); > - strvec_pushl(&child.args, "--user", enable ? "enable" : "disable", > - "--now", NULL); > - strvec_pushf(&child.args, SYSTEMD_UNIT_FORMAT, frequency, "timer"); > + > + strvec_pushl(&child.args, "--user", "--force", "--now", > + enable ? "enable" : "disable", > + "git-maintenance@hourly.timer", > + "git-maintenance@daily.timer", > + "git-maintenance@weekly.timer", NULL); > + /* > + ** --force override existing conflicting symlinks > + ** We need it because the units have changed location (~/.config -> > + ** /usr/lib) > + */ > > if (start_command(&child)) > return error(_("failed to start systemctl")); > if (finish_command(&child)) > - /* > - * Disabling an already disabled systemd unit makes > - * systemctl fail. > - * Let's ignore this failure. > - * > - * Enabling an enabled systemd unit doesn't fail. > - */ > - if (enable) > - return error(_("failed to run systemctl")); > + return error(_("failed to run systemctl")); > return 0; > } > > -/* > - * A previous version of Git wrote the timer units as template files. > - * Clean these up, if they exist. > - */ > -static void systemd_timer_delete_stale_timer_templates(void) > +static void systemd_delete_user_unit(char const *unit) > { > - char *timer_template_name = xstrfmt(SYSTEMD_UNIT_FORMAT, "", "timer"); > - char *filename = xdg_config_home_systemd(timer_template_name); > + char const file_start_stale[] = "# This file was created and is" > + " maintained by Git."; > + char file_start_user[sizeof(file_start_stale)] = {'\0'}; > + > + char *filename = xdg_config_home_systemd(unit); > + int handle = open(filename, O_RDONLY); > > - if (unlink(filename) && !is_missing_file_error(errno)) > + /* > + ** Check this is actually our file and we're not removing a legitimate > + ** user override. > + */ > + if (handle == -1 && !is_missing_file_error(errno)) > warning(_("failed to delete '%s'"), filename); > + else { > + read(handle, file_start_user, sizeof(file_start_stale) - 1); > + close(handle); > + if (strcmp(file_start_stale, file_start_user) == 0) { > + if (unlink(filename) == 0) > + warning(_("deleted stale unit file '%s'"), filename); > + else if (!is_missing_file_error(errno)) > + warning(_("failed to delete '%s'"), filename); > + } > + } > > free(filename); > - free(timer_template_name); > -} > - > -static int systemd_timer_delete_unit_files(void) > -{ > - systemd_timer_delete_stale_timer_templates(); > - > - /* Purposefully not short-circuited to make sure all are called. */ > - return systemd_timer_delete_timer_file(SCHEDULE_HOURLY) | > - systemd_timer_delete_timer_file(SCHEDULE_DAILY) | > - systemd_timer_delete_timer_file(SCHEDULE_WEEKLY) | > - systemd_timer_delete_service_template(); > -} > - > -static int systemd_timer_delete_units(void) > -{ > - int minute = get_random_minute(); > - /* Purposefully not short-circuited to make sure all are called. */ > - return systemd_timer_enable_unit(0, SCHEDULE_HOURLY, minute) | > - systemd_timer_enable_unit(0, SCHEDULE_DAILY, minute) | > - systemd_timer_enable_unit(0, SCHEDULE_WEEKLY, minute) | > - systemd_timer_delete_unit_files(); > -} > - > -static int systemd_timer_setup_units(void) > -{ > - int minute = get_random_minute(); > - const char *exec_path = git_exec_path(); > - > - int ret = systemd_timer_write_service_template(exec_path) || > - systemd_timer_enable_unit(1, SCHEDULE_HOURLY, minute) || > - systemd_timer_enable_unit(1, SCHEDULE_DAILY, minute) || > - systemd_timer_enable_unit(1, SCHEDULE_WEEKLY, minute); > - > - if (ret) > - systemd_timer_delete_units(); > - else > - systemd_timer_delete_stale_timer_templates(); > - > - return ret; > } > > static int systemd_timer_update_schedule(int run_maintenance, int fd UNUSED) > { > - if (run_maintenance) > - return systemd_timer_setup_units(); > - else > - return systemd_timer_delete_units(); > + /* > + * A previous version of Git wrote the units in the user configuration > + * directory. Clean these up, if they exist. > + */ > + systemd_delete_user_unit("git-maintenance@hourly.timer"); > + systemd_delete_user_unit("git-maintenance@daily.timer"); > + systemd_delete_user_unit("git-maintenance@weekly.timer"); > + systemd_delete_user_unit("git-maintenance@.timer"); > + systemd_delete_user_unit("git-maintenance@.service"); > + return systemd_set_units_state(run_maintenance); > } > > enum scheduler { > -- > 2.44.0 > >
On Thu, Mar 21, 2024 at 01:37:41PM +0100, Patrick Steinhardt wrote: > On Mon, Mar 18, 2024 at 04:31:17PM +0100, Max Gautier wrote: > > - Remove the code writing the units > > - Simplify calling systemctl > > - systemctl does not error out when disabling units which aren't > > enabled, nor when enabling already enabled units > > - call systemctl only once, with all the units. > > - Add clean-up code for leftover units in $XDG_CONFIG_HOME created by > > previous versions of git, taking care of preserving actual user > > override (by checking the start of the file). > > We very much try to avoid bulleted-list-style commit messages. Once you > have a list of changes it very often indicates that there are multiple > semi-related things going on in the same commit. At that point we prefer > to split the commit up into multiple commits so that each of the bullet > items can be explained separately. > > Also, as mentioned before, commit messages should explain what the > problem is and how that problem is solved. > > Patrick > I need to think a bit about patch ordering here because changing from the "random minute" implementation to the systemd (RandomDelaySec/FixedRandomDelay) is required to the other changes of putting the unit out of the code and in the package. I think I have an idea how, I'll work on that, thanks.
diff --git a/builtin/gc.c b/builtin/gc.c index cb80ced6cb..981db8e297 100644 --- a/builtin/gc.c +++ b/builtin/gc.c @@ -2308,276 +2308,73 @@ static char *xdg_config_home_systemd(const char *filename) return xdg_config_home_for("systemd/user", filename); } -#define SYSTEMD_UNIT_FORMAT "git-maintenance@%s.%s" - -static int systemd_timer_delete_timer_file(enum schedule_priority priority) -{ - int ret = 0; - const char *frequency = get_frequency(priority); - char *local_timer_name = xstrfmt(SYSTEMD_UNIT_FORMAT, frequency, "timer"); - char *filename = xdg_config_home_systemd(local_timer_name); - - if (unlink(filename) && !is_missing_file_error(errno)) - ret = error_errno(_("failed to delete '%s'"), filename); - - free(filename); - free(local_timer_name); - return ret; -} - -static int systemd_timer_delete_service_template(void) -{ - int ret = 0; - char *local_service_name = xstrfmt(SYSTEMD_UNIT_FORMAT, "", "service"); - char *filename = xdg_config_home_systemd(local_service_name); - if (unlink(filename) && !is_missing_file_error(errno)) - ret = error_errno(_("failed to delete '%s'"), filename); - - free(filename); - free(local_service_name); - return ret; -} - -/* - * Write the schedule information into a git-maintenance@<schedule>.timer - * file using a custom minute. This timer file cannot use the templating - * system, so we generate a specific file for each. - */ -static int systemd_timer_write_timer_file(enum schedule_priority schedule, - int minute) -{ - int res = -1; - char *filename; - FILE *file; - const char *unit; - char *schedule_pattern = NULL; - const char *frequency = get_frequency(schedule); - char *local_timer_name = xstrfmt(SYSTEMD_UNIT_FORMAT, frequency, "timer"); - - filename = xdg_config_home_systemd(local_timer_name); - - if (safe_create_leading_directories(filename)) { - error(_("failed to create directories for '%s'"), filename); - goto error; - } - file = fopen_or_warn(filename, "w"); - if (!file) - goto error; - - switch (schedule) { - case SCHEDULE_HOURLY: - schedule_pattern = xstrfmt("*-*-* 1..23:%02d:00", minute); - break; - - case SCHEDULE_DAILY: - schedule_pattern = xstrfmt("Tue..Sun *-*-* 0:%02d:00", minute); - break; - - case SCHEDULE_WEEKLY: - schedule_pattern = xstrfmt("Mon 0:%02d:00", minute); - break; - - default: - BUG("Unhandled schedule_priority"); - } - - unit = "# This file was created and is maintained by Git.\n" - "# Any edits made in this file might be replaced in the future\n" - "# by a Git command.\n" - "\n" - "[Unit]\n" - "Description=Optimize Git repositories data\n" - "\n" - "[Timer]\n" - "OnCalendar=%s\n" - "Persistent=true\n" - "\n" - "[Install]\n" - "WantedBy=timers.target\n"; - if (fprintf(file, unit, schedule_pattern) < 0) { - error(_("failed to write to '%s'"), filename); - fclose(file); - goto error; - } - if (fclose(file) == EOF) { - error_errno(_("failed to flush '%s'"), filename); - goto error; - } - - res = 0; - -error: - free(schedule_pattern); - free(local_timer_name); - free(filename); - return res; -} - -/* - * No matter the schedule, we use the same service and can make use of the - * templating system. When installing git-maintenance@<schedule>.timer, - * systemd will notice that git-maintenance@.service exists as a template - * and will use this file and insert the <schedule> into the template at - * the position of "%i". - */ -static int systemd_timer_write_service_template(const char *exec_path) -{ - int res = -1; - char *filename; - FILE *file; - const char *unit; - char *local_service_name = xstrfmt(SYSTEMD_UNIT_FORMAT, "", "service"); - - filename = xdg_config_home_systemd(local_service_name); - if (safe_create_leading_directories(filename)) { - error(_("failed to create directories for '%s'"), filename); - goto error; - } - file = fopen_or_warn(filename, "w"); - if (!file) - goto error; - - unit = "# This file was created and is maintained by Git.\n" - "# Any edits made in this file might be replaced in the future\n" - "# by a Git command.\n" - "\n" - "[Unit]\n" - "Description=Optimize Git repositories data\n" - "\n" - "[Service]\n" - "Type=oneshot\n" - "ExecStart=\"%s/git\" --exec-path=\"%s\" for-each-repo --config=maintenance.repo maintenance run --schedule=%%i\n" - "LockPersonality=yes\n" - "MemoryDenyWriteExecute=yes\n" - "NoNewPrivileges=yes\n" - "RestrictAddressFamilies=AF_UNIX AF_INET AF_INET6 AF_VSOCK\n" - "RestrictNamespaces=yes\n" - "RestrictRealtime=yes\n" - "RestrictSUIDSGID=yes\n" - "SystemCallArchitectures=native\n" - "SystemCallFilter=@system-service\n"; - if (fprintf(file, unit, exec_path, exec_path) < 0) { - error(_("failed to write to '%s'"), filename); - fclose(file); - goto error; - } - if (fclose(file) == EOF) { - error_errno(_("failed to flush '%s'"), filename); - goto error; - } - - res = 0; - -error: - free(local_service_name); - free(filename); - return res; -} - -static int systemd_timer_enable_unit(int enable, - enum schedule_priority schedule, - int minute) +static int systemd_set_units_state(int enable) { const char *cmd = "systemctl"; struct child_process child = CHILD_PROCESS_INIT; - const char *frequency = get_frequency(schedule); - - /* - * Disabling the systemd unit while it is already disabled makes - * systemctl print an error. - * Let's ignore it since it means we already are in the expected state: - * the unit is disabled. - * - * On the other hand, enabling a systemd unit which is already enabled - * produces no error. - */ - if (!enable) - child.no_stderr = 1; - else if (systemd_timer_write_timer_file(schedule, minute)) - return -1; get_schedule_cmd(&cmd, NULL); strvec_split(&child.args, cmd); - strvec_pushl(&child.args, "--user", enable ? "enable" : "disable", - "--now", NULL); - strvec_pushf(&child.args, SYSTEMD_UNIT_FORMAT, frequency, "timer"); + + strvec_pushl(&child.args, "--user", "--force", "--now", + enable ? "enable" : "disable", + "git-maintenance@hourly.timer", + "git-maintenance@daily.timer", + "git-maintenance@weekly.timer", NULL); + /* + ** --force override existing conflicting symlinks + ** We need it because the units have changed location (~/.config -> + ** /usr/lib) + */ if (start_command(&child)) return error(_("failed to start systemctl")); if (finish_command(&child)) - /* - * Disabling an already disabled systemd unit makes - * systemctl fail. - * Let's ignore this failure. - * - * Enabling an enabled systemd unit doesn't fail. - */ - if (enable) - return error(_("failed to run systemctl")); + return error(_("failed to run systemctl")); return 0; } -/* - * A previous version of Git wrote the timer units as template files. - * Clean these up, if they exist. - */ -static void systemd_timer_delete_stale_timer_templates(void) +static void systemd_delete_user_unit(char const *unit) { - char *timer_template_name = xstrfmt(SYSTEMD_UNIT_FORMAT, "", "timer"); - char *filename = xdg_config_home_systemd(timer_template_name); + char const file_start_stale[] = "# This file was created and is" + " maintained by Git."; + char file_start_user[sizeof(file_start_stale)] = {'\0'}; + + char *filename = xdg_config_home_systemd(unit); + int handle = open(filename, O_RDONLY); - if (unlink(filename) && !is_missing_file_error(errno)) + /* + ** Check this is actually our file and we're not removing a legitimate + ** user override. + */ + if (handle == -1 && !is_missing_file_error(errno)) warning(_("failed to delete '%s'"), filename); + else { + read(handle, file_start_user, sizeof(file_start_stale) - 1); + close(handle); + if (strcmp(file_start_stale, file_start_user) == 0) { + if (unlink(filename) == 0) + warning(_("deleted stale unit file '%s'"), filename); + else if (!is_missing_file_error(errno)) + warning(_("failed to delete '%s'"), filename); + } + } free(filename); - free(timer_template_name); -} - -static int systemd_timer_delete_unit_files(void) -{ - systemd_timer_delete_stale_timer_templates(); - - /* Purposefully not short-circuited to make sure all are called. */ - return systemd_timer_delete_timer_file(SCHEDULE_HOURLY) | - systemd_timer_delete_timer_file(SCHEDULE_DAILY) | - systemd_timer_delete_timer_file(SCHEDULE_WEEKLY) | - systemd_timer_delete_service_template(); -} - -static int systemd_timer_delete_units(void) -{ - int minute = get_random_minute(); - /* Purposefully not short-circuited to make sure all are called. */ - return systemd_timer_enable_unit(0, SCHEDULE_HOURLY, minute) | - systemd_timer_enable_unit(0, SCHEDULE_DAILY, minute) | - systemd_timer_enable_unit(0, SCHEDULE_WEEKLY, minute) | - systemd_timer_delete_unit_files(); -} - -static int systemd_timer_setup_units(void) -{ - int minute = get_random_minute(); - const char *exec_path = git_exec_path(); - - int ret = systemd_timer_write_service_template(exec_path) || - systemd_timer_enable_unit(1, SCHEDULE_HOURLY, minute) || - systemd_timer_enable_unit(1, SCHEDULE_DAILY, minute) || - systemd_timer_enable_unit(1, SCHEDULE_WEEKLY, minute); - - if (ret) - systemd_timer_delete_units(); - else - systemd_timer_delete_stale_timer_templates(); - - return ret; } static int systemd_timer_update_schedule(int run_maintenance, int fd UNUSED) { - if (run_maintenance) - return systemd_timer_setup_units(); - else - return systemd_timer_delete_units(); + /* + * A previous version of Git wrote the units in the user configuration + * directory. Clean these up, if they exist. + */ + systemd_delete_user_unit("git-maintenance@hourly.timer"); + systemd_delete_user_unit("git-maintenance@daily.timer"); + systemd_delete_user_unit("git-maintenance@weekly.timer"); + systemd_delete_user_unit("git-maintenance@.timer"); + systemd_delete_user_unit("git-maintenance@.service"); + return systemd_set_units_state(run_maintenance); } enum scheduler {
- Remove the code writing the units - Simplify calling systemctl - systemctl does not error out when disabling units which aren't enabled, nor when enabling already enabled units - call systemctl only once, with all the units. - Add clean-up code for leftover units in $XDG_CONFIG_HOME created by previous versions of git, taking care of preserving actual user override (by checking the start of the file). Signed-off-by: Max Gautier <mg@max.gautier.name> --- builtin/gc.c | 293 ++++++++------------------------------------------- 1 file changed, 45 insertions(+), 248 deletions(-)