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

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

[Johannes Schindelin via GitGitGadget]

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

[PATCH 1/2] maintenance: create `launchctl` configuration using a lock file

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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 <johannes.schindelin@gmx.de>
---
 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

[PATCH 2/2] maintenance: skip bootout/bootstrap when plist is registered

[Derrick Stolee via GitGitGadget]

From: Derrick Stolee <dstolee@microsoft.com>

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 <dstolee@microsoft.com>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 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

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

[Junio C Hamano]

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
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);

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

[Johannes Schindelin]

Hi Junio,

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

> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
> 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.

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

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

[PATCH] gc: remove unused launchctl_get_uid() call

[Ævar Arnfjörð Bjarmason]

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 <avarab@gmail.com>
---

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

Re: [PATCH] gc: remove unused launchctl_get_uid() call

[Johannes Schindelin]

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

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

Re: [PATCH] gc: remove unused launchctl_get_uid() call

[Ævar Arnfjörð Bjarmason]

On Thu, Sep 09 2021, Johannes Schindelin wrote:

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

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.

?

Re: [PATCH] gc: remove unused launchctl_get_uid() call

[Johannes Schindelin]

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

Hi Ævar,

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

>
> On Thu, Sep 09 2021, Johannes Schindelin wrote:
>
> > 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.
>
> 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

Re: [PATCH] gc: remove unused launchctl_get_uid() call

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

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

While I very much like to see that you subscribe to the principle,
and I do wish that the "question" were posed as a comment after the
three-dash-line in a rerolled patch that is written under the
assumption that the answer to the question is "yes", which would
have saved one extra roundtrip, I do not think the question was not
something to be scolded like the above.

All writers tend to assume that their readers know more than they
do, and that is where the "do not force the reader to know or go
look at things if they don't" principle comes from.  But you're
assuming that your "what's the fallout?" question did not need
clarification, but it is understandable if it weren't.

> But I'm sure you plan on contributing other patches. Hopefully it will be
> more efficient next time.

Thanks.

[PATCH v2] gc: remove unused launchctl_get_uid() call

[Ævar Arnfjörð Bjarmason]

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 <avarab@gmail.com>
---

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 <avarab@gmail.com>
     
      ## 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