{"thread":{"id":"62705","subject":"[PATCH] builtin/gc: Ignore random minute field when registering macOS services","startedAt":"2024-12-28T03:13:16Z","lastAt":"2024-12-28T18:58:04Z","messageCount":3,"participants":["Byoungchan Lee","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"509666","messageId":"CABbmNUHA5F_ZMzngYHBzuqLLJ-3+87yVU_gZxGxhCYuMP64QKg@mail.gmail.com","threadId":"62705","inReplyTo":null,"subject":"[PATCH] builtin/gc: Ignore random minute field when registering macOS services","fromName":"Byoungchan Lee","fromEmail":"byoungchan.lee@gmx.com","sentAt":"2024-12-28T03:13:03Z","receivedAt":"2024-12-28T03:13:16Z","isPatch":true,"sender":{"key":"byoungchan.lee@gmx.com","avatar":null},"body":"In macOS, `git-maintenance` registers several launchctl services\nto periodically run Git maintenance tasks by creating plist files\nin `~/Library/LaunchAgents/`.\nTo avoid re-registering services unnecessarily, we check if a service\nis already registered by verifying the existence and contents\nof the corresponding plist file.\n\nHowever, these plist files include a random value in the minute field\nto distribute maintenance tasks over time. Because this value changes\nwith each registration attempt, a direct comparison of the entire file\n(via `strbuf_cmp()`) often fails, causing services to be erroneously\nre-registered. As a result, users may see multiple services registered\nand receive repeated “Background Items Added” notifications.\n\nTo resolve this, introduce `launchctl_plist_cmp_ignore_minute()`,\nwhich compares the content of the plist file while ignoring\nthe random minute field. This ensures that services are not\nneedlessly re-registered when the only difference in the plist file\nis the randomized minute value.\n\nSigned-off-by: Byoungchan Lee \n---\n builtin/gc.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++----\n 1 file changed, 47 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex a9b1c36de2..6405f4d332 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -1951,6 +1951,51 @@ static char *launchctl_get_uid(void)\n return xstrfmt(\"gui/%d\", getuid());\n }\n\n+/*\n+ * Compare two buffers that represent launchctl property lists, but ignore\n+ * lines that contain Minute... because the\n+ * minute values are not significant for comparison.\n+ */\n+static int launchctl_plist_cmp_ignore_minute(const struct strbuf *a,\n+ const struct strbuf *b)\n+{\n+ char *buf_a = xstrndup(a->buf, a->len);\n+ char *buf_b = xstrndup(b->buf, b->len);\n+ char *line_a = buf_a;\n+ char *line_b = buf_b;\n+ int result = 0;\n+\n+ while (line_a && line_b) {\n+ char *next_line_a = strchr(line_a, '\\n');\n+ char *next_line_b = strchr(line_b, '\\n');\n+\n+ if (next_line_a)\n+ *next_line_a = '\\0';\n+ if (next_line_b)\n+ *next_line_b = '\\0';\n+\n+ if (strstr(line_a, \"Minute\") &&\n+ strstr(line_a, \"\") &&\n+ strstr(line_b, \"Minute\") &&\n+ strstr(line_b, \"\")) {\n+ line_a = next_line_a ? next_line_a + 1 : NULL;\n+ line_b = next_line_b ? next_line_b + 1 : NULL;\n+ continue;\n+ }\n+\n+ result = strcmp(line_a, line_b);\n+ if (result)\n+ break;\n+\n+ line_a = next_line_a ? next_line_a + 1 : NULL;\n+ line_b = next_line_b ? next_line_b + 1 : NULL;\n+ }\n+\n+ free(buf_a);\n+ free(buf_b);\n+ return result;\n+}\n+\n static int launchctl_boot_plist(int enable, const char *filename)\n {\n char *cmd;\n@@ -2022,7 +2067,6 @@ static int launchctl_schedule_plist(const char\n*exec_path, enum schedule_priorit\n struct lock_file lk = LOCK_INIT;\n static unsigned long lock_file_timeout_ms = ULONG_MAX;\n struct strbuf plist = STRBUF_INIT, plist2 = STRBUF_INIT;\n- struct stat st;\n char *cmd;\n int minute = get_random_minute();\n\n@@ -2100,9 +2144,8 @@ static int launchctl_schedule_plist(const char\n*exec_path, enum schedule_priorit\n * Does this file already exist? With the intended contents? Is it\n * registered already? Then it does not need to be re-registered.\n */\n- if (!stat(filename, &st) && st.st_size == plist.len &&\n- strbuf_read_file(&plist2, filename, plist.len) == plist.len &&\n- !strbuf_cmp(&plist, &plist2) &&\n+ if (strbuf_read_file(&plist2, filename, plist.len) >= 0 &&\n+ !launchctl_plist_cmp_ignore_minute(&plist, &plist2) &&\n launchctl_list_contains_plist(name, cmd))\n rollback_lock_file(&lk);\n else {\n-- \n2.47.1\n"},{"id":"509689","messageId":"xmqqmsgf3faf.fsf@gitster.g","threadId":"62705","inReplyTo":"CABbmNUHA5F_ZMzngYHBzuqLLJ-3+87yVU_gZxGxhCYuMP64QKg@mail.gmail.com","subject":"Re: [PATCH] builtin/gc: Ignore random minute field when registering macOS services","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-12-28T17:09:44Z","receivedAt":"2024-12-28T17:09:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Byoungchan Lee writes:\n\n> In macOS, `git-maintenance` registers several launchctl services\n> to periodically run Git maintenance tasks by creating plist files\n> in `~/Library/LaunchAgents/`.\n> To avoid re-registering services unnecessarily, we check if a service\n> is already registered by verifying the existence and contents\n> of the corresponding plist file.\n>\n> However, these plist files include a random value in the minute field\n> to distribute maintenance tasks over time. Because this value changes\n> with each registration attempt, a direct comparison of the entire file\n> (via `strbuf_cmp()`) often fails, causing services to be erroneously\n> re-registered. As a result, users may see multiple services registered\n> and receive repeated “Background Items Added” notifications.\n>\n> To resolve this, introduce `launchctl_plist_cmp_ignore_minute()`,\n> which compares the content of the plist file while ignoring\n> the random minute field. This ensures that services are not\n> needlessly re-registered when the only difference in the plist file\n> is the randomized minute value.\n>\n> Signed-off-by: Byoungchan Lee \n> ---\n> builtin/gc.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++----\n> 1 file changed, 47 insertions(+), 4 deletions(-)\n\nA few comments on the design.\n\n\"ah, the minute part needs to be ignored when comparing with the\nexisting configuration\" smells like a poor strategy for two reasons.\n\n (1) maybe the part that gets fuzzed would become different over\n time and this new code may need to ignore differently.\n\n (2) the need to compare with the existing configuration would not\n be limited to macOS, would it? If anybody wants to avoid\n re-registering with the same configuration again, such a\n selective comparison needs to be reimplemented on every\n backends.\n\nI wonder if we want to tweak get_random_minute() logic to be\ndeterministic to avoid need for such a fuzzy comparison at its root.\n\nA few possible ideas are to read the value from the existing\nconfiguration and reuse that instead of coming up with a new random\nvalue, or to hash the hostname (or something similar that is\nreasonably stable) to use the result as the seed. Derrick, what do\nyou think?\n\n\nAs to the patch, as I suspect we may not want a code with the\nproposed design, I won't look at it deeply at this point, but please\nconsult Documentation/CodingGuidelines and/or make sure your patch\nwill not be whitespace damaged during transit from your repository\nto people's mailbox. For example:\n\n> diff --git a/builtin/gc.c b/builtin/gc.c\n> index a9b1c36de2..6405f4d332 100644\n> --- a/builtin/gc.c\n> +++ b/builtin/gc.c\n> @@ -1951,6 +1951,51 @@ static char *launchctl_get_uid(void)\n> return xstrfmt(\"gui/%d\", getuid());\n> }\n\nThese two lines are supposed to be what already appear in our\ncodebase, but we of course do not use a single-space indent. There\nis something funny going on.\n\nThanks.\n"},{"id":"509693","messageId":"24ba736c-6700-4037-8147-40ae8c789969@gmx.com","threadId":"62705","inReplyTo":"xmqqmsgf3faf.fsf@gitster.g","subject":"Re: [PATCH] builtin/gc: Ignore random minute field when registering macOS services","fromName":"Byoungchan Lee","fromEmail":"byoungchan.lee@gmx.com","sentAt":"2024-12-28T18:57:52Z","receivedAt":"2024-12-28T18:58:04Z","isPatch":true,"sender":{"key":"byoungchan.lee@gmx.com","avatar":null},"body":"On 24. 12. 29. 02:09, Junio C Hamano wrote:\n> Byoungchan Lee writes:\n>\n>> In macOS, `git-maintenance` registers several launchctl services\n>> to periodically run Git maintenance tasks by creating plist files\n>> in `~/Library/LaunchAgents/`.\n>> To avoid re-registering services unnecessarily, we check if a service\n>> is already registered by verifying the existence and contents\n>> of the corresponding plist file.\n>>\n>> However, these plist files include a random value in the minute field\n>> to distribute maintenance tasks over time. Because this value changes\n>> with each registration attempt, a direct comparison of the entire file\n>> (via `strbuf_cmp()`) often fails, causing services to be erroneously\n>> re-registered. As a result, users may see multiple services registered\n>> and receive repeated “Background Items Added” notifications.\n>>\n>> To resolve this, introduce `launchctl_plist_cmp_ignore_minute()`,\n>> which compares the content of the plist file while ignoring\n>> the random minute field. This ensures that services are not\n>> needlessly re-registered when the only difference in the plist file\n>> is the randomized minute value.\n>>\n>> Signed-off-by: Byoungchan Lee \n>> ---\n>> builtin/gc.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++----\n>> 1 file changed, 47 insertions(+), 4 deletions(-)\n> A few comments on the design.\n>\n> \"ah, the minute part needs to be ignored when comparing with the\n> existing configuration\" smells like a poor strategy for two reasons.\n>\n> (1) maybe the part that gets fuzzed would become different over\n> time and this new code may need to ignore differently.\n>\n> (2) the need to compare with the existing configuration would not\n> be limited to macOS, would it? If anybody wants to avoid\n> re-registering with the same configuration again, such a\n> selective comparison needs to be reimplemented on every\n> backends.\n\n\nFor justifying my design, I believe we do not need any additional\nrandomization.\n\nMinute-level randomization is sufficient for tasks repeated on an hourly\nbasis,\n\nso no further extensibility is necessary.\n\n\nI also aimed for practicality, because this issue (the repeated\nannoyance messages)\n\nonly occurred on macOS. I also use Linux with systemd for programming, \n\nbut Linux does not bother me. I am unsure about other operating systems.\n\n> I wonder if we want to tweak get_random_minute() logic to be\n> deterministic to avoid need for such a fuzzy comparison at its root.\n>\n> A few possible ideas are to read the value from the existing\n> configuration and reuse that instead of coming up with a new random\n> value, or to hash the hostname (or something similar that is\n> reasonably stable) to use the result as the seed. Derrick, what do\n> you think?\n>\n>\n> As to the patch, as I suspect we may not want a code with the\n> proposed design, I won't look at it deeply at this point, but please\n> consult Documentation/CodingGuidelines and/or make sure your patch\n> will not be whitespace damaged during transit from your repository\n> to people's mailbox. For example:\n>\n>> diff --git a/builtin/gc.c b/builtin/gc.c\n>> index a9b1c36de2..6405f4d332 100644\n>> --- a/builtin/gc.c\n>> +++ b/builtin/gc.c\n>> @@ -1951,6 +1951,51 @@ static char *launchctl_get_uid(void)\n>> return xstrfmt(\"gui/%d\", getuid());\n>> }\n> These two lines are supposed to be what already appear in our\n> codebase, but we of course do not use a single-space indent. There\n> is something funny going on.\n>\n> Thanks.\n>\n"}]}