(no subject)

[Max Gautier]

From: Max Gautier <mg@max.gautier.name>
To: git@vger.kernel.org
Cc: Lénaïc Huard <lenaic@lhuard.fr>, Derrick Stolee <stolee@gmail.com>
Bcc: 
Reply-To: 
Subject: [RFC PATCH 0/5] maintenance: use packaged systemd units
In-Reply-To: 

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. It's also less code.

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=<specific calender spec for daily>

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:
Best way to approximate having the systemd units in /usr/lib would be to
either :
- sudo cp systemd/user/git-maintenance* /etc/systemd/user/
- sudo systemctl link --global systemd/user/git-maintenance* -> it's not
  exactly the same when enabling the you'll have a link in
  $XDG_CONFIG_HOME to your git source directory, so if you re-run `git
  start maintenance with the previous code, it will change your source.
  IMO the first method is less confusing.

Unresolved (reasons 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

[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

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