{"thread":{"id":"56367","subject":"[PATCH 0/2] macos: safeguard git maintenance against highly concurrent operations","startedAt":"2021-08-24T15:44:05Z","lastAt":"2021-09-12T00:24:55Z","messageCount":11,"participants":["Johannes Schindelin via GitGitGadget","Derrick Stolee via GitGitGadget","Junio C Hamano","Johannes Schindelin","Ævar Arnfjörð Bjarmason"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"433561","messageId":"pull.1024.git.1629819840.gitgitgadget@gmail.com","threadId":"56367","inReplyTo":null,"subject":"[PATCH 0/2] macos: safeguard git maintenance against highly concurrent operations","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-08-24T15:43:58Z","receivedAt":"2021-08-24T15:44:05Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"When we started porting Scalar to C, one of the first things we did was to\nrun Scalar's quite extensive set of functional tests. And there, we ran into\nimmediate problems in the macOS job because git maintenance was registering\na large number of repositories concurrently, and our code could be more\nrobust in such scenarios.\n\nThe culprit lies not with Scalar, of course, but with the way git\nmaintenance wants to write the plist for use by launchctl and register it,\nevery time, and if two concurrent processes try to do that, they stumble\nover each other.\n\nThis pair of patches makes git maintenance much less fragile in those\nsituations.\n\nPlease note that this patch series conflicts with lh/systemd-timers,\nalthough in a trivial way: the latter changes the signature of\nlaunchctl_schedule_plist() to lose its cmd parameter. The resolution is to\nadjust the conflicting code to lose the cmd parameter, and also drop it from\nlaunchctl_list_contains_plist() (and define it in the same way as\nlaunchctl_boot_plist() does). I assume that lh/systemd-timers will advance\nto next pretty soon; I plan on rebasing this patch series on top of it at\nthat stage.\n\nDerrick Stolee (1):\n maintenance: skip bootout/bootstrap when plist is registered\n\nJohannes Schindelin (1):\n maintenance: create `launchctl` configuration using a lock file\n\n builtin/gc.c | 91 ++++++++++++++++++++++++++++++++----------\n t/t7900-maintenance.sh | 17 ++++++++\n 2 files changed, 87 insertions(+), 21 deletions(-)\n\n\nbase-commit: 48bf2fa8bad054d66bd79c6ba903c89c704201f7\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1024%2Fdscho%2Fmaintenance%2Flaunchctl-concurrent-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1024/dscho/maintenance/launchctl-concurrent-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1024\n-- \ngitgitgadget\n"},{"id":"433562","messageId":"62ad2255e6eb0a6aa0995a96a80e40d3342eb394.1629819840.git.gitgitgadget@gmail.com","threadId":"56367","inReplyTo":"pull.1024.git.1629819840.gitgitgadget@gmail.com","subject":"[PATCH 1/2] maintenance: create `launchctl` configuration using a lock file","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-08-24T15:43:59Z","receivedAt":"2021-08-24T15:44:09Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nWhen two `git maintenance` processes try to write the `.plist` file, we\nneed to help them with serializing their efforts.\n\nThe 150ms time-out value was determined from thin air.\n\nSigned-off-by: Johannes Schindelin \n---\n builtin/gc.c | 47 ++++++++++++++++++++++++++++++-----------------\n 1 file changed, 30 insertions(+), 17 deletions(-)\n\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex ef7226d7bca..4a78b497d0e 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -1617,16 +1617,14 @@ static int launchctl_remove_plists(const char *cmd)\n \n static int launchctl_schedule_plist(const char *exec_path, enum schedule_priority schedule, const char *cmd)\n {\n-\tFILE *plist;\n-\tint i;\n+\tint i, fd;\n \tconst char *preamble, *repeat;\n \tconst char *frequency = get_frequency(schedule);\n \tchar *name = launchctl_service_name(frequency);\n \tchar *filename = launchctl_service_filename(name);\n-\n-\tif (safe_create_leading_directories(filename))\n-\t\tdie(_(\"failed to create directories for '%s'\"), filename);\n-\tplist = xfopen(filename, \"w\");\n+\tstruct lock_file lk = LOCK_INIT;\n+\tstatic unsigned long lock_file_timeout_ms = ULONG_MAX;\n+\tstruct strbuf plist = STRBUF_INIT;\n \n \tpreamble = \"\\n\"\n \t\t \"\\n\"\n@@ -1645,7 +1643,7 @@ static int launchctl_schedule_plist(const char *exec_path, enum schedule_priorit\n \t\t \"\\n\"\n \t\t \"StartCalendarInterval\\n\"\n \t\t \"\\n\";\n-\tfprintf(plist, preamble, name, exec_path, exec_path, frequency);\n+\tstrbuf_addf(&plist, preamble, name, exec_path, exec_path, frequency);\n \n \tswitch (schedule) {\n \tcase SCHEDULE_HOURLY:\n@@ -1654,7 +1652,7 @@ static int launchctl_schedule_plist(const char *exec_path, enum schedule_priorit\n \t\t\t \"Minute0\\n\"\n \t\t\t \"\\n\";\n \t\tfor (i = 1; i <= 23; i++)\n-\t\t\tfprintf(plist, repeat, i);\n+\t\t\tstrbuf_addf(&plist, repeat, i);\n \t\tbreak;\n \n \tcase SCHEDULE_DAILY:\n@@ -1664,24 +1662,38 @@ static int launchctl_schedule_plist(const char *exec_path, enum schedule_priorit\n \t\t\t \"Minute0\\n\"\n \t\t\t \"\\n\";\n \t\tfor (i = 1; i <= 6; i++)\n-\t\t\tfprintf(plist, repeat, i);\n+\t\t\tstrbuf_addf(&plist, repeat, i);\n \t\tbreak;\n \n \tcase SCHEDULE_WEEKLY:\n-\t\tfprintf(plist,\n-\t\t\t\"\\n\"\n-\t\t\t\"Day0\\n\"\n-\t\t\t\"Hour0\\n\"\n-\t\t\t\"Minute0\\n\"\n-\t\t\t\"\\n\");\n+\t\tstrbuf_addstr(&plist,\n+\t\t\t \"\\n\"\n+\t\t\t \"Day0\\n\"\n+\t\t\t \"Hour0\\n\"\n+\t\t\t \"Minute0\\n\"\n+\t\t\t \"\\n\");\n \t\tbreak;\n \n \tdefault:\n \t\t/* unreachable */\n \t\tbreak;\n \t}\n-\tfprintf(plist, \"\\n\\n\\n\");\n-\tfclose(plist);\n+\tstrbuf_addstr(&plist, \"\\n\\n\\n\");\n+\n+\tif (safe_create_leading_directories(filename))\n+\t\tdie(_(\"failed to create directories for '%s'\"), filename);\n+\n+\tif ((long)lock_file_timeout_ms < 0 &&\n+\t git_config_get_ulong(\"gc.launchctlplistlocktimeoutms\",\n+\t\t\t\t &lock_file_timeout_ms))\n+\t\tlock_file_timeout_ms = 150;\n+\n+\tfd = hold_lock_file_for_update_timeout(&lk, filename, LOCK_DIE_ON_ERROR,\n+\t\t\t\t\t lock_file_timeout_ms);\n+\n+\tif (write_in_full(fd, plist.buf, plist.len) < 0 ||\n+\t commit_lock_file(&lk))\n+\t\tdie_errno(_(\"could not write '%s'\"), filename);\n \n \t/* bootout might fail if not already running, so ignore */\n \tlaunchctl_boot_plist(0, filename, cmd);\n@@ -1690,6 +1702,7 @@ static int launchctl_schedule_plist(const char *exec_path, enum schedule_priorit\n \n \tfree(filename);\n \tfree(name);\n+\tstrbuf_release(&plist);\n \treturn 0;\n }\n \n-- \ngitgitgadget\n\n"},{"id":"433563","messageId":"b0d6bb0b07f29e68f5bcdf4c69d3d726d77882c0.1629819840.git.gitgitgadget@gmail.com","threadId":"56367","inReplyTo":"pull.1024.git.1629819840.gitgitgadget@gmail.com","subject":"[PATCH 2/2] maintenance: skip bootout/bootstrap when plist is registered","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-08-24T15:44:00Z","receivedAt":"2021-08-24T15:44:13Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nOn macOS, we use launchctl to manage the background maintenance\nschedule. This uses a set of .plist files to describe the schedule, but\nthese files are also registered with 'launchctl bootstrap'. If multiple\n'git maintenance start' commands run concurrently, then they can collide\nreplacing these schedule files and registering them with launchctl.\n\nTo avoid extra launchctl commands, do a check for the .plist files on\ndisk and check if they are registered using 'launchctl list '.\nThis command will return with exit code 0 if it exists, or exit code 113\nif it does not.\n\nWe can test this behavior using the GIT_TEST_MAINT_SCHEDULER environment\nvariable.\n\nSigned-off-by: Derrick Stolee \nSigned-off-by: Johannes Schindelin \n---\n builtin/gc.c | 54 +++++++++++++++++++++++++++++++++++-------\n t/t7900-maintenance.sh | 17 +++++++++++++\n 2 files changed, 62 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex 4a78b497d0e..02bbe889ca6 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -1615,6 +1615,29 @@ static int launchctl_remove_plists(const char *cmd)\n \t\tlaunchctl_remove_plist(SCHEDULE_WEEKLY, cmd);\n }\n \n+static int launchctl_list_contains_plist(const char *name, const char *cmd)\n+{\n+\tint result;\n+\tstruct child_process child = CHILD_PROCESS_INIT;\n+\tchar *uid = launchctl_get_uid();\n+\n+\tstrvec_split(&child.args, cmd);\n+\tstrvec_pushl(&child.args, \"list\", name, NULL);\n+\n+\tchild.no_stderr = 1;\n+\tchild.no_stdout = 1;\n+\n+\tif (start_command(&child))\n+\t\tdie(_(\"failed to start launchctl\"));\n+\n+\tresult = finish_command(&child);\n+\n+\tfree(uid);\n+\n+\t/* Returns failure if 'name' doesn't exist. */\n+\treturn !result;\n+}\n+\n static int launchctl_schedule_plist(const char *exec_path, enum schedule_priority schedule, const char *cmd)\n {\n \tint i, fd;\n@@ -1624,7 +1647,8 @@ static int launchctl_schedule_plist(const char *exec_path, enum schedule_priorit\n \tchar *filename = launchctl_service_filename(name);\n \tstruct lock_file lk = LOCK_INIT;\n \tstatic unsigned long lock_file_timeout_ms = ULONG_MAX;\n-\tstruct strbuf plist = STRBUF_INIT;\n+\tstruct strbuf plist = STRBUF_INIT, plist2 = STRBUF_INIT;\n+\tstruct stat st;\n \n \tpreamble = \"\\n\"\n \t\t \"\\n\"\n@@ -1691,18 +1715,30 @@ static int launchctl_schedule_plist(const char *exec_path, enum schedule_priorit\n \tfd = hold_lock_file_for_update_timeout(&lk, filename, LOCK_DIE_ON_ERROR,\n \t\t\t\t\t lock_file_timeout_ms);\n \n-\tif (write_in_full(fd, plist.buf, plist.len) < 0 ||\n-\t commit_lock_file(&lk))\n-\t\tdie_errno(_(\"could not write '%s'\"), filename);\n-\n-\t/* bootout might fail if not already running, so ignore */\n-\tlaunchctl_boot_plist(0, filename, cmd);\n-\tif (launchctl_boot_plist(1, filename, cmd))\n-\t\tdie(_(\"failed to bootstrap service %s\"), filename);\n+\t/*\n+\t * Does this file already exist? With the intended contents? Is it\n+\t * registered already? Then it does not need to be re-registered.\n+\t */\n+\tif (!stat(filename, &st) && st.st_size == plist.len &&\n+\t strbuf_read_file(&plist2, filename, plist.len) == plist.len &&\n+\t !strbuf_cmp(&plist, &plist2) &&\n+\t launchctl_list_contains_plist(name, cmd))\n+\t\trollback_lock_file(&lk);\n+\telse {\n+\t\tif (write_in_full(fd, plist.buf, plist.len) < 0 ||\n+\t\t commit_lock_file(&lk))\n+\t\t\tdie_errno(_(\"could not write '%s'\"), filename);\n+\n+\t\t/* bootout might fail if not already running, so ignore */\n+\t\tlaunchctl_boot_plist(0, filename, cmd);\n+\t\tif (launchctl_boot_plist(1, filename, cmd))\n+\t\t\tdie(_(\"failed to bootstrap service %s\"), filename);\n+\t}\n \n \tfree(filename);\n \tfree(name);\n \tstrbuf_release(&plist);\n+\tstrbuf_release(&plist2);\n \treturn 0;\n }\n \ndiff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh\nindex 2412d8c5c00..14b30f37ec7 100755\n--- a/t/t7900-maintenance.sh\n+++ b/t/t7900-maintenance.sh\n@@ -578,6 +578,23 @@ test_expect_success 'start and stop macOS maintenance' '\n \ttest_line_count = 0 actual\n '\n \n+test_expect_success 'use launchctl list to prevent extra work' '\n+\t# ensure we are registered\n+\tGIT_TEST_MAINT_SCHEDULER=launchctl:./print-args git maintenance start &&\n+\n+\t# do it again on a fresh args file\n+\trm -f args &&\n+\tGIT_TEST_MAINT_SCHEDULER=launchctl:./print-args git maintenance start &&\n+\n+\tls \"$HOME/Library/LaunchAgents\" >actual &&\n+\tcat >expect <<-\\EOF &&\n+\tlist org.git-scm.git.hourly\n+\tlist org.git-scm.git.daily\n+\tlist org.git-scm.git.weekly\n+\tEOF\n+\ttest_cmp expect args\n+'\n+\n test_expect_success 'start and stop Windows maintenance' '\n \twrite_script print-args <<-\\EOF &&\n \techo $* >>args\n-- \ngitgitgadget\n"},{"id":"433683","messageId":"xmqqwnoajql1.fsf@gitster.g","threadId":"56367","inReplyTo":"pull.1024.git.1629819840.gitgitgadget@gmail.com","subject":"Re: [PATCH 0/2] macos: safeguard git maintenance against highly concurrent operations","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-08-25T00:49:46Z","receivedAt":"2021-08-25T00:49:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Johannes Schindelin via GitGitGadget\" \nwrites:\n\n> Please note that this patch series conflicts with lh/systemd-timers,\n> although in a trivial way: the latter changes the signature of\n> launchctl_schedule_plist() to lose its cmd parameter. The resolution is to\n> adjust the conflicting code to lose the cmd parameter, and also drop it from\n> launchctl_list_contains_plist() (and define it in the same way as\n> launchctl_boot_plist() does). I assume that lh/systemd-timers will advance\n> to next pretty soon; I plan on rebasing this patch series on top of it at\n> that stage.\n\nSounds like a plan.\n\nHere is my attempt to merge lh/systemd-timers into the result of\napplying these two to 'master', with focus on the top part of the\nlaunchctl_schedule_plist(). Sanity-checking is appreciated.\n\ndiff --cc builtin/gc.c\nindex 22e670b508,6a57d0fde5..0000000000\n--- i/builtin/gc.c\n+++ w/builtin/gc.c\n@@@ -1593,48 -1678,26 +1678,50 @@@ static int launchctl_remove_plist(enum \n \treturn result;\n }\n \n- static int launchctl_remove_plists(const char *cmd)\n+ static int launchctl_remove_plists(void)\n {\n- \treturn launchctl_remove_plist(SCHEDULE_HOURLY, cmd) ||\n- \t\tlaunchctl_remove_plist(SCHEDULE_DAILY, cmd) ||\n- \t\tlaunchctl_remove_plist(SCHEDULE_WEEKLY, cmd);\n+ \treturn launchctl_remove_plist(SCHEDULE_HOURLY) ||\n+ \t launchctl_remove_plist(SCHEDULE_DAILY) ||\n+ \t launchctl_remove_plist(SCHEDULE_WEEKLY);\n }\n \n +static int launchctl_list_contains_plist(const char *name, const char *cmd)\n +{\n +\tint result;\n +\tstruct child_process child = CHILD_PROCESS_INIT;\n +\tchar *uid = launchctl_get_uid();\n +\n +\tstrvec_split(&child.args, cmd);\n +\tstrvec_pushl(&child.args, \"list\", name, NULL);\n +\n +\tchild.no_stderr = 1;\n +\tchild.no_stdout = 1;\n +\n +\tif (start_command(&child))\n +\t\tdie(_(\"failed to start launchctl\"));\n +\n +\tresult = finish_command(&child);\n +\n +\tfree(uid);\n +\n +\t/* Returns failure if 'name' doesn't exist. */\n +\treturn !result;\n +}\n +\n- static int launchctl_schedule_plist(const char *exec_path, enum schedule_priority schedule, const char *cmd)\n+ static int launchctl_schedule_plist(const char *exec_path, enum schedule_priority schedule)\n {\n -\tFILE *plist;\n -\tint i;\n +\tint i, fd;\n \tconst char *preamble, *repeat;\n \tconst char *frequency = get_frequency(schedule);\n \tchar *name = launchctl_service_name(frequency);\n \tchar *filename = launchctl_service_filename(name);\n +\tstruct lock_file lk = LOCK_INIT;\n +\tstatic unsigned long lock_file_timeout_ms = ULONG_MAX;\n +\tstruct strbuf plist = STRBUF_INIT, plist2 = STRBUF_INIT;\n +\tstruct stat st;\n++\tconst char *cmd = \"launchctl\";\n \n -\tif (safe_create_leading_directories(filename))\n -\t\tdie(_(\"failed to create directories for '%s'\"), filename);\n -\tplist = xfopen(filename, \"w\");\n -\n++\tget_schedule_cmd(&cmd, NULL);\n \tpreamble = \"\\n\"\n \t\t \"\\n\"\n \t\t \"\"\n@@@ -1687,38 -1750,13 +1774,38 @@@\n \t\t/* unreachable */\n \t\tbreak;\n \t}\n -\tfprintf(plist, \"\\n\\n\\n\");\n -\tfclose(plist);\n +\tstrbuf_addstr(&plist, \"\\n\\n\\n\");\n \n -\t/* bootout might fail if not already running, so ignore */\n -\tlaunchctl_boot_plist(0, filename);\n -\tif (launchctl_boot_plist(1, filename))\n -\t\tdie(_(\"failed to bootstrap service %s\"), filename);\n +\tif (safe_create_leading_directories(filename))\n +\t\tdie(_(\"failed to create directories for '%s'\"), filename);\n +\n +\tif ((long)lock_file_timeout_ms < 0 &&\n +\t git_config_get_ulong(\"gc.launchctlplistlocktimeoutms\",\n +\t\t\t\t &lock_file_timeout_ms))\n +\t\tlock_file_timeout_ms = 150;\n +\n +\tfd = hold_lock_file_for_update_timeout(&lk, filename, LOCK_DIE_ON_ERROR,\n +\t\t\t\t\t lock_file_timeout_ms);\n +\n +\t/*\n +\t * Does this file already exist? With the intended contents? Is it\n +\t * registered already? Then it does not need to be re-registered.\n +\t */\n +\tif (!stat(filename, &st) && st.st_size == plist.len &&\n +\t strbuf_read_file(&plist2, filename, plist.len) == plist.len &&\n +\t !strbuf_cmp(&plist, &plist2) &&\n +\t launchctl_list_contains_plist(name, cmd))\n +\t\trollback_lock_file(&lk);\n +\telse {\n +\t\tif (write_in_full(fd, plist.buf, plist.len) < 0 ||\n +\t\t commit_lock_file(&lk))\n +\t\t\tdie_errno(_(\"could not write '%s'\"), filename);\n +\n +\t\t/* bootout might fail if not already running, so ignore */\n- \t\tlaunchctl_boot_plist(0, filename, cmd);\n- \t\tif (launchctl_boot_plist(1, filename, cmd))\n++\t\tlaunchctl_boot_plist(0, filename);\n++\t\tif (launchctl_boot_plist(1, filename))\n +\t\t\tdie(_(\"failed to bootstrap service %s\"), filename);\n +\t}\n \n \tfree(filename);\n \tfree(name);\n"},{"id":"433730","messageId":"nycvar.QRO.7.76.6.2108251420541.55@tvgsbejvaqbjf.bet","threadId":"56367","inReplyTo":"xmqqwnoajql1.fsf@gitster.g","subject":"Re: [PATCH 0/2] macos: safeguard git maintenance against highly concurrent operations","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2021-08-25T12:23:11Z","receivedAt":"2021-08-25T12:23:19Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Tue, 24 Aug 2021, Junio C Hamano wrote:\n\n> \"Johannes Schindelin via GitGitGadget\" \n> writes:\n>\n> > Please note that this patch series conflicts with lh/systemd-timers,\n> > although in a trivial way: the latter changes the signature of\n> > launchctl_schedule_plist() to lose its cmd parameter. The resolution is to\n> > adjust the conflicting code to lose the cmd parameter, and also drop it from\n> > launchctl_list_contains_plist() (and define it in the same way as\n> > launchctl_boot_plist() does). I assume that lh/systemd-timers will advance\n> > to next pretty soon; I plan on rebasing this patch series on top of it at\n> > that stage.\n>\n> Sounds like a plan.\n>\n> Here is my attempt to merge lh/systemd-timers into the result of\n> applying these two to 'master', with focus on the top part of the\n> launchctl_schedule_plist(). Sanity-checking is appreciated.\n\nMy local version (hence `git reset -hard`'ed away) looked almost precisely\nlike yours, I only added the definition of `cmd` to the top of\n`launchctl_list_contains_plist()` and removed its `cmd` parameter (and\nadjusted the callers). But that's the only difference I can spot.\n\nThanks,\nDscho\n\n>\n> diff --cc builtin/gc.c\n> index 22e670b508,6a57d0fde5..0000000000\n> --- i/builtin/gc.c\n> +++ w/builtin/gc.c\n> @@@ -1593,48 -1678,26 +1678,50 @@@ static int launchctl_remove_plist(enum\n> \treturn result;\n> }\n>\n> - static int launchctl_remove_plists(const char *cmd)\n> + static int launchctl_remove_plists(void)\n> {\n> - \treturn launchctl_remove_plist(SCHEDULE_HOURLY, cmd) ||\n> - \t\tlaunchctl_remove_plist(SCHEDULE_DAILY, cmd) ||\n> - \t\tlaunchctl_remove_plist(SCHEDULE_WEEKLY, cmd);\n> + \treturn launchctl_remove_plist(SCHEDULE_HOURLY) ||\n> + \t launchctl_remove_plist(SCHEDULE_DAILY) ||\n> + \t launchctl_remove_plist(SCHEDULE_WEEKLY);\n> }\n>\n> +static int launchctl_list_contains_plist(const char *name, const char *cmd)\n> +{\n> +\tint result;\n> +\tstruct child_process child = CHILD_PROCESS_INIT;\n> +\tchar *uid = launchctl_get_uid();\n> +\n> +\tstrvec_split(&child.args, cmd);\n> +\tstrvec_pushl(&child.args, \"list\", name, NULL);\n> +\n> +\tchild.no_stderr = 1;\n> +\tchild.no_stdout = 1;\n> +\n> +\tif (start_command(&child))\n> +\t\tdie(_(\"failed to start launchctl\"));\n> +\n> +\tresult = finish_command(&child);\n> +\n> +\tfree(uid);\n> +\n> +\t/* Returns failure if 'name' doesn't exist. */\n> +\treturn !result;\n> +}\n> +\n> - static int launchctl_schedule_plist(const char *exec_path, enum schedule_priority schedule, const char *cmd)\n> + static int launchctl_schedule_plist(const char *exec_path, enum schedule_priority schedule)\n> {\n> -\tFILE *plist;\n> -\tint i;\n> +\tint i, fd;\n> \tconst char *preamble, *repeat;\n> \tconst char *frequency = get_frequency(schedule);\n> \tchar *name = launchctl_service_name(frequency);\n> \tchar *filename = launchctl_service_filename(name);\n> +\tstruct lock_file lk = LOCK_INIT;\n> +\tstatic unsigned long lock_file_timeout_ms = ULONG_MAX;\n> +\tstruct strbuf plist = STRBUF_INIT, plist2 = STRBUF_INIT;\n> +\tstruct stat st;\n> ++\tconst char *cmd = \"launchctl\";\n>\n> -\tif (safe_create_leading_directories(filename))\n> -\t\tdie(_(\"failed to create directories for '%s'\"), filename);\n> -\tplist = xfopen(filename, \"w\");\n> -\n> ++\tget_schedule_cmd(&cmd, NULL);\n> \tpreamble = \"\\n\"\n> \t\t \"\\n\"\n> \t\t \"\"\n> @@@ -1687,38 -1750,13 +1774,38 @@@\n> \t\t/* unreachable */\n> \t\tbreak;\n> \t}\n> -\tfprintf(plist, \"\\n\\n\\n\");\n> -\tfclose(plist);\n> +\tstrbuf_addstr(&plist, \"\\n\\n\\n\");\n>\n> -\t/* bootout might fail if not already running, so ignore */\n> -\tlaunchctl_boot_plist(0, filename);\n> -\tif (launchctl_boot_plist(1, filename))\n> -\t\tdie(_(\"failed to bootstrap service %s\"), filename);\n> +\tif (safe_create_leading_directories(filename))\n> +\t\tdie(_(\"failed to create directories for '%s'\"), filename);\n> +\n> +\tif ((long)lock_file_timeout_ms < 0 &&\n> +\t git_config_get_ulong(\"gc.launchctlplistlocktimeoutms\",\n> +\t\t\t\t &lock_file_timeout_ms))\n> +\t\tlock_file_timeout_ms = 150;\n> +\n> +\tfd = hold_lock_file_for_update_timeout(&lk, filename, LOCK_DIE_ON_ERROR,\n> +\t\t\t\t\t lock_file_timeout_ms);\n> +\n> +\t/*\n> +\t * Does this file already exist? With the intended contents? Is it\n> +\t * registered already? Then it does not need to be re-registered.\n> +\t */\n> +\tif (!stat(filename, &st) && st.st_size == plist.len &&\n> +\t strbuf_read_file(&plist2, filename, plist.len) == plist.len &&\n> +\t !strbuf_cmp(&plist, &plist2) &&\n> +\t launchctl_list_contains_plist(name, cmd))\n> +\t\trollback_lock_file(&lk);\n> +\telse {\n> +\t\tif (write_in_full(fd, plist.buf, plist.len) < 0 ||\n> +\t\t commit_lock_file(&lk))\n> +\t\t\tdie_errno(_(\"could not write '%s'\"), filename);\n> +\n> +\t\t/* bootout might fail if not already running, so ignore */\n> - \t\tlaunchctl_boot_plist(0, filename, cmd);\n> - \t\tif (launchctl_boot_plist(1, filename, cmd))\n> ++\t\tlaunchctl_boot_plist(0, filename);\n> ++\t\tif (launchctl_boot_plist(1, filename))\n> +\t\t\tdie(_(\"failed to bootstrap service %s\"), filename);\n> +\t}\n>\n> \tfree(filename);\n> \tfree(name);\n>\n"},{"id":"435197","messageId":"patch-1.1-93adb856b0c-20210909T012244Z-avarab@gmail.com","threadId":"56367","inReplyTo":"b0d6bb0b07f29e68f5bcdf4c69d3d726d77882c0.1629819840.git.gitgitgadget@gmail.com","subject":"[PATCH] gc: remove unused launchctl_get_uid() call","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-09T01:25:05Z","receivedAt":"2021-09-09T01:35:53Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"When the launchctl_boot_plist() function was added in\na16eb6b1ff3 (maintenance: skip bootout/bootstrap when plist is\nregistered, 2021-08-24), an unused call to launchctl_get_uid() was\nadded along with it. That call appears to have been copy/pasted from\nlaunchctl_boot_plist().\n\nSince we can remove that, we can also get rid of the \"result\"\nvariable, whose only purpose was allow for the free() between its\nassignment and the return. That pattern also appears to have been\ncopy/pasted from launchctl_boot_plist().\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n\nI happen to have a local topic that refactored the launchctl_get_uid()\nfunction away, that didn't compile with the updated \"master\", I\nfigured I'd need to add it back since it had a new user, but as it\nturns out that hopefully won't be needed.\n\n builtin/gc.c | 8 +-------\n 1 file changed, 1 insertion(+), 7 deletions(-)\n\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex 9a48eb535fb..b024f0e04e9 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -1602,9 +1602,7 @@ static int launchctl_remove_plists(const char *cmd)\n \n static int launchctl_list_contains_plist(const char *name, const char *cmd)\n {\n-\tint result;\n \tstruct child_process child = CHILD_PROCESS_INIT;\n-\tchar *uid = launchctl_get_uid();\n \n \tstrvec_split(&child.args, cmd);\n \tstrvec_pushl(&child.args, \"list\", name, NULL);\n@@ -1615,12 +1613,8 @@ static int launchctl_list_contains_plist(const char *name, const char *cmd)\n \tif (start_command(&child))\n \t\tdie(_(\"failed to start launchctl\"));\n \n-\tresult = finish_command(&child);\n-\n-\tfree(uid);\n-\n \t/* Returns failure if 'name' doesn't exist. */\n-\treturn !result;\n+\treturn !finish_command(&child);\n }\n \n static int launchctl_schedule_plist(const char *exec_path, enum schedule_priority schedule, const char *cmd)\n-- \n2.33.0.825.gdc3f7a2a6c7\n\n"},{"id":"435233","messageId":"nycvar.QRO.7.76.6.2109091222260.59@tvgsbejvaqbjf.bet","threadId":"56367","inReplyTo":"patch-1.1-93adb856b0c-20210909T012244Z-avarab@gmail.com","subject":"Re: [PATCH] gc: remove unused launchctl_get_uid() call","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2021-09-09T10:24:07Z","receivedAt":"2021-09-09T10:24:20Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Ævar,\n\nOn Thu, 9 Sep 2021, Ævar Arnfjörð Bjarmason wrote:\n\n> When the launchctl_boot_plist() function was added in\n> a16eb6b1ff3 (maintenance: skip bootout/bootstrap when plist is\n> registered, 2021-08-24), an unused call to launchctl_get_uid() was\n> added along with it. That call appears to have been copy/pasted from\n> launchctl_boot_plist().\n>\n> Since we can remove that, we can also get rid of the \"result\"\n> variable, whose only purpose was allow for the free() between its\n> assignment and the return. That pattern also appears to have been\n> copy/pasted from launchctl_boot_plist().\n\nI don't find the most crucial information in that commit message: what is\nthe fall-out of the removal of this call?\n\nSuch an analysis (_with_ a summary of it in the commit message) is\ndefinitely required. And it should not be left as an exercise for the\nreader.\n\nCiao,\nJohannes\n"},{"id":"435249","messageId":"87bl52dkv1.fsf@evledraar.gmail.com","threadId":"56367","inReplyTo":"nycvar.QRO.7.76.6.2109091222260.59@tvgsbejvaqbjf.bet","subject":"Re: [PATCH] gc: remove unused launchctl_get_uid() call","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-09T11:53:58Z","receivedAt":"2021-09-09T13:37:04Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Sep 09 2021, Johannes Schindelin wrote:\n\n> Hi Ævar,\n>\n> On Thu, 9 Sep 2021, Ævar Arnfjörð Bjarmason wrote:\n>\n>> When the launchctl_boot_plist() function was added in\n>> a16eb6b1ff3 (maintenance: skip bootout/bootstrap when plist is\n>> registered, 2021-08-24), an unused call to launchctl_get_uid() was\n>> added along with it. That call appears to have been copy/pasted from\n>> launchctl_boot_plist().\n>>\n>> Since we can remove that, we can also get rid of the \"result\"\n>> variable, whose only purpose was allow for the free() between its\n>> assignment and the return. That pattern also appears to have been\n>> copy/pasted from launchctl_boot_plist().\n>\n> I don't find the most crucial information in that commit message: what is\n> the fall-out of the removal of this call?\n>\n> Such an analysis (_with_ a summary of it in the commit message) is\n> definitely required. And it should not be left as an exercise for the\n> reader.\n\nDo you mean an assurance to the reader that the removed code doesn't\nhave any side-effects? E.g. an addition of\n\n As the patch shows the returned value wasn't used at all in this\n function, the launchctl_get_uid() function itself just calls\n xstrfmt() and getuid(), neither of which have any subtle global\n side-effects, so this removal is safe.\n\n?\n"},{"id":"435250","messageId":"nycvar.QRO.7.76.6.2109091541370.59@tvgsbejvaqbjf.bet","threadId":"56367","inReplyTo":"87bl52dkv1.fsf@evledraar.gmail.com","subject":"Re: [PATCH] gc: remove unused launchctl_get_uid() call","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2021-09-09T13:45:33Z","receivedAt":"2021-09-09T13:47:49Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Ævar,\n\nOn Thu, 9 Sep 2021, Ævar Arnfjörð Bjarmason wrote:\n\n>\n> On Thu, Sep 09 2021, Johannes Schindelin wrote:\n>\n> > Hi Ævar,\n> >\n> > On Thu, 9 Sep 2021, Ævar Arnfjörð Bjarmason wrote:\n> >\n> >> When the launchctl_boot_plist() function was added in\n> >> a16eb6b1ff3 (maintenance: skip bootout/bootstrap when plist is\n> >> registered, 2021-08-24), an unused call to launchctl_get_uid() was\n> >> added along with it. That call appears to have been copy/pasted from\n> >> launchctl_boot_plist().\n> >>\n> >> Since we can remove that, we can also get rid of the \"result\"\n> >> variable, whose only purpose was allow for the free() between its\n> >> assignment and the return. That pattern also appears to have been\n> >> copy/pasted from launchctl_boot_plist().\n> >\n> > I don't find the most crucial information in that commit message: what is\n> > the fall-out of the removal of this call?\n> >\n> > Such an analysis (_with_ a summary of it in the commit message) is\n> > definitely required. And it should not be left as an exercise for the\n> > reader.\n>\n> Do you mean an assurance to the reader that the removed code doesn't\n> have any side-effects? E.g. an addition of\n>\n> As the patch shows the returned value wasn't used at all in this\n> function, the launchctl_get_uid() function itself just calls\n> xstrfmt() and getuid(), neither of which have any subtle global\n> side-effects, so this removal is safe.\n>\n> ?\n\nYes. You want to refrain from forcing every reader to have to go look at\nthe definition of that function at that revision. The accumulated time\nspent tallies up rather in disfavor of doing the work diligently on the\ncontributor's side and save every reader some time. I mean, you forced me\nto spend the time, and then to spend more time to point out the missing\nanalysis, and then you provided the paragraph as a question, forcing me to\nspend even more time on answering. All this time could have been saved in\nthe first place. In this instance, it is too late to do anything about it.\nBut I'm sure you plan on contributing other patches. Hopefully it will be\nmore efficient next time.\n\nCiao,\nJohannes\n"},{"id":"435288","messageId":"xmqqwnnpzkex.fsf@gitster.g","threadId":"56367","inReplyTo":"nycvar.QRO.7.76.6.2109091541370.59@tvgsbejvaqbjf.bet","subject":"Re: [PATCH] gc: remove unused launchctl_get_uid() call","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-09-09T18:13:58Z","receivedAt":"2021-09-09T18:14:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> Yes. You want to refrain from forcing every reader to have to go look at\n> the definition of that function at that revision. The accumulated time\n> spent tallies up rather in disfavor of doing the work diligently on the\n> contributor's side and save every reader some time. I mean, you forced me\n> to spend the time, and then to spend more time to point out the missing\n> analysis, and then you provided the paragraph as a question, forcing me to\n> spend even more time on answering. All this time could have been saved in\n> the first place. In this instance, it is too late to do anything about it.\n\nWhile I very much like to see that you subscribe to the principle,\nand I do wish that the \"question\" were posed as a comment after the\nthree-dash-line in a rerolled patch that is written under the\nassumption that the answer to the question is \"yes\", which would\nhave saved one extra roundtrip, I do not think the question was not\nsomething to be scolded like the above.\n\nAll writers tend to assume that their readers know more than they\ndo, and that is where the \"do not force the reader to know or go\nlook at things if they don't\" principle comes from. But you're\nassuming that your \"what's the fallout?\" question did not need\nclarification, but it is understandable if it weren't.\n\n> But I'm sure you plan on contributing other patches. Hopefully it will be\n> more efficient next time.\n\nThanks.\n"},{"id":"435614","messageId":"patch-v2-1.1-794e68e7722-20210912T002334Z-avarab@gmail.com","threadId":"56367","inReplyTo":"87bl52dkv1.fsf@evledraar.gmail.com","subject":"[PATCH v2] gc: remove unused launchctl_get_uid() call","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-12T00:24:40Z","receivedAt":"2021-09-12T00:24:55Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"When the launchctl_boot_plist() function was added in\na16eb6b1ff3 (maintenance: skip bootout/bootstrap when plist is\nregistered, 2021-08-24), an unused call to launchctl_get_uid() was\nadded along with it. That call appears to have been copy/pasted from\nlaunchctl_boot_plist().\n\nSince we can remove that, we can also get rid of the \"result\"\nvariable, whose only purpose was allow for the free() between its\nassignment and the return. That pattern also appears to have been\ncopy/pasted from launchctl_boot_plist().\n\nAs the patch shows the returned value from launchctl_get_uid() wasn't\nused at all in this function. The launchctl_get_uid() function itself\njust calls xstrfmt() and getuid(), neither of which have any subtle\nglobal side-effects, so this removal is safe.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n\nAddresses a comment about the clarity of the commit message in v1:\nhttps://lore.kernel.org/git/87bl52dkv1.fsf@evledraar.gmail.com/\n\nRange-diff against v1:\n1: 93adb856b0c ! 1: 794e68e7722 gc: remove unused launchctl_get_uid() call\n @@ Commit message\n assignment and the return. That pattern also appears to have been\n copy/pasted from launchctl_boot_plist().\n \n + As the patch shows the returned value from launchctl_get_uid() wasn't\n + used at all in this function. The launchctl_get_uid() function itself\n + just calls xstrfmt() and getuid(), neither of which have any subtle\n + global side-effects, so this removal is safe.\n +\n Signed-off-by: Ævar Arnfjörð Bjarmason \n \n ## builtin/gc.c ##\n\n builtin/gc.c | 8 +-------\n 1 file changed, 1 insertion(+), 7 deletions(-)\n\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex 43c36024cbe..db76af4f31c 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -1602,9 +1602,7 @@ static int launchctl_remove_plists(const char *cmd)\n \n static int launchctl_list_contains_plist(const char *name, const char *cmd)\n {\n-\tint result;\n \tstruct child_process child = CHILD_PROCESS_INIT;\n-\tchar *uid = launchctl_get_uid();\n \n \tstrvec_split(&child.args, cmd);\n \tstrvec_pushl(&child.args, \"list\", name, NULL);\n@@ -1615,12 +1613,8 @@ static int launchctl_list_contains_plist(const char *name, const char *cmd)\n \tif (start_command(&child))\n \t\tdie(_(\"failed to start launchctl\"));\n \n-\tresult = finish_command(&child);\n-\n-\tfree(uid);\n-\n \t/* Returns failure if 'name' doesn't exist. */\n-\treturn !result;\n+\treturn !finish_command(&child);\n }\n \n static int launchctl_schedule_plist(const char *exec_path, enum schedule_priority schedule, const char *cmd)\n-- \n2.33.0.998.ga4d44345d43\n\n"}]}