[PATCH 0/2] macos: safeguard git maintenance against highly concurrent operations

When we started porting Scalar to C, one of the first things we did was to
run Scalar's quite extensive set of functional tests. And there, we ran into
immediate problems in the macOS job because git maintenance was registering
a large number of repositories concurrently, and our code could be more
robust in such scenarios.

The culprit lies not with Scalar, of course, but with the way git
maintenance wants to write the plist for use by launchctl and register it,
every time, and if two concurrent processes try to do that, they stumble
over each other.

This pair of patches makes git maintenance much less fragile in those
situations.

Please note that this patch series conflicts with lh/systemd-timers,
although in a trivial way: the latter changes the signature of
launchctl_schedule_plist() to lose its cmd parameter. The resolution is to
adjust the conflicting code to lose the cmd parameter, and also drop it from
launchctl_list_contains_plist() (and define it in the same way as
launchctl_boot_plist() does). I assume that lh/systemd-timers will advance
to next pretty soon; I plan on rebasing this patch series on top of it at
that stage.

Derrick Stolee (1):
  maintenance: skip bootout/bootstrap when plist is registered

Johannes Schindelin (1):
  maintenance: create `launchctl` configuration using a lock file

 builtin/gc.c           | 91 ++++++++++++++++++++++++++++++++----------
 t/t7900-maintenance.sh | 17 ++++++++
 2 files changed, 87 insertions(+), 21 deletions(-)


base-commit: 48bf2fa8bad054d66bd79c6ba903c89c704201f7
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1024%2Fdscho%2Fmaintenance%2Flaunchctl-concurrent-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1024/dscho/maintenance/launchctl-concurrent-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1024
-- 
gitgitgadget

From: Johannes Schindelin <redacted>

When two `git maintenance` processes try to write the `.plist` file, we
need to help them with serializing their efforts.

The 150ms time-out value was determined from thin air.

Signed-off-by: Johannes Schindelin <redacted>
---
 builtin/gc.c | 47 ++++++++++++++++++++++++++++++-----------------
 1 file changed, 30 insertions(+), 17 deletions(-)

diff --git a/builtin/gc.c b/builtin/gc.c
index ef7226d7bca..4a78b497d0e 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c

