[RFC PATCH 4/5] maintenance: update systemd scheduler docs

[RFC PATCH 4/5] maintenance: update systemd scheduler docs

[Max Gautier]

Signed-off-by: Max Gautier <mg@max.gautier.name>
---
 Documentation/git-maintenance.txt | 33 +++++++++++++++----------------
 1 file changed, 16 insertions(+), 17 deletions(-)

diff --git a/Documentation/git-maintenance.txt b/Documentation/git-maintenance.txt
index 51d0f7e94b..6511c3f3f1 100644
--- a/Documentation/git-maintenance.txt
+++ b/Documentation/git-maintenance.txt
@@ -304,10 +304,9 @@ distributions, systemd timers are superseding it.
 If user systemd timers are available, they will be used as a replacement
 of `cron`.
 
-In this case, `git maintenance start` will create user systemd timer units
-and start the timers. The current list of user-scheduled tasks can be found
-by running `systemctl --user list-timers`. The timers written by `git
-maintenance start` are similar to this:
+In this case, `git maintenance start` will enable user systemd timer units
+and start them. The current list of user-scheduled tasks can be found by
+running `systemctl --user list-timers`. These timers are similar to this:
 
 -----------------------------------------------------------------------
 $ systemctl --user list-timers
@@ -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
 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
 -----------------------------------------------------------------------
 
-One timer is registered for each `--schedule=<frequency>` option.
+One timer instance is enabled for each `--schedule=<frequency>` option.
 
-The definition of the systemd units can be inspected in the following files:
+The definition of the systemd units can be inspected this way:
 
 -----------------------------------------------------------------------
-~/.config/systemd/user/git-maintenance@.timer
-~/.config/systemd/user/git-maintenance@.service
-~/.config/systemd/user/timers.target.wants/git-maintenance@hourly.timer
-~/.config/systemd/user/timers.target.wants/git-maintenance@daily.timer
-~/.config/systemd/user/timers.target.wants/git-maintenance@weekly.timer
+$ systemctl cat --user git-maintenance@.timer
+$ systemctl cat --user git-maintenance@.service
 -----------------------------------------------------------------------
 
-`git maintenance start` will overwrite these files and start the timer
-again with `systemctl --user`, so any customization should be done by
-creating a drop-in file, i.e. a `.conf` suffixed file in the
-`~/.config/systemd/user/git-maintenance@.service.d` directory.
+Customization of the timer or service can be performed with the usual systemd
+tooling:
+-----------------------------------------------------------------------
+$ systemctl edit --user git-maintenance@.timer # all the timers
+$ systemctl edit --user git-maintenance@hourly.timer # the hourly timer
+$ systemctl edit --user git-maintenance@.service # all the services
+$ systemctl edit --user git-maintenance@hourly.service # the hourly run
+-----------------------------------------------------------------------
 
-`git maintenance stop` will stop the user systemd timers and delete
-the above mentioned files.
+`git maintenance stop` will disable and stop the user systemd timers.
 
 For more details, see `systemd.timer(5)`.
 
-- 
2.44.0

[RFC PATCH 0/5] maintenance: use packaged systemd units

[Max Gautier]

Please ignore the previous series, I messed up the cover letter and Cc
with git-send-email somehow...

-----

Hello,

This is a proposal to distribute the timers of the systemd scheduler of
git-maintenance directly, rather than inlining them into the code and
writing them on demand.
IMO, this is better than using the user $XDG_CONFIG_HOME, and allows
that user to override them if they so wish.

