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

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