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

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

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