We also move away from using the random minute, and instead rely on
systemd features to achieve the same goal (see patch 2). This allows us
to go back to using unit templating for the timers.
(Not that even if we really more specific OnCalendar= settings for each
timer, we should still do it that way, but instead distribute override
alongside the template, i.e

/usr/lib/systemd-user/git-maintenance@daily.timer.d/override.conf:
[Timer]
OnCalendar=<daily specific calendar spec>

The cleanup code for the units written in $XDG_CONFIG_HOME is adapted,
and takes care of not removing legitimate user overrides, by checking
the file start.
Patch 5 removes the cleanup code, it should not be applied right away,
but once enough time has passed and we can be reasonably sure that no
one still has the old units laying around.

Testing:
The simplest way to simulate having the units in /usr/lib is probably to
copy them in /etc/systemd/user.

Unresolved (reason why it's still an RFC):
- Should I implement conditional install of systemd units (if systemd is
  available) ? I've avoided digging too deep in the Makefile, but that's
  doable, I think.


---
 Documentation/git-maintenance.txt     |  33 ++--
 Makefile                              |   4 +
 builtin/gc.c                          | 279 ++--------------------------------
 systemd/user/git-maintenance@.service |  17 +++
 systemd/user/git-maintenance@.timer   |  12 ++
 5 files changed, 63 insertions(+), 282 deletions(-)

[RFC PATCH 1/5] maintenance: package systemd units

[Max Gautier]

Signed-off-by: Max Gautier <mg@max.gautier.name>
---
 Makefile                              |  4 ++++
 systemd/user/git-maintenance@.service | 16 ++++++++++++++++
 systemd/user/git-maintenance@.timer   |  9 +++++++++
 3 files changed, 29 insertions(+)
 create mode 100644 systemd/user/git-maintenance@.service
 create mode 100644 systemd/user/git-maintenance@.timer

diff --git a/Makefile b/Makefile
index 4e255c81f2..276b4373c6 100644
--- a/Makefile
+++ b/Makefile
@@ -619,6 +619,7 @@ htmldir = $(prefix)/share/doc/git-doc
 ETC_GITCONFIG = $(sysconfdir)/gitconfig
 ETC_GITATTRIBUTES = $(sysconfdir)/gitattributes
 lib = lib
+libdir = $(prefix)/lib
 # DESTDIR =
 pathsep = :
 
@@ -1328,6 +1329,8 @@ BUILTIN_OBJS += builtin/verify-tag.o
 BUILTIN_OBJS += builtin/worktree.o
 BUILTIN_OBJS += builtin/write-tree.o
 
+SYSTEMD_USER_UNITS := $(wildcard systemd/user/*)
+
 # THIRD_PARTY_SOURCES is a list of patterns compatible with the
 # $(filter) and $(filter-out) family of functions. They specify source
 # files which are taken from some third-party source where we want to be
@@ -3469,6 +3472,7 @@ install: all
 	$(INSTALL) -m 644 $(SCRIPT_LIB) '$(DESTDIR_SQ)$(gitexec_instdir_SQ)'
 	$(INSTALL) $(INSTALL_STRIP) $(install_bindir_xprograms) '$(DESTDIR_SQ)$(bindir_SQ)'
 	$(INSTALL) $(BINDIR_PROGRAMS_NO_X) '$(DESTDIR_SQ)$(bindir_SQ)'
+	$(INSTALL) -Dm 644 -t '$(DESTDIR_SQ)$(libdir)/systemd/user' $(SYSTEMD_USER_UNITS)
 
 ifdef MSVC
 	# We DO NOT install the individual foo.o.pdb files because they
diff --git a/systemd/user/git-maintenance@.service b/systemd/user/git-maintenance@.service
new file mode 100644
index 0000000000..87ac0c86e6
--- /dev/null
+++ b/systemd/user/git-maintenance@.service
@@ -0,0 +1,16 @@
+[Unit]
+Description=Optimize Git repositories data
+
+[Service]
+Type=oneshot
+ExecStart=git for-each-repo --config=maintenance.repo \
+          maintenance run --schedule=%i
+LockPersonality=yes
+MemoryDenyWriteExecute=yes
+NoNewPrivileges=yes
+RestrictAddressFamilies=AF_UNIX AF_INET AF_INET6 AF_VSOCK
+RestrictNamespaces=yes
+RestrictRealtime=yes
+RestrictSUIDSGID=yes
+SystemCallArchitectures=native
+SystemCallFilter=@system-service
diff --git a/systemd/user/git-maintenance@.timer b/systemd/user/git-maintenance@.timer
new file mode 100644
index 0000000000..40fbc77a62
--- /dev/null
+++ b/systemd/user/git-maintenance@.timer
@@ -0,0 +1,9 @@
+[Unit]
+Description=Optimize Git repositories data
+
+[Timer]
+OnCalendar=%i
+Persistent=true
+
+[Install]
+WantedBy=timers.target
-- 
2.44.0

[RFC PATCH 2/5] maintenance: add fixed random delay to systemd timers

[Max Gautier]

Ensures that:
- git maintenance timers have a fixed time interval between execution.
- the three timers are not executed at the same time.

This is intended to implement an alternative to the two followings
commits:
c97ec0378b (maintenance: fix systemd schedule overlaps, 2023-08-10)
daa787010c (maintenance: use random minute in systemd scheduler, 2023-08-10)

Instead of manually adding a specific minute (which is reset on each
invocation of `git maintenance start`), we use systemd timers
RandomizedDelaySec and FixedRandomDelay functionalities.

From man systemd.timer:
>FixedRandomDelay=
>  Takes a boolean argument. When enabled, the randomized offset
>  specified by RandomizedDelaySec= is reused for all firings of the
>  same timer. For a given timer unit, **the offset depends on the
>  machine ID, user identifier and timer name**, which means that it is
>  stable between restarts of the manager. This effectively creates a
>  fixed offset for an individual timer, reducing the jitter in
>  firings of this timer, while still avoiding firing at the same time
>  as other similarly configured timers.

-> which is exactly the use case for git-maintenance timers.

Signed-off-by: Max Gautier <mg@max.gautier.name>
---
 systemd/user/git-maintenance@.service | 1 +
 systemd/user/git-maintenance@.timer   | 3 +++
 2 files changed, 4 insertions(+)

diff --git a/systemd/user/git-maintenance@.service b/systemd/user/git-maintenance@.service
index 87ac0c86e6..f949e1a217 100644
--- a/systemd/user/git-maintenance@.service
+++ b/systemd/user/git-maintenance@.service
@@ -1,5 +1,6 @@
 [Unit]
 Description=Optimize Git repositories data
+Documentation=man:git-maintenance(1)
 
 [Service]
 Type=oneshot
diff --git a/systemd/user/git-maintenance@.timer b/systemd/user/git-maintenance@.timer
index 40fbc77a62..667c5998ba 100644
--- a/systemd/user/git-maintenance@.timer
+++ b/systemd/user/git-maintenance@.timer
@@ -1,9 +1,12 @@
 [Unit]
 Description=Optimize Git repositories data
+Documentation=man:git-maintenance(1)
 
 [Timer]
 OnCalendar=%i
 Persistent=true
+RandomizedDelaySec=1800
+FixedRandomDelay=true
 
 [Install]
 WantedBy=timers.target
-- 
2.44.0

[RFC PATCH 3/5] maintenance: use packaged systemd units

[Max Gautier]

- Remove the code writing the units
- Simplify calling systemctl
  - systemctl does not error out when disabling units which aren't
    enabled, nor when enabling already enabled units
  - call systemctl only once, with all the units.
- Add clean-up code for leftover units in $XDG_CONFIG_HOME created by
  previous versions of git, taking care of preserving actual user
  override (by checking the start of the file).

Signed-off-by: Max Gautier <mg@max.gautier.name>
---
 builtin/gc.c | 293 ++++++++-------------------------------------------
 1 file changed, 45 insertions(+), 248 deletions(-)

diff --git a/builtin/gc.c b/builtin/gc.c
index cb80ced6cb..981db8e297 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -2308,276 +2308,73 @@ static char *xdg_config_home_systemd(const char *filename)
 	return xdg_config_home_for("systemd/user", filename);
 }
 
-#define SYSTEMD_UNIT_FORMAT "git-maintenance@%s.%s"
-
-static int systemd_timer_delete_timer_file(enum schedule_priority priority)
-{
-	int ret = 0;
-	const char *frequency = get_frequency(priority);
-	char *local_timer_name = xstrfmt(SYSTEMD_UNIT_FORMAT, frequency, "timer");
-	char *filename = xdg_config_home_systemd(local_timer_name);
-
-	if (unlink(filename) && !is_missing_file_error(errno))
-		ret = error_errno(_("failed to delete '%s'"), filename);
-
-	free(filename);
-	free(local_timer_name);
-	return ret;
-}
-
-static int systemd_timer_delete_service_template(void)
-{
-	int ret = 0;
-	char *local_service_name = xstrfmt(SYSTEMD_UNIT_FORMAT, "", "service");
-	char *filename = xdg_config_home_systemd(local_service_name);
-	if (unlink(filename) && !is_missing_file_error(errno))
-		ret = error_errno(_("failed to delete '%s'"), filename);
-
-	free(filename);
-	free(local_service_name);
-	return ret;
-}
-
-/*
- * Write the schedule information into a git-maintenance@<schedule>.timer
- * file using a custom minute. This timer file cannot use the templating
- * system, so we generate a specific file for each.
- */
-static int systemd_timer_write_timer_file(enum schedule_priority schedule,
-					  int minute)
-{
-	int res = -1;
-	char *filename;
-	FILE *file;
-	const char *unit;
-	char *schedule_pattern = NULL;
-	const char *frequency = get_frequency(schedule);
-	char *local_timer_name = xstrfmt(SYSTEMD_UNIT_FORMAT, frequency, "timer");
-
-	filename = xdg_config_home_systemd(local_timer_name);
-
-	if (safe_create_leading_directories(filename)) {
-		error(_("failed to create directories for '%s'"), filename);
-		goto error;
-	}
-	file = fopen_or_warn(filename, "w");
-	if (!file)
-		goto error;
-
-	switch (schedule) {
-	case SCHEDULE_HOURLY:
-		schedule_pattern = xstrfmt("*-*-* 1..23:%02d:00", minute);
-		break;
-
-	case SCHEDULE_DAILY:
-		schedule_pattern = xstrfmt("Tue..Sun *-*-* 0:%02d:00", minute);
-		break;
-
-	case SCHEDULE_WEEKLY:
-		schedule_pattern = xstrfmt("Mon 0:%02d:00", minute);
-		break;
-
-	default:
-		BUG("Unhandled schedule_priority");
-	}
-
-	unit = "# This file was created and is maintained by Git.\n"
-	       "# Any edits made in this file might be replaced in the future\n"
-	       "# by a Git command.\n"
-	       "\n"
-	       "[Unit]\n"
-	       "Description=Optimize Git repositories data\n"
-	       "\n"
-	       "[Timer]\n"
-	       "OnCalendar=%s\n"
-	       "Persistent=true\n"
-	       "\n"
-	       "[Install]\n"
-	       "WantedBy=timers.target\n";
-	if (fprintf(file, unit, schedule_pattern) < 0) {
-		error(_("failed to write to '%s'"), filename);
-		fclose(file);
-		goto error;
-	}
-	if (fclose(file) == EOF) {
-		error_errno(_("failed to flush '%s'"), filename);
-		goto error;
-	}
-
-	res = 0;
-
-error:
-	free(schedule_pattern);
-	free(local_timer_name);
-	free(filename);
-	return res;
-}
-
-/*
- * No matter the schedule, we use the same service and can make use of the
- * templating system. When installing git-maintenance@<schedule>.timer,
- * systemd will notice that git-maintenance@.service exists as a template
- * and will use this file and insert the <schedule> into the template at
- * the position of "%i".
- */
-static int systemd_timer_write_service_template(const char *exec_path)
-{
-	int res = -1;
-	char *filename;
-	FILE *file;
-	const char *unit;
-	char *local_service_name = xstrfmt(SYSTEMD_UNIT_FORMAT, "", "service");
-
-	filename = xdg_config_home_systemd(local_service_name);
-	if (safe_create_leading_directories(filename)) {
-		error(_("failed to create directories for '%s'"), filename);
-		goto error;
-	}
-	file = fopen_or_warn(filename, "w");
-	if (!file)
-		goto error;
-
-	unit = "# This file was created and is maintained by Git.\n"
-	       "# Any edits made in this file might be replaced in the future\n"
-	       "# by a Git command.\n"
-	       "\n"
-	       "[Unit]\n"
-	       "Description=Optimize Git repositories data\n"
-	       "\n"
-	       "[Service]\n"
-	       "Type=oneshot\n"
-	       "ExecStart=\"%s/git\" --exec-path=\"%s\" for-each-repo --config=maintenance.repo maintenance run --schedule=%%i\n"
-	       "LockPersonality=yes\n"
-	       "MemoryDenyWriteExecute=yes\n"
-	       "NoNewPrivileges=yes\n"
-	       "RestrictAddressFamilies=AF_UNIX AF_INET AF_INET6 AF_VSOCK\n"
-	       "RestrictNamespaces=yes\n"
-	       "RestrictRealtime=yes\n"
-	       "RestrictSUIDSGID=yes\n"
-	       "SystemCallArchitectures=native\n"
-	       "SystemCallFilter=@system-service\n";
-	if (fprintf(file, unit, exec_path, exec_path) < 0) {
-		error(_("failed to write to '%s'"), filename);
-		fclose(file);
-		goto error;
-	}
-	if (fclose(file) == EOF) {
-		error_errno(_("failed to flush '%s'"), filename);
-		goto error;
-	}
-
-	res = 0;
-
-error:
-	free(local_service_name);
-	free(filename);
-	return res;
-}
-
-static int systemd_timer_enable_unit(int enable,
-				     enum schedule_priority schedule,
-				     int minute)
+static int systemd_set_units_state(int enable)
 {
 	const char *cmd = "systemctl";
 	struct child_process child = CHILD_PROCESS_INIT;
-	const char *frequency = get_frequency(schedule);
-
-	/*
-	 * Disabling the systemd unit while it is already disabled makes
-	 * systemctl print an error.
-	 * Let's ignore it since it means we already are in the expected state:
-	 * the unit is disabled.
-	 *
-	 * On the other hand, enabling a systemd unit which is already enabled
-	 * produces no error.
-	 */
-	if (!enable)
-		child.no_stderr = 1;
-	else if (systemd_timer_write_timer_file(schedule, minute))
-		return -1;
 
 	get_schedule_cmd(&cmd, NULL);
 	strvec_split(&child.args, cmd);
-	strvec_pushl(&child.args, "--user", enable ? "enable" : "disable",
-		     "--now", NULL);
-	strvec_pushf(&child.args, SYSTEMD_UNIT_FORMAT, frequency, "timer");
+
+	strvec_pushl(&child.args, "--user", "--force", "--now",
+			enable ? "enable" : "disable",
+			"git-maintenance@hourly.timer",
+			"git-maintenance@daily.timer",
+			"git-maintenance@weekly.timer", NULL);
+	/*
+	** --force override existing conflicting symlinks
+	** We need it because the units have changed location (~/.config ->
+	** /usr/lib)
+	*/
 
 	if (start_command(&child))
 		return error(_("failed to start systemctl"));
 	if (finish_command(&child))
-		/*
-		 * Disabling an already disabled systemd unit makes
-		 * systemctl fail.
-		 * Let's ignore this failure.
-		 *
-		 * Enabling an enabled systemd unit doesn't fail.
-		 */
-		if (enable)
-			return error(_("failed to run systemctl"));
+		return error(_("failed to run systemctl"));
 	return 0;
 }
 
-/*
- * A previous version of Git wrote the timer units as template files.
- * Clean these up, if they exist.
- */
-static void systemd_timer_delete_stale_timer_templates(void)
+static void systemd_delete_user_unit(char const *unit)
 {
-	char *timer_template_name = xstrfmt(SYSTEMD_UNIT_FORMAT, "", "timer");
-	char *filename = xdg_config_home_systemd(timer_template_name);
+	char const	file_start_stale[] =	"# This file was created and is"
+						" maintained by Git.";
+	char		file_start_user[sizeof(file_start_stale)] = {'\0'};
+
+	char *filename = xdg_config_home_systemd(unit);
+	int handle = open(filename, O_RDONLY);
 
-	if (unlink(filename) && !is_missing_file_error(errno))
+	/*
+	** Check this is actually our file and we're not removing a legitimate
+	** user override.
+	*/
+	if (handle == -1 && !is_missing_file_error(errno))
 		warning(_("failed to delete '%s'"), filename);
+	else {
+		read(handle, file_start_user, sizeof(file_start_stale) - 1);
+		close(handle);
+		if (strcmp(file_start_stale, file_start_user) == 0) {
+			if (unlink(filename) == 0)
+				warning(_("deleted stale unit file '%s'"), filename);
+			else if (!is_missing_file_error(errno))
+				warning(_("failed to delete '%s'"), filename);
+		}
+	}
 
 	free(filename);
-	free(timer_template_name);
-}
-
-static int systemd_timer_delete_unit_files(void)
-{
-	systemd_timer_delete_stale_timer_templates();
-
-	/* Purposefully not short-circuited to make sure all are called. */
-	return systemd_timer_delete_timer_file(SCHEDULE_HOURLY) |
-	       systemd_timer_delete_timer_file(SCHEDULE_DAILY) |
-	       systemd_timer_delete_timer_file(SCHEDULE_WEEKLY) |
-	       systemd_timer_delete_service_template();
-}
-
-static int systemd_timer_delete_units(void)
-{
-	int minute = get_random_minute();
-	/* Purposefully not short-circuited to make sure all are called. */
-	return systemd_timer_enable_unit(0, SCHEDULE_HOURLY, minute) |
-	       systemd_timer_enable_unit(0, SCHEDULE_DAILY, minute) |
-	       systemd_timer_enable_unit(0, SCHEDULE_WEEKLY, minute) |
-	       systemd_timer_delete_unit_files();
-}
-
-static int systemd_timer_setup_units(void)
-{
-	int minute = get_random_minute();
-	const char *exec_path = git_exec_path();
-
-	int ret = systemd_timer_write_service_template(exec_path) ||
-		  systemd_timer_enable_unit(1, SCHEDULE_HOURLY, minute) ||
-		  systemd_timer_enable_unit(1, SCHEDULE_DAILY, minute) ||
-		  systemd_timer_enable_unit(1, SCHEDULE_WEEKLY, minute);
-
-	if (ret)
-		systemd_timer_delete_units();
-	else
-		systemd_timer_delete_stale_timer_templates();
-
-	return ret;
 }
 
 static int systemd_timer_update_schedule(int run_maintenance, int fd UNUSED)
 {
-	if (run_maintenance)
-		return systemd_timer_setup_units();
-	else
-		return systemd_timer_delete_units();
+	/*
+	 * A previous version of Git wrote the units in the user configuration
+	 * directory. Clean these up, if they exist.
+	 */
+	systemd_delete_user_unit("git-maintenance@hourly.timer");
+	systemd_delete_user_unit("git-maintenance@daily.timer");
+	systemd_delete_user_unit("git-maintenance@weekly.timer");
+	systemd_delete_user_unit("git-maintenance@.timer");
+	systemd_delete_user_unit("git-maintenance@.service");
+	return systemd_set_units_state(run_maintenance);
 }
 
 enum scheduler {
-- 
2.44.0

[RFC PATCH 4/5] maintenance: update systemd scheduler docs

[Max Gautier]

Signed-off-by: Max Gautier <mg@max.gautier.name>
---
 Documentation/git-maintenance.txt | 33 +++++++++++++++----------------
 1 file changed, 16 insertions(+), 17 deletions(-)

diff --git a/Documentation/git-maintenance.txt b/Documentation/git-maintenance.txt
index 51d0f7e94b..6511c3f3f1 100644
--- a/Documentation/git-maintenance.txt
+++ b/Documentation/git-maintenance.txt
@@ -304,10 +304,9 @@ distributions, systemd timers are superseding it.
 If user systemd timers are available, they will be used as a replacement
 of `cron`.
 
-In this case, `git maintenance start` will create user systemd timer units
-and start the timers. The current list of user-scheduled tasks can be found
-by running `systemctl --user list-timers`. The timers written by `git
-maintenance start` are similar to this:
+In this case, `git maintenance start` will enable user systemd timer units
+and start them. The current list of user-scheduled tasks can be found by
+running `systemctl --user list-timers`. These timers are similar to this:
 
 -----------------------------------------------------------------------
 $ systemctl --user list-timers
@@ -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
 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
 -----------------------------------------------------------------------
 
-One timer is registered for each `--schedule=<frequency>` option.
+One timer instance is enabled for each `--schedule=<frequency>` option.
 
-The definition of the systemd units can be inspected in the following files:
+The definition of the systemd units can be inspected this way:
 
 -----------------------------------------------------------------------
-~/.config/systemd/user/git-maintenance@.timer
-~/.config/systemd/user/git-maintenance@.service
-~/.config/systemd/user/timers.target.wants/git-maintenance@hourly.timer
-~/.config/systemd/user/timers.target.wants/git-maintenance@daily.timer
-~/.config/systemd/user/timers.target.wants/git-maintenance@weekly.timer
+$ systemctl cat --user git-maintenance@.timer
+$ systemctl cat --user git-maintenance@.service
 -----------------------------------------------------------------------
 
-`git maintenance start` will overwrite these files and start the timer
-again with `systemctl --user`, so any customization should be done by
-creating a drop-in file, i.e. a `.conf` suffixed file in the
-`~/.config/systemd/user/git-maintenance@.service.d` directory.
+Customization of the timer or service can be performed with the usual systemd
+tooling:
+-----------------------------------------------------------------------
+$ systemctl edit --user git-maintenance@.timer # all the timers
+$ systemctl edit --user git-maintenance@hourly.timer # the hourly timer
+$ systemctl edit --user git-maintenance@.service # all the services
+$ systemctl edit --user git-maintenance@hourly.service # the hourly run
+-----------------------------------------------------------------------
 
-`git maintenance stop` will stop the user systemd timers and delete
-the above mentioned files.
+`git maintenance stop` will disable and stop the user systemd timers.
 
 For more details, see `systemd.timer(5)`.
 
-- 
2.44.0

[RFC PATCH 5/5] DON'T APPLY YET: maintenance: remove cleanup code

[Max Gautier]

This removes code to remove old git-maintenance systemd timer and
service user units which were written in $XDG_CONFIG_HOME by git
previous versions.

Signed-off-by: Max Gautier <mg@max.gautier.name>
---
 builtin/gc.c | 54 +++-------------------------------------------------
 1 file changed, 3 insertions(+), 51 deletions(-)

diff --git a/builtin/gc.c b/builtin/gc.c
index 981db8e297..6ac184eaf5 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -2303,12 +2303,7 @@ static int is_systemd_timer_available(void)
 	return real_is_systemd_timer_available();
 }
 
-static char *xdg_config_home_systemd(const char *filename)
-{
-	return xdg_config_home_for("systemd/user", filename);
-}
-
-static int systemd_set_units_state(int enable)
+static int systemd_set_units_state(int run_maintenance, int fd UNUSED)
 {
 	const char *cmd = "systemctl";
 	struct child_process child = CHILD_PROCESS_INIT;
@@ -2317,7 +2312,7 @@ static int systemd_set_units_state(int enable)
 	strvec_split(&child.args, cmd);
 
 	strvec_pushl(&child.args, "--user", "--force", "--now",
-			enable ? "enable" : "disable",
+			run_maintenance ? "enable" : "disable",
 			"git-maintenance@hourly.timer",
 			"git-maintenance@daily.timer",
 			"git-maintenance@weekly.timer", NULL);
@@ -2334,49 +2329,6 @@ static int systemd_set_units_state(int enable)
 	return 0;
 }
 
-static void systemd_delete_user_unit(char const *unit)
-{
-	char const	file_start_stale[] =	"# This file was created and is"
-						" maintained by Git.";
-	char		file_start_user[sizeof(file_start_stale)] = {'\0'};
-
-	char *filename = xdg_config_home_systemd(unit);
-	int handle = open(filename, O_RDONLY);
-
-	/*
-	** Check this is actually our file and we're not removing a legitimate
-	** user override.
-	*/
-	if (handle == -1 && !is_missing_file_error(errno))
-		warning(_("failed to delete '%s'"), filename);
-	else {
-		read(handle, file_start_user, sizeof(file_start_stale) - 1);
-		close(handle);
-		if (strcmp(file_start_stale, file_start_user) == 0) {
-			if (unlink(filename) == 0)
-				warning(_("deleted stale unit file '%s'"), filename);
-			else if (!is_missing_file_error(errno))
-				warning(_("failed to delete '%s'"), filename);
-		}
-	}
-
-	free(filename);
-}
-
-static int systemd_timer_update_schedule(int run_maintenance, int fd UNUSED)
-{
-	/*
-	 * A previous version of Git wrote the units in the user configuration
-	 * directory. Clean these up, if they exist.
-	 */
-	systemd_delete_user_unit("git-maintenance@hourly.timer");
-	systemd_delete_user_unit("git-maintenance@daily.timer");
-	systemd_delete_user_unit("git-maintenance@weekly.timer");
-	systemd_delete_user_unit("git-maintenance@.timer");
-	systemd_delete_user_unit("git-maintenance@.service");
-	return systemd_set_units_state(run_maintenance);
-}
-
 enum scheduler {
 	SCHEDULER_INVALID = -1,
 	SCHEDULER_AUTO,
@@ -2399,7 +2351,7 @@ static const struct {
 	[SCHEDULER_SYSTEMD] = {
 		.name = "systemctl",
 		.is_available = is_systemd_timer_available,
-		.update_schedule = systemd_timer_update_schedule,
+		.update_schedule = systemd_set_units_state,
 	},
 	[SCHEDULER_LAUNCHCTL] = {
 		.name = "launchctl",
-- 
2.44.0

Re: [RFC PATCH 3/5] maintenance: use packaged systemd units

[Max Gautier]

I'm working on updating the test in t7900-maintenance.sh, but I might be
missing something here:

>test_expect_success 'start and stop Linux/systemd maintenance' '
>   write_script print-args <<-\EOF &&
>   printf "%s\n" "$*" >>args
>   EOF
>   
>   XDG_CONFIG_HOME="$PWD" &&
>   export XDG_CONFIG_HOME &&
>   rm -f args &&
>   GIT_TEST_MAINT_SCHEDULER="systemctl:./print-args" git maintenance start --scheduler=systemd-timer &&

Do I understand correctly that this means we're not actually running
systemctl here, just printing the arguments to our file ?

>	# start registers the repo
>	git config --get --global --fixed-value maintenance.repo "$(pwd)" &&
>
>	for schedule in hourly daily weekly
>	do
>		test_path_is_file "systemd/user/git-maintenance@$schedule.timer" || return 1
>	done &&
>	test_path_is_file "systemd/user/git-maintenance@.service" &&
>
>	test_systemd_analyze_verify "systemd/user/git-maintenance@hourly.service" &&
>	test_systemd_analyze_verify "systemd/user/git-maintenance@daily.service" &&
>	test_systemd_analyze_verify "systemd/user/git-maintenance@weekly.service" &&
>
>	printf -- "--user enable --now git-maintenance@%s.timer\n" hourly daily weekly >expect &&
>	test_cmp expect args &&
>
>	rm -f args &&
>	GIT_TEST_MAINT_SCHEDULER="systemctl:./print-args" git maintenance stop &&
>
>	# stop does not unregister the repo
>	git config --get --global --fixed-value maintenance.repo "$(pwd)" &&
>
>	for schedule in hourly daily weekly
>	do
>		test_path_is_missing "systemd/user/git-maintenance@$schedule.timer" || return 1
>	done &&
>	test_path_is_missing "systemd/user/git-maintenance@.service" &&
>
>	printf -- "--user disable --now git-maintenance@%s.timer\n" hourly daily weekly >expect &&
>	test_cmp expect args

The rest of the systemd tests only check that the service file are in
XDG_CONFIG_HOME, which should not be the case anymore.

However, the test does not actually check we have enabled and started
the timers as it is , right ?

Should I add that ? I'm not sure how, because it does not seem like the
tests run in a isolated env, so it would mess with the systemd user
manager of the developper running the tests...

Regarding systemd-analyze verify, do the tests have access to the source
directory in a special way, or is using '../..' enough ?

Thanks

-- 
Max Gautier

Re: [RFC PATCH 3/5] maintenance: use packaged systemd units

[Eric Sunshine]

On Tue, Mar 19, 2024 at 8:10 AM Max Gautier <mg@max.gautier.name> wrote:
> I'm working on updating the test in t7900-maintenance.sh, but I might be
> missing something here:
>
> >test_expect_success 'start and stop Linux/systemd maintenance' '
> >   write_script print-args <<-\EOF &&
> >   printf "%s\n" "$*" >>args
> >   EOF
> >
> >   XDG_CONFIG_HOME="$PWD" &&
> >   export XDG_CONFIG_HOME &&
> >   rm -f args &&
> >   GIT_TEST_MAINT_SCHEDULER="systemctl:./print-args" git maintenance start --scheduler=systemd-timer &&
>
> Do I understand correctly that this means we're not actually running
> systemctl here, just printing the arguments to our file ?

That's correct. The purpose of GIT_TEST_MAINT_SCHEDULER is twofold.

The primary purpose is to test as much as possible without actually
mucking with the user's real scheduler-related configuration (whether
it be systemd, cron, launchctl, etc.). This means that we want to
verify that the expected files are created or removed by
git-maintenance, that they are well-formed, and that git-maintenance
is invoking the correct platform-specific scheduler-related command
with correct arguments (without actually invoking that command and
messing up the user's personal configuration).

The secondary purpose is to allow these otherwise platform-specific
tests to run on any platform. This is possible since, as noted above,
we're not actually running the platform-specific scheduler-related
command, but instead only capturing the command and arguments that
would have been applied had git-maintenace been run "for real" outside
of the test framework.

> >       for schedule in hourly daily weekly
> >       do
> >               test_path_is_missing "systemd/user/git-maintenance@$schedule.timer" || return 1
> >       done &&
> >       test_path_is_missing "systemd/user/git-maintenance@.service" &&
> >
> >       printf -- "--user disable --now git-maintenance@%s.timer\n" hourly daily weekly >expect &&
> >       test_cmp expect args
>
> The rest of the systemd tests only check that the service file are in
> XDG_CONFIG_HOME, which should not be the case anymore.
>
> However, the test does not actually check we have enabled and started
> the timers as it is , right ?

Correct. As noted above, we don't want to muck with the user's real
configuration, and we certainly don't want the system-specific
scheduler to actually kick off some command we're testing in the
user's account while the test script is running.

> Should I add that ? I'm not sure how, because it does not seem like the
> tests run in a isolated env, so it would mess with the systemd user
> manager of the developper running the tests...

No you don't want to add that since, as you state and as stated above,
it would muck with the user's own configuration which would be
undesirable.

> Regarding systemd-analyze verify, do the tests have access to the source
> directory in a special way, or is using '../..' enough ?

You can't assume that the source directory is available at "../.."
since the --root option (see t/README) allows the root of the tests
working-tree to reside outside of the project directory.

You may be able to use "$TEST_DIRECTORY/.." to reference files in the
source tree, though the documentation in t/test-lib.sh doesn't seem to
state explicitly that this is intended or supported use. However, a
few existing tests (t1021, t3000, t4023) already access files in the
source tree in this fashion, so there is precedent.

Re: [RFC PATCH 3/5] maintenance: use packaged systemd units

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> You may be able to use "$TEST_DIRECTORY/.." to reference files in the
> source tree, though the documentation in t/test-lib.sh doesn't seem to
> state explicitly that this is intended or supported use. However, a
> few existing tests (t1021, t3000, t4023) already access files in the
> source tree in this fashion, so there is precedent.

I think the following from t/test-lib.sh should give us sufficient
assurance.

	# The TEST_DIRECTORY will always be the path to the "t"
	# directory in the git.git checkout. This is overridden by
	# e.g. t/lib-subtest.sh, but only because its $(pwd) is
	# different. Those tests still set "$TEST_DIRECTORY" to the
	# same path.

Everything you said in your review is correct.  Thanks.

Re: [RFC PATCH 3/5] maintenance: use packaged systemd units

[Max Gautier]

Le 19 mars 2024 18:17:27 GMT+01:00, Eric Sunshine <sunshine@sunshineco.com> a écrit :
>On Tue, Mar 19, 2024 at 8:10 AM Max Gautier <mg@max.gautier.name> wrote:
>> I'm working on updating the test in t7900-maintenance.sh, but I might be
>> missing something here:
>>
>> >test_expect_success 'start and stop Linux/systemd maintenance' '
>> >   write_script print-args <<-\EOF &&
>> >   printf "%s\n" "$*" >>args
>> >   EOF
>> >
>> >   XDG_CONFIG_HOME="$PWD" &&
>> >   export XDG_CONFIG_HOME &&
>> >   rm -f args &&
>> >   GIT_TEST_MAINT_SCHEDULER="systemctl:./print-args" git maintenance start --scheduler=systemd-timer &&
>>
>> Do I understand correctly that this means we're not actually running
>> systemctl here, just printing the arguments to our file ?
>
>That's correct. The purpose of GIT_TEST_MAINT_SCHEDULER is twofold.
>
>The primary purpose is to test as much as possible without actually
>mucking with the user's real scheduler-related configuration (whether
>it be systemd, cron, launchctl, etc.). This means that we want to
>verify that the expected files are created or removed by
>git-maintenance, that they are well-formed, and that git-maintenance
>is invoking the correct platform-specific scheduler-related command
>with correct arguments (without actually invoking that command and
>messing up the user's personal configuration).
>
>The secondary purpose is to allow these otherwise platform-specific
>tests to run on any platform. This is possible since, as noted above,
>we're not actually running the platform-specific scheduler-related
>command, but instead only capturing the command and arguments that
>would have been applied had git-maintenace been run "for real" outside
>of the test framework.

Ok thanks, now I see why it's done this way.

>
>> >       for schedule in hourly daily weekly
>> >       do
>> >               test_path_is_missing "systemd/user/git-maintenance@$schedule.timer" || return 1
>> >       done &&
>> >       test_path_is_missing "systemd/user/git-maintenance@.service" &&
>> >
>> >       printf -- "--user disable --now git-maintenance@%s.timer\n" hourly daily weekly >expect &&
>> >       test_cmp expect args
>>
>> The rest of the systemd tests only check that the service file are in
>> XDG_CONFIG_HOME, which should not be the case anymore.
>>
>> However, the test does not actually check we have enabled and started
>> the timers as it is , right ?
>
>Correct. As noted above, we don't want to muck with the user's real
>configuration, and we certainly don't want the system-specific
>scheduler to actually kick off some command we're testing in the
>user's account while the test script is running.
>
>> Should I add that ? I'm not sure how, because it does not seem like the
>> tests run in a isolated env, so it would mess with the systemd user
>> manager of the developper running the tests...
>
>No you don't want to add that since, as you state and as stated above,
>it would muck with the user's own configuration which would be
>undesirable.
>

That makes things easier then :)

>> Regarding systemd-analyze verify, do the tests have access to the source
>> directory in a special way, or is using '../..' enough ?
>
>You can't assume that the source directory is available at "../.."
>since the --root option (see t/README) allows the root of the tests
>working-tree to reside outside of the project directory.
>
>You may be able to use "$TEST_DIRECTORY/.." to reference files in the
>source tree, though the documentation in t/test-lib.sh doesn't seem to
>state explicitly that this is intended or supported use. However, a
>few existing tests (t1021, t3000, t4023) already access files in the
>source tree in this fashion, so there is precedent.


I'll look into those. Thanks for all the info !

-- 
Max Gautier

Re: [RFC PATCH 1/5] maintenance: package systemd units

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 3459 bytes --]

On Mon, Mar 18, 2024 at 04:31:15PM +0100, Max Gautier wrote:

It would be great to document _why_ we want to package the systemd units
alongside with Git.

> Signed-off-by: Max Gautier <mg@max.gautier.name>
> ---
>  Makefile                              |  4 ++++
>  systemd/user/git-maintenance@.service | 16 ++++++++++++++++
>  systemd/user/git-maintenance@.timer   |  9 +++++++++
>  3 files changed, 29 insertions(+)
>  create mode 100644 systemd/user/git-maintenance@.service
>  create mode 100644 systemd/user/git-maintenance@.timer
> 
> diff --git a/Makefile b/Makefile
> index 4e255c81f2..276b4373c6 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -619,6 +619,7 @@ htmldir = $(prefix)/share/doc/git-doc
>  ETC_GITCONFIG = $(sysconfdir)/gitconfig
>  ETC_GITATTRIBUTES = $(sysconfdir)/gitattributes
>  lib = lib
> +libdir = $(prefix)/lib
>  # DESTDIR =
>  pathsep = :
>  
> @@ -1328,6 +1329,8 @@ BUILTIN_OBJS += builtin/verify-tag.o
>  BUILTIN_OBJS += builtin/worktree.o
>  BUILTIN_OBJS += builtin/write-tree.o
>  
> +SYSTEMD_USER_UNITS := $(wildcard systemd/user/*)
> +
>  # THIRD_PARTY_SOURCES is a list of patterns compatible with the
>  # $(filter) and $(filter-out) family of functions. They specify source
>  # files which are taken from some third-party source where we want to be
> @@ -3469,6 +3472,7 @@ install: all
>  	$(INSTALL) -m 644 $(SCRIPT_LIB) '$(DESTDIR_SQ)$(gitexec_instdir_SQ)'
>  	$(INSTALL) $(INSTALL_STRIP) $(install_bindir_xprograms) '$(DESTDIR_SQ)$(bindir_SQ)'
>  	$(INSTALL) $(BINDIR_PROGRAMS_NO_X) '$(DESTDIR_SQ)$(bindir_SQ)'
> +	$(INSTALL) -Dm 644 -t '$(DESTDIR_SQ)$(libdir)/systemd/user' $(SYSTEMD_USER_UNITS)

I wonder whether we want to unconditionally install those units. Many of
the platforms that we support don't even have systemd available, so
certainly it wouldn't make any sense to install it on those platforms.

Assuming that this is something we want in the first place I thus think
that we should at least make this conditional and add some platform
specific quirk to "config.mak.uname".

>  ifdef MSVC
>  	# We DO NOT install the individual foo.o.pdb files because they
> diff --git a/systemd/user/git-maintenance@.service b/systemd/user/git-maintenance@.service
> new file mode 100644
> index 0000000000..87ac0c86e6
> --- /dev/null
> +++ b/systemd/user/git-maintenance@.service
> @@ -0,0 +1,16 @@
> +[Unit]
> +Description=Optimize Git repositories data
> +
> +[Service]
> +Type=oneshot
> +ExecStart=git for-each-repo --config=maintenance.repo \
> +          maintenance run --schedule=%i
> +LockPersonality=yes
> +MemoryDenyWriteExecute=yes
> +NoNewPrivileges=yes
> +RestrictAddressFamilies=AF_UNIX AF_INET AF_INET6 AF_VSOCK
> +RestrictNamespaces=yes
> +RestrictRealtime=yes
> +RestrictSUIDSGID=yes
> +SystemCallArchitectures=native
> +SystemCallFilter=@system-service

Curious, but how did you arrive at these particular restrictions for the
unit? Might be something to explain in the commit message, as well.

Patrick

> diff --git a/systemd/user/git-maintenance@.timer b/systemd/user/git-maintenance@.timer
> new file mode 100644
> index 0000000000..40fbc77a62
> --- /dev/null
> +++ b/systemd/user/git-maintenance@.timer
> @@ -0,0 +1,9 @@
> +[Unit]
> +Description=Optimize Git repositories data
> +
> +[Timer]
> +OnCalendar=%i
> +Persistent=true
> +
> +[Install]
> +WantedBy=timers.target
> -- 
> 2.44.0
> 
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [RFC PATCH 2/5] maintenance: add fixed random delay to systemd timers

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 3002 bytes --]

On Mon, Mar 18, 2024 at 04:31:16PM +0100, Max Gautier wrote:
> Ensures that:
> - git maintenance timers have a fixed time interval between execution.
> - the three timers are not executed at the same time.

Commit messages are typically structured so that you first explain what
the problem is that you're trying to solve, and then you explain how you
solve it. Your commit message is missing the first part.

> This is intended to implement an alternative to the two followings
> commits:
> c97ec0378b (maintenance: fix systemd schedule overlaps, 2023-08-10)
> daa787010c (maintenance: use random minute in systemd scheduler, 2023-08-10)
> 
> Instead of manually adding a specific minute (which is reset on each
> invocation of `git maintenance start`), we use systemd timers
> RandomizedDelaySec and FixedRandomDelay functionalities.

I think it would help to not list commits, but put the commit references
in a paragraph. Something in the spirit of "In commit c97ec0378b we
already tried to address this issue in such and such a way, but that is
quite limiting due to that and that. Similarly, in commit daa787010c..".

> From man systemd.timer:
> >FixedRandomDelay=
> >  Takes a boolean argument. When enabled, the randomized offset
> >  specified by RandomizedDelaySec= is reused for all firings of the
> >  same timer. For a given timer unit, **the offset depends on the
> >  machine ID, user identifier and timer name**, which means that it is
> >  stable between restarts of the manager. This effectively creates a
> >  fixed offset for an individual timer, reducing the jitter in
> >  firings of this timer, while still avoiding firing at the same time
> >  as other similarly configured timers.
> 
> -> which is exactly the use case for git-maintenance timers.
> 
> Signed-off-by: Max Gautier <mg@max.gautier.name>
> ---
>  systemd/user/git-maintenance@.service | 1 +
>  systemd/user/git-maintenance@.timer   | 3 +++
>  2 files changed, 4 insertions(+)
> 
> diff --git a/systemd/user/git-maintenance@.service b/systemd/user/git-maintenance@.service
> index 87ac0c86e6..f949e1a217 100644
> --- a/systemd/user/git-maintenance@.service
> +++ b/systemd/user/git-maintenance@.service
> @@ -1,5 +1,6 @@
>  [Unit]
>  Description=Optimize Git repositories data
> +Documentation=man:git-maintenance(1)

This change feels unrelated and should likely go into the first commit.

>  
>  [Service]
>  Type=oneshot
> diff --git a/systemd/user/git-maintenance@.timer b/systemd/user/git-maintenance@.timer
> index 40fbc77a62..667c5998ba 100644
> --- a/systemd/user/git-maintenance@.timer
> +++ b/systemd/user/git-maintenance@.timer
> @@ -1,9 +1,12 @@
>  [Unit]
>  Description=Optimize Git repositories data
> +Documentation=man:git-maintenance(1)

Same.

Patrick

>  [Timer]
>  OnCalendar=%i
>  Persistent=true
> +RandomizedDelaySec=1800
> +FixedRandomDelay=true
>  
>  [Install]
>  WantedBy=timers.target
> -- 
> 2.44.0
> 
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [RFC PATCH 3/5] maintenance: use packaged systemd units

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 12256 bytes --]

On Mon, Mar 18, 2024 at 04:31:17PM +0100, Max Gautier wrote:
> - Remove the code writing the units
> - Simplify calling systemctl
>   - systemctl does not error out when disabling units which aren't
>     enabled, nor when enabling already enabled units
>   - call systemctl only once, with all the units.
> - Add clean-up code for leftover units in $XDG_CONFIG_HOME created by
>   previous versions of git, taking care of preserving actual user
>   override (by checking the start of the file).

We very much try to avoid bulleted-list-style commit messages. Once you
have a list of changes it very often indicates that there are multiple
semi-related things going on in the same commit. At that point we prefer
to split the commit up into multiple commits so that each of the bullet
items can be explained separately.

Also, as mentioned before, commit messages should explain what the
problem is and how that problem is solved. 

Patrick

> Signed-off-by: Max Gautier <mg@max.gautier.name>
> ---
>  builtin/gc.c | 293 ++++++++-------------------------------------------
>  1 file changed, 45 insertions(+), 248 deletions(-)
> 
> diff --git a/builtin/gc.c b/builtin/gc.c
> index cb80ced6cb..981db8e297 100644
> --- a/builtin/gc.c
> +++ b/builtin/gc.c
> @@ -2308,276 +2308,73 @@ static char *xdg_config_home_systemd(const char *filename)
>  	return xdg_config_home_for("systemd/user", filename);
>  }
>  
> -#define SYSTEMD_UNIT_FORMAT "git-maintenance@%s.%s"
> -
> -static int systemd_timer_delete_timer_file(enum schedule_priority priority)
> -{
> -	int ret = 0;
> -	const char *frequency = get_frequency(priority);
> -	char *local_timer_name = xstrfmt(SYSTEMD_UNIT_FORMAT, frequency, "timer");
> -	char *filename = xdg_config_home_systemd(local_timer_name);
> -
> -	if (unlink(filename) && !is_missing_file_error(errno))
> -		ret = error_errno(_("failed to delete '%s'"), filename);
> -
> -	free(filename);
> -	free(local_timer_name);
> -	return ret;
> -}
> -
> -static int systemd_timer_delete_service_template(void)
> -{
> -	int ret = 0;
> -	char *local_service_name = xstrfmt(SYSTEMD_UNIT_FORMAT, "", "service");
> -	char *filename = xdg_config_home_systemd(local_service_name);
> -	if (unlink(filename) && !is_missing_file_error(errno))
> -		ret = error_errno(_("failed to delete '%s'"), filename);
> -
> -	free(filename);
> -	free(local_service_name);
> -	return ret;
> -}
> -
> -/*
> - * Write the schedule information into a git-maintenance@<schedule>.timer
> - * file using a custom minute. This timer file cannot use the templating
> - * system, so we generate a specific file for each.
> - */
> -static int systemd_timer_write_timer_file(enum schedule_priority schedule,
> -					  int minute)
> -{
> -	int res = -1;
> -	char *filename;
> -	FILE *file;
> -	const char *unit;
> -	char *schedule_pattern = NULL;
> -	const char *frequency = get_frequency(schedule);
> -	char *local_timer_name = xstrfmt(SYSTEMD_UNIT_FORMAT, frequency, "timer");
> -
> -	filename = xdg_config_home_systemd(local_timer_name);
> -
> -	if (safe_create_leading_directories(filename)) {
> -		error(_("failed to create directories for '%s'"), filename);
> -		goto error;
> -	}
> -	file = fopen_or_warn(filename, "w");
> -	if (!file)
> -		goto error;
> -
> -	switch (schedule) {
> -	case SCHEDULE_HOURLY:
> -		schedule_pattern = xstrfmt("*-*-* 1..23:%02d:00", minute);
> -		break;
> -
> -	case SCHEDULE_DAILY:
> -		schedule_pattern = xstrfmt("Tue..Sun *-*-* 0:%02d:00", minute);
> -		break;
> -
> -	case SCHEDULE_WEEKLY:
> -		schedule_pattern = xstrfmt("Mon 0:%02d:00", minute);
> -		break;
> -
> -	default:
> -		BUG("Unhandled schedule_priority");
> -	}
> -
> -	unit = "# This file was created and is maintained by Git.\n"
> -	       "# Any edits made in this file might be replaced in the future\n"
> -	       "# by a Git command.\n"
> -	       "\n"
> -	       "[Unit]\n"
> -	       "Description=Optimize Git repositories data\n"
> -	       "\n"
> -	       "[Timer]\n"
> -	       "OnCalendar=%s\n"
> -	       "Persistent=true\n"
> -	       "\n"
> -	       "[Install]\n"
> -	       "WantedBy=timers.target\n";
> -	if (fprintf(file, unit, schedule_pattern) < 0) {
> -		error(_("failed to write to '%s'"), filename);
> -		fclose(file);
> -		goto error;
> -	}
> -	if (fclose(file) == EOF) {
> -		error_errno(_("failed to flush '%s'"), filename);
> -		goto error;
> -	}
> -
> -	res = 0;
> -
> -error:
> -	free(schedule_pattern);
> -	free(local_timer_name);
> -	free(filename);
> -	return res;
> -}
> -
> -/*
> - * No matter the schedule, we use the same service and can make use of the
> - * templating system. When installing git-maintenance@<schedule>.timer,
> - * systemd will notice that git-maintenance@.service exists as a template
> - * and will use this file and insert the <schedule> into the template at
> - * the position of "%i".
> - */
> -static int systemd_timer_write_service_template(const char *exec_path)
> -{
> -	int res = -1;
> -	char *filename;
> -	FILE *file;
> -	const char *unit;
> -	char *local_service_name = xstrfmt(SYSTEMD_UNIT_FORMAT, "", "service");
> -
> -	filename = xdg_config_home_systemd(local_service_name);
> -	if (safe_create_leading_directories(filename)) {
> -		error(_("failed to create directories for '%s'"), filename);
> -		goto error;
> -	}
> -	file = fopen_or_warn(filename, "w");
> -	if (!file)
> -		goto error;
> -
> -	unit = "# This file was created and is maintained by Git.\n"
> -	       "# Any edits made in this file might be replaced in the future\n"
> -	       "# by a Git command.\n"
> -	       "\n"
> -	       "[Unit]\n"
> -	       "Description=Optimize Git repositories data\n"
> -	       "\n"
> -	       "[Service]\n"
> -	       "Type=oneshot\n"
> -	       "ExecStart=\"%s/git\" --exec-path=\"%s\" for-each-repo --config=maintenance.repo maintenance run --schedule=%%i\n"
> -	       "LockPersonality=yes\n"
> -	       "MemoryDenyWriteExecute=yes\n"
> -	       "NoNewPrivileges=yes\n"
> -	       "RestrictAddressFamilies=AF_UNIX AF_INET AF_INET6 AF_VSOCK\n"
> -	       "RestrictNamespaces=yes\n"
> -	       "RestrictRealtime=yes\n"
> -	       "RestrictSUIDSGID=yes\n"
> -	       "SystemCallArchitectures=native\n"
> -	       "SystemCallFilter=@system-service\n";
> -	if (fprintf(file, unit, exec_path, exec_path) < 0) {
> -		error(_("failed to write to '%s'"), filename);
> -		fclose(file);
> -		goto error;
> -	}
> -	if (fclose(file) == EOF) {
> -		error_errno(_("failed to flush '%s'"), filename);
> -		goto error;
> -	}
> -
> -	res = 0;
> -
> -error:
> -	free(local_service_name);
> -	free(filename);
> -	return res;
> -}
> -
> -static int systemd_timer_enable_unit(int enable,
> -				     enum schedule_priority schedule,
> -				     int minute)
> +static int systemd_set_units_state(int enable)
>  {
>  	const char *cmd = "systemctl";
>  	struct child_process child = CHILD_PROCESS_INIT;
> -	const char *frequency = get_frequency(schedule);
> -
> -	/*
> -	 * Disabling the systemd unit while it is already disabled makes
> -	 * systemctl print an error.
> -	 * Let's ignore it since it means we already are in the expected state:
> -	 * the unit is disabled.
> -	 *
> -	 * On the other hand, enabling a systemd unit which is already enabled
> -	 * produces no error.
> -	 */
> -	if (!enable)
> -		child.no_stderr = 1;
> -	else if (systemd_timer_write_timer_file(schedule, minute))
> -		return -1;
>  
>  	get_schedule_cmd(&cmd, NULL);
>  	strvec_split(&child.args, cmd);
> -	strvec_pushl(&child.args, "--user", enable ? "enable" : "disable",
> -		     "--now", NULL);
> -	strvec_pushf(&child.args, SYSTEMD_UNIT_FORMAT, frequency, "timer");
> +
> +	strvec_pushl(&child.args, "--user", "--force", "--now",
> +			enable ? "enable" : "disable",
> +			"git-maintenance@hourly.timer",
> +			"git-maintenance@daily.timer",
> +			"git-maintenance@weekly.timer", NULL);
> +	/*
> +	** --force override existing conflicting symlinks
> +	** We need it because the units have changed location (~/.config ->
> +	** /usr/lib)
> +	*/
>  
>  	if (start_command(&child))
>  		return error(_("failed to start systemctl"));
>  	if (finish_command(&child))
> -		/*
> -		 * Disabling an already disabled systemd unit makes
> -		 * systemctl fail.
> -		 * Let's ignore this failure.
> -		 *
> -		 * Enabling an enabled systemd unit doesn't fail.
> -		 */
> -		if (enable)
> -			return error(_("failed to run systemctl"));
> +		return error(_("failed to run systemctl"));
>  	return 0;
>  }
>  
> -/*
> - * A previous version of Git wrote the timer units as template files.
> - * Clean these up, if they exist.
> - */
> -static void systemd_timer_delete_stale_timer_templates(void)
> +static void systemd_delete_user_unit(char const *unit)
>  {
> -	char *timer_template_name = xstrfmt(SYSTEMD_UNIT_FORMAT, "", "timer");
> -	char *filename = xdg_config_home_systemd(timer_template_name);
> +	char const	file_start_stale[] =	"# This file was created and is"
> +						" maintained by Git.";
> +	char		file_start_user[sizeof(file_start_stale)] = {'\0'};
> +
> +	char *filename = xdg_config_home_systemd(unit);
> +	int handle = open(filename, O_RDONLY);
>  
> -	if (unlink(filename) && !is_missing_file_error(errno))
> +	/*
> +	** Check this is actually our file and we're not removing a legitimate
> +	** user override.
> +	*/
> +	if (handle == -1 && !is_missing_file_error(errno))
>  		warning(_("failed to delete '%s'"), filename);
> +	else {
> +		read(handle, file_start_user, sizeof(file_start_stale) - 1);
> +		close(handle);
> +		if (strcmp(file_start_stale, file_start_user) == 0) {
> +			if (unlink(filename) == 0)
> +				warning(_("deleted stale unit file '%s'"), filename);
> +			else if (!is_missing_file_error(errno))
> +				warning(_("failed to delete '%s'"), filename);
> +		}
> +	}
>  
>  	free(filename);
> -	free(timer_template_name);
> -}
> -
> -static int systemd_timer_delete_unit_files(void)
> -{
> -	systemd_timer_delete_stale_timer_templates();
> -
> -	/* Purposefully not short-circuited to make sure all are called. */
> -	return systemd_timer_delete_timer_file(SCHEDULE_HOURLY) |
> -	       systemd_timer_delete_timer_file(SCHEDULE_DAILY) |
> -	       systemd_timer_delete_timer_file(SCHEDULE_WEEKLY) |
> -	       systemd_timer_delete_service_template();
> -}
> -
> -static int systemd_timer_delete_units(void)
> -{
> -	int minute = get_random_minute();
> -	/* Purposefully not short-circuited to make sure all are called. */
> -	return systemd_timer_enable_unit(0, SCHEDULE_HOURLY, minute) |
> -	       systemd_timer_enable_unit(0, SCHEDULE_DAILY, minute) |
> -	       systemd_timer_enable_unit(0, SCHEDULE_WEEKLY, minute) |
> -	       systemd_timer_delete_unit_files();
> -}
> -
> -static int systemd_timer_setup_units(void)
> -{
> -	int minute = get_random_minute();
> -	const char *exec_path = git_exec_path();
> -
> -	int ret = systemd_timer_write_service_template(exec_path) ||
> -		  systemd_timer_enable_unit(1, SCHEDULE_HOURLY, minute) ||
> -		  systemd_timer_enable_unit(1, SCHEDULE_DAILY, minute) ||
> -		  systemd_timer_enable_unit(1, SCHEDULE_WEEKLY, minute);
> -
> -	if (ret)
> -		systemd_timer_delete_units();
> -	else
> -		systemd_timer_delete_stale_timer_templates();
> -
> -	return ret;
>  }
>  
>  static int systemd_timer_update_schedule(int run_maintenance, int fd UNUSED)
>  {
> -	if (run_maintenance)
> -		return systemd_timer_setup_units();
> -	else
> -		return systemd_timer_delete_units();
> +	/*
> +	 * A previous version of Git wrote the units in the user configuration
> +	 * directory. Clean these up, if they exist.
> +	 */
> +	systemd_delete_user_unit("git-maintenance@hourly.timer");
> +	systemd_delete_user_unit("git-maintenance@daily.timer");
> +	systemd_delete_user_unit("git-maintenance@weekly.timer");
> +	systemd_delete_user_unit("git-maintenance@.timer");
> +	systemd_delete_user_unit("git-maintenance@.service");
> +	return systemd_set_units_state(run_maintenance);
>  }
>  
>  enum scheduler {
> -- 
> 2.44.0
> 
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [RFC PATCH 4/5] maintenance: update systemd scheduler docs

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 3815 bytes --]

On Mon, Mar 18, 2024 at 04:31:18PM +0100, Max Gautier wrote:

This commit is missing an explanation what exactly you are updating.
While you mention that you update something, the reader now has to
find out by themselves what exactly you update by reading through the
whole commit diff.

Patrick

> Signed-off-by: Max Gautier <mg@max.gautier.name>
> ---
>  Documentation/git-maintenance.txt | 33 +++++++++++++++----------------
>  1 file changed, 16 insertions(+), 17 deletions(-)
> 
> diff --git a/Documentation/git-maintenance.txt b/Documentation/git-maintenance.txt
> index 51d0f7e94b..6511c3f3f1 100644
> --- a/Documentation/git-maintenance.txt
> +++ b/Documentation/git-maintenance.txt
> @@ -304,10 +304,9 @@ distributions, systemd timers are superseding it.
>  If user systemd timers are available, they will be used as a replacement
>  of `cron`.
>  
> -In this case, `git maintenance start` will create user systemd timer units
> -and start the timers. The current list of user-scheduled tasks can be found
> -by running `systemctl --user list-timers`. The timers written by `git
> -maintenance start` are similar to this:
> +In this case, `git maintenance start` will enable user systemd timer units
> +and start them. The current list of user-scheduled tasks can be found by
> +running `systemctl --user list-timers`. These timers are similar to this:
>  
>  -----------------------------------------------------------------------
>  $ systemctl --user list-timers
> @@ -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
>  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
>  -----------------------------------------------------------------------
>  
> -One timer is registered for each `--schedule=<frequency>` option.
> +One timer instance is enabled for each `--schedule=<frequency>` option.
>  
> -The definition of the systemd units can be inspected in the following files:
> +The definition of the systemd units can be inspected this way:
>  
>  -----------------------------------------------------------------------
> -~/.config/systemd/user/git-maintenance@.timer
> -~/.config/systemd/user/git-maintenance@.service
> -~/.config/systemd/user/timers.target.wants/git-maintenance@hourly.timer
> -~/.config/systemd/user/timers.target.wants/git-maintenance@daily.timer
> -~/.config/systemd/user/timers.target.wants/git-maintenance@weekly.timer
> +$ systemctl cat --user git-maintenance@.timer
> +$ systemctl cat --user git-maintenance@.service
>  -----------------------------------------------------------------------
>  
> -`git maintenance start` will overwrite these files and start the timer
> -again with `systemctl --user`, so any customization should be done by
> -creating a drop-in file, i.e. a `.conf` suffixed file in the
> -`~/.config/systemd/user/git-maintenance@.service.d` directory.
> +Customization of the timer or service can be performed with the usual systemd
> +tooling:
> +-----------------------------------------------------------------------
> +$ systemctl edit --user git-maintenance@.timer # all the timers
> +$ systemctl edit --user git-maintenance@hourly.timer # the hourly timer
> +$ systemctl edit --user git-maintenance@.service # all the services
> +$ systemctl edit --user git-maintenance@hourly.service # the hourly run
> +-----------------------------------------------------------------------
>  
> -`git maintenance stop` will stop the user systemd timers and delete
> -the above mentioned files.
> +`git maintenance stop` will disable and stop the user systemd timers.
>  
>  For more details, see `systemd.timer(5)`.
>  
> -- 
> 2.44.0
> 
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [RFC PATCH 1/5] maintenance: package systemd units

[Max Gautier]

On Thu, Mar 21, 2024 at 01:37:30PM +0100, Patrick Steinhardt wrote:
> On Mon, Mar 18, 2024 at 04:31:15PM +0100, Max Gautier wrote:
> 
> It would be great to document _why_ we want to package the systemd units
> alongside with Git.
> 

Hum, I wrote that in the cover, but you're right, it should be in the
commit itself.
Ack.

> > ...
> > diff --git a/systemd/user/git-maintenance@.service b/systemd/user/git-maintenance@.service
> > new file mode 100644
> > index 0000000000..87ac0c86e6
> > --- /dev/null
> > +++ b/systemd/user/git-maintenance@.service
> > @@ -0,0 +1,16 @@
> > +[Unit]
> > +Description=Optimize Git repositories data
> > +
> > +[Service]
> > +Type=oneshot
> > +ExecStart=git for-each-repo --config=maintenance.repo \
> > +          maintenance run --schedule=%i
> > +LockPersonality=yes
> > +MemoryDenyWriteExecute=yes
> > +NoNewPrivileges=yes
> > +RestrictAddressFamilies=AF_UNIX AF_INET AF_INET6 AF_VSOCK
> > +RestrictNamespaces=yes
> > +RestrictRealtime=yes
> > +RestrictSUIDSGID=yes
> > +SystemCallArchitectures=native
> > +SystemCallFilter=@system-service
> 
> Curious, but how did you arrive at these particular restrictions for the
> unit? Might be something to explain in the commit message, as well.
> 
> Patrick

I copied the unit file which is defined in strings in builtin/gc.c,
which I delete in patch 3.
Should the moving be inside one commit, in order to explicit the fact
that it's only moving things around ?

-- 
Max Gautier

Re: [RFC PATCH 2/5] maintenance: add fixed random delay to systemd timers

[Max Gautier]

On Thu, Mar 21, 2024 at 01:37:35PM +0100, Patrick Steinhardt wrote:
> On Mon, Mar 18, 2024 at 04:31:16PM +0100, Max Gautier wrote:
> > Ensures that:
> > - git maintenance timers have a fixed time interval between execution.
> > - the three timers are not executed at the same time.
> 
> Commit messages are typically structured so that you first explain what
> the problem is that you're trying to solve, and then you explain how you
> solve it. Your commit message is missing the first part.
> 
> > This is intended to implement an alternative to the two followings
> > commits:
> > c97ec0378b (maintenance: fix systemd schedule overlaps, 2023-08-10)
> > daa787010c (maintenance: use random minute in systemd scheduler, 2023-08-10)
> > 
> > Instead of manually adding a specific minute (which is reset on each
> > invocation of `git maintenance start`), we use systemd timers
> > RandomizedDelaySec and FixedRandomDelay functionalities.
> 
> I think it would help to not list commits, but put the commit references
> in a paragraph. Something in the spirit of "In commit c97ec0378b we
> already tried to address this issue in such and such a way, but that is
> quite limiting due to that and that. Similarly, in commit daa787010c..".
> 

Ok I see how that would work.
Stating the limitations of the implemenation added by those commits
would be the problem we're trying to solve, then the proposed solution.

> > From man systemd.timer:
> > >FixedRandomDelay=
> > >  Takes a boolean argument. When enabled, the randomized offset
> > >  specified by RandomizedDelaySec= is reused for all firings of the
> > >  same timer. For a given timer unit, **the offset depends on the
> > >  machine ID, user identifier and timer name**, which means that it is
> > >  stable between restarts of the manager. This effectively creates a
> > >  fixed offset for an individual timer, reducing the jitter in
> > >  firings of this timer, while still avoiding firing at the same time
> > >  as other similarly configured timers.
> > 
> > -> which is exactly the use case for git-maintenance timers.
> > 
> > Signed-off-by: Max Gautier <mg@max.gautier.name>
> > ---
> >  systemd/user/git-maintenance@.service | 1 +
> >  systemd/user/git-maintenance@.timer   | 3 +++
> >  2 files changed, 4 insertions(+)
> > 
> > diff --git a/systemd/user/git-maintenance@.service b/systemd/user/git-maintenance@.service
> > index 87ac0c86e6..f949e1a217 100644
> > --- a/systemd/user/git-maintenance@.service
> > +++ b/systemd/user/git-maintenance@.service
> > @@ -1,5 +1,6 @@
> >  [Unit]
> >  Description=Optimize Git repositories data
> > +Documentation=man:git-maintenance(1)
> 
> This change feels unrelated and should likely go into the first commit.
> 
> >  
> >  [Service]
> >  Type=oneshot
> > diff --git a/systemd/user/git-maintenance@.timer b/systemd/user/git-maintenance@.timer
> > index 40fbc77a62..667c5998ba 100644
> > --- a/systemd/user/git-maintenance@.timer
> > +++ b/systemd/user/git-maintenance@.timer
> > @@ -1,9 +1,12 @@
> >  [Unit]
> >  Description=Optimize Git repositories data
> > +Documentation=man:git-maintenance(1)
> 
> Same.
> 
> Patrick

Ack, will do.

> 
> >  [Timer]
> >  OnCalendar=%i
> >  Persistent=true
> > +RandomizedDelaySec=1800
> > +FixedRandomDelay=true
> >  
> >  [Install]
> >  WantedBy=timers.target
> > -- 
> > 2.44.0
> > 
> > 



-- 
Max Gautier

Re: [RFC PATCH 3/5] maintenance: use packaged systemd units

[Max Gautier]

On Thu, Mar 21, 2024 at 01:37:41PM +0100, Patrick Steinhardt wrote:
> On Mon, Mar 18, 2024 at 04:31:17PM +0100, Max Gautier wrote:
> > - Remove the code writing the units
> > - Simplify calling systemctl
> >   - systemctl does not error out when disabling units which aren't
> >     enabled, nor when enabling already enabled units
> >   - call systemctl only once, with all the units.
> > - Add clean-up code for leftover units in $XDG_CONFIG_HOME created by
> >   previous versions of git, taking care of preserving actual user
> >   override (by checking the start of the file).
> 
> We very much try to avoid bulleted-list-style commit messages. Once you
> have a list of changes it very often indicates that there are multiple
> semi-related things going on in the same commit. At that point we prefer
> to split the commit up into multiple commits so that each of the bullet
> items can be explained separately.
> 
> Also, as mentioned before, commit messages should explain what the
> problem is and how that problem is solved. 
> 
> Patrick
> 

I need to think a bit about patch ordering here because changing from
the "random minute" implementation to the systemd
(RandomDelaySec/FixedRandomDelay) is required to the other changes of
putting the unit out of the code and in the package.

I think I have an idea how, I'll work on that, thanks.

-- 
Max Gautier

Re: [RFC PATCH 1/5] maintenance: package systemd units

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 1808 bytes --]

On Thu, Mar 21, 2024 at 02:07:02PM +0100, Max Gautier wrote:
> On Thu, Mar 21, 2024 at 01:37:30PM +0100, Patrick Steinhardt wrote:
> > On Mon, Mar 18, 2024 at 04:31:15PM +0100, Max Gautier wrote:
> > 
> > It would be great to document _why_ we want to package the systemd units
> > alongside with Git.
> > 
> 
> Hum, I wrote that in the cover, but you're right, it should be in the
> commit itself.
> Ack.
> 
> > > ...
> > > diff --git a/systemd/user/git-maintenance@.service b/systemd/user/git-maintenance@.service
> > > new file mode 100644
> > > index 0000000000..87ac0c86e6
> > > --- /dev/null
> > > +++ b/systemd/user/git-maintenance@.service
> > > @@ -0,0 +1,16 @@
> > > +[Unit]
> > > +Description=Optimize Git repositories data
> > > +
> > > +[Service]
> > > +Type=oneshot
> > > +ExecStart=git for-each-repo --config=maintenance.repo \
> > > +          maintenance run --schedule=%i
> > > +LockPersonality=yes
> > > +MemoryDenyWriteExecute=yes
> > > +NoNewPrivileges=yes
> > > +RestrictAddressFamilies=AF_UNIX AF_INET AF_INET6 AF_VSOCK
> > > +RestrictNamespaces=yes
> > > +RestrictRealtime=yes
> > > +RestrictSUIDSGID=yes
> > > +SystemCallArchitectures=native
> > > +SystemCallFilter=@system-service
> > 
> > Curious, but how did you arrive at these particular restrictions for the
> > unit? Might be something to explain in the commit message, as well.
> > 
> > Patrick
> 
> I copied the unit file which is defined in strings in builtin/gc.c,
> which I delete in patch 3.
> Should the moving be inside one commit, in order to explicit the fact
> that it's only moving things around ?

I don't really think it's necessary. But pointing that out in the commit
message would go a long way to explain where exactly these definitions
come from.

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [RFC PATCH 1/5] maintenance: package systemd units

[Max Gautier]

On Thu, Mar 21, 2024 at 01:37:30PM +0100, Patrick Steinhardt wrote:
> On Mon, Mar 18, 2024 at 04:31:15PM +0100, Max Gautier wrote:
> 
> It would be great to document _why_ we want to package the systemd units
> alongside with Git.
> 
> > Signed-off-by: Max Gautier <mg@max.gautier.name>
> > ---
> >  Makefile                              |  4 ++++
> >  systemd/user/git-maintenance@.service | 16 ++++++++++++++++
> >  systemd/user/git-maintenance@.timer   |  9 +++++++++
> >  3 files changed, 29 insertions(+)
> >  create mode 100644 systemd/user/git-maintenance@.service
> >  create mode 100644 systemd/user/git-maintenance@.timer
> > 
> > diff --git a/Makefile b/Makefile
> > index 4e255c81f2..276b4373c6 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -619,6 +619,7 @@ htmldir = $(prefix)/share/doc/git-doc
> >  ETC_GITCONFIG = $(sysconfdir)/gitconfig
> >  ETC_GITATTRIBUTES = $(sysconfdir)/gitattributes
> >  lib = lib
> > +libdir = $(prefix)/lib
> >  # DESTDIR =
> >  pathsep = :
> >  
> > @@ -1328,6 +1329,8 @@ BUILTIN_OBJS += builtin/verify-tag.o
> >  BUILTIN_OBJS += builtin/worktree.o
> >  BUILTIN_OBJS += builtin/write-tree.o
> >  
> > +SYSTEMD_USER_UNITS := $(wildcard systemd/user/*)
> > +
> >  # THIRD_PARTY_SOURCES is a list of patterns compatible with the
> >  # $(filter) and $(filter-out) family of functions. They specify source
> >  # files which are taken from some third-party source where we want to be
> > @@ -3469,6 +3472,7 @@ install: all
> >  	$(INSTALL) -m 644 $(SCRIPT_LIB) '$(DESTDIR_SQ)$(gitexec_instdir_SQ)'
> >  	$(INSTALL) $(INSTALL_STRIP) $(install_bindir_xprograms) '$(DESTDIR_SQ)$(bindir_SQ)'
> >  	$(INSTALL) $(BINDIR_PROGRAMS_NO_X) '$(DESTDIR_SQ)$(bindir_SQ)'
> > +	$(INSTALL) -Dm 644 -t '$(DESTDIR_SQ)$(libdir)/systemd/user' $(SYSTEMD_USER_UNITS)
> 
> I wonder whether we want to unconditionally install those units. Many of
> the platforms that we support don't even have systemd available, so
> certainly it wouldn't make any sense to install it on those platforms.
> 
> Assuming that this is something we want in the first place I thus think
> that we should at least make this conditional and add some platform
> specific quirk to "config.mak.uname".
> 

We probably want that (conditional install) but I'm not sure where that
should go ; I'm not super familiar with autoconf. 

I just noticed than man 7 daemon (shipped by systemd) propose the
following snippet for installing systemd system services (should be easy
enough to adapt for user ervices, I think):

PKG_PROG_PKG_CONFIG()
AC_ARG_WITH([systemdsystemunitdir],
    [AS_HELP_STRING([--with-systemdsystemunitdir=DIR], [Directory for systemd service files])],,
    [with_systemdsystemunitdir=auto])
AS_IF([test "x$with_systemdsystemunitdir" = "xyes" -o "x$with_systemdsystemunitdir" = "xauto"], [
    def_systemdsystemunitdir=$($PKG_CONFIG --variable=systemdsystemunitdir systemd)

    AS_IF([test "x$def_systemdsystemunitdir" = "x"],
  [AS_IF([test "x$with_systemdsystemunitdir" = "xyes"],
   [AC_MSG_ERROR([systemd support requested but pkg-config unable to query systemd package])])
   with_systemdsystemunitdir=no],
  [with_systemdsystemunitdir="$def_systemdsystemunitdir"])])
AS_IF([test "x$with_systemdsystemunitdir" != "xno"],
     [AC_SUBST([systemdsystemunitdir], [$with_systemdsystemunitdir])])
AM_CONDITIONAL([HAVE_SYSTEMD], [test "x$with_systemdsystemunitdir" != "xno"])

Would something like that work ?

-- 
Max Gautier

Re: [RFC PATCH 1/5] maintenance: package systemd units

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 3955 bytes --]

On Thu, Mar 21, 2024 at 02:38:36PM +0100, Max Gautier wrote:
> On Thu, Mar 21, 2024 at 01:37:30PM +0100, Patrick Steinhardt wrote:
> > On Mon, Mar 18, 2024 at 04:31:15PM +0100, Max Gautier wrote:
> > 
> > It would be great to document _why_ we want to package the systemd units
> > alongside with Git.
> > 
> > > Signed-off-by: Max Gautier <mg@max.gautier.name>
> > > ---
> > >  Makefile                              |  4 ++++
> > >  systemd/user/git-maintenance@.service | 16 ++++++++++++++++
> > >  systemd/user/git-maintenance@.timer   |  9 +++++++++
> > >  3 files changed, 29 insertions(+)
> > >  create mode 100644 systemd/user/git-maintenance@.service
> > >  create mode 100644 systemd/user/git-maintenance@.timer
> > > 
> > > diff --git a/Makefile b/Makefile
> > > index 4e255c81f2..276b4373c6 100644
> > > --- a/Makefile
> > > +++ b/Makefile
> > > @@ -619,6 +619,7 @@ htmldir = $(prefix)/share/doc/git-doc
> > >  ETC_GITCONFIG = $(sysconfdir)/gitconfig
> > >  ETC_GITATTRIBUTES = $(sysconfdir)/gitattributes
> > >  lib = lib
> > > +libdir = $(prefix)/lib
> > >  # DESTDIR =
> > >  pathsep = :
> > >  
> > > @@ -1328,6 +1329,8 @@ BUILTIN_OBJS += builtin/verify-tag.o
> > >  BUILTIN_OBJS += builtin/worktree.o
> > >  BUILTIN_OBJS += builtin/write-tree.o
> > >  
> > > +SYSTEMD_USER_UNITS := $(wildcard systemd/user/*)
> > > +
> > >  # THIRD_PARTY_SOURCES is a list of patterns compatible with the
> > >  # $(filter) and $(filter-out) family of functions. They specify source
> > >  # files which are taken from some third-party source where we want to be
> > > @@ -3469,6 +3472,7 @@ install: all
> > >  	$(INSTALL) -m 644 $(SCRIPT_LIB) '$(DESTDIR_SQ)$(gitexec_instdir_SQ)'
> > >  	$(INSTALL) $(INSTALL_STRIP) $(install_bindir_xprograms) '$(DESTDIR_SQ)$(bindir_SQ)'
> > >  	$(INSTALL) $(BINDIR_PROGRAMS_NO_X) '$(DESTDIR_SQ)$(bindir_SQ)'
> > > +	$(INSTALL) -Dm 644 -t '$(DESTDIR_SQ)$(libdir)/systemd/user' $(SYSTEMD_USER_UNITS)
> > 
> > I wonder whether we want to unconditionally install those units. Many of
> > the platforms that we support don't even have systemd available, so
> > certainly it wouldn't make any sense to install it on those platforms.
> > 
> > Assuming that this is something we want in the first place I thus think
> > that we should at least make this conditional and add some platform
> > specific quirk to "config.mak.uname".
> > 
> 
> We probably want that (conditional install) but I'm not sure where that
> should go ; I'm not super familiar with autoconf. 
> 
> I just noticed than man 7 daemon (shipped by systemd) propose the
> following snippet for installing systemd system services (should be easy
> enough to adapt for user ervices, I think):
> 
> PKG_PROG_PKG_CONFIG()
> AC_ARG_WITH([systemdsystemunitdir],
>     [AS_HELP_STRING([--with-systemdsystemunitdir=DIR], [Directory for systemd service files])],,
>     [with_systemdsystemunitdir=auto])
> AS_IF([test "x$with_systemdsystemunitdir" = "xyes" -o "x$with_systemdsystemunitdir" = "xauto"], [
>     def_systemdsystemunitdir=$($PKG_CONFIG --variable=systemdsystemunitdir systemd)
> 
>     AS_IF([test "x$def_systemdsystemunitdir" = "x"],
>   [AS_IF([test "x$with_systemdsystemunitdir" = "xyes"],
>    [AC_MSG_ERROR([systemd support requested but pkg-config unable to query systemd package])])
>    with_systemdsystemunitdir=no],
>   [with_systemdsystemunitdir="$def_systemdsystemunitdir"])])
> AS_IF([test "x$with_systemdsystemunitdir" != "xno"],
>      [AC_SUBST([systemdsystemunitdir], [$with_systemdsystemunitdir])])
> AM_CONDITIONAL([HAVE_SYSTEMD], [test "x$with_systemdsystemunitdir" != "xno"])
> 
> Would something like that work ?

Probably. But while we do have autoconf wired up, the primary way of
building Git does not use it, but rather uses `config.mak.uname`. I'd
recommend to have a look at it to figure out how we handle other
build-time options there.

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [RFC PATCH 1/5] maintenance: package systemd units

[Max Gautier]

On Thu, Mar 21, 2024 at 02:38:36PM +0100, Max Gautier wrote:
> On Thu, Mar 21, 2024 at 01:37:30PM +0100, Patrick Steinhardt wrote:
> > On Mon, Mar 18, 2024 at 04:31:15PM +0100, Max Gautier wrote:
> > 
> > It would be great to document _why_ we want to package the systemd units
> > alongside with Git.
> > 
> > > Signed-off-by: Max Gautier <mg@max.gautier.name>
> > > ---
> > >  Makefile                              |  4 ++++
> > >  systemd/user/git-maintenance@.service | 16 ++++++++++++++++
> > >  systemd/user/git-maintenance@.timer   |  9 +++++++++
> > >  3 files changed, 29 insertions(+)
> > >  create mode 100644 systemd/user/git-maintenance@.service
> > >  create mode 100644 systemd/user/git-maintenance@.timer
> > > 
> > > diff --git a/Makefile b/Makefile
> > > index 4e255c81f2..276b4373c6 100644
> > > --- a/Makefile
> > > +++ b/Makefile
> > > @@ -619,6 +619,7 @@ htmldir = $(prefix)/share/doc/git-doc
> > >  ETC_GITCONFIG = $(sysconfdir)/gitconfig
> > >  ETC_GITATTRIBUTES = $(sysconfdir)/gitattributes
> > >  lib = lib
> > > +libdir = $(prefix)/lib
> > >  # DESTDIR =
> > >  pathsep = :
> > >  
> > > @@ -1328,6 +1329,8 @@ BUILTIN_OBJS += builtin/verify-tag.o
> > >  BUILTIN_OBJS += builtin/worktree.o
> > >  BUILTIN_OBJS += builtin/write-tree.o
> > >  
> > > +SYSTEMD_USER_UNITS := $(wildcard systemd/user/*)
> > > +
> > >  # THIRD_PARTY_SOURCES is a list of patterns compatible with the
> > >  # $(filter) and $(filter-out) family of functions. They specify source
> > >  # files which are taken from some third-party source where we want to be
> > > @@ -3469,6 +3472,7 @@ install: all
> > >  	$(INSTALL) -m 644 $(SCRIPT_LIB) '$(DESTDIR_SQ)$(gitexec_instdir_SQ)'
> > >  	$(INSTALL) $(INSTALL_STRIP) $(install_bindir_xprograms) '$(DESTDIR_SQ)$(bindir_SQ)'
> > >  	$(INSTALL) $(BINDIR_PROGRAMS_NO_X) '$(DESTDIR_SQ)$(bindir_SQ)'
> > > +	$(INSTALL) -Dm 644 -t '$(DESTDIR_SQ)$(libdir)/systemd/user' $(SYSTEMD_USER_UNITS)
> > 
> > I wonder whether we want to unconditionally install those units. Many of
> > the platforms that we support don't even have systemd available, so
> > certainly it wouldn't make any sense to install it on those platforms.
> > 
> > Assuming that this is something we want in the first place I thus think
> > that we should at least make this conditional and add some platform
> > specific quirk to "config.mak.uname".
> > 
> 
> We probably want that (conditional install) but I'm not sure where that
> should go ; I'm not super familiar with autoconf. 
> 
> I just noticed than man 7 daemon (shipped by systemd) propose the
> following snippet for installing systemd system services (should be easy
> enough to adapt for user ervices, I think):
> 
> PKG_PROG_PKG_CONFIG()
> AC_ARG_WITH([systemdsystemunitdir],
>     [AS_HELP_STRING([--with-systemdsystemunitdir=DIR], [Directory for systemd service files])],,
>     [with_systemdsystemunitdir=auto])
> AS_IF([test "x$with_systemdsystemunitdir" = "xyes" -o "x$with_systemdsystemunitdir" = "xauto"], [
>     def_systemdsystemunitdir=$($PKG_CONFIG --variable=systemdsystemunitdir systemd)
> 
>     AS_IF([test "x$def_systemdsystemunitdir" = "x"],
>   [AS_IF([test "x$with_systemdsystemunitdir" = "xyes"],
>    [AC_MSG_ERROR([systemd support requested but pkg-config unable to query systemd package])])
>    with_systemdsystemunitdir=no],
>   [with_systemdsystemunitdir="$def_systemdsystemunitdir"])])
> AS_IF([test "x$with_systemdsystemunitdir" != "xno"],
>      [AC_SUBST([systemdsystemunitdir], [$with_systemdsystemunitdir])])
> AM_CONDITIONAL([HAVE_SYSTEMD], [test "x$with_systemdsystemunitdir" != "xno"])
> 
> Would something like that work ?

Ugh, that snippet apparently requires that I delete aclocal.m4, execute
`autoreconf --install`, then concat together the previous aclocal.m4 and
the newly generated one.

I'm adding the last authors on aclocal.m4 to the discussion, maybe they
have some lights ? (Though they don't have commits in the last ten
years, so it might be a very long shot ...)

-- 
Max Gautier

Re: [RFC PATCH 1/5] maintenance: package systemd units

[Max Gautier]

On Thu, Mar 21, 2024 at 03:44:05PM +0100, Patrick Steinhardt wrote:
> On Thu, Mar 21, 2024 at 02:38:36PM +0100, Max Gautier wrote:
> > On Thu, Mar 21, 2024 at 01:37:30PM +0100, Patrick Steinhardt wrote:
> > > On Mon, Mar 18, 2024 at 04:31:15PM +0100, Max Gautier wrote:
> > > 
> > > It would be great to document _why_ we want to package the systemd units
> > > alongside with Git.
> > > 
> > > > Signed-off-by: Max Gautier <mg@max.gautier.name>
> > > > ---
> > > >  Makefile                              |  4 ++++
> > > >  systemd/user/git-maintenance@.service | 16 ++++++++++++++++
> > > >  systemd/user/git-maintenance@.timer   |  9 +++++++++
> > > >  3 files changed, 29 insertions(+)
> > > >  create mode 100644 systemd/user/git-maintenance@.service
> > > >  create mode 100644 systemd/user/git-maintenance@.timer
> > > > 
> > > > diff --git a/Makefile b/Makefile
> > > > index 4e255c81f2..276b4373c6 100644
> > > > --- a/Makefile
> > > > +++ b/Makefile
> > > > @@ -619,6 +619,7 @@ htmldir = $(prefix)/share/doc/git-doc
> > > >  ETC_GITCONFIG = $(sysconfdir)/gitconfig
> > > >  ETC_GITATTRIBUTES = $(sysconfdir)/gitattributes
> > > >  lib = lib
> > > > +libdir = $(prefix)/lib
> > > >  # DESTDIR =
> > > >  pathsep = :
> > > >  
> > > > @@ -1328,6 +1329,8 @@ BUILTIN_OBJS += builtin/verify-tag.o
> > > >  BUILTIN_OBJS += builtin/worktree.o
> > > >  BUILTIN_OBJS += builtin/write-tree.o
> > > >  
> > > > +SYSTEMD_USER_UNITS := $(wildcard systemd/user/*)
> > > > +
> > > >  # THIRD_PARTY_SOURCES is a list of patterns compatible with the
> > > >  # $(filter) and $(filter-out) family of functions. They specify source
> > > >  # files which are taken from some third-party source where we want to be
> > > > @@ -3469,6 +3472,7 @@ install: all
> > > >  	$(INSTALL) -m 644 $(SCRIPT_LIB) '$(DESTDIR_SQ)$(gitexec_instdir_SQ)'
> > > >  	$(INSTALL) $(INSTALL_STRIP) $(install_bindir_xprograms) '$(DESTDIR_SQ)$(bindir_SQ)'
> > > >  	$(INSTALL) $(BINDIR_PROGRAMS_NO_X) '$(DESTDIR_SQ)$(bindir_SQ)'
> > > > +	$(INSTALL) -Dm 644 -t '$(DESTDIR_SQ)$(libdir)/systemd/user' $(SYSTEMD_USER_UNITS)
> > > 
> > > I wonder whether we want to unconditionally install those units. Many of
> > > the platforms that we support don't even have systemd available, so
> > > certainly it wouldn't make any sense to install it on those platforms.
> > > 
> > > Assuming that this is something we want in the first place I thus think
> > > that we should at least make this conditional and add some platform
> > > specific quirk to "config.mak.uname".
> > > 
> > 
> > We probably want that (conditional install) but I'm not sure where that
> > should go ; I'm not super familiar with autoconf. 
> > 
> > I just noticed than man 7 daemon (shipped by systemd) propose the
> > following snippet for installing systemd system services (should be easy
> > enough to adapt for user ervices, I think):
> > 
> > PKG_PROG_PKG_CONFIG()
> > AC_ARG_WITH([systemdsystemunitdir],
> >     [AS_HELP_STRING([--with-systemdsystemunitdir=DIR], [Directory for systemd service files])],,
> >     [with_systemdsystemunitdir=auto])
> > AS_IF([test "x$with_systemdsystemunitdir" = "xyes" -o "x$with_systemdsystemunitdir" = "xauto"], [
> >     def_systemdsystemunitdir=$($PKG_CONFIG --variable=systemdsystemunitdir systemd)
> > 
> >     AS_IF([test "x$def_systemdsystemunitdir" = "x"],
> >   [AS_IF([test "x$with_systemdsystemunitdir" = "xyes"],
> >    [AC_MSG_ERROR([systemd support requested but pkg-config unable to query systemd package])])
> >    with_systemdsystemunitdir=no],
> >   [with_systemdsystemunitdir="$def_systemdsystemunitdir"])])
> > AS_IF([test "x$with_systemdsystemunitdir" != "xno"],
> >      [AC_SUBST([systemdsystemunitdir], [$with_systemdsystemunitdir])])
> > AM_CONDITIONAL([HAVE_SYSTEMD], [test "x$with_systemdsystemunitdir" != "xno"])
> > 
> > Would something like that work ?
> 
> Probably. But while we do have autoconf wired up, the primary way of
> building Git does not use it, but rather uses `config.mak.uname`. I'd
> recommend to have a look at it to figure out how we handle other
> build-time options there.
> 
> Patrick

I'll take a look, thanks.


-- 
Max Gautier

[PATCH v2 0/6] maintenance: use packaged systemd units

[Max Gautier]

* Distribute the systemd timers used by the `git maintenance start` with
  the systemd scheduler as part of git, rather than writing them in
  $XDG_CONFIG_HOME.

This allows users to override the units if they wish, and is more
in-line with the usual practices of distribution for systemd units.

We also move away from using the random minute, and instead rely on
systemd features to achieve the same goal (see patch 2). This allows us
to go back to using unit templating for the timers. This is also a
prerequisite to have static unit files.

Note that even if we really need more specific OnCalendar= settings for
each timer, we should still do it that way, but instead distribute
override alongside the template, for instance for weekly:

/usr/lib/systemd-user/git-maintenance@daily.timer.d/override.conf:
[Timer]
OnCalendar=<daily specific calendar spec>

The cleanup code for the units written in $XDG_CONFIG_HOME is adapted,
and takes care of not removing legitimate user overrides, by checking
the file start.

Testing:
The simplest way to simulate having the units in /usr/lib is probably to
copy them in /etc/systemd/user.

Changes since v1:
- Reorganization of the commits and their messages to try to address
  review comments
- Dropped the DON'T APPLY PATCH, added a TODO to the cleanup code
  instead
- Updated the git-maintenance tests to work with the new logic.
- Conditional installation of the units files
- Fixing some style/consistency issues
- template the systemd service file to use $(bindir)

Max Gautier (6):
  maintenance: use systemd timers builtin randomization
  maintenance: use packaged systemd units
  maintenance: simplify systemctl calls
  maintenance: cleanup $XDG_CONFIG_HOME/systemd/user
  maintenance: update systemd scheduler docs
  maintenance: update tests for systemd scheduler

 Documentation/git-maintenance.txt        |  33 ++-
 Makefile                                 |   5 +
 builtin/gc.c                             | 298 ++++-------------------
 config.mak.uname                         |  10 +
 systemd/user/git-maintenance@.service.in |  17 ++
 systemd/user/git-maintenance@.timer      |  12 +
 t/t7900-maintenance.sh                   |  50 ++--
 7 files changed, 126 insertions(+), 299 deletions(-)
 create mode 100644 systemd/user/git-maintenance@.service.in
 create mode 100644 systemd/user/git-maintenance@.timer

Range-diff against v1:
1:  ea54a6e50e < -:  ---------- maintenance: package systemd units
2:  b29dbb9fdd < -:  ---------- maintenance: use packaged systemd units
3:  47bd6712b8 < -:  ---------- maintenance: add fixed random delay to systemd timers
-:  ---------- > 1:  42d88c7f81 maintenance: use systemd timers builtin randomization
-:  ---------- > 2:  18d51b1dd1 maintenance: use packaged systemd units
-:  ---------- > 3:  3aa7446e95 maintenance: simplify systemctl calls
-:  ---------- > 4:  daff7b4d60 maintenance: cleanup $XDG_CONFIG_HOME/systemd/user
4:  fac57db55e ! 5:  5f6a8e141f maintenance: update systemd scheduler docs
    @@ Metadata
      ## Commit message ##
         maintenance: update systemd scheduler docs
     
    +    The `git maintenance` systemd scheduler no longer writes units in
    +    $XDG_CONFIG_HOME.
    +
    +    Describe the new behavior.
    +    Instead of explaining manual ways to modify the timer, suggest the
    +    systemd standard tool: `systemctl edit`.
    +
         Signed-off-by: Max Gautier <mg@max.gautier.name>
     
      ## Documentation/git-maintenance.txt ##
5:  d888fbd0c3 < -:  ---------- DON'T APPLY YET: maintenance: remove cleanup code
-:  ---------- > 6:  4d4bcd6233 maintenance: update tests for systemd scheduler
-- 
2.44.0

[PATCH v2 1/6] maintenance: use systemd timers builtin randomization

[Max Gautier]

Commit daa787010c (maintenance: use random minute in systemd scheduler,
2023-08-10) hooked the systemd timers with the get_random_minute()
function, which is used to spread the load generated by clients, while
keeping one hour between each run (for the hourly run).

There is two problems with this approach:
1. The random minute is the same for all timers, which can lead to
   overlapping execution, and in practice skipped execution for some of
   the timers.
2. The timers are overwritten on each execution of `git maintenance
   start`, with a new random minute, which can lead to missing a daily
   or weekly timer.

   Consider this (unlikely) sequence of events:
   - weekly timer schedule is "Mon 0:52:00".
   - user run `git maintenance start` Monday, at 0:30:00.
   - the random minute happens to be 22, making the new weekly timer
     schedule "Mon 0:22:00".
   - the weekly timer does not fire until the next Monday.

Commit c97ec0378b (maintenance: fix systemd schedule overlaps,
2023-08-10) resolve problem 1 by skipping some executions of the timers
to avoid overlap, but not problem 2.

Revert both commits to instead use two native systemd timers properties:
- RandomizedDelaySec: as the name suggest, this adds a random offset
  to each timer execution. Use 3540 seconds (59 minutes) to spread
  execution as much as possible.
- FixedRandomDelay: this makes the offset from the previous setting
  deterministic, depending on machine ID, timer name and user
  identifier.

(For more details on these settings, see man 5 systemd.timer)

This give us timers which:
- don't overlap (schedule depend on timer name)
- are spread out by user (schedule depend on machine ID and user ID)
- happen at fixed intervals

This also restore the ability to use the systemd templating system, and
keep the systemd unit strings embedded in the C code simpler, with no
need for format strings.

Signed-off-by: Max Gautier <mg@max.gautier.name>
---
 builtin/gc.c           | 154 +++++++----------------------------------
 t/t7900-maintenance.sh |  15 +---
 2 files changed, 27 insertions(+), 142 deletions(-)

diff --git a/builtin/gc.c b/builtin/gc.c
index cb80ced6cb..dac59414f0 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -2308,54 +2308,29 @@ static char *xdg_config_home_systemd(const char *filename)
 	return xdg_config_home_for("systemd/user", filename);
 }
 
-#define SYSTEMD_UNIT_FORMAT "git-maintenance@%s.%s"
-
-static int systemd_timer_delete_timer_file(enum schedule_priority priority)
+static int systemd_timer_delete_unit_templates(void)
 {
 	int ret = 0;
-	const char *frequency = get_frequency(priority);
-	char *local_timer_name = xstrfmt(SYSTEMD_UNIT_FORMAT, frequency, "timer");
-	char *filename = xdg_config_home_systemd(local_timer_name);
-
+	char *filename = xdg_config_home_systemd("git-maintenance@.timer");
 	if (unlink(filename) && !is_missing_file_error(errno))
 		ret = error_errno(_("failed to delete '%s'"), filename);
+	FREE_AND_NULL(filename);
 
-	free(filename);
-	free(local_timer_name);
-	return ret;
-}
-
-static int systemd_timer_delete_service_template(void)
-{
-	int ret = 0;
-	char *local_service_name = xstrfmt(SYSTEMD_UNIT_FORMAT, "", "service");
-	char *filename = xdg_config_home_systemd(local_service_name);
+	filename = xdg_config_home_systemd("git-maintenance@.service");
 	if (unlink(filename) && !is_missing_file_error(errno))
 		ret = error_errno(_("failed to delete '%s'"), filename);
 
 	free(filename);
-	free(local_service_name);
 	return ret;
 }
 
-/*
- * Write the schedule information into a git-maintenance@<schedule>.timer
- * file using a custom minute. This timer file cannot use the templating
- * system, so we generate a specific file for each.
- */
-static int systemd_timer_write_timer_file(enum schedule_priority schedule,
-					  int minute)
+static int systemd_timer_write_unit_templates(const char *exec_path)
 {
-	int res = -1;
 	char *filename;
 	FILE *file;
 	const char *unit;
-	char *schedule_pattern = NULL;
-	const char *frequency = get_frequency(schedule);
-	char *local_timer_name = xstrfmt(SYSTEMD_UNIT_FORMAT, frequency, "timer");
-
-	filename = xdg_config_home_systemd(local_timer_name);
 
+	filename = xdg_config_home_systemd("git-maintenance@.timer");
 	if (safe_create_leading_directories(filename)) {
 		error(_("failed to create directories for '%s'"), filename);
 		goto error;
@@ -2364,23 +2339,6 @@ static int systemd_timer_write_timer_file(enum schedule_priority schedule,
 	if (!file)
 		goto error;
 
-	switch (schedule) {
-	case SCHEDULE_HOURLY:
-		schedule_pattern = xstrfmt("*-*-* 1..23:%02d:00", minute);
-		break;
-
-	case SCHEDULE_DAILY:
-		schedule_pattern = xstrfmt("Tue..Sun *-*-* 0:%02d:00", minute);
-		break;
-
-	case SCHEDULE_WEEKLY:
-		schedule_pattern = xstrfmt("Mon 0:%02d:00", minute);
-		break;
-
-	default:
-		BUG("Unhandled schedule_priority");
-	}
-
 	unit = "# This file was created and is maintained by Git.\n"
 	       "# Any edits made in this file might be replaced in the future\n"
 	       "# by a Git command.\n"
@@ -2389,12 +2347,14 @@ static int systemd_timer_write_timer_file(enum schedule_priority schedule,
 	       "Description=Optimize Git repositories data\n"
 	       "\n"
 	       "[Timer]\n"
-	       "OnCalendar=%s\n"
+	       "OnCalendar=%i\n"
 	       "Persistent=true\n"
+	       "RandomizedDelaySecond=3540\n"
+	       "FixedRandomDelay=true\n"
 	       "\n"
 	       "[Install]\n"
 	       "WantedBy=timers.target\n";
-	if (fprintf(file, unit, schedule_pattern) < 0) {
+	if (fputs(unit, file) == EOF) {
 		error(_("failed to write to '%s'"), filename);
 		fclose(file);
 		goto error;
@@ -2403,36 +2363,9 @@ static int systemd_timer_write_timer_file(enum schedule_priority schedule,
 		error_errno(_("failed to flush '%s'"), filename);
 		goto error;
 	}
-
-	res = 0;
-
-error:
-	free(schedule_pattern);
-	free(local_timer_name);
 	free(filename);
-	return res;
-}
 
-/*
- * No matter the schedule, we use the same service and can make use of the
- * templating system. When installing git-maintenance@<schedule>.timer,
- * systemd will notice that git-maintenance@.service exists as a template
- * and will use this file and insert the <schedule> into the template at
- * the position of "%i".
- */
-static int systemd_timer_write_service_template(const char *exec_path)
-{
-	int res = -1;
-	char *filename;
-	FILE *file;
-	const char *unit;
-	char *local_service_name = xstrfmt(SYSTEMD_UNIT_FORMAT, "", "service");
-
-	filename = xdg_config_home_systemd(local_service_name);
-	if (safe_create_leading_directories(filename)) {
-		error(_("failed to create directories for '%s'"), filename);
-		goto error;
-	}
+	filename = xdg_config_home_systemd("git-maintenance@.service");
 	file = fopen_or_warn(filename, "w");
 	if (!file)
 		goto error;
@@ -2465,18 +2398,17 @@ static int systemd_timer_write_service_template(const char *exec_path)
 		error_errno(_("failed to flush '%s'"), filename);
 		goto error;
 	}
-
-	res = 0;
+	free(filename);
+	return 0;
 
 error:
-	free(local_service_name);
 	free(filename);
-	return res;
+	systemd_timer_delete_unit_templates();
+	return -1;
 }
 
 static int systemd_timer_enable_unit(int enable,
-				     enum schedule_priority schedule,
-				     int minute)
+				     enum schedule_priority schedule)
 {
 	const char *cmd = "systemctl";
 	struct child_process child = CHILD_PROCESS_INIT;
@@ -2493,14 +2425,12 @@ static int systemd_timer_enable_unit(int enable,
 	 */
 	if (!enable)
 		child.no_stderr = 1;
-	else if (systemd_timer_write_timer_file(schedule, minute))
-		return -1;
 
 	get_schedule_cmd(&cmd, NULL);
 	strvec_split(&child.args, cmd);
 	strvec_pushl(&child.args, "--user", enable ? "enable" : "disable",
 		     "--now", NULL);
-	strvec_pushf(&child.args, SYSTEMD_UNIT_FORMAT, frequency, "timer");
+	strvec_pushf(&child.args, "git-maintenance@%s.timer", frequency);
 
 	if (start_command(&child))
 		return error(_("failed to start systemctl"));
@@ -2517,58 +2447,24 @@ static int systemd_timer_enable_unit(int enable,
 	return 0;
 }
 
-/*
- * A previous version of Git wrote the timer units as template files.
- * Clean these up, if they exist.
- */
-static void systemd_timer_delete_stale_timer_templates(void)
-{
-	char *timer_template_name = xstrfmt(SYSTEMD_UNIT_FORMAT, "", "timer");
-	char *filename = xdg_config_home_systemd(timer_template_name);
-
-	if (unlink(filename) && !is_missing_file_error(errno))
-		warning(_("failed to delete '%s'"), filename);
-
-	free(filename);
-	free(timer_template_name);
-}
-
-static int systemd_timer_delete_unit_files(void)
-{
-	systemd_timer_delete_stale_timer_templates();
-
-	/* Purposefully not short-circuited to make sure all are called. */
-	return systemd_timer_delete_timer_file(SCHEDULE_HOURLY) |
-	       systemd_timer_delete_timer_file(SCHEDULE_DAILY) |
-	       systemd_timer_delete_timer_file(SCHEDULE_WEEKLY) |
-	       systemd_timer_delete_service_template();
-}
-
 static int systemd_timer_delete_units(void)
 {
-	int minute = get_random_minute();
-	/* Purposefully not short-circuited to make sure all are called. */
-	return systemd_timer_enable_unit(0, SCHEDULE_HOURLY, minute) |
-	       systemd_timer_enable_unit(0, SCHEDULE_DAILY, minute) |
-	       systemd_timer_enable_unit(0, SCHEDULE_WEEKLY, minute) |
-	       systemd_timer_delete_unit_files();
+	return systemd_timer_enable_unit(0, SCHEDULE_HOURLY) ||
+	       systemd_timer_enable_unit(0, SCHEDULE_DAILY) ||
+	       systemd_timer_enable_unit(0, SCHEDULE_WEEKLY) ||
+	       systemd_timer_delete_unit_templates();
 }
 
 static int systemd_timer_setup_units(void)
 {
-	int minute = get_random_minute();
 	const char *exec_path = git_exec_path();
 
-	int ret = systemd_timer_write_service_template(exec_path) ||
-		  systemd_timer_enable_unit(1, SCHEDULE_HOURLY, minute) ||
-		  systemd_timer_enable_unit(1, SCHEDULE_DAILY, minute) ||
-		  systemd_timer_enable_unit(1, SCHEDULE_WEEKLY, minute);
-
+	int ret = systemd_timer_write_unit_templates(exec_path) ||
+		  systemd_timer_enable_unit(1, SCHEDULE_HOURLY) ||
+		  systemd_timer_enable_unit(1, SCHEDULE_DAILY) ||
+		  systemd_timer_enable_unit(1, SCHEDULE_WEEKLY);
 	if (ret)
 		systemd_timer_delete_units();
-	else
-		systemd_timer_delete_stale_timer_templates();
-
 	return ret;
 }
 
diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
index 0943dfa18a..37aa408d26 100755
--- a/t/t7900-maintenance.sh
+++ b/t/t7900-maintenance.sh
@@ -790,15 +790,7 @@ test_expect_success 'start and stop Linux/systemd maintenance' '
 	# start registers the repo
 	git config --get --global --fixed-value maintenance.repo "$(pwd)" &&
 
-	for schedule in hourly daily weekly
-	do
-		test_path_is_file "systemd/user/git-maintenance@$schedule.timer" || return 1
-	done &&
-	test_path_is_file "systemd/user/git-maintenance@.service" &&
-
-	test_systemd_analyze_verify "systemd/user/git-maintenance@hourly.service" &&
-	test_systemd_analyze_verify "systemd/user/git-maintenance@daily.service" &&
-	test_systemd_analyze_verify "systemd/user/git-maintenance@weekly.service" &&
+	test_systemd_analyze_verify "systemd/user/git-maintenance@.service" &&
 
 	printf -- "--user enable --now git-maintenance@%s.timer\n" hourly daily weekly >expect &&
 	test_cmp expect args &&
@@ -809,10 +801,7 @@ test_expect_success 'start and stop Linux/systemd maintenance' '
 	# stop does not unregister the repo
 	git config --get --global --fixed-value maintenance.repo "$(pwd)" &&
 
-	for schedule in hourly daily weekly
-	do
-		test_path_is_missing "systemd/user/git-maintenance@$schedule.timer" || return 1
-	done &&
+	test_path_is_missing "systemd/user/git-maintenance@.timer" &&
 	test_path_is_missing "systemd/user/git-maintenance@.service" &&
 
 	printf -- "--user disable --now git-maintenance@%s.timer\n" hourly daily weekly >expect &&
-- 
2.44.0

[PATCH v2 2/6] maintenance: use packaged systemd units

[Max Gautier]

Currently, `git maintenance start`, if it uses the systemd scheduler,
will create systemd user units in $XDG_CONFIG_HOME, and overwrite the
files on each invocation.
Conversely, `git maintenance stop` will remove the unit files.

This is problematic mostly because it steps on user toes (and their
configuration, also). You can't mask the git maintenance systemd units,
because they are in $XDG_CONFIG_HOME and you can only mask "vendor"
units (see man systemctl), you can't override them either.

Additionally, it necessitates creating, modifying and removing files
from builtin/gc.c, which adds code and some avoidable complexity.

Package the systemd user units (timer and service) with git in
$(prefix)/lib/systemd/user (or $XDG_DATA_HOME for $HOME installation),
and remove code for writing and deleting the units from builtin/gc.c.
Determine the correct git path at install time by for the service unit.

Detect systemd timers support (at install time) by relying on systemctl
presence, since we use it as the control interface for the systemd
scheduler.

Signed-off-by: Max Gautier <mg@max.gautier.name>
---

Notes:
    I'm not completely sure if it's ok to do install time templating like
    this, but I couldn't find a similar enough example in the Makefile. Any
    suggestion for a better way ?

 Makefile                                 |   5 +
 builtin/gc.c                             | 117 +----------------------
 config.mak.uname                         |  10 ++
 systemd/user/git-maintenance@.service.in |  17 ++++
 systemd/user/git-maintenance@.timer      |  12 +++
 5 files changed, 49 insertions(+), 112 deletions(-)
 create mode 100644 systemd/user/git-maintenance@.service.in
 create mode 100644 systemd/user/git-maintenance@.timer

diff --git a/Makefile b/Makefile
index 4e255c81f2..4fb015478e 100644
--- a/Makefile
+++ b/Makefile
@@ -3469,6 +3469,11 @@ install: all
 	$(INSTALL) -m 644 $(SCRIPT_LIB) '$(DESTDIR_SQ)$(gitexec_instdir_SQ)'
 	$(INSTALL) $(INSTALL_STRIP) $(install_bindir_xprograms) '$(DESTDIR_SQ)$(bindir_SQ)'
 	$(INSTALL) $(BINDIR_PROGRAMS_NO_X) '$(DESTDIR_SQ)$(bindir_SQ)'
+ifdef SYSTEMD_USER_UNIT_DIR
+	$(INSTALL) -Dm 644 -t '$(DESTDIR_SQ)$(SYSTEMD_USER_UNIT_DIR)' systemd/user/git-maintenance@.timer
+	sed 's+@BINDIR@+$(bindir_SQ)+' systemd/user/git-maintenance@.service.in | \
+		$(INSTALL) -Dm 644 /dev/stdin '$(DESTDIR_SQ)$(SYSTEMD_USER_UNIT_DIR)/git-maintenance@.service'
+endif
 
 ifdef MSVC
 	# We DO NOT install the individual foo.o.pdb files because they
diff --git a/builtin/gc.c b/builtin/gc.c
index dac59414f0..199c8e6240 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -2303,110 +2303,6 @@ static int is_systemd_timer_available(void)
 	return real_is_systemd_timer_available();
 }
 
-static char *xdg_config_home_systemd(const char *filename)
-{
-	return xdg_config_home_for("systemd/user", filename);
-}
-
-static int systemd_timer_delete_unit_templates(void)
-{
-	int ret = 0;
-	char *filename = xdg_config_home_systemd("git-maintenance@.timer");
-	if (unlink(filename) && !is_missing_file_error(errno))
-		ret = error_errno(_("failed to delete '%s'"), filename);
-	FREE_AND_NULL(filename);
-
-	filename = xdg_config_home_systemd("git-maintenance@.service");
-	if (unlink(filename) && !is_missing_file_error(errno))
-		ret = error_errno(_("failed to delete '%s'"), filename);
-
-	free(filename);
-	return ret;
-}
-
-static int systemd_timer_write_unit_templates(const char *exec_path)
-{
-	char *filename;
-	FILE *file;
-	const char *unit;
-
-	filename = xdg_config_home_systemd("git-maintenance@.timer");
-	if (safe_create_leading_directories(filename)) {
-		error(_("failed to create directories for '%s'"), filename);
-		goto error;
-	}
-	file = fopen_or_warn(filename, "w");
-	if (!file)
-		goto error;
-
-	unit = "# This file was created and is maintained by Git.\n"
-	       "# Any edits made in this file might be replaced in the future\n"
-	       "# by a Git command.\n"
-	       "\n"
-	       "[Unit]\n"
-	       "Description=Optimize Git repositories data\n"
-	       "\n"
-	       "[Timer]\n"
-	       "OnCalendar=%i\n"
-	       "Persistent=true\n"
-	       "RandomizedDelaySecond=3540\n"
-	       "FixedRandomDelay=true\n"
-	       "\n"
-	       "[Install]\n"
-	       "WantedBy=timers.target\n";
-	if (fputs(unit, file) == EOF) {
-		error(_("failed to write to '%s'"), filename);
-		fclose(file);
-		goto error;
-	}
-	if (fclose(file) == EOF) {
-		error_errno(_("failed to flush '%s'"), filename);
-		goto error;
-	}
-	free(filename);
-
-	filename = xdg_config_home_systemd("git-maintenance@.service");
-	file = fopen_or_warn(filename, "w");
-	if (!file)
-		goto error;
-
-	unit = "# This file was created and is maintained by Git.\n"
-	       "# Any edits made in this file might be replaced in the future\n"
-	       "# by a Git command.\n"
-	       "\n"
-	       "[Unit]\n"
-	       "Description=Optimize Git repositories data\n"
-	       "\n"
-	       "[Service]\n"
-	       "Type=oneshot\n"
-	       "ExecStart=\"%s/git\" --exec-path=\"%s\" for-each-repo --config=maintenance.repo maintenance run --schedule=%%i\n"
-	       "LockPersonality=yes\n"
-	       "MemoryDenyWriteExecute=yes\n"
-	       "NoNewPrivileges=yes\n"
-	       "RestrictAddressFamilies=AF_UNIX AF_INET AF_INET6 AF_VSOCK\n"
-	       "RestrictNamespaces=yes\n"
-	       "RestrictRealtime=yes\n"
-	       "RestrictSUIDSGID=yes\n"
-	       "SystemCallArchitectures=native\n"
-	       "SystemCallFilter=@system-service\n";
-	if (fprintf(file, unit, exec_path, exec_path) < 0) {
-		error(_("failed to write to '%s'"), filename);
-		fclose(file);
-		goto error;
-	}
-	if (fclose(file) == EOF) {
-		error_errno(_("failed to flush '%s'"), filename);
-		goto error;
-	}
-	free(filename);
-	return 0;
-
-error:
-	free(filename);
-	systemd_timer_delete_unit_templates();
-	return -1;
-}
-
 static int systemd_timer_enable_unit(int enable,
 				     enum schedule_priority schedule)
 {
@@ -2447,24 +2343,21 @@ static int systemd_timer_enable_unit(int enable,
 	return 0;
 }
 
-static int systemd_timer_delete_units(void)
+static int systemd_timer_disable_units(void)
 {
 	return systemd_timer_enable_unit(0, SCHEDULE_HOURLY) ||
 	       systemd_timer_enable_unit(0, SCHEDULE_DAILY) ||
-	       systemd_timer_enable_unit(0, SCHEDULE_WEEKLY) ||
-	       systemd_timer_delete_unit_templates();
+	       systemd_timer_enable_unit(0, SCHEDULE_WEEKLY);
 }
 
 static int systemd_timer_setup_units(void)
 {
-	const char *exec_path = git_exec_path();
 
-	int ret = systemd_timer_write_unit_templates(exec_path) ||
-		  systemd_timer_enable_unit(1, SCHEDULE_HOURLY) ||
+	int ret = systemd_timer_enable_unit(1, SCHEDULE_HOURLY) ||
 		  systemd_timer_enable_unit(1, SCHEDULE_DAILY) ||
 		  systemd_timer_enable_unit(1, SCHEDULE_WEEKLY);
 	if (ret)
-		systemd_timer_delete_units();
+		systemd_timer_disable_units();
 	return ret;
 }
 
@@ -2473,7 +2366,7 @@ static int systemd_timer_update_schedule(int run_maintenance, int fd UNUSED)
 	if (run_maintenance)
 		return systemd_timer_setup_units();
 	else
-		return systemd_timer_delete_units();
+		return systemd_timer_disable_units();
 }
 
 enum scheduler {
diff --git a/config.mak.uname b/config.mak.uname
index d0dcca2ec5..35ca236874 100644
--- a/config.mak.uname
+++ b/config.mak.uname
@@ -68,6 +68,16 @@ ifeq ($(uname_S),Linux)
 	ifneq ($(findstring .el7.,$(uname_R)),)
 		BASIC_CFLAGS += -std=c99
 	endif
+	ifeq ($(shell command -v systemctl >/dev/null ?&& echo y),y)
+		XDG_DATA_HOME ?= $(HOME)/.local/share
+		# systemd user units of programm installed in the home directory
+		# (meaning prefix == $HOME) shall go in XDG_DATA_HOME
+		# (from man 5 systemd.unit)
+		SYSTEMD_USER_UNIT_DIR = $(strip $(if $(and \
+			$(findstring $(prefix),$(HOME)),\
+			$(findstring $(HOME),$(prefix))),\
+			$(XDG_DATA_HOME),$(prefix)/$(lib)))/systemd/user
+	endif
 endif
 ifeq ($(uname_S),GNU/kFreeBSD)
 	HAVE_ALLOCA_H = YesPlease
diff --git a/systemd/user/git-maintenance@.service.in b/systemd/user/git-maintenance@.service.in
new file mode 100644
index 0000000000..649ba87e7e
--- /dev/null
+++ b/systemd/user/git-maintenance@.service.in
@@ -0,0 +1,17 @@
+[Unit]
+Description=Optimize Git repositories data
+Documentation=man:git-maintenance(1)
+
+[Service]
+Type=oneshot
+ExecStart=@BINDIR@/git for-each-repo --config=maintenance.repo \
+          maintenance run --schedule=%i
+LockPersonality=yes
+MemoryDenyWriteExecute=yes
+NoNewPrivileges=yes
+RestrictAddressFamilies=AF_UNIX AF_INET AF_INET6 AF_VSOCK
+RestrictNamespaces=yes
+RestrictRealtime=yes
+RestrictSUIDSGID=yes
+SystemCallArchitectures=native
+SystemCallFilter=@system-service
diff --git a/systemd/user/git-maintenance@.timer b/systemd/user/git-maintenance@.timer
new file mode 100644
index 0000000000..2834bac365
--- /dev/null
+++ b/systemd/user/git-maintenance@.timer
@@ -0,0 +1,12 @@
+[Unit]
+Description=Optimize Git repositories data
+Documentation=man:git-maintenance(1)
+
+[Timer]
+OnCalendar=%i
+Persistent=true
+RandomizedDelaySec=3540
+FixedRandomDelay=true
+
+[Install]
+WantedBy=timers.target
-- 
2.44.0

[PATCH v2 3/6] maintenance: simplify systemctl calls

[Max Gautier]

The systemctl invocation to enable or disable the git maintenance timers
is needlessly complicated:
- systemctl does not mind at all enabling already enabled units, nor
  disabling already disabled units.
- it can act on several units at once instead of only one.

Use only one systemctl invocation per `git maintenance start/stop`.
Transparently pass its status and output.
Add the --force option to override conflicting symlinks to previous
instances of our units files which lived in $XDG_CONFIG_HOME.

Signed-off-by: Max Gautier <mg@max.gautier.name>
---
 builtin/gc.c | 63 ++++++++++++----------------------------------------
 1 file changed, 14 insertions(+), 49 deletions(-)

diff --git a/builtin/gc.c b/builtin/gc.c
index 199c8e6240..aaee91451a 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -2303,70 +2303,35 @@ static int is_systemd_timer_available(void)
 	return real_is_systemd_timer_available();
 }
 
-static int systemd_timer_enable_unit(int enable,
-				     enum schedule_priority schedule)
+static int systemd_set_units_state(int enable)
 {
 	const char *cmd = "systemctl";
 	struct child_process child = CHILD_PROCESS_INIT;
-	const char *frequency = get_frequency(schedule);
-
-	/*
-	 * Disabling the systemd unit while it is already disabled makes
-	 * systemctl print an error.
-	 * Let's ignore it since it means we already are in the expected state:
-	 * the unit is disabled.
-	 *
-	 * On the other hand, enabling a systemd unit which is already enabled
-	 * produces no error.
-	 */
-	if (!enable)
-		child.no_stderr = 1;
 
 	get_schedule_cmd(&cmd, NULL);
 	strvec_split(&child.args, cmd);
-	strvec_pushl(&child.args, "--user", enable ? "enable" : "disable",
-		     "--now", NULL);
-	strvec_pushf(&child.args, "git-maintenance@%s.timer", frequency);
+
+	strvec_pushl(&child.args, "--user", "--force", "--now",
+			enable ? "enable" : "disable",
+			"git-maintenance@hourly.timer",
+			"git-maintenance@daily.timer",
+			"git-maintenance@weekly.timer", NULL);
+	/*
+	 * --force override existing conflicting symlinks
+	 * We need it because the units have changed location (~/.config ->
+	 * /usr/lib)
+	 */
 
 	if (start_command(&child))
 		return error(_("failed to start systemctl"));
 	if (finish_command(&child))
-		/*
-		 * Disabling an already disabled systemd unit makes
-		 * systemctl fail.
-		 * Let's ignore this failure.
-		 *
-		 * Enabling an enabled systemd unit doesn't fail.
-		 */
-		if (enable)
-			return error(_("failed to run systemctl"));
+		return error(_("failed to run systemctl"));
 	return 0;
 }
 
-static int systemd_timer_disable_units(void)
-{
-	return systemd_timer_enable_unit(0, SCHEDULE_HOURLY) ||
-	       systemd_timer_enable_unit(0, SCHEDULE_DAILY) ||
-	       systemd_timer_enable_unit(0, SCHEDULE_WEEKLY);
-}
-
-static int systemd_timer_setup_units(void)
-{
-
-	int ret = systemd_timer_enable_unit(1, SCHEDULE_HOURLY) ||
-		  systemd_timer_enable_unit(1, SCHEDULE_DAILY) ||
-		  systemd_timer_enable_unit(1, SCHEDULE_WEEKLY);
-	if (ret)
-		systemd_timer_disable_units();
-	return ret;
-}
-
 static int systemd_timer_update_schedule(int run_maintenance, int fd UNUSED)
 {
-	if (run_maintenance)
-		return systemd_timer_setup_units();
-	else
-		return systemd_timer_disable_units();
+	return systemd_set_units_state(run_maintenance);
 }
 
 enum scheduler {
-- 
2.44.0

[PATCH v2 4/6] maintenance: cleanup $XDG_CONFIG_HOME/systemd/user

[Max Gautier]

Before commit 976640edbb (maintenance: use packaged systemd units,
2024-03-21), we we're putting systemd unit files in $XDG_CONFIG_HOME ;
these could mask those we are now distributing as part of git.

Remove all the systemd units possibly created by previous version of
git when running `git maintenance start/stop`.
Avoid overwriting units we didn't write, by comparing the first line
with the start of the comment we added to our unit files previously.

Signed-off-by: Max Gautier <mg@max.gautier.name>
---

Notes:
    How should I refer to a commit which is part of the same patch series ?
    The commit id will change so the message won't be correct anymore, right
    ?

 builtin/gc.c | 42 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 42 insertions(+)

diff --git a/builtin/gc.c b/builtin/gc.c
index aaee91451a..99b158e481 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -2329,8 +2329,50 @@ static int systemd_set_units_state(int enable)
 	return 0;
 }
 
+/*
+ * TODO: in the future (~2026 ?) remove this cleanup code
+ */
+static void systemd_delete_user_unit(char const *unit)
+{
+	char const	file_start_stale[] =	"# This file was created and is"
+						" maintained by Git.";
+	char		file_start_user[sizeof(file_start_stale)] = {'\0'};
+
+	char *filename = xdg_config_home_for("systemd/user", unit);
+	int handle = open(filename, O_RDONLY);
+
+	/*
+	 * Check this is actually our file and we're not removing a legitimate
+	 * user override.
+	 */
+	if (handle == -1 && !is_missing_file_error(errno))
+		warning(_("failed to delete '%s'"), filename);
+	else {
+		read(handle, file_start_user, sizeof(file_start_stale) - 1);
+		close(handle);
+		if (strcmp(file_start_stale, file_start_user) == 0) {
+			if (unlink(filename) == 0)
+				warning(_("deleted stale unit file '%s'"), filename);
+			else if (!is_missing_file_error(errno))
+				warning(_("failed to delete '%s'"), filename);
+		}
+	}
+
+	free(filename);
+}
+
 static int systemd_timer_update_schedule(int run_maintenance, int fd UNUSED)
 {
+	/*
+	 * A previous version of Git wrote the units in the user configuration
+	 * directory. Clean these up, if they exist.
+	 * TODO: in the future (~2026 ?) remove this cleanup code
+	 */
+	systemd_delete_user_unit("git-maintenance@hourly.timer");
+	systemd_delete_user_unit("git-maintenance@daily.timer");
+	systemd_delete_user_unit("git-maintenance@weekly.timer");
+	systemd_delete_user_unit("git-maintenance@.timer");
+	systemd_delete_user_unit("git-maintenance@.service");
 	return systemd_set_units_state(run_maintenance);
 }
 
-- 
2.44.0

[PATCH v2 5/6] maintenance: update systemd scheduler docs

[Max Gautier]

The `git maintenance` systemd scheduler no longer writes units in
$XDG_CONFIG_HOME.

Describe the new behavior.
Instead of explaining manual ways to modify the timer, suggest the
systemd standard tool: `systemctl edit`.

Signed-off-by: Max Gautier <mg@max.gautier.name>
---
 Documentation/git-maintenance.txt | 33 +++++++++++++++----------------
 1 file changed, 16 insertions(+), 17 deletions(-)

diff --git a/Documentation/git-maintenance.txt b/Documentation/git-maintenance.txt
index 51d0f7e94b..6511c3f3f1 100644
--- a/Documentation/git-maintenance.txt
+++ b/Documentation/git-maintenance.txt
@@ -304,10 +304,9 @@ distributions, systemd timers are superseding it.
 If user systemd timers are available, they will be used as a replacement
 of `cron`.
 
-In this case, `git maintenance start` will create user systemd timer units
-and start the timers. The current list of user-scheduled tasks can be found
-by running `systemctl --user list-timers`. The timers written by `git
-maintenance start` are similar to this:
+In this case, `git maintenance start` will enable user systemd timer units
+and start them. The current list of user-scheduled tasks can be found by
+running `systemctl --user list-timers`. These timers are similar to this:
 
 -----------------------------------------------------------------------
 $ systemctl --user list-timers
@@ -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
 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
 -----------------------------------------------------------------------
 
-One timer is registered for each `--schedule=<frequency>` option.
+One timer instance is enabled for each `--schedule=<frequency>` option.
 
-The definition of the systemd units can be inspected in the following files:
+The definition of the systemd units can be inspected this way:
 
 -----------------------------------------------------------------------
-~/.config/systemd/user/git-maintenance@.timer
-~/.config/systemd/user/git-maintenance@.service
-~/.config/systemd/user/timers.target.wants/git-maintenance@hourly.timer
-~/.config/systemd/user/timers.target.wants/git-maintenance@daily.timer
-~/.config/systemd/user/timers.target.wants/git-maintenance@weekly.timer
+$ systemctl cat --user git-maintenance@.timer
+$ systemctl cat --user git-maintenance@.service
 -----------------------------------------------------------------------
 
-`git maintenance start` will overwrite these files and start the timer
-again with `systemctl --user`, so any customization should be done by
-creating a drop-in file, i.e. a `.conf` suffixed file in the
-`~/.config/systemd/user/git-maintenance@.service.d` directory.
+Customization of the timer or service can be performed with the usual systemd
+tooling:
+-----------------------------------------------------------------------
+$ systemctl edit --user git-maintenance@.timer # all the timers
+$ systemctl edit --user git-maintenance@hourly.timer # the hourly timer
+$ systemctl edit --user git-maintenance@.service # all the services
+$ systemctl edit --user git-maintenance@hourly.service # the hourly run
+-----------------------------------------------------------------------
 
-`git maintenance stop` will stop the user systemd timers and delete
-the above mentioned files.
+`git maintenance stop` will disable and stop the user systemd timers.
 
 For more details, see `systemd.timer(5)`.
 
-- 
2.44.0

[PATCH v2 6/6] maintenance: update tests for systemd scheduler

[Max Gautier]

The systemd units are now in the source tree, rather than produced when
running git maitenance start. There is no need anymore to couple
validating the units and testing `git maintenance start`.

Adjust the test to verify the new `systemctl` command used, discard
checks for presence/absence of unit files in $XDG_CONFIG_HOME.

Validate the systemd units in the source tree, with one test per unit to
have more distinct failures.

Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Max Gautier <mg@max.gautier.name>
---

Notes:
    I encountered the same issue that
    cover.1697319294.git.code@khaugsbakk.name tried to solve; I was
    expecting multiples tests to be independent while they weren't (in
    particular the MacOS ones).
    
    Cc: Kristoffer Haugsbakk <code@khaugsbakk.name>

 t/t7900-maintenance.sh | 39 +++++++++++++++++++--------------------
 1 file changed, 19 insertions(+), 20 deletions(-)

diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
index 37aa408d26..dc0bf39250 100755
--- a/t/t7900-maintenance.sh
+++ b/t/t7900-maintenance.sh
@@ -24,13 +24,6 @@ test_lazy_prereq SYSTEMD_ANALYZE '
 	systemd-analyze verify /lib/systemd/system/basic.target
 '
 
-test_systemd_analyze_verify () {
-	if test_have_prereq SYSTEMD_ANALYZE
-	then
-		systemd-analyze verify "$@"
-	fi
-}
-
 test_expect_success 'help text' '
 	test_expect_code 129 git maintenance -h >actual &&
 	test_grep "usage: git maintenance <subcommand>" actual &&
@@ -776,23 +769,32 @@ test_expect_success 'start and stop Windows maintenance' '
 		hourly daily weekly >expect &&
 	test_cmp expect args
 '
+test_expect_success SYSTEMD_ANALYZE 'validate maintenance systemd service unit' '
+	git_path=$(command -v git) &&
+	sed "s+@BINDIR@/git+${git_path}+" "$TEST_DIRECTORY"/../systemd/user/git-maintenance@.service.in > git-maintenance@.service &&
+	systemd-analyze verify git-maintenance@hourly.service git-maintenance@daily.service git-maintenance@weekly.service &&
+	rm git-maintenance@.service &&
+	unset git_path
+'
+
+test_expect_success SYSTEMD_ANALYZE 'validate maintenance systemd timer unit' '
+	SYSTEMD_UNIT_PATH="$TEST_DIRECTORY"/../systemd/user/: systemd-analyze verify git-maintenance@hourly.timer git-maintenance@daily.timer git-maintenance@weekly.timer
+	# ':' at the end of SYSTEMD_UNIT_PATH appends the default systemd search path
+	# This is needed because analyze tries to load implicit / default unit dependencies
+'
 
 test_expect_success 'start and stop Linux/systemd maintenance' '
 	write_script print-args <<-\EOF &&
 	printf "%s\n" "$*" >>args
 	EOF
 
-	XDG_CONFIG_HOME="$PWD" &&
-	export XDG_CONFIG_HOME &&
 	rm -f args &&
 	GIT_TEST_MAINT_SCHEDULER="systemctl:./print-args" git maintenance start --scheduler=systemd-timer &&
 
 	# start registers the repo
 	git config --get --global --fixed-value maintenance.repo "$(pwd)" &&
 
-	test_systemd_analyze_verify "systemd/user/git-maintenance@.service" &&
-
-	printf -- "--user enable --now git-maintenance@%s.timer\n" hourly daily weekly >expect &&
+	echo "--user --force --now enable" git-maintenance@hourly.timer git-maintenance@daily.timer git-maintenance@weekly.timer >expect &&
 	test_cmp expect args &&
 
 	rm -f args &&
@@ -801,10 +803,7 @@ test_expect_success 'start and stop Linux/systemd maintenance' '
 	# stop does not unregister the repo
 	git config --get --global --fixed-value maintenance.repo "$(pwd)" &&
 
-	test_path_is_missing "systemd/user/git-maintenance@.timer" &&
-	test_path_is_missing "systemd/user/git-maintenance@.service" &&
-
-	printf -- "--user disable --now git-maintenance@%s.timer\n" hourly daily weekly >expect &&
+	echo "--user --force --now disable" git-maintenance@hourly.timer git-maintenance@daily.timer git-maintenance@weekly.timer >expect &&
 	test_cmp expect args
 '
 
@@ -819,12 +818,12 @@ test_expect_success 'start and stop when several schedulers are available' '
 		hourly daily weekly >expect &&
 	printf "schtasks /delete /tn Git Maintenance (%s) /f\n" \
 		hourly daily weekly >>expect &&
-	printf -- "systemctl --user enable --now git-maintenance@%s.timer\n" hourly daily weekly >>expect &&
+	echo "systemctl --user --force --now enable" git-maintenance@hourly.timer git-maintenance@daily.timer git-maintenance@weekly.timer >>expect &&
 	test_cmp expect args &&
 
 	rm -f args &&
 	GIT_TEST_MAINT_SCHEDULER="systemctl:./print-args systemctl,launchctl:./print-args launchctl,schtasks:./print-args schtasks" git maintenance start --scheduler=launchctl &&
-	printf -- "systemctl --user disable --now git-maintenance@%s.timer\n" hourly daily weekly >expect &&
+	echo "systemctl --user --force --now disable" git-maintenance@hourly.timer git-maintenance@daily.timer git-maintenance@weekly.timer >expect &&
 	printf "schtasks /delete /tn Git Maintenance (%s) /f\n" \
 		hourly daily weekly >>expect &&
 	for frequency in hourly daily weekly
@@ -837,7 +836,7 @@ test_expect_success 'start and stop when several schedulers are available' '
 
 	rm -f args &&
 	GIT_TEST_MAINT_SCHEDULER="systemctl:./print-args systemctl,launchctl:./print-args launchctl,schtasks:./print-args schtasks" git maintenance start --scheduler=schtasks &&
-	printf -- "systemctl --user disable --now git-maintenance@%s.timer\n" hourly daily weekly >expect &&
+	echo "systemctl --user --force --now disable" git-maintenance@hourly.timer git-maintenance@daily.timer git-maintenance@weekly.timer >expect &&
 	printf "launchctl bootout gui/[UID] $pfx/Library/LaunchAgents/org.git-scm.git.%s.plist\n" \
 		hourly daily weekly >>expect &&
 	printf "schtasks /create /tn Git Maintenance (%s) /f /xml\n" \
@@ -846,7 +845,7 @@ test_expect_success 'start and stop when several schedulers are available' '
 
 	rm -f args &&
 	GIT_TEST_MAINT_SCHEDULER="systemctl:./print-args systemctl,launchctl:./print-args launchctl,schtasks:./print-args schtasks" git maintenance stop &&
-	printf -- "systemctl --user disable --now git-maintenance@%s.timer\n" hourly daily weekly >expect &&
+	echo "systemctl --user --force --now disable" git-maintenance@hourly.timer git-maintenance@daily.timer git-maintenance@weekly.timer >expect &&
 	printf "launchctl bootout gui/[UID] $pfx/Library/LaunchAgents/org.git-scm.git.%s.plist\n" \
 		hourly daily weekly >>expect &&
 	printf "schtasks /delete /tn Git Maintenance (%s) /f\n" \
-- 
2.44.0

Re: [PATCH v2 4/6] maintenance: cleanup $XDG_CONFIG_HOME/systemd/user

[Kristoffer Haugsbakk]

On Fri, Mar 22, 2024, at 23:11, Max Gautier wrote:
> Notes:
>     How should I refer to a commit which is part of the same patch series ?
>     The commit id will change so the message won't be correct anymore, right
>     ?

It looks like a fair few say “in the previous commit” for the one just
before this one and “in a previous commit” for some commit that was
before this in the series but not the immediate previous one. I guess
that’s okay, no?

-- 
Kristoffer Haugsbakk

Re: [PATCH v2 4/6] maintenance: cleanup $XDG_CONFIG_HOME/systemd/user

[Junio C Hamano]

"Kristoffer Haugsbakk" <code@khaugsbakk.name> writes:

> On Fri, Mar 22, 2024, at 23:11, Max Gautier wrote:
>> Notes:
>>     How should I refer to a commit which is part of the same patch series ?
>>     The commit id will change so the message won't be correct anymore, right
>>     ?
>
> It looks like a fair few say “in the previous commit” for the one just
> before this one and “in a previous commit” for some commit that was
> before this in the series but not the immediate previous one. I guess
> that’s okay, no?

I saw some series say "Earlier in the series, X has learned to do Y,
so now we take advantage of that new feature", and found it quite
readable.

Re: [PATCH v2 6/6] maintenance: update tests for systemd scheduler

[Eric Sunshine]

On Fri, Mar 22, 2024 at 6:13 PM Max Gautier <mg@max.gautier.name> wrote:
> The systemd units are now in the source tree, rather than produced when
> running git maitenance start. There is no need anymore to couple
> validating the units and testing `git maintenance start`.

s/maitenance/maintenance/

> Adjust the test to verify the new `systemctl` command used, discard
> checks for presence/absence of unit files in $XDG_CONFIG_HOME.
>
> Validate the systemd units in the source tree, with one test per unit to
> have more distinct failures.
>
> Signed-off-by: Max Gautier <mg@max.gautier.name>

In a patch series, in order to preserve "bisectability", we want to
ensure that the entire test suite continues to pass after each patch
is applied. A such, we normally update tests -- to ensure that they
continue passing -- in each patch which changes some
testable/observable behavior. However, this series only updates test
in the final patch. Doesn't that break bisectability? Or am I
misunderstanding something?

Re: [PATCH v2 3/6] maintenance: simplify systemctl calls

[Eric Sunshine]

On Fri, Mar 22, 2024 at 6:13 PM Max Gautier <mg@max.gautier.name> wrote:
> The systemctl invocation to enable or disable the git maintenance timers
> is needlessly complicated:
> - systemctl does not mind at all enabling already enabled units, nor
>   disabling already disabled units.

Has systemctl behavior changed...

> Use only one systemctl invocation per `git maintenance start/stop`.
> Transparently pass its status and output.
> Add the --force option to override conflicting symlinks to previous
> instances of our units files which lived in $XDG_CONFIG_HOME.
>
> Signed-off-by: Max Gautier <mg@max.gautier.name>
> ---
> diff --git a/builtin/gc.c b/builtin/gc.c
> @@ -2303,70 +2303,35 @@ static int is_systemd_timer_available(void)
> -        * Disabling the systemd unit while it is already disabled makes
> -        * systemctl print an error.
> -        * Let's ignore it since it means we already are in the expected state:
> -        * the unit is disabled.

... since this and...

> -                * Disabling an already disabled systemd unit makes
> -                * systemctl fail.
> -                * Let's ignore this failure.

... this were written?

If so, do we still need to worry about older systems in which
systemctl prints errors and/or fails outright?

Re: [PATCH v2 2/6] maintenance: use packaged systemd units

[Eric Sunshine]

On Sat, Mar 23, 2024 at 4:21 AM Max Gautier <mg@max.gautier.name> wrote:
> [...]
> Package the systemd user units (timer and service) with git in
> $(prefix)/lib/systemd/user (or $XDG_DATA_HOME for $HOME installation),
> and remove code for writing and deleting the units from builtin/gc.c.
> Determine the correct git path at install time by for the service unit.
>
> Detect systemd timers support (at install time) by relying on systemctl
> presence, since we use it as the control interface for the systemd
> scheduler.
>
> Signed-off-by: Max Gautier <mg@max.gautier.name>
> ---
> Notes:
>     I'm not completely sure if it's ok to do install time templating like
>     this, but I couldn't find a similar enough example in the Makefile. Any
>     suggestion for a better way ?
>
> diff --git a/Makefile b/Makefile
> @@ -3469,6 +3469,11 @@ install: all
> +ifdef SYSTEMD_USER_UNIT_DIR
> +       $(INSTALL) -Dm 644 -t '$(DESTDIR_SQ)$(SYSTEMD_USER_UNIT_DIR)' systemd/user/git-maintenance@.timer
> +       sed 's+@BINDIR@+$(bindir_SQ)+' systemd/user/git-maintenance@.service.in | \
> +               $(INSTALL) -Dm 644 /dev/stdin '$(DESTDIR_SQ)$(SYSTEMD_USER_UNIT_DIR)/git-maintenance@.service'
> +endif

This is the first use of /dev/stdin in the project and I might worry a
bit about portability. Granted, a system in which systemd is installed
is likely to have /dev/stdin available, but it's often a good idea to
be cautious when introducing something new into the project.

I would think it would be possible to instead generate the
`git-maintenance@.service` file locally from the template
`git-maintenance@.service.in` as part of the normal build process, and
then install the built `git-maintenance@.service` at "install" time.
That seems more in line with how other resources are handled, avoids
the novel use of /dev/stdin, and answers the question you ask above.

> diff --git a/config.mak.uname b/config.mak.uname
> @@ -68,6 +68,16 @@ ifeq ($(uname_S),Linux)
> +       ifeq ($(shell command -v systemctl >/dev/null ?&& echo y),y)

What is "?&&"?

> +               XDG_DATA_HOME ?= $(HOME)/.local/share
> +               # systemd user units of programm installed in the home directory
> +               # (meaning prefix == $HOME) shall go in XDG_DATA_HOME
> +               # (from man 5 systemd.unit)

s/programm/program/

Re: [PATCH v2 2/6] maintenance: use packaged systemd units

[Max Gautier]

On Sat, Mar 23, 2024 at 04:38:44AM -0400, Eric Sunshine wrote:
> On Sat, Mar 23, 2024 at 4:21 AM Max Gautier <mg@max.gautier.name> wrote:
> > [...]
> > Package the systemd user units (timer and service) with git in
> > $(prefix)/lib/systemd/user (or $XDG_DATA_HOME for $HOME installation),
> > and remove code for writing and deleting the units from builtin/gc.c.
> > Determine the correct git path at install time by for the service unit.
> >
> > Detect systemd timers support (at install time) by relying on systemctl
> > presence, since we use it as the control interface for the systemd
> > scheduler.
> >
> > Signed-off-by: Max Gautier <mg@max.gautier.name>
> > ---
> > Notes:
> >     I'm not completely sure if it's ok to do install time templating like
> >     this, but I couldn't find a similar enough example in the Makefile. Any
> >     suggestion for a better way ?
> >
> > diff --git a/Makefile b/Makefile
> > @@ -3469,6 +3469,11 @@ install: all
> > +ifdef SYSTEMD_USER_UNIT_DIR
> > +       $(INSTALL) -Dm 644 -t '$(DESTDIR_SQ)$(SYSTEMD_USER_UNIT_DIR)' systemd/user/git-maintenance@.timer
> > +       sed 's+@BINDIR@+$(bindir_SQ)+' systemd/user/git-maintenance@.service.in | \
> > +               $(INSTALL) -Dm 644 /dev/stdin '$(DESTDIR_SQ)$(SYSTEMD_USER_UNIT_DIR)/git-maintenance@.service'
> > +endif
> 
> This is the first use of /dev/stdin in the project and I might worry a
> bit about portability. Granted, a system in which systemd is installed
> is likely to have /dev/stdin available, but it's often a good idea to
> be cautious when introducing something new into the project.
> 
> I would think it would be possible to instead generate the
> `git-maintenance@.service` file locally from the template
> `git-maintenance@.service.in` as part of the normal build process, and
> then install the built `git-maintenance@.service` at "install" time.
> That seems more in line with how other resources are handled, avoids
> the novel use of /dev/stdin, and answers the question you ask above.

Ok.  It's not completely obvious to me how the "Detect prefix change
logic" works, but using other rules as a model, I think I can do
something like that:
systemd/user/git-maintenance@.service: systemd/user/git-maintenance@.service.in GIT-PREFIX
    sed 's+@BINDIR@+$(bindir_SQ)+' $< > $@

and depending on GIT-PREFIX should regenerate the service when changing
the prefix, correct ?
I'll need to add that to .gitignore as well.

> 
> > diff --git a/config.mak.uname b/config.mak.uname
> > @@ -68,6 +68,16 @@ ifeq ($(uname_S),Linux)
> > +       ifeq ($(shell command -v systemctl >/dev/null ?&& echo y),y)
> 
> What is "?&&"?
> 

Hum, a typo, sorry that slipped through. It apparently works regardless,
because shell expansion does something with it I guess. Curious. I'll
clean that up as well.

> > +               XDG_DATA_HOME ?= $(HOME)/.local/share
> > +               # systemd user units of programm installed in the home directory
> > +               # (meaning prefix == $HOME) shall go in XDG_DATA_HOME
> > +               # (from man 5 systemd.unit)
> 
> s/programm/program/
Ack.
That should even be 'programs' I think, that's a general rule.

-- 
Max Gautier

Re: [PATCH v2 3/6] maintenance: simplify systemctl calls

[Max Gautier]

On Fri, Mar 22, 2024 at 07:09:18PM -0400, Eric Sunshine wrote:
> On Fri, Mar 22, 2024 at 6:13 PM Max Gautier <mg@max.gautier.name> wrote:
> > The systemctl invocation to enable or disable the git maintenance timers
> > is needlessly complicated:
> > - systemctl does not mind at all enabling already enabled units, nor
> >   disabling already disabled units.
> 
> Has systemctl behavior changed...
> 
> > Use only one systemctl invocation per `git maintenance start/stop`.
> > Transparently pass its status and output.
> > Add the --force option to override conflicting symlinks to previous
> > instances of our units files which lived in $XDG_CONFIG_HOME.
> >
> > Signed-off-by: Max Gautier <mg@max.gautier.name>
> > ---
> > diff --git a/builtin/gc.c b/builtin/gc.c
> > @@ -2303,70 +2303,35 @@ static int is_systemd_timer_available(void)
> > -        * Disabling the systemd unit while it is already disabled makes
> > -        * systemctl print an error.
> > -        * Let's ignore it since it means we already are in the expected state:
> > -        * the unit is disabled.
> 
> ... since this and...
> 
> > -                * Disabling an already disabled systemd unit makes
> > -                * systemctl fail.
> > -                * Let's ignore this failure.
> 
> ... this were written?
> 
> If so, do we still need to worry about older systems in which
> systemctl prints errors and/or fails outright?

I tried the following on systemd source
$ git log -L :do_unit_file_disable:src/shared/install.c
$ git log -L :unit_file_disable:src/shared/install.c`
$ git log --grep 'already disabled'
$ git log --grep 're disable'
$ git log --grep 'disabled unit'
$ git log --grep 'systemctl disable'

That yields nothing indicating a change (in the commit messages, I
didn't look at the code in detail) when disabling already disabled
units, and testing on systemd 255 disable already disabled units (normal
and templated, same thing) without complaining or an error status.
Nothing relevant I could find on github in the issues or PRs either.

systemctl does error out or print warnings in other conditions, like
missing [Install] section, but that's not something we should ignore.

Philip Wood asked that question here in the original thread:

> What is the exit code of systemctl if a unit is already enabled and we 
> try to enbale it again (and the same for disabling a disabled unit)?
https://lore.kernel.org/git/3fd17223-8667-24be-2e65-f1970d411bdf@gmail.com/

But I can't find a follow-up email from Lénaïc, and searching for
"disabled" in the whole thread only yield the comment I'm removing.  Not
sure if Lénaïc still follows the list, but maybe he can comment on that
?
To me it looks like it was not necessary to begin with, but I might have 
just missed the discussion about it.

Adding Phillip to the discussion as well.

-- 
Max Gautier

Re: [PATCH v2 6/6] maintenance: update tests for systemd scheduler

[Max Gautier]

On Fri, Mar 22, 2024 at 07:02:36PM -0400, Eric Sunshine wrote:
> On Fri, Mar 22, 2024 at 6:13 PM Max Gautier <mg@max.gautier.name> wrote:
> > The systemd units are now in the source tree, rather than produced when
> > running git maitenance start. There is no need anymore to couple
> > validating the units and testing `git maintenance start`.
> 
> s/maitenance/maintenance/
> 
Ack
> > Adjust the test to verify the new `systemctl` command used, discard
> > checks for presence/absence of unit files in $XDG_CONFIG_HOME.
> >
> > Validate the systemd units in the source tree, with one test per unit to
> > have more distinct failures.
> >
> > Signed-off-by: Max Gautier <mg@max.gautier.name>
> 
> In a patch series, in order to preserve "bisectability", we want to
> ensure that the entire test suite continues to pass after each patch
> is applied. A such, we normally update tests -- to ensure that they
> continue passing -- in each patch which changes some
> testable/observable behavior. However, this series only updates test
> in the final patch. Doesn't that break bisectability? Or am I
> misunderstanding something?

No you're right, good point. I'll split this one up and fold the parts
where the behavior was changed.
And I'll keep that in mind in the future.

-- 
Max Gautier

Re: [PATCH v2 4/6] maintenance: cleanup $XDG_CONFIG_HOME/systemd/user

[Max Gautier]

On Fri, Mar 22, 2024 at 11:11:09PM +0100, Max Gautier wrote:
>  
> +/*
> + * TODO: in the future (~2026 ?) remove this cleanup code
> + */
> +static void systemd_delete_user_unit(char const *unit)
> +{
> +	char const	file_start_stale[] =	"# This file was created and is"
> +						" maintained by Git.";
> +	char		file_start_user[sizeof(file_start_stale)] = {'\0'};
> +
> +	char *filename = xdg_config_home_for("systemd/user", unit);
> +	int handle = open(filename, O_RDONLY);
> +
> +	/*
> +	 * Check this is actually our file and we're not removing a legitimate
> +	 * user override.
> +	 */
> +	if (handle == -1 && !is_missing_file_error(errno))
> +		warning(_("failed to delete '%s'"), filename);
> +	else {
> +		read(handle, file_start_user, sizeof(file_start_stale) - 1);

Actually that fails -Werror because I don't check read return.
Alternative below (on top of this one), with one question:
Are VLA using size_t const OK ? It's folded to a constant array by gcc
but I don't know if that causes portability problem with other platforms
? I can always repeat the sizeof expr if it's a problematic construct.

-- >8 --

diff --git a/builtin/gc.c b/builtin/gc.c
index 99b158e481..7fb25ea2b1 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -2332,11 +2332,14 @@ static int systemd_set_units_state(int enable)
 /*
  * TODO: in the future (~2026 ?) remove this cleanup code
  */
+
 static void systemd_delete_user_unit(char const *unit)
 {
 	char const	file_start_stale[] =	"# This file was created and is"
 						" maintained by Git.";
-	char		file_start_user[sizeof(file_start_stale)] = {'\0'};
+	size_t const	length = sizeof(file_start_stale);
+	char		file_start_user[length] = {'\0'};
+
 
 	char *filename = xdg_config_home_for("systemd/user", unit);
 	int handle = open(filename, O_RDONLY);
@@ -2348,14 +2351,14 @@ static void systemd_delete_user_unit(char const *unit)
 	if (handle == -1 && !is_missing_file_error(errno))
 		warning(_("failed to delete '%s'"), filename);
 	else {
-		read(handle, file_start_user, sizeof(file_start_stale) - 1);
-		close(handle);
-		if (strcmp(file_start_stale, file_start_user) == 0) {
+		if (length - 1 == read(handle, file_start_user, length - 1) &&
+				strcmp(file_start_stale, file_start_user) == 0) {
 			if (unlink(filename) == 0)
 				warning(_("deleted stale unit file '%s'"), filename);
 			else if (!is_missing_file_error(errno))
 				warning(_("failed to delete '%s'"), filename);
 		}
+		close(handle);
 	}
 
 	free(filename);

-- 
Max Gautier

Re: [PATCH v2 0/6] maintenance: use packaged systemd units

[Phillip Wood]

Hi Max

On 22/03/2024 22:11, Max Gautier wrote:
> * Distribute the systemd timers used by the `git maintenance start` with
>    the systemd scheduler as part of git, rather than writing them in
>    $XDG_CONFIG_HOME.
> 
> This allows users to override the units if they wish, and is more
> in-line with the usual practices of distribution for systemd units.

Thanks for suggesting this, I think this is a useful change, but the 
implementation could be improved.

> We also move away from using the random minute, and instead rely on
> systemd features to achieve the same goal (see patch 2). This allows us
> to go back to using unit templating for the timers. This is also a
> prerequisite to have static unit files.
> 
> Note that even if we really need more specific OnCalendar= settings for
> each timer, we should still do it that way, but instead distribute
> override alongside the template, for instance for weekly:
> 
> /usr/lib/systemd-user/git-maintenance@daily.timer.d/override.conf:
> [Timer]
> OnCalendar=<daily specific calendar spec>

We should definitely do that. Using systemd's random delay does not 
prevent the different maintenance jobs from running concurrently as one 
job may be started before a previous job has finished. It is important 
to only have one job running at a time because the first thing "git 
maintenance run" does is to try and acquire a lock file so if the hourly 
job is running when the daily jobs tries to start the daily job will not 
be run. As the daily job is a superset of the hourly job and the weekly 
job is a superset of the daily job so it does not make sense to run more 
than one job per hour.

> The cleanup code for the units written in $XDG_CONFIG_HOME is adapted,
> and takes care of not removing legitimate user overrides, by checking
> the file start.

This points to an alternate strategy for supporting user overrides - 
don't overwrite the unit files if the user has edited them. I think that 
there is still a benefit to moving to system wide unit files though as 
it means that if we improve the unit files in the future systemd will 
pick up these improvements automatically. That is an improvement over 
the status quo where the users' unit files are written once and never 
updated.

I think it would help to reorder the changes in this series as follows:

1 - simplify the invocation of "systemctl --user"
   This would be the current patch 3 without adding "--force" or
   moving "--now" combined with the relevant test changes from patch 6.
   This would make it clear that those changes are a simple clean up that
   is independent of the other changes made in this series.

2 - don't delete user edited unit files
   This would be based on the current patch 4 and would show that we can
   avoid deleting unit files that the user has edited without the other
   changes in this series. This change should have an associated test.

3 - start using systemd's random delay function
   This would be the current patch 1 without the template changes and the
   commit message should explain that it is in preparation for disturbing
   system-wide unit files.

4 - install system-wide systemd unit files
   This would be based on the current patch 2 with the addition of
   overrides to prevent more than one job running per hour. The unit
   files should be installed under $XDG_DATA_HOME when $(prefix) starts
   with $(HOME), not just when they are equal. The associated test
   changes from patch 6 should be moved here as well as the "--force"
   change from patch 3.

5 - documentation updates
   I'm on the fence about having these in a separate commit like the
   current patch 5 or updating the documentation when the code is
   changed.

Best Wishes

Phillip

> Testing:
> The simplest way to simulate having the units in /usr/lib is probably to
> copy them in /etc/systemd/user.
> 
> Changes since v1:
> - Reorganization of the commits and their messages to try to address
>    review comments
> - Dropped the DON'T APPLY PATCH, added a TODO to the cleanup code
>    instead
> - Updated the git-maintenance tests to work with the new logic.
> - Conditional installation of the units files
> - Fixing some style/consistency issues
> - template the systemd service file to use $(bindir)
> 
> Max Gautier (6):
>    maintenance: use systemd timers builtin randomization
>    maintenance: use packaged systemd units
>    maintenance: simplify systemctl calls
>    maintenance: cleanup $XDG_CONFIG_HOME/systemd/user
>    maintenance: update systemd scheduler docs
>    maintenance: update tests for systemd scheduler
> 
>   Documentation/git-maintenance.txt        |  33 ++-
>   Makefile                                 |   5 +
>   builtin/gc.c                             | 298 ++++-------------------
>   config.mak.uname                         |  10 +
>   systemd/user/git-maintenance@.service.in |  17 ++
>   systemd/user/git-maintenance@.timer      |  12 +
>   t/t7900-maintenance.sh                   |  50 ++--
>   7 files changed, 126 insertions(+), 299 deletions(-)
>   create mode 100644 systemd/user/git-maintenance@.service.in
>   create mode 100644 systemd/user/git-maintenance@.timer
> 
> Range-diff against v1:
> 1:  ea54a6e50e < -:  ---------- maintenance: package systemd units
> 2:  b29dbb9fdd < -:  ---------- maintenance: use packaged systemd units
> 3:  47bd6712b8 < -:  ---------- maintenance: add fixed random delay to systemd timers
> -:  ---------- > 1:  42d88c7f81 maintenance: use systemd timers builtin randomization
> -:  ---------- > 2:  18d51b1dd1 maintenance: use packaged systemd units
> -:  ---------- > 3:  3aa7446e95 maintenance: simplify systemctl calls
> -:  ---------- > 4:  daff7b4d60 maintenance: cleanup $XDG_CONFIG_HOME/systemd/user
> 4:  fac57db55e ! 5:  5f6a8e141f maintenance: update systemd scheduler docs
>      @@ Metadata
>        ## Commit message ##
>           maintenance: update systemd scheduler docs
>       
>      +    The `git maintenance` systemd scheduler no longer writes units in
>      +    $XDG_CONFIG_HOME.
>      +
>      +    Describe the new behavior.
>      +    Instead of explaining manual ways to modify the timer, suggest the
>      +    systemd standard tool: `systemctl edit`.
>      +
>           Signed-off-by: Max Gautier <mg@max.gautier.name>
>       
>        ## Documentation/git-maintenance.txt ##
> 5:  d888fbd0c3 < -:  ---------- DON'T APPLY YET: maintenance: remove cleanup code
> -:  ---------- > 6:  4d4bcd6233 maintenance: update tests for systemd scheduler

Re: [PATCH v2 4/6] maintenance: cleanup $XDG_CONFIG_HOME/systemd/user

[Phillip Wood]

Hi Max

On 23/03/2024 11:07, Max Gautier wrote:
> On Fri, Mar 22, 2024 at 11:11:09PM +0100, Max Gautier wrote:
>>   
>> +/*
>> + * TODO: in the future (~2026 ?) remove this cleanup code
>> + */

That is rather optimistic - users only run "git maintenance start" once 
so any unit files that have been written in the past will exist well 
beyond 2026.

>> +static void systemd_delete_user_unit(char const *unit)
>> +{
>> +	char const	file_start_stale[] =	"# This file was created and is"
>> +						" maintained by Git.";
>> +	char		file_start_user[sizeof(file_start_stale)] = {'\0'};
>> +
>> +	char *filename = xdg_config_home_for("systemd/user", unit);
>> +	int handle = open(filename, O_RDONLY);
>> +
>> +	/*
>> +	 * Check this is actually our file and we're not removing a legitimate
>> +	 * user override.
>> +	 */
>> +	if (handle == -1 && !is_missing_file_error(errno))
>> +		warning(_("failed to delete '%s'"), filename);
>> +	else {
>> +		read(handle, file_start_user, sizeof(file_start_stale) - 1);
> 
> Actually that fails -Werror because I don't check read return.
> Alternative below (on top of this one), with one question:
> Are VLA using size_t const OK ? It's folded to a constant array by gcc
> but I don't know if that causes portability problem with other platforms
> ? I can always repeat the sizeof expr if it's a problematic construct.

I think it would be easier to use strbuf_read_file() instead - it is 
only a small file so there is not really any advantage in just reading 
the first line. Something like

static int systemd_delete_user_unit(const char* unit)
{
	int ret = 0;
	struct strbuf buf = STRBUF_INIT;
	char *filename = xdg_config_home_for("systemd/user", unit);

	if (strbuf_read_file(&buf, filename, 0) < 0) {
		if (errno != ENOENT)
			ret = error_errno(_("could not read '%s'", filename));
		goto out;
	}
	if (starts_with(buf.buf,
			"# This file was created and is maintained by git.\n") &&
	    unlink(filename))
		ret = error_errno(_("could not remove '%s', filename));

out:
	free(filename);
	strbuf_release(&buf);
	return ret;
}

Best Wishes

Phillip

> -- >8 --
> 
> diff --git a/builtin/gc.c b/builtin/gc.c
> index 99b158e481..7fb25ea2b1 100644
> --- a/builtin/gc.c
> +++ b/builtin/gc.c
> @@ -2332,11 +2332,14 @@ static int systemd_set_units_state(int enable)
>   /*
>    * TODO: in the future (~2026 ?) remove this cleanup code
>    */
> +
>   static void systemd_delete_user_unit(char const *unit)
>   {
>   	char const	file_start_stale[] =	"# This file was created and is"
>   						" maintained by Git.";
> -	char		file_start_user[sizeof(file_start_stale)] = {'\0'};
> +	size_t const	length = sizeof(file_start_stale);
> +	char		file_start_user[length] = {'\0'};
> +
>   
>   	char *filename = xdg_config_home_for("systemd/user", unit);
>   	int handle = open(filename, O_RDONLY);
> @@ -2348,14 +2351,14 @@ static void systemd_delete_user_unit(char const *unit)
>   	if (handle == -1 && !is_missing_file_error(errno))
>   		warning(_("failed to delete '%s'"), filename);
>   	else {
> -		read(handle, file_start_user, sizeof(file_start_stale) - 1);
> -		close(handle);
> -		if (strcmp(file_start_stale, file_start_user) == 0) {
> +		if (length - 1 == read(handle, file_start_user, length - 1) &&
> +				strcmp(file_start_stale, file_start_user) == 0) {
>   			if (unlink(filename) == 0)
>   				warning(_("deleted stale unit file '%s'"), filename);
>   			else if (!is_missing_file_error(errno))
>   				warning(_("failed to delete '%s'"), filename);
>   		}
> +		close(handle);
>   	}
>   
>   	free(filename);
> 

Re: [PATCH v2 0/6] maintenance: use packaged systemd units

[Eric Sunshine]

On Sun, Mar 24, 2024 at 10:55 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
> 5 - documentation updates
>    I'm on the fence about having these in a separate commit like the
>    current patch 5 or updating the documentation when the code is
>    changed.

It's generally more reviewer-friendly to bundle documentation change
into the patch which changes the observable behavior. This way, a
reviewer has the behavior change fresh in mind and can verify that the
revised documentation matches the new implementation. Same goes for
revising tests in the same patch which changes behavior (though, of
course, revising tests at the same time as the change of behavior is
also mandatory for maintaining bisectability).

Re: [PATCH v2 0/6] maintenance: use packaged systemd units

[Max Gautier]

On Sun, Mar 24, 2024 at 02:54:58PM +0000, Phillip Wood wrote:
> Hi Max
> 
> On 22/03/2024 22:11, Max Gautier wrote:
> > * Distribute the systemd timers used by the `git maintenance start` with
> >    the systemd scheduler as part of git, rather than writing them in
> >    $XDG_CONFIG_HOME.
> > 
> > This allows users to override the units if they wish, and is more
> > in-line with the usual practices of distribution for systemd units.
> 
> Thanks for suggesting this, I think this is a useful change, but the
> implementation could be improved.
> 
> > We also move away from using the random minute, and instead rely on
> > systemd features to achieve the same goal (see patch 2). This allows us
> > to go back to using unit templating for the timers. This is also a
> > prerequisite to have static unit files.
> > 
> > Note that even if we really need more specific OnCalendar= settings for
> > each timer, we should still do it that way, but instead distribute
> > override alongside the template, for instance for weekly:
> > 
> > /usr/lib/systemd-user/git-maintenance@daily.timer.d/override.conf:
> > [Timer]
> > OnCalendar=<daily specific calendar spec>
> 
> We should definitely do that. Using systemd's random delay does not prevent
> the different maintenance jobs from running concurrently as one job may be
> started before a previous job has finished. It is important to only have one
> job running at a time because the first thing "git maintenance run" does is
> to try and acquire a lock file so if the hourly job is running when the
> daily jobs tries to start the daily job will not be run.

Thinking about that, it occurs to me that the current scheme does not
prevent concurrent execution either: the timers all use Persistent=true,
which means they can fire concurrently on machine boot, if two or more
would have been triggered during the time the machine was powered off
(or just the user logged out, since it's a user unit).

So maybe there should be a more robust mechanism to avoid concurrent
execution ? I assume from what you say above the lock is acquired in a
non-blocking way. Could going to a blocking one be a solution ?

> As the daily job is
> a superset of the hourly job and the weekly job is a superset of the daily
> job so it does not make sense to run more than one job per hour.

Is that set in stone, or could they perform disjoint set of tasks
instead ?

> 
> > The cleanup code for the units written in $XDG_CONFIG_HOME is adapted,
> > and takes care of not removing legitimate user overrides, by checking
> > the file start.
> 
> This points to an alternate strategy for supporting user overrides - don't
> overwrite the unit files if the user has edited them. I think that there is
> still a benefit to moving to system wide unit files though as it means that
> if we improve the unit files in the future systemd will pick up these
> improvements automatically. That is an improvement over the status quo where
> the users' unit files are written once and never updated.
> 
> I think it would help to reorder the changes in this series as follows:
> 
> 1 - simplify the invocation of "systemctl --user"
>   This would be the current patch 3 without adding "--force" or
>   moving "--now" combined with the relevant test changes from patch 6.
>   This would make it clear that those changes are a simple clean up that
>   is independent of the other changes made in this series.
> 
> 2 - don't delete user edited unit files
>   This would be based on the current patch 4 and would show that we can
>   avoid deleting unit files that the user has edited without the other
>   changes in this series. This change should have an associated test.
> 
> 3 - start using systemd's random delay function
>   This would be the current patch 1 without the template changes and the
>   commit message should explain that it is in preparation for disturbing
>   system-wide unit files.
> 
> 4 - install system-wide systemd unit files
>   This would be based on the current patch 2 with the addition of
>   overrides to prevent more than one job running per hour. The unit
>   files should be installed under $XDG_DATA_HOME when $(prefix) starts
>   with $(HOME), not just when they are equal. The associated test
>   changes from patch 6 should be moved here as well as the "--force"
>   change from patch 3.
> 
> 5 - documentation updates
>   I'm on the fence about having these in a separate commit like the
>   current patch 5 or updating the documentation when the code is
>   changed.

I had started cooking v3, I'll take into account, thanks !

-- 
Max Gautier

Re: [PATCH v2 4/6] maintenance: cleanup $XDG_CONFIG_HOME/systemd/user

[Max Gautier]

On Sun, Mar 24, 2024 at 03:45:45PM +0000, Phillip Wood wrote:
> Hi Max
> 
> On 23/03/2024 11:07, Max Gautier wrote:
> > On Fri, Mar 22, 2024 at 11:11:09PM +0100, Max Gautier wrote:
> > > +/*
> > > + * TODO: in the future (~2026 ?) remove this cleanup code
> > > + */
> 
> That is rather optimistic - users only run "git maintenance start" once so
> any unit files that have been written in the past will exist well beyond
> 2026.

In that case, should we hook the cleanup (in it's final form) in more
place ? `git maintenance register` for instance ?

> 
> > > +static void systemd_delete_user_unit(char const *unit)
> > > +{
> > > +	char const	file_start_stale[] =	"# This file was created and is"
> > > +						" maintained by Git.";
> > > +	char		file_start_user[sizeof(file_start_stale)] = {'\0'};
> > > +
> > > +	char *filename = xdg_config_home_for("systemd/user", unit);
> > > +	int handle = open(filename, O_RDONLY);
> > > +
> > > +	/*
> > > +	 * Check this is actually our file and we're not removing a legitimate
> > > +	 * user override.
> > > +	 */
> > > +	if (handle == -1 && !is_missing_file_error(errno))
> > > +		warning(_("failed to delete '%s'"), filename);
> > > +	else {
> > > +		read(handle, file_start_user, sizeof(file_start_stale) - 1);
> > 
> > Actually that fails -Werror because I don't check read return.
> > Alternative below (on top of this one), with one question:
> > Are VLA using size_t const OK ? It's folded to a constant array by gcc
> > but I don't know if that causes portability problem with other platforms
> > ? I can always repeat the sizeof expr if it's a problematic construct.
> 
> I think it would be easier to use strbuf_read_file() instead - it is only a
> small file so there is not really any advantage in just reading the first
> line. Something like
> 
> static int systemd_delete_user_unit(const char* unit)
> {
> 	int ret = 0;
> 	struct strbuf buf = STRBUF_INIT;
> 	char *filename = xdg_config_home_for("systemd/user", unit);
> 
> 	if (strbuf_read_file(&buf, filename, 0) < 0) {
> 		if (errno != ENOENT)
> 			ret = error_errno(_("could not read '%s'", filename));
> 		goto out;
> 	}
> 	if (starts_with(buf.buf,
> 			"# This file was created and is maintained by git.\n") &&
> 	    unlink(filename))
> 		ret = error_errno(_("could not remove '%s', filename));
> 
> out:
> 	free(filename);
> 	strbuf_release(&buf);
> 	return ret;
> }

Thanks, this is exactly what I needed, I looked at the strbuf API but
couldn't find this somehow.

-- 
Max Gautier

Re: [PATCH v2 0/6] maintenance: use packaged systemd units

[phillip.wood123]

Hi Max

On 25/03/2024 08:32, Max Gautier wrote:
> On Sun, Mar 24, 2024 at 02:54:58PM +0000, Phillip Wood wrote:
>> Hi Max
>>
>> On 22/03/2024 22:11, Max Gautier wrote:
>>> * Distribute the systemd timers used by the `git maintenance start` with
>>>     the systemd scheduler as part of git, rather than writing them in
>>>     $XDG_CONFIG_HOME.
>>>
>>> This allows users to override the units if they wish, and is more
>>> in-line with the usual practices of distribution for systemd units.
>>
>> Thanks for suggesting this, I think this is a useful change, but the
>> implementation could be improved.
>>
>>> We also move away from using the random minute, and instead rely on
>>> systemd features to achieve the same goal (see patch 2). This allows us
>>> to go back to using unit templating for the timers. This is also a
>>> prerequisite to have static unit files.
>>>
>>> Note that even if we really need more specific OnCalendar= settings for
>>> each timer, we should still do it that way, but instead distribute
>>> override alongside the template, for instance for weekly:
>>>
>>> /usr/lib/systemd-user/git-maintenance@daily.timer.d/override.conf:
>>> [Timer]
>>> OnCalendar=<daily specific calendar spec>
>>
>> We should definitely do that. Using systemd's random delay does not prevent
>> the different maintenance jobs from running concurrently as one job may be
>> started before a previous job has finished. It is important to only have one
>> job running at a time because the first thing "git maintenance run" does is
>> to try and acquire a lock file so if the hourly job is running when the
>> daily jobs tries to start the daily job will not be run.
> 
> Thinking about that, it occurs to me that the current scheme does not
> prevent concurrent execution either: the timers all use Persistent=true,
> which means they can fire concurrently on machine boot, if two or more
> would have been triggered during the time the machine was powered off
> (or just the user logged out, since it's a user unit).

Interesting, I wonder if the other schedulers suffer from the same problem.

> So maybe there should be a more robust mechanism to avoid concurrent
> execution ? I assume from what you say above the lock is acquired in a
> non-blocking way. Could going to a blocking one be a solution ?

It is possible to wait on a lock file but I'd be worried about building 
up an endless queue of processes if the process holding the lock file 
crashed leaving it in place without anyway to automatically remove it.

I don't think we need to solve that problem as part of this patch series 
but we should take care not to make it worse. Long term we may be better 
scheduling a single job and have "git maintenance run" decide which jobs 
to run based on the last time it run, rather than trying to schedule 
different jobs with the os scheduler.

>> As the daily job is
>> a superset of the hourly job and the weekly job is a superset of the daily
>> job so it does not make sense to run more than one job per hour.
> 
> Is that set in stone, or could they perform disjoint set of tasks
> instead ?

All of the schedulers are set up to run a single job each hour, I don't 
see why we'd start running disjoint sets of tasks in the different jobs.

>>> The cleanup code for the units written in $XDG_CONFIG_HOME is adapted,
>>> and takes care of not removing legitimate user overrides, by checking
>>> the file start.
>>
>> This points to an alternate strategy for supporting user overrides - don't
>> overwrite the unit files if the user has edited them. I think that there is
>> still a benefit to moving to system wide unit files though as it means that
>> if we improve the unit files in the future systemd will pick up these
>> improvements automatically. That is an improvement over the status quo where
>> the users' unit files are written once and never updated.
>>
>> I think it would help to reorder the changes in this series as follows:
>>
>> 1 - simplify the invocation of "systemctl --user"
>>    This would be the current patch 3 without adding "--force" or
>>    moving "--now" combined with the relevant test changes from patch 6.
>>    This would make it clear that those changes are a simple clean up that
>>    is independent of the other changes made in this series.
>>
>> 2 - don't delete user edited unit files
>>    This would be based on the current patch 4 and would show that we can
>>    avoid deleting unit files that the user has edited without the other
>>    changes in this series. This change should have an associated test.
>>
>> 3 - start using systemd's random delay function
>>    This would be the current patch 1 without the template changes and the
>>    commit message should explain that it is in preparation for disturbing
>>    system-wide unit files.
>>
>> 4 - install system-wide systemd unit files
>>    This would be based on the current patch 2 with the addition of
>>    overrides to prevent more than one job running per hour. The unit
>>    files should be installed under $XDG_DATA_HOME when $(prefix) starts
>>    with $(HOME), not just when they are equal. The associated test
>>    changes from patch 6 should be moved here as well as the "--force"
>>    change from patch 3.
>>
>> 5 - documentation updates
>>    I'm on the fence about having these in a separate commit like the
>>    current patch 5 or updating the documentation when the code is
>>    changed.
> 
> I had started cooking v3, I'll take into account, thanks !

Thanks

Phillip

Re: [PATCH v2 0/6] maintenance: use packaged systemd units

[phillip.wood123]

On 24/03/2024 17:03, Eric Sunshine wrote:
> On Sun, Mar 24, 2024 at 10:55 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
>> 5 - documentation updates
>>     I'm on the fence about having these in a separate commit like the
>>     current patch 5 or updating the documentation when the code is
>>     changed.
> 
> It's generally more reviewer-friendly to bundle documentation change
> into the patch which changes the observable behavior. This way, a
> reviewer has the behavior change fresh in mind and can verify that the
> revised documentation matches the new implementation. Same goes for
> revising tests in the same patch which changes behavior (though, of
> course, revising tests at the same time as the change of behavior is
> also mandatory for maintaining bisectability).

Good point, I think a couple of the documentation changes like 
recommending "systemctl --user edit" were improving the existing docs 
and so they should be in a separate commit at the start of the series. 
The other patches should update the documentation as the code changes.

Thanks

Phillip

Re: [PATCH v2 0/6] maintenance: use packaged systemd units

[Max Gautier]

On Mon, Mar 25, 2024 at 10:06:29AM +0000, phillip.wood123@gmail.com wrote:
> Hi Max
> 
> On 25/03/2024 08:32, Max Gautier wrote:
> > On Sun, Mar 24, 2024 at 02:54:58PM +0000, Phillip Wood wrote:
> > > Hi Max
> > > 
> > > On 22/03/2024 22:11, Max Gautier wrote:
> > > > * Distribute the systemd timers used by the `git maintenance start` with
> > > >     the systemd scheduler as part of git, rather than writing them in
> > > >     $XDG_CONFIG_HOME.
> > > > 
> > > > This allows users to override the units if they wish, and is more
> > > > in-line with the usual practices of distribution for systemd units.
> > > 
> > > Thanks for suggesting this, I think this is a useful change, but the
> > > implementation could be improved.
> > > 
> > > > We also move away from using the random minute, and instead rely on
> > > > systemd features to achieve the same goal (see patch 2). This allows us
> > > > to go back to using unit templating for the timers. This is also a
> > > > prerequisite to have static unit files.
> > > > 
> > > > Note that even if we really need more specific OnCalendar= settings for
> > > > each timer, we should still do it that way, but instead distribute
> > > > override alongside the template, for instance for weekly:
> > > > 
> > > > /usr/lib/systemd-user/git-maintenance@daily.timer.d/override.conf:
> > > > [Timer]
> > > > OnCalendar=<daily specific calendar spec>
> > > 
> > > We should definitely do that. Using systemd's random delay does not prevent
> > > the different maintenance jobs from running concurrently as one job may be
> > > started before a previous job has finished. It is important to only have one
> > > job running at a time because the first thing "git maintenance run" does is
> > > to try and acquire a lock file so if the hourly job is running when the
> > > daily jobs tries to start the daily job will not be run.
> > 
> > Thinking about that, it occurs to me that the current scheme does not
> > prevent concurrent execution either: the timers all use Persistent=true,
> > which means they can fire concurrently on machine boot, if two or more
> > would have been triggered during the time the machine was powered off
> > (or just the user logged out, since it's a user unit).
> 
> Interesting, I wonder if the other schedulers suffer from the same problem.

From what I can find (didn't dig much):
- cron does not have the problem, because it will just miss the timers
  if the machine was powered off. Not really better ^
  - anacron though is another implementation of cron which apparently
    support that semantic and is the default on ubuntu [1]
    I can't find if there is something to avoid the same problem that
    Persitent=true imply
- same goes for launchctl (Effect of Sleeping and Powering Off at the
  bottom of the page [2])
- for schtasks it's apparently possible to have a similar mechanism than
  Persistent [3]. There is a policy apparently to handle multiples
  instances [4] but I'm not completely sure whether or not theses
  instances can have different parameters.
  It's currently defined that way for the schtasks scheduler:
  "<MultipleInstancesPolicy>IgnoreNew</MultipleInstancesPolicy>\n". I
  don't think it would prevent parallel execution between the different
  schedule though, it seems to me they are different tasks.

[1]: https://serverfault.com/a/52338
[2]: https://developer.apple.com/library/archive/documentation/MacOSX/Conceptual/BPSystemStartup/Chapters/ScheduledJobs.html
[3]: https://learn.microsoft.com/en-us/troubleshoot/windows-server/system-management-components/scheduled-task-not-run-upon-reboot-machine-off
[4]: https://learn.microsoft.com/en-us/windows/win32/taskschd/tasksettings-multipleinstances

> 
> > So maybe there should be a more robust mechanism to avoid concurrent
> > execution ? I assume from what you say above the lock is acquired in a
> > non-blocking way. Could going to a blocking one be a solution ?
> 
> It is possible to wait on a lock file but I'd be worried about building up
> an endless queue of processes if the process holding the lock file crashed
> leaving it in place without anyway to automatically remove it.
> 

At least with systemd we have some mechanisms to deal with that.
- systemd timers don't touch an already running unit, so that won't
  trigger a new hourly or daily if the previous one is still running.
- for the automatically removing it, we could:
  - use XDG_RUNTIME_DIR ("%t" in systemd units) which is removed on
    logout
  - optionally add a tmpfiles fragments to delete locks which are really
    too old (tmpfiles won't delete files on which a lock is taken)
  - I thought about using a common RuntimeDirectory (see systemd.exec),
    but this is not possible due to [5]


[5]: https://github.com/systemd/systemd/issues/5394

> I don't think we need to solve that problem as part of this patch series but
> we should take care not to make it worse. Long term we may be better
> scheduling a single job and have "git maintenance run" decide which jobs to
> run based on the last time it run, rather than trying to schedule different
> jobs with the os scheduler.
> 
> > > As the daily job is
> > > a superset of the hourly job and the weekly job is a superset of the daily
> > > job so it does not make sense to run more than one job per hour.
> > 
> > Is that set in stone, or could they perform disjoint set of tasks
> > instead ?
> 
> All of the schedulers are set up to run a single job each hour, I don't see
> why we'd start running disjoint sets of tasks in the different jobs.

I was wondering if running distinct tasks would allow overlapping
execution, or if the different tasks are not safe to run concurrently
anyway. I'm not familiar enough with them and the git internals to tell.

Another option if the tasks set was distinct for each service instance
would be to use dependencies and ordering directives like this:
weekly.service
```
[Unit]
Requires=daily.service
After=daily.service

[Service]
ExecStart=<run only weekly stuff>
```

daily.service
```
[Unit]
Requires=hourly.service
After=hourly.service

[Service]
ExecStart=<run only daily stuff>
```

hourly.service
```
[Service]
ExecStart=<run only hourly stuff>
```

That would avoid concurrent execution I think.

-- 
Max Gautier

Re: [PATCH v2 0/6] maintenance: use packaged systemd units

[Max Gautier]

On Mon, Mar 25, 2024 at 10:06:29AM +0000, phillip.wood123@gmail.com wrote:
> 
> It is possible to wait on a lock file but I'd be worried about building up
> an endless queue of processes if the process holding the lock file crashed
> leaving it in place without anyway to automatically remove it.

Side question: we can't use flock(2) in git, or lockf(3) ? The latter is
in POSIX.

-- 
Max Gautier

Re: [PATCH v2 0/6] maintenance: use packaged systemd units

[Phillip Wood]

On 25/03/2024 12:27, Max Gautier wrote:
> On Mon, Mar 25, 2024 at 10:06:29AM +0000, phillip.wood123@gmail.com wrote:
>>>>> Note that even if we really need more specific OnCalendar= settings for
>>>>> each timer, we should still do it that way, but instead distribute
>>>>> override alongside the template, for instance for weekly:
>>>>>
>>>>> /usr/lib/systemd-user/git-maintenance@daily.timer.d/override.conf:
>>>>> [Timer]
>>>>> OnCalendar=<daily specific calendar spec>
>>>>
>>>> We should definitely do that. Using systemd's random delay does not prevent
>>>> the different maintenance jobs from running concurrently as one job may be
>>>> started before a previous job has finished. It is important to only have one
>>>> job running at a time because the first thing "git maintenance run" does is
>>>> to try and acquire a lock file so if the hourly job is running when the
>>>> daily jobs tries to start the daily job will not be run.
>>>
>>> Thinking about that, it occurs to me that the current scheme does not
>>> prevent concurrent execution either: the timers all use Persistent=true,
>>> which means they can fire concurrently on machine boot, if two or more
>>> would have been triggered during the time the machine was powered off
>>> (or just the user logged out, since it's a user unit).
>>
>> Interesting, I wonder if the other schedulers suffer from the same problem.
> 
>  From what I can find (didn't dig much):

Thanks for looking at this

> - cron does not have the problem, because it will just miss the timers
>    if the machine was powered off. Not really better ^

Yes, skipping the jobs is not great. On debian at least the job will be 
run if it is less than three hours since it should have been run. See
https://manpages.debian.org/bookworm/cron/cron.8.en.html

>    - anacron though is another implementation of cron which apparently
>      support that semantic and is the default on ubuntu [1]
>      I can't find if there is something to avoid the same problem that
>      Persitent=true imply

> - same goes for launchctl (Effect of Sleeping and Powering Off at the
>    bottom of the page [2])

As I read it the job is rescheduled if the computer was asleep when it 
should have run, but not if it was powered off.

> - for schtasks it's apparently possible to have a similar mechanism than
>    Persistent [3]. There is a policy apparently to handle multiples
>    instances [4] but I'm not completely sure whether or not theses
>    instances can have different parameters.
>    It's currently defined that way for the schtasks scheduler:
>    "<MultipleInstancesPolicy>IgnoreNew</MultipleInstancesPolicy>\n". I
>    don't think it would prevent parallel execution between the different
>    schedule though, it seems to me they are different tasks.
> 
> [1]: https://serverfault.com/a/52338
> [2]: https://developer.apple.com/library/archive/documentation/MacOSX/Conceptual/BPSystemStartup/Chapters/ScheduledJobs.html
> [3]: https://learn.microsoft.com/en-us/troubleshoot/windows-server/system-management-components/scheduled-task-not-run-upon-reboot-machine-off
> [4]: https://learn.microsoft.com/en-us/windows/win32/taskschd/tasksettings-multipleinstances

We should have a think about what to do about this once your patches to 
move to system-wide unit files are merged. We'll need to come up with 
something that works for all the schedulers so that we don't miss the 
daily and weekly jobs when the computer is powered off and ensures we 
don't run concurrent jobs.

Best Wishes

Phillip

>>> So maybe there should be a more robust mechanism to avoid concurrent
>>> execution ? I assume from what you say above the lock is acquired in a
>>> non-blocking way. Could going to a blocking one be a solution ?
>>
>> It is possible to wait on a lock file but I'd be worried about building up
>> an endless queue of processes if the process holding the lock file crashed
>> leaving it in place without anyway to automatically remove it.
>>
> 
> At least with systemd we have some mechanisms to deal with that.
> - systemd timers don't touch an already running unit, so that won't
>    trigger a new hourly or daily if the previous one is still running.
> - for the automatically removing it, we could:
>    - use XDG_RUNTIME_DIR ("%t" in systemd units) which is removed on
>      logout
>    - optionally add a tmpfiles fragments to delete locks which are really
>      too old (tmpfiles won't delete files on which a lock is taken)
>    - I thought about using a common RuntimeDirectory (see systemd.exec),
>      but this is not possible due to [5]
> 
> 
> [5]: https://github.com/systemd/systemd/issues/5394
> 
>> I don't think we need to solve that problem as part of this patch series but
>> we should take care not to make it worse. Long term we may be better
>> scheduling a single job and have "git maintenance run" decide which jobs to
>> run based on the last time it run, rather than trying to schedule different
>> jobs with the os scheduler.
>>
>>>> As the daily job is
>>>> a superset of the hourly job and the weekly job is a superset of the daily
>>>> job so it does not make sense to run more than one job per hour.
>>>
>>> Is that set in stone, or could they perform disjoint set of tasks
>>> instead ?
>>
>> All of the schedulers are set up to run a single job each hour, I don't see
>> why we'd start running disjoint sets of tasks in the different jobs.
> 
> I was wondering if running distinct tasks would allow overlapping
> execution, or if the different tasks are not safe to run concurrently
> anyway. I'm not familiar enough with them and the git internals to tell.
> 
> Another option if the tasks set was distinct for each service instance
> would be to use dependencies and ordering directives like this:
> weekly.service
> ```
> [Unit]
> Requires=daily.service
> After=daily.service
> 
> [Service]
> ExecStart=<run only weekly stuff>
> ```
> 
> daily.service
> ```
> [Unit]
> Requires=hourly.service
> After=hourly.service
> 
> [Service]
> ExecStart=<run only daily stuff>
> ```
> 
> hourly.service
> ```
> [Service]
> ExecStart=<run only hourly stuff>
> ```
> 
> That would avoid concurrent execution I think.
> 

Re: [PATCH v2 4/6] maintenance: cleanup $XDG_CONFIG_HOME/systemd/user

[Phillip Wood]

Hi Max

On 25/03/2024 08:36, Max Gautier wrote:
> On Sun, Mar 24, 2024 at 03:45:45PM +0000, Phillip Wood wrote:
>> Hi Max
>>
>> On 23/03/2024 11:07, Max Gautier wrote:
>>> On Fri, Mar 22, 2024 at 11:11:09PM +0100, Max Gautier wrote:
>>>> +/*
>>>> + * TODO: in the future (~2026 ?) remove this cleanup code
>>>> + */
>>
>> That is rather optimistic - users only run "git maintenance start" once so
>> any unit files that have been written in the past will exist well beyond
>> 2026.
> 
> In that case, should we hook the cleanup (in it's final form) in more
> place ? `git maintenance register` for instance ?

I'm not sure if that is needed if we leave the code to delete the unit 
files in place.

Best Wishes

Phillip

>>>> +static void systemd_delete_user_unit(char const *unit)
>>>> +{
>>>> +	char const	file_start_stale[] =	"# This file was created and is"
>>>> +						" maintained by Git.";
>>>> +	char		file_start_user[sizeof(file_start_stale)] = {'\0'};
>>>> +
>>>> +	char *filename = xdg_config_home_for("systemd/user", unit);
>>>> +	int handle = open(filename, O_RDONLY);
>>>> +
>>>> +	/*
>>>> +	 * Check this is actually our file and we're not removing a legitimate
>>>> +	 * user override.
>>>> +	 */
>>>> +	if (handle == -1 && !is_missing_file_error(errno))
>>>> +		warning(_("failed to delete '%s'"), filename);
>>>> +	else {
>>>> +		read(handle, file_start_user, sizeof(file_start_stale) - 1);
>>>
>>> Actually that fails -Werror because I don't check read return.
>>> Alternative below (on top of this one), with one question:
>>> Are VLA using size_t const OK ? It's folded to a constant array by gcc
>>> but I don't know if that causes portability problem with other platforms
>>> ? I can always repeat the sizeof expr if it's a problematic construct.
>>
>> I think it would be easier to use strbuf_read_file() instead - it is only a
>> small file so there is not really any advantage in just reading the first
>> line. Something like
>>
>> static int systemd_delete_user_unit(const char* unit)
>> {
>> 	int ret = 0;
>> 	struct strbuf buf = STRBUF_INIT;
>> 	char *filename = xdg_config_home_for("systemd/user", unit);
>>
>> 	if (strbuf_read_file(&buf, filename, 0) < 0) {
>> 		if (errno != ENOENT)
>> 			ret = error_errno(_("could not read '%s'", filename));
>> 		goto out;
>> 	}
>> 	if (starts_with(buf.buf,
>> 			"# This file was created and is maintained by git.\n") &&
>> 	    unlink(filename))
>> 		ret = error_errno(_("could not remove '%s', filename));
>>
>> out:
>> 	free(filename);
>> 	strbuf_release(&buf);
>> 	return ret;
>> }
> 
> Thanks, this is exactly what I needed, I looked at the strbuf API but
> couldn't find this somehow.
> 

Re: [PATCH v2 0/6] maintenance: use packaged systemd units

[Phillip Wood]

On 25/03/2024 13:45, Max Gautier wrote:
> On Mon, Mar 25, 2024 at 10:06:29AM +0000, phillip.wood123@gmail.com wrote:
>>
>> It is possible to wait on a lock file but I'd be worried about building up
>> an endless queue of processes if the process holding the lock file crashed
>> leaving it in place without anyway to automatically remove it.
> 
> Side question: we can't use flock(2) in git, or lockf(3) ? The latter is
> in POSIX.

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.

Best Wishes

Phillip

Re: [PATCH v2 4/6] maintenance: cleanup $XDG_CONFIG_HOME/systemd/user

[Max Gautier]

Le 25 mars 2024 17:39:23 GMT+01:00, Phillip Wood <phillip.wood123@gmail.com> a écrit :
>Hi Max
>
>On 25/03/2024 08:36, Max Gautier wrote:
>> On Sun, Mar 24, 2024 at 03:45:45PM +0000, Phillip Wood wrote:
>>> Hi Max
>>> 
>>> On 23/03/2024 11:07, Max Gautier wrote:
>>>> On Fri, Mar 22, 2024 at 11:11:09PM +0100, Max Gautier wrote:
>>>>> +/*
>>>>> + * TODO: in the future (~2026 ?) remove this cleanup code
>>>>> + */
>>> 
>>> That is rather optimistic - users only run "git maintenance start" once so
>>> any unit files that have been written in the past will exist well beyond
>>> 2026.
>> 
>> In that case, should we hook the cleanup (in it's final form) in more
>> place ? `git maintenance register` for instance ?
>
>I'm not sure if that is needed if we leave the code to delete the unit files in place.
>
>Best Wishes

But don't we want to remove it, eventually ?

-- 
Max Gautier

Re: [PATCH v2 0/6] maintenance: use packaged systemd units

[Max Gautier]

Le 25 mars 2024 17:39:47 GMT+01:00, Phillip Wood <phillip.wood123@gmail.com> a écrit :
>On 25/03/2024 13:45, Max Gautier wrote:
>> On Mon, Mar 25, 2024 at 10:06:29AM +0000, phillip.wood123@gmail.com wrote:
>>> 
>>> It is possible to wait on a lock file but I'd be worried about building up
>>> an endless queue of processes if the process holding the lock file crashed
>>> leaving it in place without anyway to automatically remove it.
>> 
>> Side question: we can't use flock(2) in git, or lockf(3) ? The latter is
>> in POSIX.
>
>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.
>
>Best Wishes
>
>Phillip
>
>

Ack, I'll leave that concern aside for now.


-- 
Max Gautier