{"thread":{"id":"61181","subject":"[PATCH v2 4/6] maintenance: cleanup $XDG_CONFIG_HOME/systemd/user","startedAt":"2024-03-22T22:14:01Z","lastAt":"2024-03-27T16:21:15Z","messageCount":30,"participants":["Max Gautier","Kristoffer Haugsbakk","Junio C Hamano","Eric Sunshine","Phillip Wood","phillip.wood123@gmail.com"],"isPatch":true,"patchVersion":2,"patchTotal":6},"messages":[{"id":"491268","messageId":"20240322221327.12204-5-mg@max.gautier.name","threadId":"61181","inReplyTo":"20240322221327.12204-1-mg@max.gautier.name","subject":"[PATCH v2 4/6] maintenance: cleanup $XDG_CONFIG_HOME/systemd/user","fromName":"Max Gautier","fromEmail":"mg@max.gautier.name","sentAt":"2024-03-22T22:11:09Z","receivedAt":"2024-03-22T22:14:01Z","isPatch":true,"sender":{"key":"mg@max.gautier.name","avatar":"https://avatars.githubusercontent.com/u/13346812?v=4"},"body":"Before commit 976640edbb (maintenance: use packaged systemd units,\n2024-03-21), we we're putting systemd unit files in $XDG_CONFIG_HOME ;\nthese could mask those we are now distributing as part of git.\n\nRemove all the systemd units possibly created by previous version of\ngit when running `git maintenance start/stop`.\nAvoid overwriting units we didn't write, by comparing the first line\nwith the start of the comment we added to our unit files previously.\n\nSigned-off-by: Max Gautier \n---\n\nNotes:\n How should I refer to a commit which is part of the same patch series ?\n The commit id will change so the message won't be correct anymore, right\n ?\n\n builtin/gc.c | 42 ++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 42 insertions(+)\n\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex aaee91451a..99b158e481 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -2329,8 +2329,50 @@ static int systemd_set_units_state(int enable)\n \treturn 0;\n }\n \n+/*\n+ * TODO: in the future (~2026 ?) remove this cleanup code\n+ */\n+static void systemd_delete_user_unit(char const *unit)\n+{\n+\tchar const\tfile_start_stale[] =\t\"# This file was created and is\"\n+\t\t\t\t\t\t\" maintained by Git.\";\n+\tchar\t\tfile_start_user[sizeof(file_start_stale)] = {'\\0'};\n+\n+\tchar *filename = xdg_config_home_for(\"systemd/user\", unit);\n+\tint handle = open(filename, O_RDONLY);\n+\n+\t/*\n+\t * Check this is actually our file and we're not removing a legitimate\n+\t * user override.\n+\t */\n+\tif (handle == -1 && !is_missing_file_error(errno))\n+\t\twarning(_(\"failed to delete '%s'\"), filename);\n+\telse {\n+\t\tread(handle, file_start_user, sizeof(file_start_stale) - 1);\n+\t\tclose(handle);\n+\t\tif (strcmp(file_start_stale, file_start_user) == 0) {\n+\t\t\tif (unlink(filename) == 0)\n+\t\t\t\twarning(_(\"deleted stale unit file '%s'\"), filename);\n+\t\t\telse if (!is_missing_file_error(errno))\n+\t\t\t\twarning(_(\"failed to delete '%s'\"), filename);\n+\t\t}\n+\t}\n+\n+\tfree(filename);\n+}\n+\n static int systemd_timer_update_schedule(int run_maintenance, int fd UNUSED)\n {\n+\t/*\n+\t * A previous version of Git wrote the units in the user configuration\n+\t * directory. Clean these up, if they exist.\n+\t * TODO: in the future (~2026 ?) remove this cleanup code\n+\t */\n+\tsystemd_delete_user_unit(\"git-maintenance@hourly.timer\");\n+\tsystemd_delete_user_unit(\"git-maintenance@daily.timer\");\n+\tsystemd_delete_user_unit(\"git-maintenance@weekly.timer\");\n+\tsystemd_delete_user_unit(\"git-maintenance@.timer\");\n+\tsystemd_delete_user_unit(\"git-maintenance@.service\");\n \treturn systemd_set_units_state(run_maintenance);\n }\n \n-- \n2.44.0\n\n"},{"id":"491269","messageId":"20240322221327.12204-1-mg@max.gautier.name","threadId":"61181","inReplyTo":"20240318153257.27451-1-mg@max.gautier.name","subject":"[PATCH v2 0/6] maintenance: use packaged systemd units","fromName":"Max Gautier","fromEmail":"mg@max.gautier.name","sentAt":"2024-03-22T22:11:05Z","receivedAt":"2024-03-22T22:14:01Z","isPatch":true,"sender":{"key":"mg@max.gautier.name","avatar":"https://avatars.githubusercontent.com/u/13346812?v=4"},"body":"* Distribute the systemd timers used by the `git maintenance start` with\n the systemd scheduler as part of git, rather than writing them in\n $XDG_CONFIG_HOME.\n\nThis allows users to override the units if they wish, and is more\nin-line with the usual practices of distribution for systemd units.\n\nWe also move away from using the random minute, and instead rely on\nsystemd features to achieve the same goal (see patch 2). This allows us\nto go back to using unit templating for the timers. This is also a\nprerequisite to have static unit files.\n\nNote that even if we really need more specific OnCalendar= settings for\neach timer, we should still do it that way, but instead distribute\noverride alongside the template, for instance for weekly:\n\n/usr/lib/systemd-user/git-maintenance@daily.timer.d/override.conf:\n[Timer]\nOnCalendar=\n\nThe cleanup code for the units written in $XDG_CONFIG_HOME is adapted,\nand takes care of not removing legitimate user overrides, by checking\nthe file start.\n\nTesting:\nThe simplest way to simulate having the units in /usr/lib is probably to\ncopy them in /etc/systemd/user.\n\nChanges since v1:\n- Reorganization of the commits and their messages to try to address\n review comments\n- Dropped the DON'T APPLY PATCH, added a TODO to the cleanup code\n instead\n- Updated the git-maintenance tests to work with the new logic.\n- Conditional installation of the units files\n- Fixing some style/consistency issues\n- template the systemd service file to use $(bindir)\n\nMax Gautier (6):\n maintenance: use systemd timers builtin randomization\n maintenance: use packaged systemd units\n maintenance: simplify systemctl calls\n maintenance: cleanup $XDG_CONFIG_HOME/systemd/user\n maintenance: update systemd scheduler docs\n maintenance: update tests for systemd scheduler\n\n Documentation/git-maintenance.txt | 33 ++-\n Makefile | 5 +\n builtin/gc.c | 298 ++++-------------------\n config.mak.uname | 10 +\n systemd/user/git-maintenance@.service.in | 17 ++\n systemd/user/git-maintenance@.timer | 12 +\n t/t7900-maintenance.sh | 50 ++--\n 7 files changed, 126 insertions(+), 299 deletions(-)\n create mode 100644 systemd/user/git-maintenance@.service.in\n create mode 100644 systemd/user/git-maintenance@.timer\n\nRange-diff against v1:\n1: ea54a6e50e < -: ---------- maintenance: package systemd units\n2: b29dbb9fdd < -: ---------- maintenance: use packaged systemd units\n3: 47bd6712b8 < -: ---------- maintenance: add fixed random delay to systemd timers\n-: ---------- > 1: 42d88c7f81 maintenance: use systemd timers builtin randomization\n-: ---------- > 2: 18d51b1dd1 maintenance: use packaged systemd units\n-: ---------- > 3: 3aa7446e95 maintenance: simplify systemctl calls\n-: ---------- > 4: daff7b4d60 maintenance: cleanup $XDG_CONFIG_HOME/systemd/user\n4: fac57db55e ! 5: 5f6a8e141f maintenance: update systemd scheduler docs\n @@ Metadata\n ## Commit message ##\n maintenance: update systemd scheduler docs\n \n + The `git maintenance` systemd scheduler no longer writes units in\n + $XDG_CONFIG_HOME.\n +\n + Describe the new behavior.\n + Instead of explaining manual ways to modify the timer, suggest the\n + systemd standard tool: `systemctl edit`.\n +\n Signed-off-by: Max Gautier \n \n ## Documentation/git-maintenance.txt ##\n5: d888fbd0c3 < -: ---------- DON'T APPLY YET: maintenance: remove cleanup code\n-: ---------- > 6: 4d4bcd6233 maintenance: update tests for systemd scheduler\n-- \n2.44.0\n\n"},{"id":"491270","messageId":"20240322221327.12204-3-mg@max.gautier.name","threadId":"61181","inReplyTo":"20240322221327.12204-1-mg@max.gautier.name","subject":"[PATCH v2 2/6] maintenance: use packaged systemd units","fromName":"Max Gautier","fromEmail":"mg@max.gautier.name","sentAt":"2024-03-22T22:11:07Z","receivedAt":"2024-03-22T22:14:01Z","isPatch":true,"sender":{"key":"mg@max.gautier.name","avatar":"https://avatars.githubusercontent.com/u/13346812?v=4"},"body":"Currently, `git maintenance start`, if it uses the systemd scheduler,\nwill create systemd user units in $XDG_CONFIG_HOME, and overwrite the\nfiles on each invocation.\nConversely, `git maintenance stop` will remove the unit files.\n\nThis is problematic mostly because it steps on user toes (and their\nconfiguration, also). You can't mask the git maintenance systemd units,\nbecause they are in $XDG_CONFIG_HOME and you can only mask \"vendor\"\nunits (see man systemctl), you can't override them either.\n\nAdditionally, it necessitates creating, modifying and removing files\nfrom builtin/gc.c, which adds code and some avoidable complexity.\n\nPackage the systemd user units (timer and service) with git in\n$(prefix)/lib/systemd/user (or $XDG_DATA_HOME for $HOME installation),\nand remove code for writing and deleting the units from builtin/gc.c.\nDetermine the correct git path at install time by for the service unit.\n\nDetect systemd timers support (at install time) by relying on systemctl\npresence, since we use it as the control interface for the systemd\nscheduler.\n\nSigned-off-by: Max Gautier \n---\n\nNotes:\n I'm not completely sure if it's ok to do install time templating like\n this, but I couldn't find a similar enough example in the Makefile. Any\n suggestion for a better way ?\n\n Makefile | 5 +\n builtin/gc.c | 117 +----------------------\n config.mak.uname | 10 ++\n systemd/user/git-maintenance@.service.in | 17 ++++\n systemd/user/git-maintenance@.timer | 12 +++\n 5 files changed, 49 insertions(+), 112 deletions(-)\n create mode 100644 systemd/user/git-maintenance@.service.in\n create mode 100644 systemd/user/git-maintenance@.timer\n\ndiff --git a/Makefile b/Makefile\nindex 4e255c81f2..4fb015478e 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -3469,6 +3469,11 @@ install: all\n \t$(INSTALL) -m 644 $(SCRIPT_LIB) '$(DESTDIR_SQ)$(gitexec_instdir_SQ)'\n \t$(INSTALL) $(INSTALL_STRIP) $(install_bindir_xprograms) '$(DESTDIR_SQ)$(bindir_SQ)'\n \t$(INSTALL) $(BINDIR_PROGRAMS_NO_X) '$(DESTDIR_SQ)$(bindir_SQ)'\n+ifdef SYSTEMD_USER_UNIT_DIR\n+\t$(INSTALL) -Dm 644 -t '$(DESTDIR_SQ)$(SYSTEMD_USER_UNIT_DIR)' systemd/user/git-maintenance@.timer\n+\tsed 's+@BINDIR@+$(bindir_SQ)+' systemd/user/git-maintenance@.service.in | \\\n+\t\t$(INSTALL) -Dm 644 /dev/stdin '$(DESTDIR_SQ)$(SYSTEMD_USER_UNIT_DIR)/git-maintenance@.service'\n+endif\n \n ifdef MSVC\n \t# We DO NOT install the individual foo.o.pdb files because they\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex dac59414f0..199c8e6240 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -2303,110 +2303,6 @@ static int is_systemd_timer_available(void)\n \treturn real_is_systemd_timer_available();\n }\n \n-static char *xdg_config_home_systemd(const char *filename)\n-{\n-\treturn xdg_config_home_for(\"systemd/user\", filename);\n-}\n-\n-static int systemd_timer_delete_unit_templates(void)\n-{\n-\tint ret = 0;\n-\tchar *filename = xdg_config_home_systemd(\"git-maintenance@.timer\");\n-\tif (unlink(filename) && !is_missing_file_error(errno))\n-\t\tret = error_errno(_(\"failed to delete '%s'\"), filename);\n-\tFREE_AND_NULL(filename);\n-\n-\tfilename = xdg_config_home_systemd(\"git-maintenance@.service\");\n-\tif (unlink(filename) && !is_missing_file_error(errno))\n-\t\tret = error_errno(_(\"failed to delete '%s'\"), filename);\n-\n-\tfree(filename);\n-\treturn ret;\n-}\n-\n-static int systemd_timer_write_unit_templates(const char *exec_path)\n-{\n-\tchar *filename;\n-\tFILE *file;\n-\tconst char *unit;\n-\n-\tfilename = xdg_config_home_systemd(\"git-maintenance@.timer\");\n-\tif (safe_create_leading_directories(filename)) {\n-\t\terror(_(\"failed to create directories for '%s'\"), filename);\n-\t\tgoto error;\n-\t}\n-\tfile = fopen_or_warn(filename, \"w\");\n-\tif (!file)\n-\t\tgoto error;\n-\n-\tunit = \"# This file was created and is maintained by Git.\\n\"\n-\t \"# Any edits made in this file might be replaced in the future\\n\"\n-\t \"# by a Git command.\\n\"\n-\t \"\\n\"\n-\t \"[Unit]\\n\"\n-\t \"Description=Optimize Git repositories data\\n\"\n-\t \"\\n\"\n-\t \"[Timer]\\n\"\n-\t \"OnCalendar=%i\\n\"\n-\t \"Persistent=true\\n\"\n-\t \"RandomizedDelaySecond=3540\\n\"\n-\t \"FixedRandomDelay=true\\n\"\n-\t \"\\n\"\n-\t \"[Install]\\n\"\n-\t \"WantedBy=timers.target\\n\";\n-\tif (fputs(unit, file) == EOF) {\n-\t\terror(_(\"failed to write to '%s'\"), filename);\n-\t\tfclose(file);\n-\t\tgoto error;\n-\t}\n-\tif (fclose(file) == EOF) {\n-\t\terror_errno(_(\"failed to flush '%s'\"), filename);\n-\t\tgoto error;\n-\t}\n-\tfree(filename);\n-\n-\tfilename = xdg_config_home_systemd(\"git-maintenance@.service\");\n-\tfile = fopen_or_warn(filename, \"w\");\n-\tif (!file)\n-\t\tgoto error;\n-\n-\tunit = \"# This file was created and is maintained by Git.\\n\"\n-\t \"# Any edits made in this file might be replaced in the future\\n\"\n-\t \"# by a Git command.\\n\"\n-\t \"\\n\"\n-\t \"[Unit]\\n\"\n-\t \"Description=Optimize Git repositories data\\n\"\n-\t \"\\n\"\n-\t \"[Service]\\n\"\n-\t \"Type=oneshot\\n\"\n-\t \"ExecStart=\\\"%s/git\\\" --exec-path=\\\"%s\\\" for-each-repo --config=maintenance.repo maintenance run --schedule=%%i\\n\"\n-\t \"LockPersonality=yes\\n\"\n-\t \"MemoryDenyWriteExecute=yes\\n\"\n-\t \"NoNewPrivileges=yes\\n\"\n-\t \"RestrictAddressFamilies=AF_UNIX AF_INET AF_INET6 AF_VSOCK\\n\"\n-\t \"RestrictNamespaces=yes\\n\"\n-\t \"RestrictRealtime=yes\\n\"\n-\t \"RestrictSUIDSGID=yes\\n\"\n-\t \"SystemCallArchitectures=native\\n\"\n-\t \"SystemCallFilter=@system-service\\n\";\n-\tif (fprintf(file, unit, exec_path, exec_path) < 0) {\n-\t\terror(_(\"failed to write to '%s'\"), filename);\n-\t\tfclose(file);\n-\t\tgoto error;\n-\t}\n-\tif (fclose(file) == EOF) {\n-\t\terror_errno(_(\"failed to flush '%s'\"), filename);\n-\t\tgoto error;\n-\t}\n-\tfree(filename);\n-\treturn 0;\n-\n-error:\n-\tfree(filename);\n-\tsystemd_timer_delete_unit_templates();\n-\treturn -1;\n-}\n-\n static int systemd_timer_enable_unit(int enable,\n \t\t\t\t enum schedule_priority schedule)\n {\n@@ -2447,24 +2343,21 @@ static int systemd_timer_enable_unit(int enable,\n \treturn 0;\n }\n \n-static int systemd_timer_delete_units(void)\n+static int systemd_timer_disable_units(void)\n {\n \treturn systemd_timer_enable_unit(0, SCHEDULE_HOURLY) ||\n \t systemd_timer_enable_unit(0, SCHEDULE_DAILY) ||\n-\t systemd_timer_enable_unit(0, SCHEDULE_WEEKLY) ||\n-\t systemd_timer_delete_unit_templates();\n+\t systemd_timer_enable_unit(0, SCHEDULE_WEEKLY);\n }\n \n static int systemd_timer_setup_units(void)\n {\n-\tconst char *exec_path = git_exec_path();\n \n-\tint ret = systemd_timer_write_unit_templates(exec_path) ||\n-\t\t systemd_timer_enable_unit(1, SCHEDULE_HOURLY) ||\n+\tint ret = systemd_timer_enable_unit(1, SCHEDULE_HOURLY) ||\n \t\t systemd_timer_enable_unit(1, SCHEDULE_DAILY) ||\n \t\t systemd_timer_enable_unit(1, SCHEDULE_WEEKLY);\n \tif (ret)\n-\t\tsystemd_timer_delete_units();\n+\t\tsystemd_timer_disable_units();\n \treturn ret;\n }\n \n@@ -2473,7 +2366,7 @@ static int systemd_timer_update_schedule(int run_maintenance, int fd UNUSED)\n \tif (run_maintenance)\n \t\treturn systemd_timer_setup_units();\n \telse\n-\t\treturn systemd_timer_delete_units();\n+\t\treturn systemd_timer_disable_units();\n }\n \n enum scheduler {\ndiff --git a/config.mak.uname b/config.mak.uname\nindex d0dcca2ec5..35ca236874 100644\n--- a/config.mak.uname\n+++ b/config.mak.uname\n@@ -68,6 +68,16 @@ ifeq ($(uname_S),Linux)\n \tifneq ($(findstring .el7.,$(uname_R)),)\n \t\tBASIC_CFLAGS += -std=c99\n \tendif\n+\tifeq ($(shell command -v systemctl >/dev/null ?&& echo y),y)\n+\t\tXDG_DATA_HOME ?= $(HOME)/.local/share\n+\t\t# systemd user units of programm installed in the home directory\n+\t\t# (meaning prefix == $HOME) shall go in XDG_DATA_HOME\n+\t\t# (from man 5 systemd.unit)\n+\t\tSYSTEMD_USER_UNIT_DIR = $(strip $(if $(and \\\n+\t\t\t$(findstring $(prefix),$(HOME)),\\\n+\t\t\t$(findstring $(HOME),$(prefix))),\\\n+\t\t\t$(XDG_DATA_HOME),$(prefix)/$(lib)))/systemd/user\n+\tendif\n endif\n ifeq ($(uname_S),GNU/kFreeBSD)\n \tHAVE_ALLOCA_H = YesPlease\ndiff --git a/systemd/user/git-maintenance@.service.in b/systemd/user/git-maintenance@.service.in\nnew file mode 100644\nindex 0000000000..649ba87e7e\n--- /dev/null\n+++ b/systemd/user/git-maintenance@.service.in\n@@ -0,0 +1,17 @@\n+[Unit]\n+Description=Optimize Git repositories data\n+Documentation=man:git-maintenance(1)\n+\n+[Service]\n+Type=oneshot\n+ExecStart=@BINDIR@/git for-each-repo --config=maintenance.repo \\\n+ 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\ndiff --git a/systemd/user/git-maintenance@.timer b/systemd/user/git-maintenance@.timer\nnew file mode 100644\nindex 0000000000..2834bac365\n--- /dev/null\n+++ b/systemd/user/git-maintenance@.timer\n@@ -0,0 +1,12 @@\n+[Unit]\n+Description=Optimize Git repositories data\n+Documentation=man:git-maintenance(1)\n+\n+[Timer]\n+OnCalendar=%i\n+Persistent=true\n+RandomizedDelaySec=3540\n+FixedRandomDelay=true\n+\n+[Install]\n+WantedBy=timers.target\n-- \n2.44.0\n\n"},{"id":"491271","messageId":"20240322221327.12204-4-mg@max.gautier.name","threadId":"61181","inReplyTo":"20240322221327.12204-1-mg@max.gautier.name","subject":"[PATCH v2 3/6] maintenance: simplify systemctl calls","fromName":"Max Gautier","fromEmail":"mg@max.gautier.name","sentAt":"2024-03-22T22:11:08Z","receivedAt":"2024-03-22T22:14:01Z","isPatch":true,"sender":{"key":"mg@max.gautier.name","avatar":"https://avatars.githubusercontent.com/u/13346812?v=4"},"body":"The systemctl invocation to enable or disable the git maintenance timers\nis needlessly complicated:\n- systemctl does not mind at all enabling already enabled units, nor\n disabling already disabled units.\n- it can act on several units at once instead of only one.\n\nUse only one systemctl invocation per `git maintenance start/stop`.\nTransparently pass its status and output.\nAdd the --force option to override conflicting symlinks to previous\ninstances of our units files which lived in $XDG_CONFIG_HOME.\n\nSigned-off-by: Max Gautier \n---\n builtin/gc.c | 63 ++++++++++++----------------------------------------\n 1 file changed, 14 insertions(+), 49 deletions(-)\n\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex 199c8e6240..aaee91451a 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -2303,70 +2303,35 @@ static int is_systemd_timer_available(void)\n \treturn real_is_systemd_timer_available();\n }\n \n-static int systemd_timer_enable_unit(int enable,\n-\t\t\t\t enum schedule_priority schedule)\n+static int systemd_set_units_state(int enable)\n {\n \tconst char *cmd = \"systemctl\";\n \tstruct child_process child = CHILD_PROCESS_INIT;\n-\tconst char *frequency = get_frequency(schedule);\n-\n-\t/*\n-\t * Disabling the systemd unit while it is already disabled makes\n-\t * systemctl print an error.\n-\t * Let's ignore it since it means we already are in the expected state:\n-\t * the unit is disabled.\n-\t *\n-\t * On the other hand, enabling a systemd unit which is already enabled\n-\t * produces no error.\n-\t */\n-\tif (!enable)\n-\t\tchild.no_stderr = 1;\n \n \tget_schedule_cmd(&cmd, NULL);\n \tstrvec_split(&child.args, cmd);\n-\tstrvec_pushl(&child.args, \"--user\", enable ? \"enable\" : \"disable\",\n-\t\t \"--now\", NULL);\n-\tstrvec_pushf(&child.args, \"git-maintenance@%s.timer\", frequency);\n+\n+\tstrvec_pushl(&child.args, \"--user\", \"--force\", \"--now\",\n+\t\t\tenable ? \"enable\" : \"disable\",\n+\t\t\t\"git-maintenance@hourly.timer\",\n+\t\t\t\"git-maintenance@daily.timer\",\n+\t\t\t\"git-maintenance@weekly.timer\", NULL);\n+\t/*\n+\t * --force override existing conflicting symlinks\n+\t * We need it because the units have changed location (~/.config ->\n+\t * /usr/lib)\n+\t */\n \n \tif (start_command(&child))\n \t\treturn error(_(\"failed to start systemctl\"));\n \tif (finish_command(&child))\n-\t\t/*\n-\t\t * Disabling an already disabled systemd unit makes\n-\t\t * systemctl fail.\n-\t\t * Let's ignore this failure.\n-\t\t *\n-\t\t * Enabling an enabled systemd unit doesn't fail.\n-\t\t */\n-\t\tif (enable)\n-\t\t\treturn error(_(\"failed to run systemctl\"));\n+\t\treturn error(_(\"failed to run systemctl\"));\n \treturn 0;\n }\n \n-static int systemd_timer_disable_units(void)\n-{\n-\treturn systemd_timer_enable_unit(0, SCHEDULE_HOURLY) ||\n-\t systemd_timer_enable_unit(0, SCHEDULE_DAILY) ||\n-\t systemd_timer_enable_unit(0, SCHEDULE_WEEKLY);\n-}\n-\n-static int systemd_timer_setup_units(void)\n-{\n-\n-\tint ret = systemd_timer_enable_unit(1, SCHEDULE_HOURLY) ||\n-\t\t systemd_timer_enable_unit(1, SCHEDULE_DAILY) ||\n-\t\t systemd_timer_enable_unit(1, SCHEDULE_WEEKLY);\n-\tif (ret)\n-\t\tsystemd_timer_disable_units();\n-\treturn ret;\n-}\n-\n static int systemd_timer_update_schedule(int run_maintenance, int fd UNUSED)\n {\n-\tif (run_maintenance)\n-\t\treturn systemd_timer_setup_units();\n-\telse\n-\t\treturn systemd_timer_disable_units();\n+\treturn systemd_set_units_state(run_maintenance);\n }\n \n enum scheduler {\n-- \n2.44.0\n\n"},{"id":"491272","messageId":"20240322221327.12204-2-mg@max.gautier.name","threadId":"61181","inReplyTo":"20240322221327.12204-1-mg@max.gautier.name","subject":"[PATCH v2 1/6] maintenance: use systemd timers builtin randomization","fromName":"Max Gautier","fromEmail":"mg@max.gautier.name","sentAt":"2024-03-22T22:11:06Z","receivedAt":"2024-03-22T22:14:01Z","isPatch":true,"sender":{"key":"mg@max.gautier.name","avatar":"https://avatars.githubusercontent.com/u/13346812?v=4"},"body":"Commit daa787010c (maintenance: use random minute in systemd scheduler,\n2023-08-10) hooked the systemd timers with the get_random_minute()\nfunction, which is used to spread the load generated by clients, while\nkeeping one hour between each run (for the hourly run).\n\nThere is two problems with this approach:\n1. The random minute is the same for all timers, which can lead to\n overlapping execution, and in practice skipped execution for some of\n the timers.\n2. The timers are overwritten on each execution of `git maintenance\n start`, with a new random minute, which can lead to missing a daily\n or weekly timer.\n\n Consider this (unlikely) sequence of events:\n - weekly timer schedule is \"Mon 0:52:00\".\n - user run `git maintenance start` Monday, at 0:30:00.\n - the random minute happens to be 22, making the new weekly timer\n schedule \"Mon 0:22:00\".\n - the weekly timer does not fire until the next Monday.\n\nCommit c97ec0378b (maintenance: fix systemd schedule overlaps,\n2023-08-10) resolve problem 1 by skipping some executions of the timers\nto avoid overlap, but not problem 2.\n\nRevert both commits to instead use two native systemd timers properties:\n- RandomizedDelaySec: as the name suggest, this adds a random offset\n to each timer execution. Use 3540 seconds (59 minutes) to spread\n execution as much as possible.\n- FixedRandomDelay: this makes the offset from the previous setting\n deterministic, depending on machine ID, timer name and user\n identifier.\n\n(For more details on these settings, see man 5 systemd.timer)\n\nThis give us timers which:\n- don't overlap (schedule depend on timer name)\n- are spread out by user (schedule depend on machine ID and user ID)\n- happen at fixed intervals\n\nThis also restore the ability to use the systemd templating system, and\nkeep the systemd unit strings embedded in the C code simpler, with no\nneed for format strings.\n\nSigned-off-by: Max Gautier \n---\n builtin/gc.c | 154 +++++++----------------------------------\n t/t7900-maintenance.sh | 15 +---\n 2 files changed, 27 insertions(+), 142 deletions(-)\n\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex cb80ced6cb..dac59414f0 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -2308,54 +2308,29 @@ static char *xdg_config_home_systemd(const char *filename)\n \treturn xdg_config_home_for(\"systemd/user\", filename);\n }\n \n-#define SYSTEMD_UNIT_FORMAT \"git-maintenance@%s.%s\"\n-\n-static int systemd_timer_delete_timer_file(enum schedule_priority priority)\n+static int systemd_timer_delete_unit_templates(void)\n {\n \tint ret = 0;\n-\tconst char *frequency = get_frequency(priority);\n-\tchar *local_timer_name = xstrfmt(SYSTEMD_UNIT_FORMAT, frequency, \"timer\");\n-\tchar *filename = xdg_config_home_systemd(local_timer_name);\n-\n+\tchar *filename = xdg_config_home_systemd(\"git-maintenance@.timer\");\n \tif (unlink(filename) && !is_missing_file_error(errno))\n \t\tret = error_errno(_(\"failed to delete '%s'\"), filename);\n+\tFREE_AND_NULL(filename);\n \n-\tfree(filename);\n-\tfree(local_timer_name);\n-\treturn ret;\n-}\n-\n-static int systemd_timer_delete_service_template(void)\n-{\n-\tint ret = 0;\n-\tchar *local_service_name = xstrfmt(SYSTEMD_UNIT_FORMAT, \"\", \"service\");\n-\tchar *filename = xdg_config_home_systemd(local_service_name);\n+\tfilename = xdg_config_home_systemd(\"git-maintenance@.service\");\n \tif (unlink(filename) && !is_missing_file_error(errno))\n \t\tret = error_errno(_(\"failed to delete '%s'\"), filename);\n \n \tfree(filename);\n-\tfree(local_service_name);\n \treturn ret;\n }\n \n-/*\n- * Write the schedule information into a git-maintenance@.timer\n- * file using a custom minute. This timer file cannot use the templating\n- * system, so we generate a specific file for each.\n- */\n-static int systemd_timer_write_timer_file(enum schedule_priority schedule,\n-\t\t\t\t\t int minute)\n+static int systemd_timer_write_unit_templates(const char *exec_path)\n {\n-\tint res = -1;\n \tchar *filename;\n \tFILE *file;\n \tconst char *unit;\n-\tchar *schedule_pattern = NULL;\n-\tconst char *frequency = get_frequency(schedule);\n-\tchar *local_timer_name = xstrfmt(SYSTEMD_UNIT_FORMAT, frequency, \"timer\");\n-\n-\tfilename = xdg_config_home_systemd(local_timer_name);\n \n+\tfilename = xdg_config_home_systemd(\"git-maintenance@.timer\");\n \tif (safe_create_leading_directories(filename)) {\n \t\terror(_(\"failed to create directories for '%s'\"), filename);\n \t\tgoto error;\n@@ -2364,23 +2339,6 @@ static int systemd_timer_write_timer_file(enum schedule_priority schedule,\n \tif (!file)\n \t\tgoto error;\n \n-\tswitch (schedule) {\n-\tcase SCHEDULE_HOURLY:\n-\t\tschedule_pattern = xstrfmt(\"*-*-* 1..23:%02d:00\", minute);\n-\t\tbreak;\n-\n-\tcase SCHEDULE_DAILY:\n-\t\tschedule_pattern = xstrfmt(\"Tue..Sun *-*-* 0:%02d:00\", minute);\n-\t\tbreak;\n-\n-\tcase SCHEDULE_WEEKLY:\n-\t\tschedule_pattern = xstrfmt(\"Mon 0:%02d:00\", minute);\n-\t\tbreak;\n-\n-\tdefault:\n-\t\tBUG(\"Unhandled schedule_priority\");\n-\t}\n-\n \tunit = \"# This file was created and is maintained by Git.\\n\"\n \t \"# Any edits made in this file might be replaced in the future\\n\"\n \t \"# by a Git command.\\n\"\n@@ -2389,12 +2347,14 @@ static int systemd_timer_write_timer_file(enum schedule_priority schedule,\n \t \"Description=Optimize Git repositories data\\n\"\n \t \"\\n\"\n \t \"[Timer]\\n\"\n-\t \"OnCalendar=%s\\n\"\n+\t \"OnCalendar=%i\\n\"\n \t \"Persistent=true\\n\"\n+\t \"RandomizedDelaySecond=3540\\n\"\n+\t \"FixedRandomDelay=true\\n\"\n \t \"\\n\"\n \t \"[Install]\\n\"\n \t \"WantedBy=timers.target\\n\";\n-\tif (fprintf(file, unit, schedule_pattern) < 0) {\n+\tif (fputs(unit, file) == EOF) {\n \t\terror(_(\"failed to write to '%s'\"), filename);\n \t\tfclose(file);\n \t\tgoto error;\n@@ -2403,36 +2363,9 @@ static int systemd_timer_write_timer_file(enum schedule_priority schedule,\n \t\terror_errno(_(\"failed to flush '%s'\"), filename);\n \t\tgoto error;\n \t}\n-\n-\tres = 0;\n-\n-error:\n-\tfree(schedule_pattern);\n-\tfree(local_timer_name);\n \tfree(filename);\n-\treturn res;\n-}\n \n-/*\n- * No matter the schedule, we use the same service and can make use of the\n- * templating system. When installing git-maintenance@.timer,\n- * systemd will notice that git-maintenance@.service exists as a template\n- * and will use this file and insert the into the template at\n- * the position of \"%i\".\n- */\n-static int systemd_timer_write_service_template(const char *exec_path)\n-{\n-\tint res = -1;\n-\tchar *filename;\n-\tFILE *file;\n-\tconst char *unit;\n-\tchar *local_service_name = xstrfmt(SYSTEMD_UNIT_FORMAT, \"\", \"service\");\n-\n-\tfilename = xdg_config_home_systemd(local_service_name);\n-\tif (safe_create_leading_directories(filename)) {\n-\t\terror(_(\"failed to create directories for '%s'\"), filename);\n-\t\tgoto error;\n-\t}\n+\tfilename = xdg_config_home_systemd(\"git-maintenance@.service\");\n \tfile = fopen_or_warn(filename, \"w\");\n \tif (!file)\n \t\tgoto error;\n@@ -2465,18 +2398,17 @@ static int systemd_timer_write_service_template(const char *exec_path)\n \t\terror_errno(_(\"failed to flush '%s'\"), filename);\n \t\tgoto error;\n \t}\n-\n-\tres = 0;\n+\tfree(filename);\n+\treturn 0;\n \n error:\n-\tfree(local_service_name);\n \tfree(filename);\n-\treturn res;\n+\tsystemd_timer_delete_unit_templates();\n+\treturn -1;\n }\n \n static int systemd_timer_enable_unit(int enable,\n-\t\t\t\t enum schedule_priority schedule,\n-\t\t\t\t int minute)\n+\t\t\t\t enum schedule_priority schedule)\n {\n \tconst char *cmd = \"systemctl\";\n \tstruct child_process child = CHILD_PROCESS_INIT;\n@@ -2493,14 +2425,12 @@ static int systemd_timer_enable_unit(int enable,\n \t */\n \tif (!enable)\n \t\tchild.no_stderr = 1;\n-\telse if (systemd_timer_write_timer_file(schedule, minute))\n-\t\treturn -1;\n \n \tget_schedule_cmd(&cmd, NULL);\n \tstrvec_split(&child.args, cmd);\n \tstrvec_pushl(&child.args, \"--user\", enable ? \"enable\" : \"disable\",\n \t\t \"--now\", NULL);\n-\tstrvec_pushf(&child.args, SYSTEMD_UNIT_FORMAT, frequency, \"timer\");\n+\tstrvec_pushf(&child.args, \"git-maintenance@%s.timer\", frequency);\n \n \tif (start_command(&child))\n \t\treturn error(_(\"failed to start systemctl\"));\n@@ -2517,58 +2447,24 @@ static int systemd_timer_enable_unit(int enable,\n \treturn 0;\n }\n \n-/*\n- * A previous version of Git wrote the timer units as template files.\n- * Clean these up, if they exist.\n- */\n-static void systemd_timer_delete_stale_timer_templates(void)\n-{\n-\tchar *timer_template_name = xstrfmt(SYSTEMD_UNIT_FORMAT, \"\", \"timer\");\n-\tchar *filename = xdg_config_home_systemd(timer_template_name);\n-\n-\tif (unlink(filename) && !is_missing_file_error(errno))\n-\t\twarning(_(\"failed to delete '%s'\"), filename);\n-\n-\tfree(filename);\n-\tfree(timer_template_name);\n-}\n-\n-static int systemd_timer_delete_unit_files(void)\n-{\n-\tsystemd_timer_delete_stale_timer_templates();\n-\n-\t/* Purposefully not short-circuited to make sure all are called. */\n-\treturn systemd_timer_delete_timer_file(SCHEDULE_HOURLY) |\n-\t systemd_timer_delete_timer_file(SCHEDULE_DAILY) |\n-\t systemd_timer_delete_timer_file(SCHEDULE_WEEKLY) |\n-\t systemd_timer_delete_service_template();\n-}\n-\n static int systemd_timer_delete_units(void)\n {\n-\tint minute = get_random_minute();\n-\t/* Purposefully not short-circuited to make sure all are called. */\n-\treturn systemd_timer_enable_unit(0, SCHEDULE_HOURLY, minute) |\n-\t systemd_timer_enable_unit(0, SCHEDULE_DAILY, minute) |\n-\t systemd_timer_enable_unit(0, SCHEDULE_WEEKLY, minute) |\n-\t systemd_timer_delete_unit_files();\n+\treturn systemd_timer_enable_unit(0, SCHEDULE_HOURLY) ||\n+\t systemd_timer_enable_unit(0, SCHEDULE_DAILY) ||\n+\t systemd_timer_enable_unit(0, SCHEDULE_WEEKLY) ||\n+\t systemd_timer_delete_unit_templates();\n }\n \n static int systemd_timer_setup_units(void)\n {\n-\tint minute = get_random_minute();\n \tconst char *exec_path = git_exec_path();\n \n-\tint ret = systemd_timer_write_service_template(exec_path) ||\n-\t\t systemd_timer_enable_unit(1, SCHEDULE_HOURLY, minute) ||\n-\t\t systemd_timer_enable_unit(1, SCHEDULE_DAILY, minute) ||\n-\t\t systemd_timer_enable_unit(1, SCHEDULE_WEEKLY, minute);\n-\n+\tint ret = systemd_timer_write_unit_templates(exec_path) ||\n+\t\t systemd_timer_enable_unit(1, SCHEDULE_HOURLY) ||\n+\t\t systemd_timer_enable_unit(1, SCHEDULE_DAILY) ||\n+\t\t systemd_timer_enable_unit(1, SCHEDULE_WEEKLY);\n \tif (ret)\n \t\tsystemd_timer_delete_units();\n-\telse\n-\t\tsystemd_timer_delete_stale_timer_templates();\n-\n \treturn ret;\n }\n \ndiff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh\nindex 0943dfa18a..37aa408d26 100755\n--- a/t/t7900-maintenance.sh\n+++ b/t/t7900-maintenance.sh\n@@ -790,15 +790,7 @@ test_expect_success 'start and stop Linux/systemd maintenance' '\n \t# start registers the repo\n \tgit config --get --global --fixed-value maintenance.repo \"$(pwd)\" &&\n \n-\tfor schedule in hourly daily weekly\n-\tdo\n-\t\ttest_path_is_file \"systemd/user/git-maintenance@$schedule.timer\" || return 1\n-\tdone &&\n-\ttest_path_is_file \"systemd/user/git-maintenance@.service\" &&\n-\n-\ttest_systemd_analyze_verify \"systemd/user/git-maintenance@hourly.service\" &&\n-\ttest_systemd_analyze_verify \"systemd/user/git-maintenance@daily.service\" &&\n-\ttest_systemd_analyze_verify \"systemd/user/git-maintenance@weekly.service\" &&\n+\ttest_systemd_analyze_verify \"systemd/user/git-maintenance@.service\" &&\n \n \tprintf -- \"--user enable --now git-maintenance@%s.timer\\n\" hourly daily weekly >expect &&\n \ttest_cmp expect args &&\n@@ -809,10 +801,7 @@ test_expect_success 'start and stop Linux/systemd maintenance' '\n \t# stop does not unregister the repo\n \tgit config --get --global --fixed-value maintenance.repo \"$(pwd)\" &&\n \n-\tfor schedule in hourly daily weekly\n-\tdo\n-\t\ttest_path_is_missing \"systemd/user/git-maintenance@$schedule.timer\" || return 1\n-\tdone &&\n+\ttest_path_is_missing \"systemd/user/git-maintenance@.timer\" &&\n \ttest_path_is_missing \"systemd/user/git-maintenance@.service\" &&\n \n \tprintf -- \"--user disable --now git-maintenance@%s.timer\\n\" hourly daily weekly >expect &&\n-- \n2.44.0\n\n"},{"id":"491273","messageId":"20240322221327.12204-6-mg@max.gautier.name","threadId":"61181","inReplyTo":"20240322221327.12204-1-mg@max.gautier.name","subject":"[PATCH v2 5/6] maintenance: update systemd scheduler docs","fromName":"Max Gautier","fromEmail":"mg@max.gautier.name","sentAt":"2024-03-22T22:11:10Z","receivedAt":"2024-03-22T22:14:05Z","isPatch":true,"sender":{"key":"mg@max.gautier.name","avatar":"https://avatars.githubusercontent.com/u/13346812?v=4"},"body":"The `git maintenance` systemd scheduler no longer writes units in\n$XDG_CONFIG_HOME.\n\nDescribe the new behavior.\nInstead of explaining manual ways to modify the timer, suggest the\nsystemd standard tool: `systemctl edit`.\n\nSigned-off-by: Max Gautier \n---\n Documentation/git-maintenance.txt | 33 +++++++++++++++----------------\n 1 file changed, 16 insertions(+), 17 deletions(-)\n\ndiff --git a/Documentation/git-maintenance.txt b/Documentation/git-maintenance.txt\nindex 51d0f7e94b..6511c3f3f1 100644\n--- a/Documentation/git-maintenance.txt\n+++ b/Documentation/git-maintenance.txt\n@@ -304,10 +304,9 @@ distributions, systemd timers are superseding it.\n If user systemd timers are available, they will be used as a replacement\n of `cron`.\n \n-In this case, `git maintenance start` will create user systemd timer units\n-and start the timers. The current list of user-scheduled tasks can be found\n-by running `systemctl --user list-timers`. The timers written by `git\n-maintenance start` are similar to this:\n+In this case, `git maintenance start` will enable user systemd timer units\n+and start them. The current list of user-scheduled tasks can be found by\n+running `systemctl --user list-timers`. These timers are similar to this:\n \n -----------------------------------------------------------------------\n $ systemctl --user list-timers\n@@ -317,25 +316,25 @@ Fri 2021-04-30 00:00:00 CEST 5h 42min left Thu 2021-04-29 00:00:11 CEST 18h ago\n Mon 2021-05-03 00:00:00 CEST 3 days left Mon 2021-04-26 00:00:11 CEST 3 days ago git-maintenance@weekly.timer git-maintenance@weekly.service\n -----------------------------------------------------------------------\n \n-One timer is registered for each `--schedule=` option.\n+One timer instance is enabled for each `--schedule=` option.\n \n-The definition of the systemd units can be inspected in the following files:\n+The definition of the systemd units can be inspected this way:\n \n -----------------------------------------------------------------------\n-~/.config/systemd/user/git-maintenance@.timer\n-~/.config/systemd/user/git-maintenance@.service\n-~/.config/systemd/user/timers.target.wants/git-maintenance@hourly.timer\n-~/.config/systemd/user/timers.target.wants/git-maintenance@daily.timer\n-~/.config/systemd/user/timers.target.wants/git-maintenance@weekly.timer\n+$ systemctl cat --user git-maintenance@.timer\n+$ systemctl cat --user git-maintenance@.service\n -----------------------------------------------------------------------\n \n-`git maintenance start` will overwrite these files and start the timer\n-again with `systemctl --user`, so any customization should be done by\n-creating a drop-in file, i.e. a `.conf` suffixed file in the\n-`~/.config/systemd/user/git-maintenance@.service.d` directory.\n+Customization of the timer or service can be performed with the usual systemd\n+tooling:\n+-----------------------------------------------------------------------\n+$ systemctl edit --user git-maintenance@.timer # all the timers\n+$ systemctl edit --user git-maintenance@hourly.timer # the hourly timer\n+$ systemctl edit --user git-maintenance@.service # all the services\n+$ systemctl edit --user git-maintenance@hourly.service # the hourly run\n+-----------------------------------------------------------------------\n \n-`git maintenance stop` will stop the user systemd timers and delete\n-the above mentioned files.\n+`git maintenance stop` will disable and stop the user systemd timers.\n \n For more details, see `systemd.timer(5)`.\n \n-- \n2.44.0\n\n"},{"id":"491274","messageId":"20240322221327.12204-7-mg@max.gautier.name","threadId":"61181","inReplyTo":"20240322221327.12204-1-mg@max.gautier.name","subject":"[PATCH v2 6/6] maintenance: update tests for systemd scheduler","fromName":"Max Gautier","fromEmail":"mg@max.gautier.name","sentAt":"2024-03-22T22:11:11Z","receivedAt":"2024-03-22T22:14:05Z","isPatch":true,"sender":{"key":"mg@max.gautier.name","avatar":"https://avatars.githubusercontent.com/u/13346812?v=4"},"body":"The systemd units are now in the source tree, rather than produced when\nrunning git maitenance start. There is no need anymore to couple\nvalidating the units and testing `git maintenance start`.\n\nAdjust the test to verify the new `systemctl` command used, discard\nchecks for presence/absence of unit files in $XDG_CONFIG_HOME.\n\nValidate the systemd units in the source tree, with one test per unit to\nhave more distinct failures.\n\nHelped-by: Eric Sunshine \nSigned-off-by: Max Gautier \n---\n\nNotes:\n I encountered the same issue that\n cover.1697319294.git.code@khaugsbakk.name tried to solve; I was\n expecting multiples tests to be independent while they weren't (in\n particular the MacOS ones).\n \n Cc: Kristoffer Haugsbakk \n\n t/t7900-maintenance.sh | 39 +++++++++++++++++++--------------------\n 1 file changed, 19 insertions(+), 20 deletions(-)\n\ndiff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh\nindex 37aa408d26..dc0bf39250 100755\n--- a/t/t7900-maintenance.sh\n+++ b/t/t7900-maintenance.sh\n@@ -24,13 +24,6 @@ test_lazy_prereq SYSTEMD_ANALYZE '\n \tsystemd-analyze verify /lib/systemd/system/basic.target\n '\n \n-test_systemd_analyze_verify () {\n-\tif test_have_prereq SYSTEMD_ANALYZE\n-\tthen\n-\t\tsystemd-analyze verify \"$@\"\n-\tfi\n-}\n-\n test_expect_success 'help text' '\n \ttest_expect_code 129 git maintenance -h >actual &&\n \ttest_grep \"usage: git maintenance \" actual &&\n@@ -776,23 +769,32 @@ test_expect_success 'start and stop Windows maintenance' '\n \t\thourly daily weekly >expect &&\n \ttest_cmp expect args\n '\n+test_expect_success SYSTEMD_ANALYZE 'validate maintenance systemd service unit' '\n+\tgit_path=$(command -v git) &&\n+\tsed \"s+@BINDIR@/git+${git_path}+\" \"$TEST_DIRECTORY\"/../systemd/user/git-maintenance@.service.in > git-maintenance@.service &&\n+\tsystemd-analyze verify git-maintenance@hourly.service git-maintenance@daily.service git-maintenance@weekly.service &&\n+\trm git-maintenance@.service &&\n+\tunset git_path\n+'\n+\n+test_expect_success SYSTEMD_ANALYZE 'validate maintenance systemd timer unit' '\n+\tSYSTEMD_UNIT_PATH=\"$TEST_DIRECTORY\"/../systemd/user/: systemd-analyze verify git-maintenance@hourly.timer git-maintenance@daily.timer git-maintenance@weekly.timer\n+\t# ':' at the end of SYSTEMD_UNIT_PATH appends the default systemd search path\n+\t# This is needed because analyze tries to load implicit / default unit dependencies\n+'\n \n test_expect_success 'start and stop Linux/systemd maintenance' '\n \twrite_script print-args <<-\\EOF &&\n \tprintf \"%s\\n\" \"$*\" >>args\n \tEOF\n \n-\tXDG_CONFIG_HOME=\"$PWD\" &&\n-\texport XDG_CONFIG_HOME &&\n \trm -f args &&\n \tGIT_TEST_MAINT_SCHEDULER=\"systemctl:./print-args\" git maintenance start --scheduler=systemd-timer &&\n \n \t# start registers the repo\n \tgit config --get --global --fixed-value maintenance.repo \"$(pwd)\" &&\n \n-\ttest_systemd_analyze_verify \"systemd/user/git-maintenance@.service\" &&\n-\n-\tprintf -- \"--user enable --now git-maintenance@%s.timer\\n\" hourly daily weekly >expect &&\n+\techo \"--user --force --now enable\" git-maintenance@hourly.timer git-maintenance@daily.timer git-maintenance@weekly.timer >expect &&\n \ttest_cmp expect args &&\n \n \trm -f args &&\n@@ -801,10 +803,7 @@ test_expect_success 'start and stop Linux/systemd maintenance' '\n \t# stop does not unregister the repo\n \tgit config --get --global --fixed-value maintenance.repo \"$(pwd)\" &&\n \n-\ttest_path_is_missing \"systemd/user/git-maintenance@.timer\" &&\n-\ttest_path_is_missing \"systemd/user/git-maintenance@.service\" &&\n-\n-\tprintf -- \"--user disable --now git-maintenance@%s.timer\\n\" hourly daily weekly >expect &&\n+\techo \"--user --force --now disable\" git-maintenance@hourly.timer git-maintenance@daily.timer git-maintenance@weekly.timer >expect &&\n \ttest_cmp expect args\n '\n \n@@ -819,12 +818,12 @@ test_expect_success 'start and stop when several schedulers are available' '\n \t\thourly daily weekly >expect &&\n \tprintf \"schtasks /delete /tn Git Maintenance (%s) /f\\n\" \\\n \t\thourly daily weekly >>expect &&\n-\tprintf -- \"systemctl --user enable --now git-maintenance@%s.timer\\n\" hourly daily weekly >>expect &&\n+\techo \"systemctl --user --force --now enable\" git-maintenance@hourly.timer git-maintenance@daily.timer git-maintenance@weekly.timer >>expect &&\n \ttest_cmp expect args &&\n \n \trm -f args &&\n \tGIT_TEST_MAINT_SCHEDULER=\"systemctl:./print-args systemctl,launchctl:./print-args launchctl,schtasks:./print-args schtasks\" git maintenance start --scheduler=launchctl &&\n-\tprintf -- \"systemctl --user disable --now git-maintenance@%s.timer\\n\" hourly daily weekly >expect &&\n+\techo \"systemctl --user --force --now disable\" git-maintenance@hourly.timer git-maintenance@daily.timer git-maintenance@weekly.timer >expect &&\n \tprintf \"schtasks /delete /tn Git Maintenance (%s) /f\\n\" \\\n \t\thourly daily weekly >>expect &&\n \tfor frequency in hourly daily weekly\n@@ -837,7 +836,7 @@ test_expect_success 'start and stop when several schedulers are available' '\n \n \trm -f args &&\n \tGIT_TEST_MAINT_SCHEDULER=\"systemctl:./print-args systemctl,launchctl:./print-args launchctl,schtasks:./print-args schtasks\" git maintenance start --scheduler=schtasks &&\n-\tprintf -- \"systemctl --user disable --now git-maintenance@%s.timer\\n\" hourly daily weekly >expect &&\n+\techo \"systemctl --user --force --now disable\" git-maintenance@hourly.timer git-maintenance@daily.timer git-maintenance@weekly.timer >expect &&\n \tprintf \"launchctl bootout gui/[UID] $pfx/Library/LaunchAgents/org.git-scm.git.%s.plist\\n\" \\\n \t\thourly daily weekly >>expect &&\n \tprintf \"schtasks /create /tn Git Maintenance (%s) /f /xml\\n\" \\\n@@ -846,7 +845,7 @@ test_expect_success 'start and stop when several schedulers are available' '\n \n \trm -f args &&\n \tGIT_TEST_MAINT_SCHEDULER=\"systemctl:./print-args systemctl,launchctl:./print-args launchctl,schtasks:./print-args schtasks\" git maintenance stop &&\n-\tprintf -- \"systemctl --user disable --now git-maintenance@%s.timer\\n\" hourly daily weekly >expect &&\n+\techo \"systemctl --user --force --now disable\" git-maintenance@hourly.timer git-maintenance@daily.timer git-maintenance@weekly.timer >expect &&\n \tprintf \"launchctl bootout gui/[UID] $pfx/Library/LaunchAgents/org.git-scm.git.%s.plist\\n\" \\\n \t\thourly daily weekly >>expect &&\n \tprintf \"schtasks /delete /tn Git Maintenance (%s) /f\\n\" \\\n-- \n2.44.0\n\n"},{"id":"491280","messageId":"5b9dc977-e8f9-472f-8476-1b13fa6e6d93@app.fastmail.com","threadId":"61181","inReplyTo":"20240322221327.12204-5-mg@max.gautier.name","subject":"Re: [PATCH v2 4/6] maintenance: cleanup $XDG_CONFIG_HOME/systemd/user","fromName":"Kristoffer Haugsbakk","fromEmail":"code@khaugsbakk.name","sentAt":"2024-03-22T22:38:09Z","receivedAt":"2024-03-22T22:38:30Z","isPatch":true,"sender":{"key":"code@khaugsbakk.name","avatar":"https://avatars.githubusercontent.com/u/2229597?v=4"},"body":"On Fri, Mar 22, 2024, at 23:11, Max Gautier wrote:\n> Notes:\n> How should I refer to a commit which is part of the same patch series ?\n> The commit id will change so the message won't be correct anymore, right\n> ?\n\nIt looks like a fair few say “in the previous commit” for the one just\nbefore this one and “in a previous commit” for some commit that was\nbefore this in the series but not the immediate previous one. I guess\nthat’s okay, no?\n\n-- \nKristoffer Haugsbakk\n\n\n"},{"id":"491282","messageId":"xmqqjzltzy3g.fsf@gitster.g","threadId":"61181","inReplyTo":"5b9dc977-e8f9-472f-8476-1b13fa6e6d93@app.fastmail.com","subject":"Re: [PATCH v2 4/6] maintenance: cleanup $XDG_CONFIG_HOME/systemd/user","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-22T22:43:47Z","receivedAt":"2024-03-22T22:43:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Kristoffer Haugsbakk\" writes:\n\n> On Fri, Mar 22, 2024, at 23:11, Max Gautier wrote:\n>> Notes:\n>> How should I refer to a commit which is part of the same patch series ?\n>> The commit id will change so the message won't be correct anymore, right\n>> ?\n>\n> It looks like a fair few say “in the previous commit” for the one just\n> before this one and “in a previous commit” for some commit that was\n> before this in the series but not the immediate previous one. I guess\n> that’s okay, no?\n\nI saw some series say \"Earlier in the series, X has learned to do Y,\nso now we take advantage of that new feature\", and found it quite\nreadable.\n"},{"id":"491283","messageId":"CAPig+cT=8WkxZF1vy-2dgSspFeGOQzUFRk6w+MYVCXyRAsfR8A@mail.gmail.com","threadId":"61181","inReplyTo":"20240322221327.12204-7-mg@max.gautier.name","subject":"Re: [PATCH v2 6/6] maintenance: update tests for systemd scheduler","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2024-03-22T23:02:36Z","receivedAt":"2024-03-22T23:02:48Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Mar 22, 2024 at 6:13 PM Max Gautier wrote:\n> The systemd units are now in the source tree, rather than produced when\n> running git maitenance start. There is no need anymore to couple\n> validating the units and testing `git maintenance start`.\n\ns/maitenance/maintenance/\n\n> Adjust the test to verify the new `systemctl` command used, discard\n> checks for presence/absence of unit files in $XDG_CONFIG_HOME.\n>\n> Validate the systemd units in the source tree, with one test per unit to\n> have more distinct failures.\n>\n> Signed-off-by: Max Gautier \n\nIn a patch series, in order to preserve \"bisectability\", we want to\nensure that the entire test suite continues to pass after each patch\nis applied. A such, we normally update tests -- to ensure that they\ncontinue passing -- in each patch which changes some\ntestable/observable behavior. However, this series only updates test\nin the final patch. Doesn't that break bisectability? Or am I\nmisunderstanding something?\n"},{"id":"491284","messageId":"CAPig+cTeXMw8pTS9on_CSJraPDYKSLaFd82LEtWjFgOBzH4=yg@mail.gmail.com","threadId":"61181","inReplyTo":"20240322221327.12204-4-mg@max.gautier.name","subject":"Re: [PATCH v2 3/6] maintenance: simplify systemctl calls","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2024-03-22T23:09:18Z","receivedAt":"2024-03-22T23:09:30Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Mar 22, 2024 at 6:13 PM Max Gautier wrote:\n> The systemctl invocation to enable or disable the git maintenance timers\n> is needlessly complicated:\n> - systemctl does not mind at all enabling already enabled units, nor\n> disabling already disabled units.\n\nHas systemctl behavior changed...\n\n> Use only one systemctl invocation per `git maintenance start/stop`.\n> Transparently pass its status and output.\n> Add the --force option to override conflicting symlinks to previous\n> instances of our units files which lived in $XDG_CONFIG_HOME.\n>\n> Signed-off-by: Max Gautier \n> ---\n> diff --git a/builtin/gc.c b/builtin/gc.c\n> @@ -2303,70 +2303,35 @@ static int is_systemd_timer_available(void)\n> - * Disabling the systemd unit while it is already disabled makes\n> - * systemctl print an error.\n> - * Let's ignore it since it means we already are in the expected state:\n> - * the unit is disabled.\n\n... since this and...\n\n> - * Disabling an already disabled systemd unit makes\n> - * systemctl fail.\n> - * Let's ignore this failure.\n\n... this were written?\n\nIf so, do we still need to worry about older systems in which\nsystemctl prints errors and/or fails outright?\n"},{"id":"491290","messageId":"CAPig+cS1YL1iOZFh5=b1+_P=KBST=DYRqp7TaD7uMVLJQjs1bA@mail.gmail.com","threadId":"61181","inReplyTo":"20240322221327.12204-3-mg@max.gautier.name","subject":"Re: [PATCH v2 2/6] maintenance: use packaged systemd units","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2024-03-23T08:38:44Z","receivedAt":"2024-03-23T08:38:56Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sat, Mar 23, 2024 at 4:21 AM Max Gautier wrote:\n> [...]\n> Package the systemd user units (timer and service) with git in\n> $(prefix)/lib/systemd/user (or $XDG_DATA_HOME for $HOME installation),\n> and remove code for writing and deleting the units from builtin/gc.c.\n> Determine the correct git path at install time by for the service unit.\n>\n> Detect systemd timers support (at install time) by relying on systemctl\n> presence, since we use it as the control interface for the systemd\n> scheduler.\n>\n> Signed-off-by: Max Gautier \n> ---\n> Notes:\n> I'm not completely sure if it's ok to do install time templating like\n> this, but I couldn't find a similar enough example in the Makefile. Any\n> suggestion for a better way ?\n>\n> diff --git a/Makefile b/Makefile\n> @@ -3469,6 +3469,11 @@ install: all\n> +ifdef SYSTEMD_USER_UNIT_DIR\n> + $(INSTALL) -Dm 644 -t '$(DESTDIR_SQ)$(SYSTEMD_USER_UNIT_DIR)' systemd/user/git-maintenance@.timer\n> + sed 's+@BINDIR@+$(bindir_SQ)+' systemd/user/git-maintenance@.service.in | \\\n> + $(INSTALL) -Dm 644 /dev/stdin '$(DESTDIR_SQ)$(SYSTEMD_USER_UNIT_DIR)/git-maintenance@.service'\n> +endif\n\nThis is the first use of /dev/stdin in the project and I might worry a\nbit about portability. Granted, a system in which systemd is installed\nis likely to have /dev/stdin available, but it's often a good idea to\nbe cautious when introducing something new into the project.\n\nI would think it would be possible to instead generate the\n`git-maintenance@.service` file locally from the template\n`git-maintenance@.service.in` as part of the normal build process, and\nthen install the built `git-maintenance@.service` at \"install\" time.\nThat seems more in line with how other resources are handled, avoids\nthe novel use of /dev/stdin, and answers the question you ask above.\n\n> diff --git a/config.mak.uname b/config.mak.uname\n> @@ -68,6 +68,16 @@ ifeq ($(uname_S),Linux)\n> + ifeq ($(shell command -v systemctl >/dev/null ?&& echo y),y)\n\nWhat is \"?&&\"?\n\n> + XDG_DATA_HOME ?= $(HOME)/.local/share\n> + # systemd user units of programm installed in the home directory\n> + # (meaning prefix == $HOME) shall go in XDG_DATA_HOME\n> + # (from man 5 systemd.unit)\n\ns/programm/program/\n"},{"id":"491292","messageId":"Zf6mcBhd31WYkwob@framework","threadId":"61181","inReplyTo":"CAPig+cS1YL1iOZFh5=b1+_P=KBST=DYRqp7TaD7uMVLJQjs1bA@mail.gmail.com","subject":"Re: [PATCH v2 2/6] maintenance: use packaged systemd units","fromName":"Max Gautier","fromEmail":"mg@max.gautier.name","sentAt":"2024-03-23T09:52:48Z","receivedAt":"2024-03-23T09:55:06Z","isPatch":true,"sender":{"key":"mg@max.gautier.name","avatar":"https://avatars.githubusercontent.com/u/13346812?v=4"},"body":"On Sat, Mar 23, 2024 at 04:38:44AM -0400, Eric Sunshine wrote:\n> On Sat, Mar 23, 2024 at 4:21 AM Max Gautier wrote:\n> > [...]\n> > Package the systemd user units (timer and service) with git in\n> > $(prefix)/lib/systemd/user (or $XDG_DATA_HOME for $HOME installation),\n> > and remove code for writing and deleting the units from builtin/gc.c.\n> > Determine the correct git path at install time by for the service unit.\n> >\n> > Detect systemd timers support (at install time) by relying on systemctl\n> > presence, since we use it as the control interface for the systemd\n> > scheduler.\n> >\n> > Signed-off-by: Max Gautier \n> > ---\n> > Notes:\n> > I'm not completely sure if it's ok to do install time templating like\n> > this, but I couldn't find a similar enough example in the Makefile. Any\n> > suggestion for a better way ?\n> >\n> > diff --git a/Makefile b/Makefile\n> > @@ -3469,6 +3469,11 @@ install: all\n> > +ifdef SYSTEMD_USER_UNIT_DIR\n> > + $(INSTALL) -Dm 644 -t '$(DESTDIR_SQ)$(SYSTEMD_USER_UNIT_DIR)' systemd/user/git-maintenance@.timer\n> > + sed 's+@BINDIR@+$(bindir_SQ)+' systemd/user/git-maintenance@.service.in | \\\n> > + $(INSTALL) -Dm 644 /dev/stdin '$(DESTDIR_SQ)$(SYSTEMD_USER_UNIT_DIR)/git-maintenance@.service'\n> > +endif\n> \n> This is the first use of /dev/stdin in the project and I might worry a\n> bit about portability. Granted, a system in which systemd is installed\n> is likely to have /dev/stdin available, but it's often a good idea to\n> be cautious when introducing something new into the project.\n> \n> I would think it would be possible to instead generate the\n> `git-maintenance@.service` file locally from the template\n> `git-maintenance@.service.in` as part of the normal build process, and\n> then install the built `git-maintenance@.service` at \"install\" time.\n> That seems more in line with how other resources are handled, avoids\n> the novel use of /dev/stdin, and answers the question you ask above.\n\nOk. It's not completely obvious to me how the \"Detect prefix change\nlogic\" works, but using other rules as a model, I think I can do\nsomething like that:\nsystemd/user/git-maintenance@.service: systemd/user/git-maintenance@.service.in GIT-PREFIX\n sed 's+@BINDIR@+$(bindir_SQ)+' $< > $@\n\nand depending on GIT-PREFIX should regenerate the service when changing\nthe prefix, correct ?\nI'll need to add that to .gitignore as well.\n\n> \n> > diff --git a/config.mak.uname b/config.mak.uname\n> > @@ -68,6 +68,16 @@ ifeq ($(uname_S),Linux)\n> > + ifeq ($(shell command -v systemctl >/dev/null ?&& echo y),y)\n> \n> What is \"?&&\"?\n> \n\nHum, a typo, sorry that slipped through. It apparently works regardless,\nbecause shell expansion does something with it I guess. Curious. I'll\nclean that up as well.\n\n> > + XDG_DATA_HOME ?= $(HOME)/.local/share\n> > + # systemd user units of programm installed in the home directory\n> > + # (meaning prefix == $HOME) shall go in XDG_DATA_HOME\n> > + # (from man 5 systemd.unit)\n> \n> s/programm/program/\nAck.\nThat should even be 'programs' I think, that's a general rule.\n\n-- \nMax Gautier\n"},{"id":"491294","messageId":"Zf6uISqdCn9Ld4ir@framework","threadId":"61181","inReplyTo":"CAPig+cTeXMw8pTS9on_CSJraPDYKSLaFd82LEtWjFgOBzH4=yg@mail.gmail.com","subject":"Re: [PATCH v2 3/6] maintenance: simplify systemctl calls","fromName":"Max Gautier","fromEmail":"mg@max.gautier.name","sentAt":"2024-03-23T10:25:37Z","receivedAt":"2024-03-23T10:25:39Z","isPatch":true,"sender":{"key":"mg@max.gautier.name","avatar":"https://avatars.githubusercontent.com/u/13346812?v=4"},"body":"On Fri, Mar 22, 2024 at 07:09:18PM -0400, Eric Sunshine wrote:\n> On Fri, Mar 22, 2024 at 6:13 PM Max Gautier wrote:\n> > The systemctl invocation to enable or disable the git maintenance timers\n> > is needlessly complicated:\n> > - systemctl does not mind at all enabling already enabled units, nor\n> > disabling already disabled units.\n> \n> Has systemctl behavior changed...\n> \n> > Use only one systemctl invocation per `git maintenance start/stop`.\n> > Transparently pass its status and output.\n> > Add the --force option to override conflicting symlinks to previous\n> > instances of our units files which lived in $XDG_CONFIG_HOME.\n> >\n> > Signed-off-by: Max Gautier \n> > ---\n> > diff --git a/builtin/gc.c b/builtin/gc.c\n> > @@ -2303,70 +2303,35 @@ static int is_systemd_timer_available(void)\n> > - * Disabling the systemd unit while it is already disabled makes\n> > - * systemctl print an error.\n> > - * Let's ignore it since it means we already are in the expected state:\n> > - * the unit is disabled.\n> \n> ... since this and...\n> \n> > - * Disabling an already disabled systemd unit makes\n> > - * systemctl fail.\n> > - * Let's ignore this failure.\n> \n> ... this were written?\n> \n> If so, do we still need to worry about older systems in which\n> systemctl prints errors and/or fails outright?\n\nI tried the following on systemd source\n$ git log -L :do_unit_file_disable:src/shared/install.c\n$ git log -L :unit_file_disable:src/shared/install.c`\n$ git log --grep 'already disabled'\n$ git log --grep 're disable'\n$ git log --grep 'disabled unit'\n$ git log --grep 'systemctl disable'\n\nThat yields nothing indicating a change (in the commit messages, I\ndidn't look at the code in detail) when disabling already disabled\nunits, and testing on systemd 255 disable already disabled units (normal\nand templated, same thing) without complaining or an error status.\nNothing relevant I could find on github in the issues or PRs either.\n\nsystemctl does error out or print warnings in other conditions, like\nmissing [Install] section, but that's not something we should ignore.\n\nPhilip Wood asked that question here in the original thread:\n\n> What is the exit code of systemctl if a unit is already enabled and we \n> try to enbale it again (and the same for disabling a disabled unit)?\nhttps://lore.kernel.org/git/3fd17223-8667-24be-2e65-f1970d411bdf@gmail.com/\n\nBut I can't find a follow-up email from Lénaïc, and searching for\n\"disabled\" in the whole thread only yield the comment I'm removing. Not\nsure if Lénaïc still follows the list, but maybe he can comment on that\n?\nTo me it looks like it was not necessary to begin with, but I might have \njust missed the discussion about it.\n\nAdding Phillip to the discussion as well.\n\n-- \nMax Gautier\n"},{"id":"491295","messageId":"Zf6ut_ZTCi-csjd3@framework","threadId":"61181","inReplyTo":"CAPig+cT=8WkxZF1vy-2dgSspFeGOQzUFRk6w+MYVCXyRAsfR8A@mail.gmail.com","subject":"Re: [PATCH v2 6/6] maintenance: update tests for systemd scheduler","fromName":"Max Gautier","fromEmail":"mg@max.gautier.name","sentAt":"2024-03-23T10:28:07Z","receivedAt":"2024-03-23T10:28:09Z","isPatch":true,"sender":{"key":"mg@max.gautier.name","avatar":"https://avatars.githubusercontent.com/u/13346812?v=4"},"body":"On Fri, Mar 22, 2024 at 07:02:36PM -0400, Eric Sunshine wrote:\n> On Fri, Mar 22, 2024 at 6:13 PM Max Gautier wrote:\n> > The systemd units are now in the source tree, rather than produced when\n> > running git maitenance start. There is no need anymore to couple\n> > validating the units and testing `git maintenance start`.\n> \n> s/maitenance/maintenance/\n> \nAck\n> > Adjust the test to verify the new `systemctl` command used, discard\n> > checks for presence/absence of unit files in $XDG_CONFIG_HOME.\n> >\n> > Validate the systemd units in the source tree, with one test per unit to\n> > have more distinct failures.\n> >\n> > Signed-off-by: Max Gautier \n> \n> In a patch series, in order to preserve \"bisectability\", we want to\n> ensure that the entire test suite continues to pass after each patch\n> is applied. A such, we normally update tests -- to ensure that they\n> continue passing -- in each patch which changes some\n> testable/observable behavior. However, this series only updates test\n> in the final patch. Doesn't that break bisectability? Or am I\n> misunderstanding something?\n\nNo you're right, good point. I'll split this one up and fold the parts\nwhere the behavior was changed.\nAnd I'll keep that in mind in the future.\n\n-- \nMax Gautier\n"},{"id":"491296","messageId":"Zf636bsHs9woXRAS@framework","threadId":"61181","inReplyTo":"20240322221327.12204-5-mg@max.gautier.name","subject":"Re: [PATCH v2 4/6] maintenance: cleanup $XDG_CONFIG_HOME/systemd/user","fromName":"Max Gautier","fromEmail":"mg@max.gautier.name","sentAt":"2024-03-23T11:07:21Z","receivedAt":"2024-03-23T11:07:24Z","isPatch":true,"sender":{"key":"mg@max.gautier.name","avatar":"https://avatars.githubusercontent.com/u/13346812?v=4"},"body":"On Fri, Mar 22, 2024 at 11:11:09PM +0100, Max Gautier wrote:\n> \n> +/*\n> + * TODO: in the future (~2026 ?) remove this cleanup code\n> + */\n> +static void systemd_delete_user_unit(char const *unit)\n> +{\n> +\tchar const\tfile_start_stale[] =\t\"# This file was created and is\"\n> +\t\t\t\t\t\t\" maintained by Git.\";\n> +\tchar\t\tfile_start_user[sizeof(file_start_stale)] = {'\\0'};\n> +\n> +\tchar *filename = xdg_config_home_for(\"systemd/user\", unit);\n> +\tint handle = open(filename, O_RDONLY);\n> +\n> +\t/*\n> +\t * Check this is actually our file and we're not removing a legitimate\n> +\t * user override.\n> +\t */\n> +\tif (handle == -1 && !is_missing_file_error(errno))\n> +\t\twarning(_(\"failed to delete '%s'\"), filename);\n> +\telse {\n> +\t\tread(handle, file_start_user, sizeof(file_start_stale) - 1);\n\nActually that fails -Werror because I don't check read return.\nAlternative below (on top of this one), with one question:\nAre VLA using size_t const OK ? It's folded to a constant array by gcc\nbut I don't know if that causes portability problem with other platforms\n? I can always repeat the sizeof expr if it's a problematic construct.\n\n-- >8 --\n\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex 99b158e481..7fb25ea2b1 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -2332,11 +2332,14 @@ static int systemd_set_units_state(int enable)\n /*\n * TODO: in the future (~2026 ?) remove this cleanup code\n */\n+\n static void systemd_delete_user_unit(char const *unit)\n {\n \tchar const\tfile_start_stale[] =\t\"# This file was created and is\"\n \t\t\t\t\t\t\" maintained by Git.\";\n-\tchar\t\tfile_start_user[sizeof(file_start_stale)] = {'\\0'};\n+\tsize_t const\tlength = sizeof(file_start_stale);\n+\tchar\t\tfile_start_user[length] = {'\\0'};\n+\n \n \tchar *filename = xdg_config_home_for(\"systemd/user\", unit);\n \tint handle = open(filename, O_RDONLY);\n@@ -2348,14 +2351,14 @@ static void systemd_delete_user_unit(char const *unit)\n \tif (handle == -1 && !is_missing_file_error(errno))\n \t\twarning(_(\"failed to delete '%s'\"), filename);\n \telse {\n-\t\tread(handle, file_start_user, sizeof(file_start_stale) - 1);\n-\t\tclose(handle);\n-\t\tif (strcmp(file_start_stale, file_start_user) == 0) {\n+\t\tif (length - 1 == read(handle, file_start_user, length - 1) &&\n+\t\t\t\tstrcmp(file_start_stale, file_start_user) == 0) {\n \t\t\tif (unlink(filename) == 0)\n \t\t\t\twarning(_(\"deleted stale unit file '%s'\"), filename);\n \t\t\telse if (!is_missing_file_error(errno))\n \t\t\t\twarning(_(\"failed to delete '%s'\"), filename);\n \t\t}\n+\t\tclose(handle);\n \t}\n \n \tfree(filename);\n\n-- \nMax Gautier\n"},{"id":"491352","messageId":"80580cc5-0285-43d2-ac51-71dce16f0028@gmail.com","threadId":"61181","inReplyTo":"20240322221327.12204-1-mg@max.gautier.name","subject":"Re: [PATCH v2 0/6] maintenance: use packaged systemd units","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-03-24T14:54:58Z","receivedAt":"2024-03-24T14:55:01Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Max\n\nOn 22/03/2024 22:11, Max Gautier wrote:\n> * Distribute the systemd timers used by the `git maintenance start` with\n> the systemd scheduler as part of git, rather than writing them in\n> $XDG_CONFIG_HOME.\n> \n> This allows users to override the units if they wish, and is more\n> in-line with the usual practices of distribution for systemd units.\n\nThanks for suggesting this, I think this is a useful change, but the \nimplementation could be improved.\n\n> We also move away from using the random minute, and instead rely on\n> systemd features to achieve the same goal (see patch 2). This allows us\n> to go back to using unit templating for the timers. This is also a\n> prerequisite to have static unit files.\n> \n> Note that even if we really need more specific OnCalendar= settings for\n> each timer, we should still do it that way, but instead distribute\n> override alongside the template, for instance for weekly:\n> \n> /usr/lib/systemd-user/git-maintenance@daily.timer.d/override.conf:\n> [Timer]\n> OnCalendar=\n\nWe should definitely do that. Using systemd's random delay does not \nprevent the different maintenance jobs from running concurrently as one \njob may be started before a previous job has finished. It is important \nto only have one job running at a time because the first thing \"git \nmaintenance run\" does is to try and acquire a lock file so if the hourly \njob is running when the daily jobs tries to start the daily job will not \nbe run. As the daily job is a superset of the hourly job and the weekly \njob is a superset of the daily job so it does not make sense to run more \nthan one job per hour.\n\n> The cleanup code for the units written in $XDG_CONFIG_HOME is adapted,\n> and takes care of not removing legitimate user overrides, by checking\n> the file start.\n\nThis points to an alternate strategy for supporting user overrides - \ndon't overwrite the unit files if the user has edited them. I think that \nthere is still a benefit to moving to system wide unit files though as \nit means that if we improve the unit files in the future systemd will \npick up these improvements automatically. That is an improvement over \nthe status quo where the users' unit files are written once and never \nupdated.\n\nI think it would help to reorder the changes in this series as follows:\n\n1 - simplify the invocation of \"systemctl --user\"\n This would be the current patch 3 without adding \"--force\" or\n moving \"--now\" combined with the relevant test changes from patch 6.\n This would make it clear that those changes are a simple clean up that\n is independent of the other changes made in this series.\n\n2 - don't delete user edited unit files\n This would be based on the current patch 4 and would show that we can\n avoid deleting unit files that the user has edited without the other\n changes in this series. This change should have an associated test.\n\n3 - start using systemd's random delay function\n This would be the current patch 1 without the template changes and the\n commit message should explain that it is in preparation for disturbing\n system-wide unit files.\n\n4 - install system-wide systemd unit files\n This would be based on the current patch 2 with the addition of\n overrides to prevent more than one job running per hour. The unit\n files should be installed under $XDG_DATA_HOME when $(prefix) starts\n with $(HOME), not just when they are equal. The associated test\n changes from patch 6 should be moved here as well as the \"--force\"\n change from patch 3.\n\n5 - documentation updates\n I'm on the fence about having these in a separate commit like the\n current patch 5 or updating the documentation when the code is\n changed.\n\nBest Wishes\n\nPhillip\n\n> Testing:\n> The simplest way to simulate having the units in /usr/lib is probably to\n> copy them in /etc/systemd/user.\n> \n> Changes since v1:\n> - Reorganization of the commits and their messages to try to address\n> review comments\n> - Dropped the DON'T APPLY PATCH, added a TODO to the cleanup code\n> instead\n> - Updated the git-maintenance tests to work with the new logic.\n> - Conditional installation of the units files\n> - Fixing some style/consistency issues\n> - template the systemd service file to use $(bindir)\n> \n> Max Gautier (6):\n> maintenance: use systemd timers builtin randomization\n> maintenance: use packaged systemd units\n> maintenance: simplify systemctl calls\n> maintenance: cleanup $XDG_CONFIG_HOME/systemd/user\n> maintenance: update systemd scheduler docs\n> maintenance: update tests for systemd scheduler\n> \n> Documentation/git-maintenance.txt | 33 ++-\n> Makefile | 5 +\n> builtin/gc.c | 298 ++++-------------------\n> config.mak.uname | 10 +\n> systemd/user/git-maintenance@.service.in | 17 ++\n> systemd/user/git-maintenance@.timer | 12 +\n> t/t7900-maintenance.sh | 50 ++--\n> 7 files changed, 126 insertions(+), 299 deletions(-)\n> create mode 100644 systemd/user/git-maintenance@.service.in\n> create mode 100644 systemd/user/git-maintenance@.timer\n> \n> Range-diff against v1:\n> 1: ea54a6e50e < -: ---------- maintenance: package systemd units\n> 2: b29dbb9fdd < -: ---------- maintenance: use packaged systemd units\n> 3: 47bd6712b8 < -: ---------- maintenance: add fixed random delay to systemd timers\n> -: ---------- > 1: 42d88c7f81 maintenance: use systemd timers builtin randomization\n> -: ---------- > 2: 18d51b1dd1 maintenance: use packaged systemd units\n> -: ---------- > 3: 3aa7446e95 maintenance: simplify systemctl calls\n> -: ---------- > 4: daff7b4d60 maintenance: cleanup $XDG_CONFIG_HOME/systemd/user\n> 4: fac57db55e ! 5: 5f6a8e141f maintenance: update systemd scheduler docs\n> @@ Metadata\n> ## Commit message ##\n> maintenance: update systemd scheduler docs\n> \n> + The `git maintenance` systemd scheduler no longer writes units in\n> + $XDG_CONFIG_HOME.\n> +\n> + Describe the new behavior.\n> + Instead of explaining manual ways to modify the timer, suggest the\n> + systemd standard tool: `systemctl edit`.\n> +\n> Signed-off-by: Max Gautier \n> \n> ## Documentation/git-maintenance.txt ##\n> 5: d888fbd0c3 < -: ---------- DON'T APPLY YET: maintenance: remove cleanup code\n> -: ---------- > 6: 4d4bcd6233 maintenance: update tests for systemd scheduler\n"},{"id":"491355","messageId":"1cce271f-c3df-432b-94ff-bf7b11c6924d@gmail.com","threadId":"61181","inReplyTo":"Zf636bsHs9woXRAS@framework","subject":"Re: [PATCH v2 4/6] maintenance: cleanup $XDG_CONFIG_HOME/systemd/user","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-03-24T15:45:45Z","receivedAt":"2024-03-24T15:45:47Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Max\n\nOn 23/03/2024 11:07, Max Gautier wrote:\n> On Fri, Mar 22, 2024 at 11:11:09PM +0100, Max Gautier wrote:\n>> \n>> +/*\n>> + * TODO: in the future (~2026 ?) remove this cleanup code\n>> + */\n\nThat is rather optimistic - users only run \"git maintenance start\" once \nso any unit files that have been written in the past will exist well \nbeyond 2026.\n\n>> +static void systemd_delete_user_unit(char const *unit)\n>> +{\n>> +\tchar const\tfile_start_stale[] =\t\"# This file was created and is\"\n>> +\t\t\t\t\t\t\" maintained by Git.\";\n>> +\tchar\t\tfile_start_user[sizeof(file_start_stale)] = {'\\0'};\n>> +\n>> +\tchar *filename = xdg_config_home_for(\"systemd/user\", unit);\n>> +\tint handle = open(filename, O_RDONLY);\n>> +\n>> +\t/*\n>> +\t * Check this is actually our file and we're not removing a legitimate\n>> +\t * user override.\n>> +\t */\n>> +\tif (handle == -1 && !is_missing_file_error(errno))\n>> +\t\twarning(_(\"failed to delete '%s'\"), filename);\n>> +\telse {\n>> +\t\tread(handle, file_start_user, sizeof(file_start_stale) - 1);\n> \n> Actually that fails -Werror because I don't check read return.\n> Alternative below (on top of this one), with one question:\n> Are VLA using size_t const OK ? It's folded to a constant array by gcc\n> but I don't know if that causes portability problem with other platforms\n> ? I can always repeat the sizeof expr if it's a problematic construct.\n\nI think it would be easier to use strbuf_read_file() instead - it is \nonly a small file so there is not really any advantage in just reading \nthe first line. Something like\n\nstatic int systemd_delete_user_unit(const char* unit)\n{\n\tint ret = 0;\n\tstruct strbuf buf = STRBUF_INIT;\n\tchar *filename = xdg_config_home_for(\"systemd/user\", unit);\n\n\tif (strbuf_read_file(&buf, filename, 0) < 0) {\n\t\tif (errno != ENOENT)\n\t\t\tret = error_errno(_(\"could not read '%s'\", filename));\n\t\tgoto out;\n\t}\n\tif (starts_with(buf.buf,\n\t\t\t\"# This file was created and is maintained by git.\\n\") &&\n\t unlink(filename))\n\t\tret = error_errno(_(\"could not remove '%s', filename));\n\nout:\n\tfree(filename);\n\tstrbuf_release(&buf);\n\treturn ret;\n}\n\nBest Wishes\n\nPhillip\n\n> -- >8 --\n> \n> diff --git a/builtin/gc.c b/builtin/gc.c\n> index 99b158e481..7fb25ea2b1 100644\n> --- a/builtin/gc.c\n> +++ b/builtin/gc.c\n> @@ -2332,11 +2332,14 @@ static int systemd_set_units_state(int enable)\n> /*\n> * TODO: in the future (~2026 ?) remove this cleanup code\n> */\n> +\n> static void systemd_delete_user_unit(char const *unit)\n> {\n> \tchar const\tfile_start_stale[] =\t\"# This file was created and is\"\n> \t\t\t\t\t\t\" maintained by Git.\";\n> -\tchar\t\tfile_start_user[sizeof(file_start_stale)] = {'\\0'};\n> +\tsize_t const\tlength = sizeof(file_start_stale);\n> +\tchar\t\tfile_start_user[length] = {'\\0'};\n> +\n> \n> \tchar *filename = xdg_config_home_for(\"systemd/user\", unit);\n> \tint handle = open(filename, O_RDONLY);\n> @@ -2348,14 +2351,14 @@ static void systemd_delete_user_unit(char const *unit)\n> \tif (handle == -1 && !is_missing_file_error(errno))\n> \t\twarning(_(\"failed to delete '%s'\"), filename);\n> \telse {\n> -\t\tread(handle, file_start_user, sizeof(file_start_stale) - 1);\n> -\t\tclose(handle);\n> -\t\tif (strcmp(file_start_stale, file_start_user) == 0) {\n> +\t\tif (length - 1 == read(handle, file_start_user, length - 1) &&\n> +\t\t\t\tstrcmp(file_start_stale, file_start_user) == 0) {\n> \t\t\tif (unlink(filename) == 0)\n> \t\t\t\twarning(_(\"deleted stale unit file '%s'\"), filename);\n> \t\t\telse if (!is_missing_file_error(errno))\n> \t\t\t\twarning(_(\"failed to delete '%s'\"), filename);\n> \t\t}\n> +\t\tclose(handle);\n> \t}\n> \n> \tfree(filename);\n> \n"},{"id":"491364","messageId":"CAPig+cT1Bz9M5zsB_OSEu-LTTVMBPY6U4vaDHmoHtmoDfH+hpg@mail.gmail.com","threadId":"61181","inReplyTo":"80580cc5-0285-43d2-ac51-71dce16f0028@gmail.com","subject":"Re: [PATCH v2 0/6] maintenance: use packaged systemd units","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2024-03-24T17:03:42Z","receivedAt":"2024-03-24T17:03:54Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Mar 24, 2024 at 10:55 AM Phillip Wood wrote:\n> 5 - documentation updates\n> I'm on the fence about having these in a separate commit like the\n> current patch 5 or updating the documentation when the code is\n> changed.\n\nIt's generally more reviewer-friendly to bundle documentation change\ninto the patch which changes the observable behavior. This way, a\nreviewer has the behavior change fresh in mind and can verify that the\nrevised documentation matches the new implementation. Same goes for\nrevising tests in the same patch which changes behavior (though, of\ncourse, revising tests at the same time as the change of behavior is\nalso mandatory for maintaining bisectability).\n"},{"id":"491402","messageId":"ZgE2pFt-pXurYnKU@framework","threadId":"61181","inReplyTo":"80580cc5-0285-43d2-ac51-71dce16f0028@gmail.com","subject":"Re: [PATCH v2 0/6] maintenance: use packaged systemd units","fromName":"Max Gautier","fromEmail":"mg@max.gautier.name","sentAt":"2024-03-25T08:32:36Z","receivedAt":"2024-03-25T08:32:45Z","isPatch":true,"sender":{"key":"mg@max.gautier.name","avatar":"https://avatars.githubusercontent.com/u/13346812?v=4"},"body":"On Sun, Mar 24, 2024 at 02:54:58PM +0000, Phillip Wood wrote:\n> Hi Max\n> \n> On 22/03/2024 22:11, Max Gautier wrote:\n> > * Distribute the systemd timers used by the `git maintenance start` with\n> > the systemd scheduler as part of git, rather than writing them in\n> > $XDG_CONFIG_HOME.\n> > \n> > This allows users to override the units if they wish, and is more\n> > in-line with the usual practices of distribution for systemd units.\n> \n> Thanks for suggesting this, I think this is a useful change, but the\n> implementation could be improved.\n> \n> > We also move away from using the random minute, and instead rely on\n> > systemd features to achieve the same goal (see patch 2). This allows us\n> > to go back to using unit templating for the timers. This is also a\n> > prerequisite to have static unit files.\n> > \n> > Note that even if we really need more specific OnCalendar= settings for\n> > each timer, we should still do it that way, but instead distribute\n> > override alongside the template, for instance for weekly:\n> > \n> > /usr/lib/systemd-user/git-maintenance@daily.timer.d/override.conf:\n> > [Timer]\n> > OnCalendar=\n> \n> We should definitely do that. Using systemd's random delay does not prevent\n> the different maintenance jobs from running concurrently as one job may be\n> started before a previous job has finished. It is important to only have one\n> job running at a time because the first thing \"git maintenance run\" does is\n> to try and acquire a lock file so if the hourly job is running when the\n> daily jobs tries to start the daily job will not be run.\n\nThinking about that, it occurs to me that the current scheme does not\nprevent concurrent execution either: the timers all use Persistent=true,\nwhich means they can fire concurrently on machine boot, if two or more\nwould have been triggered during the time the machine was powered off\n(or just the user logged out, since it's a user unit).\n\nSo maybe there should be a more robust mechanism to avoid concurrent\nexecution ? I assume from what you say above the lock is acquired in a\nnon-blocking way. Could going to a blocking one be a solution ?\n\n> As the daily job is\n> a superset of the hourly job and the weekly job is a superset of the daily\n> job so it does not make sense to run more than one job per hour.\n\nIs that set in stone, or could they perform disjoint set of tasks\ninstead ?\n\n> \n> > The cleanup code for the units written in $XDG_CONFIG_HOME is adapted,\n> > and takes care of not removing legitimate user overrides, by checking\n> > the file start.\n> \n> This points to an alternate strategy for supporting user overrides - don't\n> overwrite the unit files if the user has edited them. I think that there is\n> still a benefit to moving to system wide unit files though as it means that\n> if we improve the unit files in the future systemd will pick up these\n> improvements automatically. That is an improvement over the status quo where\n> the users' unit files are written once and never updated.\n> \n> I think it would help to reorder the changes in this series as follows:\n> \n> 1 - simplify the invocation of \"systemctl --user\"\n> This would be the current patch 3 without adding \"--force\" or\n> moving \"--now\" combined with the relevant test changes from patch 6.\n> This would make it clear that those changes are a simple clean up that\n> is independent of the other changes made in this series.\n> \n> 2 - don't delete user edited unit files\n> This would be based on the current patch 4 and would show that we can\n> avoid deleting unit files that the user has edited without the other\n> changes in this series. This change should have an associated test.\n> \n> 3 - start using systemd's random delay function\n> This would be the current patch 1 without the template changes and the\n> commit message should explain that it is in preparation for disturbing\n> system-wide unit files.\n> \n> 4 - install system-wide systemd unit files\n> This would be based on the current patch 2 with the addition of\n> overrides to prevent more than one job running per hour. The unit\n> files should be installed under $XDG_DATA_HOME when $(prefix) starts\n> with $(HOME), not just when they are equal. The associated test\n> changes from patch 6 should be moved here as well as the \"--force\"\n> change from patch 3.\n> \n> 5 - documentation updates\n> I'm on the fence about having these in a separate commit like the\n> current patch 5 or updating the documentation when the code is\n> changed.\n\nI had started cooking v3, I'll take into account, thanks !\n\n-- \nMax Gautier\n"},{"id":"491403","messageId":"ZgE3mHsPfKbuQLQP@framework","threadId":"61181","inReplyTo":"1cce271f-c3df-432b-94ff-bf7b11c6924d@gmail.com","subject":"Re: [PATCH v2 4/6] maintenance: cleanup $XDG_CONFIG_HOME/systemd/user","fromName":"Max Gautier","fromEmail":"mg@max.gautier.name","sentAt":"2024-03-25T08:36:40Z","receivedAt":"2024-03-25T08:36:42Z","isPatch":true,"sender":{"key":"mg@max.gautier.name","avatar":"https://avatars.githubusercontent.com/u/13346812?v=4"},"body":"On Sun, Mar 24, 2024 at 03:45:45PM +0000, Phillip Wood wrote:\n> Hi Max\n> \n> On 23/03/2024 11:07, Max Gautier wrote:\n> > On Fri, Mar 22, 2024 at 11:11:09PM +0100, Max Gautier wrote:\n> > > +/*\n> > > + * TODO: in the future (~2026 ?) remove this cleanup code\n> > > + */\n> \n> That is rather optimistic - users only run \"git maintenance start\" once so\n> any unit files that have been written in the past will exist well beyond\n> 2026.\n\nIn that case, should we hook the cleanup (in it's final form) in more\nplace ? `git maintenance register` for instance ?\n\n> \n> > > +static void systemd_delete_user_unit(char const *unit)\n> > > +{\n> > > +\tchar const\tfile_start_stale[] =\t\"# This file was created and is\"\n> > > +\t\t\t\t\t\t\" maintained by Git.\";\n> > > +\tchar\t\tfile_start_user[sizeof(file_start_stale)] = {'\\0'};\n> > > +\n> > > +\tchar *filename = xdg_config_home_for(\"systemd/user\", unit);\n> > > +\tint handle = open(filename, O_RDONLY);\n> > > +\n> > > +\t/*\n> > > +\t * Check this is actually our file and we're not removing a legitimate\n> > > +\t * user override.\n> > > +\t */\n> > > +\tif (handle == -1 && !is_missing_file_error(errno))\n> > > +\t\twarning(_(\"failed to delete '%s'\"), filename);\n> > > +\telse {\n> > > +\t\tread(handle, file_start_user, sizeof(file_start_stale) - 1);\n> > \n> > Actually that fails -Werror because I don't check read return.\n> > Alternative below (on top of this one), with one question:\n> > Are VLA using size_t const OK ? It's folded to a constant array by gcc\n> > but I don't know if that causes portability problem with other platforms\n> > ? I can always repeat the sizeof expr if it's a problematic construct.\n> \n> I think it would be easier to use strbuf_read_file() instead - it is only a\n> small file so there is not really any advantage in just reading the first\n> line. Something like\n> \n> static int systemd_delete_user_unit(const char* unit)\n> {\n> \tint ret = 0;\n> \tstruct strbuf buf = STRBUF_INIT;\n> \tchar *filename = xdg_config_home_for(\"systemd/user\", unit);\n> \n> \tif (strbuf_read_file(&buf, filename, 0) < 0) {\n> \t\tif (errno != ENOENT)\n> \t\t\tret = error_errno(_(\"could not read '%s'\", filename));\n> \t\tgoto out;\n> \t}\n> \tif (starts_with(buf.buf,\n> \t\t\t\"# This file was created and is maintained by git.\\n\") &&\n> \t unlink(filename))\n> \t\tret = error_errno(_(\"could not remove '%s', filename));\n> \n> out:\n> \tfree(filename);\n> \tstrbuf_release(&buf);\n> \treturn ret;\n> }\n\nThanks, this is exactly what I needed, I looked at the strbuf API but\ncouldn't find this somehow.\n\n-- \nMax Gautier\n"},{"id":"491430","messageId":"03513931-7070-4430-bfae-aa039f73d74b@gmail.com","threadId":"61181","inReplyTo":"ZgE2pFt-pXurYnKU@framework","subject":"Re: [PATCH v2 0/6] maintenance: use packaged systemd units","fromName":"","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-03-25T10:06:29Z","receivedAt":"2024-03-25T10:06:32Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Max\n\nOn 25/03/2024 08:32, Max Gautier wrote:\n> On Sun, Mar 24, 2024 at 02:54:58PM +0000, Phillip Wood wrote:\n>> Hi Max\n>>\n>> On 22/03/2024 22:11, Max Gautier wrote:\n>>> * Distribute the systemd timers used by the `git maintenance start` with\n>>> the systemd scheduler as part of git, rather than writing them in\n>>> $XDG_CONFIG_HOME.\n>>>\n>>> This allows users to override the units if they wish, and is more\n>>> in-line with the usual practices of distribution for systemd units.\n>>\n>> Thanks for suggesting this, I think this is a useful change, but the\n>> implementation could be improved.\n>>\n>>> We also move away from using the random minute, and instead rely on\n>>> systemd features to achieve the same goal (see patch 2). This allows us\n>>> to go back to using unit templating for the timers. This is also a\n>>> prerequisite to have static unit files.\n>>>\n>>> Note that even if we really need more specific OnCalendar= settings for\n>>> each timer, we should still do it that way, but instead distribute\n>>> override alongside the template, for instance for weekly:\n>>>\n>>> /usr/lib/systemd-user/git-maintenance@daily.timer.d/override.conf:\n>>> [Timer]\n>>> OnCalendar=\n>>\n>> We should definitely do that. Using systemd's random delay does not prevent\n>> the different maintenance jobs from running concurrently as one job may be\n>> started before a previous job has finished. It is important to only have one\n>> job running at a time because the first thing \"git maintenance run\" does is\n>> to try and acquire a lock file so if the hourly job is running when the\n>> daily jobs tries to start the daily job will not be run.\n> \n> Thinking about that, it occurs to me that the current scheme does not\n> prevent concurrent execution either: the timers all use Persistent=true,\n> which means they can fire concurrently on machine boot, if two or more\n> would have been triggered during the time the machine was powered off\n> (or just the user logged out, since it's a user unit).\n\nInteresting, I wonder if the other schedulers suffer from the same problem.\n\n> So maybe there should be a more robust mechanism to avoid concurrent\n> execution ? I assume from what you say above the lock is acquired in a\n> non-blocking way. Could going to a blocking one be a solution ?\n\nIt is possible to wait on a lock file but I'd be worried about building \nup an endless queue of processes if the process holding the lock file \ncrashed leaving it in place without anyway to automatically remove it.\n\nI don't think we need to solve that problem as part of this patch series \nbut we should take care not to make it worse. Long term we may be better \nscheduling a single job and have \"git maintenance run\" decide which jobs \nto run based on the last time it run, rather than trying to schedule \ndifferent jobs with the os scheduler.\n\n>> As the daily job is\n>> a superset of the hourly job and the weekly job is a superset of the daily\n>> job so it does not make sense to run more than one job per hour.\n> \n> Is that set in stone, or could they perform disjoint set of tasks\n> instead ?\n\nAll of the schedulers are set up to run a single job each hour, I don't \nsee why we'd start running disjoint sets of tasks in the different jobs.\n\n>>> The cleanup code for the units written in $XDG_CONFIG_HOME is adapted,\n>>> and takes care of not removing legitimate user overrides, by checking\n>>> the file start.\n>>\n>> This points to an alternate strategy for supporting user overrides - don't\n>> overwrite the unit files if the user has edited them. I think that there is\n>> still a benefit to moving to system wide unit files though as it means that\n>> if we improve the unit files in the future systemd will pick up these\n>> improvements automatically. That is an improvement over the status quo where\n>> the users' unit files are written once and never updated.\n>>\n>> I think it would help to reorder the changes in this series as follows:\n>>\n>> 1 - simplify the invocation of \"systemctl --user\"\n>> This would be the current patch 3 without adding \"--force\" or\n>> moving \"--now\" combined with the relevant test changes from patch 6.\n>> This would make it clear that those changes are a simple clean up that\n>> is independent of the other changes made in this series.\n>>\n>> 2 - don't delete user edited unit files\n>> This would be based on the current patch 4 and would show that we can\n>> avoid deleting unit files that the user has edited without the other\n>> changes in this series. This change should have an associated test.\n>>\n>> 3 - start using systemd's random delay function\n>> This would be the current patch 1 without the template changes and the\n>> commit message should explain that it is in preparation for disturbing\n>> system-wide unit files.\n>>\n>> 4 - install system-wide systemd unit files\n>> This would be based on the current patch 2 with the addition of\n>> overrides to prevent more than one job running per hour. The unit\n>> files should be installed under $XDG_DATA_HOME when $(prefix) starts\n>> with $(HOME), not just when they are equal. The associated test\n>> changes from patch 6 should be moved here as well as the \"--force\"\n>> change from patch 3.\n>>\n>> 5 - documentation updates\n>> I'm on the fence about having these in a separate commit like the\n>> current patch 5 or updating the documentation when the code is\n>> changed.\n> \n> I had started cooking v3, I'll take into account, thanks !\n\nThanks\n\nPhillip\n"},{"id":"491432","messageId":"82415cdc-6544-4bb2-8080-90b002b1ed1d@gmail.com","threadId":"61181","inReplyTo":"CAPig+cT1Bz9M5zsB_OSEu-LTTVMBPY6U4vaDHmoHtmoDfH+hpg@mail.gmail.com","subject":"Re: [PATCH v2 0/6] maintenance: use packaged systemd units","fromName":"","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-03-25T10:08:52Z","receivedAt":"2024-03-25T10:08:55Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 24/03/2024 17:03, Eric Sunshine wrote:\n> On Sun, Mar 24, 2024 at 10:55 AM Phillip Wood wrote:\n>> 5 - documentation updates\n>> I'm on the fence about having these in a separate commit like the\n>> current patch 5 or updating the documentation when the code is\n>> changed.\n> \n> It's generally more reviewer-friendly to bundle documentation change\n> into the patch which changes the observable behavior. This way, a\n> reviewer has the behavior change fresh in mind and can verify that the\n> revised documentation matches the new implementation. Same goes for\n> revising tests in the same patch which changes behavior (though, of\n> course, revising tests at the same time as the change of behavior is\n> also mandatory for maintaining bisectability).\n\nGood point, I think a couple of the documentation changes like \nrecommending \"systemctl --user edit\" were improving the existing docs \nand so they should be in a separate commit at the start of the series. \nThe other patches should update the documentation as the code changes.\n\nThanks\n\nPhillip\n"},{"id":"491447","messageId":"ZgFtorXnGPm45oO0@framework","threadId":"61181","inReplyTo":"03513931-7070-4430-bfae-aa039f73d74b@gmail.com","subject":"Re: [PATCH v2 0/6] maintenance: use packaged systemd units","fromName":"Max Gautier","fromEmail":"mg@max.gautier.name","sentAt":"2024-03-25T12:27:14Z","receivedAt":"2024-03-25T12:27:29Z","isPatch":true,"sender":{"key":"mg@max.gautier.name","avatar":"https://avatars.githubusercontent.com/u/13346812?v=4"},"body":"On Mon, Mar 25, 2024 at 10:06:29AM +0000, phillip.wood123@gmail.com wrote:\n> Hi Max\n> \n> On 25/03/2024 08:32, Max Gautier wrote:\n> > On Sun, Mar 24, 2024 at 02:54:58PM +0000, Phillip Wood wrote:\n> > > Hi Max\n> > > \n> > > On 22/03/2024 22:11, Max Gautier wrote:\n> > > > * Distribute the systemd timers used by the `git maintenance start` with\n> > > > the systemd scheduler as part of git, rather than writing them in\n> > > > $XDG_CONFIG_HOME.\n> > > > \n> > > > This allows users to override the units if they wish, and is more\n> > > > in-line with the usual practices of distribution for systemd units.\n> > > \n> > > Thanks for suggesting this, I think this is a useful change, but the\n> > > implementation could be improved.\n> > > \n> > > > We also move away from using the random minute, and instead rely on\n> > > > systemd features to achieve the same goal (see patch 2). This allows us\n> > > > to go back to using unit templating for the timers. This is also a\n> > > > prerequisite to have static unit files.\n> > > > \n> > > > Note that even if we really need more specific OnCalendar= settings for\n> > > > each timer, we should still do it that way, but instead distribute\n> > > > override alongside the template, for instance for weekly:\n> > > > \n> > > > /usr/lib/systemd-user/git-maintenance@daily.timer.d/override.conf:\n> > > > [Timer]\n> > > > OnCalendar=\n> > > \n> > > We should definitely do that. Using systemd's random delay does not prevent\n> > > the different maintenance jobs from running concurrently as one job may be\n> > > started before a previous job has finished. It is important to only have one\n> > > job running at a time because the first thing \"git maintenance run\" does is\n> > > to try and acquire a lock file so if the hourly job is running when the\n> > > daily jobs tries to start the daily job will not be run.\n> > \n> > Thinking about that, it occurs to me that the current scheme does not\n> > prevent concurrent execution either: the timers all use Persistent=true,\n> > which means they can fire concurrently on machine boot, if two or more\n> > would have been triggered during the time the machine was powered off\n> > (or just the user logged out, since it's a user unit).\n> \n> Interesting, I wonder if the other schedulers suffer from the same problem.\n\nFrom what I can find (didn't dig much):\n- cron does not have the problem, because it will just miss the timers\n if the machine was powered off. Not really better ^\n - anacron though is another implementation of cron which apparently\n support that semantic and is the default on ubuntu [1]\n I can't find if there is something to avoid the same problem that\n Persitent=true imply\n- same goes for launchctl (Effect of Sleeping and Powering Off at the\n bottom of the page [2])\n- for schtasks it's apparently possible to have a similar mechanism than\n Persistent [3]. There is a policy apparently to handle multiples\n instances [4] but I'm not completely sure whether or not theses\n instances can have different parameters.\n It's currently defined that way for the schtasks scheduler:\n \"IgnoreNew\\n\". I\n don't think it would prevent parallel execution between the different\n schedule though, it seems to me they are different tasks.\n\n[1]: https://serverfault.com/a/52338\n[2]: https://developer.apple.com/library/archive/documentation/MacOSX/Conceptual/BPSystemStartup/Chapters/ScheduledJobs.html\n[3]: https://learn.microsoft.com/en-us/troubleshoot/windows-server/system-management-components/scheduled-task-not-run-upon-reboot-machine-off\n[4]: https://learn.microsoft.com/en-us/windows/win32/taskschd/tasksettings-multipleinstances\n\n> \n> > So maybe there should be a more robust mechanism to avoid concurrent\n> > execution ? I assume from what you say above the lock is acquired in a\n> > non-blocking way. Could going to a blocking one be a solution ?\n> \n> It is possible to wait on a lock file but I'd be worried about building up\n> an endless queue of processes if the process holding the lock file crashed\n> leaving it in place without anyway to automatically remove it.\n> \n\nAt least with systemd we have some mechanisms to deal with that.\n- systemd timers don't touch an already running unit, so that won't\n trigger a new hourly or daily if the previous one is still running.\n- for the automatically removing it, we could:\n - use XDG_RUNTIME_DIR (\"%t\" in systemd units) which is removed on\n logout\n - optionally add a tmpfiles fragments to delete locks which are really\n too old (tmpfiles won't delete files on which a lock is taken)\n - I thought about using a common RuntimeDirectory (see systemd.exec),\n but this is not possible due to [5]\n\n\n[5]: https://github.com/systemd/systemd/issues/5394\n\n> I don't think we need to solve that problem as part of this patch series but\n> we should take care not to make it worse. Long term we may be better\n> scheduling a single job and have \"git maintenance run\" decide which jobs to\n> run based on the last time it run, rather than trying to schedule different\n> jobs with the os scheduler.\n> \n> > > As the daily job is\n> > > a superset of the hourly job and the weekly job is a superset of the daily\n> > > job so it does not make sense to run more than one job per hour.\n> > \n> > Is that set in stone, or could they perform disjoint set of tasks\n> > instead ?\n> \n> All of the schedulers are set up to run a single job each hour, I don't see\n> why we'd start running disjoint sets of tasks in the different jobs.\n\nI was wondering if running distinct tasks would allow overlapping\nexecution, or if the different tasks are not safe to run concurrently\nanyway. I'm not familiar enough with them and the git internals to tell.\n\nAnother option if the tasks set was distinct for each service instance\nwould be to use dependencies and ordering directives like this:\nweekly.service\n```\n[Unit]\nRequires=daily.service\nAfter=daily.service\n\n[Service]\nExecStart=\n```\n\ndaily.service\n```\n[Unit]\nRequires=hourly.service\nAfter=hourly.service\n\n[Service]\nExecStart=\n```\n\nhourly.service\n```\n[Service]\nExecStart=\n```\n\nThat would avoid concurrent execution I think.\n\n-- \nMax Gautier\n"},{"id":"491456","messageId":"ZgGAA7uwx3aqy0pH@framework","threadId":"61181","inReplyTo":"03513931-7070-4430-bfae-aa039f73d74b@gmail.com","subject":"Re: [PATCH v2 0/6] maintenance: use packaged systemd units","fromName":"Max Gautier","fromEmail":"mg@max.gautier.name","sentAt":"2024-03-25T13:45:39Z","receivedAt":"2024-03-25T13:45:49Z","isPatch":true,"sender":{"key":"mg@max.gautier.name","avatar":"https://avatars.githubusercontent.com/u/13346812?v=4"},"body":"On Mon, Mar 25, 2024 at 10:06:29AM +0000, phillip.wood123@gmail.com wrote:\n> \n> It is possible to wait on a lock file but I'd be worried about building up\n> an endless queue of processes if the process holding the lock file crashed\n> leaving it in place without anyway to automatically remove it.\n\nSide question: we can't use flock(2) in git, or lockf(3) ? The latter is\nin POSIX.\n\n-- \nMax Gautier\n"},{"id":"491461","messageId":"4f439090-0af4-4105-b009-250f522409c8@gmail.com","threadId":"61181","inReplyTo":"ZgFtorXnGPm45oO0@framework","subject":"Re: [PATCH v2 0/6] maintenance: use packaged systemd units","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-03-25T16:39:16Z","receivedAt":"2024-03-25T16:39:17Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 25/03/2024 12:27, Max Gautier wrote:\n> On Mon, Mar 25, 2024 at 10:06:29AM +0000, phillip.wood123@gmail.com wrote:\n>>>>> Note that even if we really need more specific OnCalendar= settings for\n>>>>> each timer, we should still do it that way, but instead distribute\n>>>>> override alongside the template, for instance for weekly:\n>>>>>\n>>>>> /usr/lib/systemd-user/git-maintenance@daily.timer.d/override.conf:\n>>>>> [Timer]\n>>>>> OnCalendar=\n>>>>\n>>>> We should definitely do that. Using systemd's random delay does not prevent\n>>>> the different maintenance jobs from running concurrently as one job may be\n>>>> started before a previous job has finished. It is important to only have one\n>>>> job running at a time because the first thing \"git maintenance run\" does is\n>>>> to try and acquire a lock file so if the hourly job is running when the\n>>>> daily jobs tries to start the daily job will not be run.\n>>>\n>>> Thinking about that, it occurs to me that the current scheme does not\n>>> prevent concurrent execution either: the timers all use Persistent=true,\n>>> which means they can fire concurrently on machine boot, if two or more\n>>> would have been triggered during the time the machine was powered off\n>>> (or just the user logged out, since it's a user unit).\n>>\n>> Interesting, I wonder if the other schedulers suffer from the same problem.\n> \n> From what I can find (didn't dig much):\n\nThanks for looking at this\n\n> - cron does not have the problem, because it will just miss the timers\n> if the machine was powered off. Not really better ^\n\nYes, skipping the jobs is not great. On debian at least the job will be \nrun if it is less than three hours since it should have been run. See\nhttps://manpages.debian.org/bookworm/cron/cron.8.en.html\n\n> - anacron though is another implementation of cron which apparently\n> support that semantic and is the default on ubuntu [1]\n> I can't find if there is something to avoid the same problem that\n> Persitent=true imply\n\n> - same goes for launchctl (Effect of Sleeping and Powering Off at the\n> bottom of the page [2])\n\nAs I read it the job is rescheduled if the computer was asleep when it \nshould have run, but not if it was powered off.\n\n> - for schtasks it's apparently possible to have a similar mechanism than\n> Persistent [3]. There is a policy apparently to handle multiples\n> instances [4] but I'm not completely sure whether or not theses\n> instances can have different parameters.\n> It's currently defined that way for the schtasks scheduler:\n> \"IgnoreNew\\n\". I\n> don't think it would prevent parallel execution between the different\n> schedule though, it seems to me they are different tasks.\n> \n> [1]: https://serverfault.com/a/52338\n> [2]: https://developer.apple.com/library/archive/documentation/MacOSX/Conceptual/BPSystemStartup/Chapters/ScheduledJobs.html\n> [3]: https://learn.microsoft.com/en-us/troubleshoot/windows-server/system-management-components/scheduled-task-not-run-upon-reboot-machine-off\n> [4]: https://learn.microsoft.com/en-us/windows/win32/taskschd/tasksettings-multipleinstances\n\nWe should have a think about what to do about this once your patches to \nmove to system-wide unit files are merged. We'll need to come up with \nsomething that works for all the schedulers so that we don't miss the \ndaily and weekly jobs when the computer is powered off and ensures we \ndon't run concurrent jobs.\n\nBest Wishes\n\nPhillip\n\n>>> So maybe there should be a more robust mechanism to avoid concurrent\n>>> execution ? I assume from what you say above the lock is acquired in a\n>>> non-blocking way. Could going to a blocking one be a solution ?\n>>\n>> It is possible to wait on a lock file but I'd be worried about building up\n>> an endless queue of processes if the process holding the lock file crashed\n>> leaving it in place without anyway to automatically remove it.\n>>\n> \n> At least with systemd we have some mechanisms to deal with that.\n> - systemd timers don't touch an already running unit, so that won't\n> trigger a new hourly or daily if the previous one is still running.\n> - for the automatically removing it, we could:\n> - use XDG_RUNTIME_DIR (\"%t\" in systemd units) which is removed on\n> logout\n> - optionally add a tmpfiles fragments to delete locks which are really\n> too old (tmpfiles won't delete files on which a lock is taken)\n> - I thought about using a common RuntimeDirectory (see systemd.exec),\n> but this is not possible due to [5]\n> \n> \n> [5]: https://github.com/systemd/systemd/issues/5394\n> \n>> I don't think we need to solve that problem as part of this patch series but\n>> we should take care not to make it worse. Long term we may be better\n>> scheduling a single job and have \"git maintenance run\" decide which jobs to\n>> run based on the last time it run, rather than trying to schedule different\n>> jobs with the os scheduler.\n>>\n>>>> As the daily job is\n>>>> a superset of the hourly job and the weekly job is a superset of the daily\n>>>> job so it does not make sense to run more than one job per hour.\n>>>\n>>> Is that set in stone, or could they perform disjoint set of tasks\n>>> instead ?\n>>\n>> All of the schedulers are set up to run a single job each hour, I don't see\n>> why we'd start running disjoint sets of tasks in the different jobs.\n> \n> I was wondering if running distinct tasks would allow overlapping\n> execution, or if the different tasks are not safe to run concurrently\n> anyway. I'm not familiar enough with them and the git internals to tell.\n> \n> Another option if the tasks set was distinct for each service instance\n> would be to use dependencies and ordering directives like this:\n> weekly.service\n> ```\n> [Unit]\n> Requires=daily.service\n> After=daily.service\n> \n> [Service]\n> ExecStart=\n> ```\n> \n> daily.service\n> ```\n> [Unit]\n> Requires=hourly.service\n> After=hourly.service\n> \n> [Service]\n> ExecStart=\n> ```\n> \n> hourly.service\n> ```\n> [Service]\n> ExecStart=\n> ```\n> \n> That would avoid concurrent execution I think.\n> \n"},{"id":"491462","messageId":"cd7bc865-4a01-46f6-8d68-24f02fdabcb9@gmail.com","threadId":"61181","inReplyTo":"ZgE3mHsPfKbuQLQP@framework","subject":"Re: [PATCH v2 4/6] maintenance: cleanup $XDG_CONFIG_HOME/systemd/user","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-03-25T16:39:23Z","receivedAt":"2024-03-25T16:39:24Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Max\n\nOn 25/03/2024 08:36, Max Gautier wrote:\n> On Sun, Mar 24, 2024 at 03:45:45PM +0000, Phillip Wood wrote:\n>> Hi Max\n>>\n>> On 23/03/2024 11:07, Max Gautier wrote:\n>>> On Fri, Mar 22, 2024 at 11:11:09PM +0100, Max Gautier wrote:\n>>>> +/*\n>>>> + * TODO: in the future (~2026 ?) remove this cleanup code\n>>>> + */\n>>\n>> That is rather optimistic - users only run \"git maintenance start\" once so\n>> any unit files that have been written in the past will exist well beyond\n>> 2026.\n> \n> In that case, should we hook the cleanup (in it's final form) in more\n> place ? `git maintenance register` for instance ?\n\nI'm not sure if that is needed if we leave the code to delete the unit \nfiles in place.\n\nBest Wishes\n\nPhillip\n\n>>>> +static void systemd_delete_user_unit(char const *unit)\n>>>> +{\n>>>> +\tchar const\tfile_start_stale[] =\t\"# This file was created and is\"\n>>>> +\t\t\t\t\t\t\" maintained by Git.\";\n>>>> +\tchar\t\tfile_start_user[sizeof(file_start_stale)] = {'\\0'};\n>>>> +\n>>>> +\tchar *filename = xdg_config_home_for(\"systemd/user\", unit);\n>>>> +\tint handle = open(filename, O_RDONLY);\n>>>> +\n>>>> +\t/*\n>>>> +\t * Check this is actually our file and we're not removing a legitimate\n>>>> +\t * user override.\n>>>> +\t */\n>>>> +\tif (handle == -1 && !is_missing_file_error(errno))\n>>>> +\t\twarning(_(\"failed to delete '%s'\"), filename);\n>>>> +\telse {\n>>>> +\t\tread(handle, file_start_user, sizeof(file_start_stale) - 1);\n>>>\n>>> Actually that fails -Werror because I don't check read return.\n>>> Alternative below (on top of this one), with one question:\n>>> Are VLA using size_t const OK ? It's folded to a constant array by gcc\n>>> but I don't know if that causes portability problem with other platforms\n>>> ? I can always repeat the sizeof expr if it's a problematic construct.\n>>\n>> I think it would be easier to use strbuf_read_file() instead - it is only a\n>> small file so there is not really any advantage in just reading the first\n>> line. Something like\n>>\n>> static int systemd_delete_user_unit(const char* unit)\n>> {\n>> \tint ret = 0;\n>> \tstruct strbuf buf = STRBUF_INIT;\n>> \tchar *filename = xdg_config_home_for(\"systemd/user\", unit);\n>>\n>> \tif (strbuf_read_file(&buf, filename, 0) < 0) {\n>> \t\tif (errno != ENOENT)\n>> \t\t\tret = error_errno(_(\"could not read '%s'\", filename));\n>> \t\tgoto out;\n>> \t}\n>> \tif (starts_with(buf.buf,\n>> \t\t\t\"# This file was created and is maintained by git.\\n\") &&\n>> \t unlink(filename))\n>> \t\tret = error_errno(_(\"could not remove '%s', filename));\n>>\n>> out:\n>> \tfree(filename);\n>> \tstrbuf_release(&buf);\n>> \treturn ret;\n>> }\n> \n> Thanks, this is exactly what I needed, I looked at the strbuf API but\n> couldn't find this somehow.\n> \n"},{"id":"491463","messageId":"398f3763-be1e-40f3-8b91-d0a62a530f64@gmail.com","threadId":"61181","inReplyTo":"ZgGAA7uwx3aqy0pH@framework","subject":"Re: [PATCH v2 0/6] maintenance: use packaged systemd units","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-03-25T16:39:47Z","receivedAt":"2024-03-25T16:39:48Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 25/03/2024 13:45, Max Gautier wrote:\n> On Mon, Mar 25, 2024 at 10:06:29AM +0000, phillip.wood123@gmail.com wrote:\n>>\n>> It is possible to wait on a lock file but I'd be worried about building up\n>> an endless queue of processes if the process holding the lock file crashed\n>> leaving it in place without anyway to automatically remove it.\n> \n> Side question: we can't use flock(2) in git, or lockf(3) ? The latter is\n> in POSIX.\n\nWe need something cross-platform, I don't think there is any appetite to \nmove away from our current lock file implementation. We can think about \nhow to handle concurrent maintenance jobs once these patches are merged.\n\nBest Wishes\n\nPhillip\n\n\n"},{"id":"491703","messageId":"9426FFE4-09FE-4918-92A7-DE22648962D0@max.gautier.name","threadId":"61181","inReplyTo":"cd7bc865-4a01-46f6-8d68-24f02fdabcb9@gmail.com","subject":"Re: [PATCH v2 4/6] maintenance: cleanup $XDG_CONFIG_HOME/systemd/user","fromName":"Max Gautier","fromEmail":"mg@max.gautier.name","sentAt":"2024-03-27T16:20:27Z","receivedAt":"2024-03-27T16:20:39Z","isPatch":true,"sender":{"key":"mg@max.gautier.name","avatar":"https://avatars.githubusercontent.com/u/13346812?v=4"},"body":"Le 25 mars 2024 17:39:23 GMT+01:00, Phillip Wood a écrit :\n>Hi Max\n>\n>On 25/03/2024 08:36, Max Gautier wrote:\n>> On Sun, Mar 24, 2024 at 03:45:45PM +0000, Phillip Wood wrote:\n>>> Hi Max\n>>> \n>>> On 23/03/2024 11:07, Max Gautier wrote:\n>>>> On Fri, Mar 22, 2024 at 11:11:09PM +0100, Max Gautier wrote:\n>>>>> +/*\n>>>>> + * TODO: in the future (~2026 ?) remove this cleanup code\n>>>>> + */\n>>> \n>>> That is rather optimistic - users only run \"git maintenance start\" once so\n>>> any unit files that have been written in the past will exist well beyond\n>>> 2026.\n>> \n>> In that case, should we hook the cleanup (in it's final form) in more\n>> place ? `git maintenance register` for instance ?\n>\n>I'm not sure if that is needed if we leave the code to delete the unit files in place.\n>\n>Best Wishes\n\nBut don't we want to remove it, eventually ?\n\n-- \nMax Gautier\n"},{"id":"491704","messageId":"9EAC734D-3614-4237-8F17-DDDF060201F3@max.gautier.name","threadId":"61181","inReplyTo":"398f3763-be1e-40f3-8b91-d0a62a530f64@gmail.com","subject":"Re: [PATCH v2 0/6] maintenance: use packaged systemd units","fromName":"Max Gautier","fromEmail":"mg@max.gautier.name","sentAt":"2024-03-27T16:21:12Z","receivedAt":"2024-03-27T16:21:15Z","isPatch":true,"sender":{"key":"mg@max.gautier.name","avatar":"https://avatars.githubusercontent.com/u/13346812?v=4"},"body":"Le 25 mars 2024 17:39:47 GMT+01:00, Phillip Wood a écrit :\n>On 25/03/2024 13:45, Max Gautier wrote:\n>> On Mon, Mar 25, 2024 at 10:06:29AM +0000, phillip.wood123@gmail.com wrote:\n>>> \n>>> It is possible to wait on a lock file but I'd be worried about building up\n>>> an endless queue of processes if the process holding the lock file crashed\n>>> leaving it in place without anyway to automatically remove it.\n>> \n>> Side question: we can't use flock(2) in git, or lockf(3) ? The latter is\n>> in POSIX.\n>\n>We need something cross-platform, I don't think there is any appetite to move away from our current lock file implementation. We can think about how to handle concurrent maintenance jobs once these patches are merged.\n>\n>Best Wishes\n>\n>Phillip\n>\n>\n\nAck, I'll leave that concern aside for now.\n\n\n-- \nMax Gautier\n"}]}