@@ -1617,16 +1617,14 @@ static int launchctl_remove_plists(const char *cmd)
 
 static int launchctl_schedule_plist(const char *exec_path, enum schedule_priority schedule, const char *cmd)
 {
-	FILE *plist;
-	int i;
+	int i, fd;
 	const char *preamble, *repeat;
 	const char *frequency = get_frequency(schedule);
 	char *name = launchctl_service_name(frequency);
 	char *filename = launchctl_service_filename(name);
-
-	if (safe_create_leading_directories(filename))
-		die(_("failed to create directories for '%s'"), filename);
-	plist = xfopen(filename, "w");
+	struct lock_file lk = LOCK_INIT;
+	static unsigned long lock_file_timeout_ms = ULONG_MAX;
+	struct strbuf plist = STRBUF_INIT;
 
 	preamble = "<?xml version=\"1.0\"?>\n"
 		   "<!DOCTYPE plist PUBLIC \"-//Apple//DTD PLIST 1.0//EN\" \"http://www.apple.com/DTDs/PropertyList-1.0.dtd\">\n"

@@ -1645,7 +1643,7 @@ static int launchctl_schedule_plist(const char *exec_path, enum schedule_priorit
 		   "</array>\n"
 		   "<key>StartCalendarInterval</key>\n"
 		   "<array>\n";
-	fprintf(plist, preamble, name, exec_path, exec_path, frequency);
+	strbuf_addf(&plist, preamble, name, exec_path, exec_path, frequency);
 
 	switch (schedule) {
 	case SCHEDULE_HOURLY:

@@ -1654,7 +1652,7 @@ static int launchctl_schedule_plist(const char *exec_path, enum schedule_priorit
 			 "<key>Minute</key><integer>0</integer>\n"
 			 "</dict>\n";
 		for (i = 1; i <= 23; i++)
-			fprintf(plist, repeat, i);
+			strbuf_addf(&plist, repeat, i);
 		break;
 
 	case SCHEDULE_DAILY:

@@ -1664,24 +1662,38 @@ static int launchctl_schedule_plist(const char *exec_path, enum schedule_priorit
 			 "<key>Minute</key><integer>0</integer>\n"
 			 "</dict>\n";
 		for (i = 1; i <= 6; i++)
-			fprintf(plist, repeat, i);
+			strbuf_addf(&plist, repeat, i);
 		break;
 
 	case SCHEDULE_WEEKLY:
-		fprintf(plist,
-			"<dict>\n"
-			"<key>Day</key><integer>0</integer>\n"
-			"<key>Hour</key><integer>0</integer>\n"
-			"<key>Minute</key><integer>0</integer>\n"
-			"</dict>\n");
+		strbuf_addstr(&plist,
+			      "<dict>\n"
+			      "<key>Day</key><integer>0</integer>\n"
+			      "<key>Hour</key><integer>0</integer>\n"
+			      "<key>Minute</key><integer>0</integer>\n"
+			      "</dict>\n");
 		break;
 
 	default:
 		/* unreachable */
 		break;
 	}
-	fprintf(plist, "</array>\n</dict>\n</plist>\n");
-	fclose(plist);
+	strbuf_addstr(&plist, "</array>\n</dict>\n</plist>\n");
+
+	if (safe_create_leading_directories(filename))
+		die(_("failed to create directories for '%s'"), filename);
+
+	if ((long)lock_file_timeout_ms < 0 &&
+	    git_config_get_ulong("gc.launchctlplistlocktimeoutms",
+				 &lock_file_timeout_ms))
+		lock_file_timeout_ms = 150;
+
+	fd = hold_lock_file_for_update_timeout(&lk, filename, LOCK_DIE_ON_ERROR,
+					       lock_file_timeout_ms);
+
+	if (write_in_full(fd, plist.buf, plist.len) < 0 ||
+	    commit_lock_file(&lk))
+		die_errno(_("could not write '%s'"), filename);
 
 	/* bootout might fail if not already running, so ignore */
 	launchctl_boot_plist(0, filename, cmd);

@@ -1690,6 +1702,7 @@ static int launchctl_schedule_plist(const char *exec_path, enum schedule_priorit
 
 	free(filename);
 	free(name);
+	strbuf_release(&plist);
 	return 0;
 }
 

-- 
gitgitgadget

From: Derrick Stolee <redacted>

On macOS, we use launchctl to manage the background maintenance
schedule. This uses a set of .plist files to describe the schedule, but
these files are also registered with 'launchctl bootstrap'. If multiple
'git maintenance start' commands run concurrently, then they can collide
replacing these schedule files and registering them with launchctl.

To avoid extra launchctl commands, do a check for the .plist files on
disk and check if they are registered using 'launchctl list <name>'.
This command will return with exit code 0 if it exists, or exit code 113
if it does not.

We can test this behavior using the GIT_TEST_MAINT_SCHEDULER environment
variable.

Signed-off-by: Derrick Stolee <redacted>
Signed-off-by: Johannes Schindelin <redacted>
---
 builtin/gc.c           | 54 +++++++++++++++++++++++++++++++++++-------
 t/t7900-maintenance.sh | 17 +++++++++++++
 2 files changed, 62 insertions(+), 9 deletions(-)

diff --git a/builtin/gc.c b/builtin/gc.c
index 4a78b497d0e..02bbe889ca6 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c

@@ -1615,6 +1615,29 @@ static int launchctl_remove_plists(const char *cmd)
 		launchctl_remove_plist(SCHEDULE_WEEKLY, cmd);
 }
 
+static int launchctl_list_contains_plist(const char *name, const char *cmd)
+{
+	int result;
+	struct child_process child = CHILD_PROCESS_INIT;
+	char *uid = launchctl_get_uid();
+
+	strvec_split(&child.args, cmd);
+	strvec_pushl(&child.args, "list", name, NULL);
+
+	child.no_stderr = 1;
+	child.no_stdout = 1;
+
+	if (start_command(&child))
+		die(_("failed to start launchctl"));
+
+	result = finish_command(&child);
+
+	free(uid);
+
+	/* Returns failure if 'name' doesn't exist. */
+	return !result;
+}
+
 static int launchctl_schedule_plist(const char *exec_path, enum schedule_priority schedule, const char *cmd)
 {
 	int i, fd;

@@ -1624,7 +1647,8 @@ static int launchctl_schedule_plist(const char *exec_path, enum schedule_priorit
 	char *filename = launchctl_service_filename(name);
 	struct lock_file lk = LOCK_INIT;
 	static unsigned long lock_file_timeout_ms = ULONG_MAX;
-	struct strbuf plist = STRBUF_INIT;
+	struct strbuf plist = STRBUF_INIT, plist2 = STRBUF_INIT;
+	struct stat st;
 
 	preamble = "<?xml version=\"1.0\"?>\n"
 		   "<!DOCTYPE plist PUBLIC \"-//Apple//DTD PLIST 1.0//EN\" \"http://www.apple.com/DTDs/PropertyList-1.0.dtd\">\n"

@@ -1691,18 +1715,30 @@ static int launchctl_schedule_plist(const char *exec_path, enum schedule_priorit
 	fd = hold_lock_file_for_update_timeout(&lk, filename, LOCK_DIE_ON_ERROR,
 					       lock_file_timeout_ms);
 
-	if (write_in_full(fd, plist.buf, plist.len) < 0 ||
-	    commit_lock_file(&lk))
-		die_errno(_("could not write '%s'"), filename);
-
-	/* bootout might fail if not already running, so ignore */
-	launchctl_boot_plist(0, filename, cmd);
-	if (launchctl_boot_plist(1, filename, cmd))
-		die(_("failed to bootstrap service %s"), filename);
+	/*
+	 * Does this file already exist? With the intended contents? Is it
+	 * registered already? Then it does not need to be re-registered.
+	 */
+	if (!stat(filename, &st) && st.st_size == plist.len &&
+	    strbuf_read_file(&plist2, filename, plist.len) == plist.len &&
+	    !strbuf_cmp(&plist, &plist2) &&
+	    launchctl_list_contains_plist(name, cmd))
+		rollback_lock_file(&lk);
+	else {
+		if (write_in_full(fd, plist.buf, plist.len) < 0 ||
+		    commit_lock_file(&lk))
+			die_errno(_("could not write '%s'"), filename);
+
+		/* bootout might fail if not already running, so ignore */
+		launchctl_boot_plist(0, filename, cmd);
+		if (launchctl_boot_plist(1, filename, cmd))
+			die(_("failed to bootstrap service %s"), filename);
+	}
 
 	free(filename);
 	free(name);
 	strbuf_release(&plist);
+	strbuf_release(&plist2);
 	return 0;
 }
 

diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
index 2412d8c5c00..14b30f37ec7 100755
--- a/t/t7900-maintenance.sh
+++ b/t/t7900-maintenance.sh

@@ -578,6 +578,23 @@ test_expect_success 'start and stop macOS maintenance' '
 	test_line_count = 0 actual
 '
 
+test_expect_success 'use launchctl list to prevent extra work' '
+	# ensure we are registered
+	GIT_TEST_MAINT_SCHEDULER=launchctl:./print-args git maintenance start &&
+
+	# do it again on a fresh args file
+	rm -f args &&
+	GIT_TEST_MAINT_SCHEDULER=launchctl:./print-args git maintenance start &&
+
+	ls "$HOME/Library/LaunchAgents" >actual &&
+	cat >expect <<-\EOF &&
+	list org.git-scm.git.hourly
+	list org.git-scm.git.daily
+	list org.git-scm.git.weekly
+	EOF
+	test_cmp expect args
+'
+
 test_expect_success 'start and stop Windows maintenance' '
 	write_script print-args <<-\EOF &&
 	echo $* >>args

-- 
gitgitgadget

When the launchctl_boot_plist() function was added in
a16eb6b1ff3 (maintenance: skip bootout/bootstrap when plist is
registered, 2021-08-24), an unused call to launchctl_get_uid() was
added along with it. That call appears to have been copy/pasted from
launchctl_boot_plist().

Since we can remove that, we can also get rid of the "result"
variable, whose only purpose was allow for the free() between its
assignment and the return. That pattern also appears to have been
copy/pasted from launchctl_boot_plist().

Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---

I happen to have a local topic that refactored the launchctl_get_uid()
function away, that didn't compile with the updated "master", I
figured I'd need to add it back since it had a new user, but as it
turns out that hopefully won't be needed.

 builtin/gc.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/builtin/gc.c b/builtin/gc.c
index 9a48eb535fb..b024f0e04e9 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c

@@ -1602,9 +1602,7 @@ static int launchctl_remove_plists(const char *cmd)
 
 static int launchctl_list_contains_plist(const char *name, const char *cmd)
 {
-	int result;
 	struct child_process child = CHILD_PROCESS_INIT;
-	char *uid = launchctl_get_uid();
 
 	strvec_split(&child.args, cmd);
 	strvec_pushl(&child.args, "list", name, NULL);

@@ -1615,12 +1613,8 @@ static int launchctl_list_contains_plist(const char *name, const char *cmd)
 	if (start_command(&child))
 		die(_("failed to start launchctl"));
 
-	result = finish_command(&child);
-
-	free(uid);
-
 	/* Returns failure if 'name' doesn't exist. */
-	return !result;
+	return !finish_command(&child);
 }
 
 static int launchctl_schedule_plist(const char *exec_path, enum schedule_priority schedule, const char *cmd)

-- 
2.33.0.825.gdc3f7a2a6c7

Hi Ævar,

On Thu, 9 Sep 2021, Ævar Arnfjörð Bjarmason wrote:

When the launchctl_boot_plist() function was added in
a16eb6b1ff3 (maintenance: skip bootout/bootstrap when plist is
registered, 2021-08-24), an unused call to launchctl_get_uid() was
added along with it. That call appears to have been copy/pasted from
launchctl_boot_plist().

Since we can remove that, we can also get rid of the "result"
variable, whose only purpose was allow for the free() between its
assignment and the return. That pattern also appears to have been
copy/pasted from launchctl_boot_plist().

I don't find the most crucial information in that commit message: what is
the fall-out of the removal of this call?

Such an analysis (_with_ a summary of it in the commit message) is
definitely required. And it should not be left as an exercise for the
reader.

Ciao,
Johannes

On Thu, Sep 09 2021, Johannes Schindelin wrote:

Hi Ævar,

On Thu, 9 Sep 2021, Ævar Arnfjörð Bjarmason wrote:

quoted

When the launchctl_boot_plist() function was added in
a16eb6b1ff3 (maintenance: skip bootout/bootstrap when plist is
registered, 2021-08-24), an unused call to launchctl_get_uid() was
added along with it. That call appears to have been copy/pasted from
launchctl_boot_plist().

Since we can remove that, we can also get rid of the "result"
variable, whose only purpose was allow for the free() between its
assignment and the return. That pattern also appears to have been
copy/pasted from launchctl_boot_plist().

I don't find the most crucial information in that commit message: what is
the fall-out of the removal of this call?

Such an analysis (_with_ a summary of it in the commit message) is
definitely required. And it should not be left as an exercise for the
reader.

Do you mean an assurance to the reader that the removed code doesn't
have any side-effects? E.g. an addition of

    As the patch shows the returned value wasn't used at all in this
    function, the launchctl_get_uid() function itself just calls
    xstrfmt() and getuid(), neither of which have any subtle global
    side-effects, so this removal is safe.

?

Hi Ævar,

On Thu, 9 Sep 2021, Ævar Arnfjörð Bjarmason wrote:

On Thu, Sep 09 2021, Johannes Schindelin wrote:

quoted

Hi Ævar,

On Thu, 9 Sep 2021, Ævar Arnfjörð Bjarmason wrote:

quoted

When the launchctl_boot_plist() function was added in
a16eb6b1ff3 (maintenance: skip bootout/bootstrap when plist is
registered, 2021-08-24), an unused call to launchctl_get_uid() was
added along with it. That call appears to have been copy/pasted from
launchctl_boot_plist().

Since we can remove that, we can also get rid of the "result"
variable, whose only purpose was allow for the free() between its
assignment and the return. That pattern also appears to have been
copy/pasted from launchctl_boot_plist().

I don't find the most crucial information in that commit message: what is
the fall-out of the removal of this call?

Such an analysis (_with_ a summary of it in the commit message) is
definitely required. And it should not be left as an exercise for the
reader.

Do you mean an assurance to the reader that the removed code doesn't
have any side-effects? E.g. an addition of

    As the patch shows the returned value wasn't used at all in this
    function, the launchctl_get_uid() function itself just calls
    xstrfmt() and getuid(), neither of which have any subtle global
    side-effects, so this removal is safe.

?

Yes. You want to refrain from forcing every reader to have to go look at
the definition of that function at that revision. The accumulated time
spent tallies up rather in disfavor of doing the work diligently on the
contributor's side and save every reader some time. I mean, you forced me
to spend the time, and then to spend more time to point out the missing
analysis, and then you provided the paragraph as a question, forcing me to
spend even more time on answering. All this time could have been saved in
the first place. In this instance, it is too late to do anything about it.
But I'm sure you plan on contributing other patches. Hopefully it will be
more efficient next time.

Ciao,
Johannes

When the launchctl_boot_plist() function was added in
a16eb6b1ff3 (maintenance: skip bootout/bootstrap when plist is
registered, 2021-08-24), an unused call to launchctl_get_uid() was
added along with it. That call appears to have been copy/pasted from
launchctl_boot_plist().

Since we can remove that, we can also get rid of the "result"
variable, whose only purpose was allow for the free() between its
assignment and the return. That pattern also appears to have been
copy/pasted from launchctl_boot_plist().

As the patch shows the returned value from launchctl_get_uid() wasn't
used at all in this function. The launchctl_get_uid() function itself
just calls xstrfmt() and getuid(), neither of which have any subtle
global side-effects, so this removal is safe.

Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---

Addresses a comment about the clarity of the commit message in v1:
https://lore.kernel.org/git/87bl52dkv1.fsf@evledraar.gmail.com/

Range-diff against v1:
1:  93adb856b0c ! 1:  794e68e7722 gc: remove unused launchctl_get_uid() call
    @@ Commit message
         assignment and the return. That pattern also appears to have been
         copy/pasted from launchctl_boot_plist().
     
    +    As the patch shows the returned value from launchctl_get_uid() wasn't
    +    used at all in this function. The launchctl_get_uid() function itself
    +    just calls xstrfmt() and getuid(), neither of which have any subtle
    +    global side-effects, so this removal is safe.
    +
         Signed-off-by: Ævar Arnfjörð Bjarmason [off-list ref]
     
      ## builtin/gc.c ##

 builtin/gc.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/builtin/gc.c b/builtin/gc.c
index 43c36024cbe..db76af4f31c 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c

@@ -1602,9 +1602,7 @@ static int launchctl_remove_plists(const char *cmd)
 
 static int launchctl_list_contains_plist(const char *name, const char *cmd)
 {
-	int result;
 	struct child_process child = CHILD_PROCESS_INIT;
-	char *uid = launchctl_get_uid();
 
 	strvec_split(&child.args, cmd);
 	strvec_pushl(&child.args, "list", name, NULL);

@@ -1615,12 +1613,8 @@ static int launchctl_list_contains_plist(const char *name, const char *cmd)
 	if (start_command(&child))
 		die(_("failed to start launchctl"));
 
-	result = finish_command(&child);
-
-	free(uid);
-
 	/* Returns failure if 'name' doesn't exist. */
-	return !result;
+	return !finish_command(&child);
 }
 
 static int launchctl_schedule_plist(const char *exec_path, enum schedule_priority schedule, const char *cmd)

-- 
2.33.0.998.ga4d44345d43

"Johannes Schindelin via GitGitGadget" [off-list ref]
writes:

Please note that this patch series conflicts with lh/systemd-timers,
although in a trivial way: the latter changes the signature of
launchctl_schedule_plist() to lose its cmd parameter. The resolution is to
adjust the conflicting code to lose the cmd parameter, and also drop it from
launchctl_list_contains_plist() (and define it in the same way as
launchctl_boot_plist() does). I assume that lh/systemd-timers will advance
to next pretty soon; I plan on rebasing this patch series on top of it at
that stage.

Sounds like a plan.

Here is my attempt to merge lh/systemd-timers into the result of
applying these two to 'master', with focus on the top part of the
launchctl_schedule_plist().  Sanity-checking is appreciated.

diff --cc builtin/gc.c
index 22e670b508,6a57d0fde5..0000000000

--- i/builtin/gc.c
+++ w/builtin/gc.c

@@@ -1593,48 -1678,26 +1678,50 @@@ static int launchctl_remove_plist(enum 
  	return result;
  }
  
- static int launchctl_remove_plists(const char *cmd)
+ static int launchctl_remove_plists(void)
  {
- 	return launchctl_remove_plist(SCHEDULE_HOURLY, cmd) ||
- 		launchctl_remove_plist(SCHEDULE_DAILY, cmd) ||
- 		launchctl_remove_plist(SCHEDULE_WEEKLY, cmd);
+ 	return launchctl_remove_plist(SCHEDULE_HOURLY) ||
+ 	       launchctl_remove_plist(SCHEDULE_DAILY) ||
+ 	       launchctl_remove_plist(SCHEDULE_WEEKLY);
  }
  
 +static int launchctl_list_contains_plist(const char *name, const char *cmd)
 +{
 +	int result;
 +	struct child_process child = CHILD_PROCESS_INIT;
 +	char *uid = launchctl_get_uid();
 +
 +	strvec_split(&child.args, cmd);
 +	strvec_pushl(&child.args, "list", name, NULL);
 +
 +	child.no_stderr = 1;
 +	child.no_stdout = 1;
 +
 +	if (start_command(&child))
 +		die(_("failed to start launchctl"));
 +
 +	result = finish_command(&child);
 +
 +	free(uid);
 +
 +	/* Returns failure if 'name' doesn't exist. */
 +	return !result;
 +}
 +
- static int launchctl_schedule_plist(const char *exec_path, enum schedule_priority schedule, const char *cmd)
+ static int launchctl_schedule_plist(const char *exec_path, enum schedule_priority schedule)
  {
 -	FILE *plist;
 -	int i;
 +	int i, fd;
  	const char *preamble, *repeat;
  	const char *frequency = get_frequency(schedule);
  	char *name = launchctl_service_name(frequency);
  	char *filename = launchctl_service_filename(name);
 +	struct lock_file lk = LOCK_INIT;
 +	static unsigned long lock_file_timeout_ms = ULONG_MAX;
 +	struct strbuf plist = STRBUF_INIT, plist2 = STRBUF_INIT;
 +	struct stat st;
++	const char *cmd = "launchctl";
  
 -	if (safe_create_leading_directories(filename))
 -		die(_("failed to create directories for '%s'"), filename);
 -	plist = xfopen(filename, "w");
 -
++	get_schedule_cmd(&cmd, NULL);
  	preamble = "<?xml version=\"1.0\"?>\n"
  		   "<!DOCTYPE plist PUBLIC \"-//Apple//DTD PLIST 1.0//EN\" \"http://www.apple.com/DTDs/PropertyList-1.0.dtd\">\n"
  		   "<plist version=\"1.0\">"
@@@ -1687,38 -1750,13 +1774,38 @@@
  		/* unreachable */
  		break;
  	}
 -	fprintf(plist, "</array>\n</dict>\n</plist>\n");
 -	fclose(plist);
 +	strbuf_addstr(&plist, "</array>\n</dict>\n</plist>\n");
  
 -	/* bootout might fail if not already running, so ignore */
 -	launchctl_boot_plist(0, filename);
 -	if (launchctl_boot_plist(1, filename))
 -		die(_("failed to bootstrap service %s"), filename);
 +	if (safe_create_leading_directories(filename))
 +		die(_("failed to create directories for '%s'"), filename);
 +
 +	if ((long)lock_file_timeout_ms < 0 &&
 +	    git_config_get_ulong("gc.launchctlplistlocktimeoutms",
 +				 &lock_file_timeout_ms))
 +		lock_file_timeout_ms = 150;
 +
 +	fd = hold_lock_file_for_update_timeout(&lk, filename, LOCK_DIE_ON_ERROR,
 +					       lock_file_timeout_ms);
 +
 +	/*
 +	 * Does this file already exist? With the intended contents? Is it
 +	 * registered already? Then it does not need to be re-registered.
 +	 */
 +	if (!stat(filename, &st) && st.st_size == plist.len &&
 +	    strbuf_read_file(&plist2, filename, plist.len) == plist.len &&
 +	    !strbuf_cmp(&plist, &plist2) &&
 +	    launchctl_list_contains_plist(name, cmd))
 +		rollback_lock_file(&lk);
 +	else {
 +		if (write_in_full(fd, plist.buf, plist.len) < 0 ||
 +		    commit_lock_file(&lk))
 +			die_errno(_("could not write '%s'"), filename);
 +
 +		/* bootout might fail if not already running, so ignore */
- 		launchctl_boot_plist(0, filename, cmd);
- 		if (launchctl_boot_plist(1, filename, cmd))
++		launchctl_boot_plist(0, filename);
++		if (launchctl_boot_plist(1, filename))
 +			die(_("failed to bootstrap service %s"), filename);
 +	}
  
  	free(filename);
  	free(name);

Hi Junio,

On Tue, 24 Aug 2021, Junio C Hamano wrote:

"Johannes Schindelin via GitGitGadget" [off-list ref]
writes:

quoted

Please note that this patch series conflicts with lh/systemd-timers,
although in a trivial way: the latter changes the signature of
launchctl_schedule_plist() to lose its cmd parameter. The resolution is to
adjust the conflicting code to lose the cmd parameter, and also drop it from
launchctl_list_contains_plist() (and define it in the same way as
launchctl_boot_plist() does). I assume that lh/systemd-timers will advance
to next pretty soon; I plan on rebasing this patch series on top of it at
that stage.

Sounds like a plan.

Here is my attempt to merge lh/systemd-timers into the result of
applying these two to 'master', with focus on the top part of the
launchctl_schedule_plist().  Sanity-checking is appreciated.

My local version (hence `git reset -hard`'ed away) looked almost precisely
like yours, I only added the definition of `cmd` to the top of
`launchctl_list_contains_plist()` and removed its `cmd` parameter (and
adjusted the callers). But that's the only difference I can spot.

Thanks,
Dscho