[PATCH] builtin/gc: Ignore random minute field when registering macOS services

[PATCH] builtin/gc: Ignore random minute field when registering macOS services

[Byoungchan Lee]

In macOS, `git-maintenance` registers several launchctl services
to periodically run Git maintenance tasks by creating plist files
in `~/Library/LaunchAgents/`.
To avoid re-registering services unnecessarily, we check if a service
is already registered by verifying the existence and contents
of the corresponding plist file.

However, these plist files include a random value in the minute field
to distribute maintenance tasks over time. Because this value changes
with each registration attempt, a direct comparison of the entire file
(via `strbuf_cmp()`) often fails, causing services to be erroneously
re-registered. As a result, users may see multiple services registered
and receive repeated “Background Items Added” notifications.

To resolve this, introduce `launchctl_plist_cmp_ignore_minute()`,
which compares the content of the plist file while ignoring
the random minute field. This ensures that services are not
needlessly re-registered when the only difference in the plist file
is the randomized minute value.

Signed-off-by: Byoungchan Lee <byoungchan.lee@gmx.com>
---
 builtin/gc.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 47 insertions(+), 4 deletions(-)

diff --git a/builtin/gc.c b/builtin/gc.c
index a9b1c36de2..6405f4d332 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -1951,6 +1951,51 @@ static char *launchctl_get_uid(void)
  return xstrfmt("gui/%d", getuid());
 }

+/*
+ * Compare two buffers that represent launchctl property lists, but ignore
+ * lines that contain <key>Minute</key><integer>...</integer> because the
+ * minute values are not significant for comparison.
+ */
+static int launchctl_plist_cmp_ignore_minute(const struct strbuf *a,
+          const struct strbuf *b)
+{
+ char *buf_a = xstrndup(a->buf, a->len);
+ char *buf_b = xstrndup(b->buf, b->len);
+ char *line_a = buf_a;
+ char *line_b = buf_b;
+ int result = 0;
+
+ while (line_a && line_b) {
+  char *next_line_a = strchr(line_a, '\n');
+  char *next_line_b = strchr(line_b, '\n');
+
+  if (next_line_a)
+   *next_line_a = '\0';
+  if (next_line_b)
+   *next_line_b = '\0';
+
+  if (strstr(line_a, "<key>Minute</key><integer>") &&
+      strstr(line_a, "</integer>") &&
+      strstr(line_b, "<key>Minute</key><integer>") &&
+      strstr(line_b, "</integer>")) {
+   line_a = next_line_a ? next_line_a + 1 : NULL;
+   line_b = next_line_b ? next_line_b + 1 : NULL;
+   continue;
+  }
+
+  result = strcmp(line_a, line_b);
+  if (result)
+   break;
+
+  line_a = next_line_a ? next_line_a + 1 : NULL;
+  line_b = next_line_b ? next_line_b + 1 : NULL;
+ }
+
+ free(buf_a);
+ free(buf_b);
+ return result;
+}
+
 static int launchctl_boot_plist(int enable, const char *filename)
 {
  char *cmd;
@@ -2022,7 +2067,6 @@ static int launchctl_schedule_plist(const char
*exec_path, enum schedule_priorit
  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;
  char *cmd;
  int minute = get_random_minute();

@@ -2100,9 +2144,8 @@ static int launchctl_schedule_plist(const char
*exec_path, enum schedule_priorit
   * 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) &&
+ if (strbuf_read_file(&plist2, filename, plist.len) >= 0 &&
+     !launchctl_plist_cmp_ignore_minute(&plist, &plist2) &&
      launchctl_list_contains_plist(name, cmd))
   rollback_lock_file(&lk);
  else {
-- 
2.47.1

Re: [PATCH] builtin/gc: Ignore random minute field when registering macOS services

[Junio C Hamano]

Byoungchan Lee <byoungchan.lee@gmx.com> writes:

> In macOS, `git-maintenance` registers several launchctl services
> to periodically run Git maintenance tasks by creating plist files
> in `~/Library/LaunchAgents/`.
> To avoid re-registering services unnecessarily, we check if a service
> is already registered by verifying the existence and contents
> of the corresponding plist file.
>
> However, these plist files include a random value in the minute field
> to distribute maintenance tasks over time. Because this value changes
> with each registration attempt, a direct comparison of the entire file
> (via `strbuf_cmp()`) often fails, causing services to be erroneously
> re-registered. As a result, users may see multiple services registered
> and receive repeated “Background Items Added” notifications.
>
> To resolve this, introduce `launchctl_plist_cmp_ignore_minute()`,
> which compares the content of the plist file while ignoring
> the random minute field. This ensures that services are not
> needlessly re-registered when the only difference in the plist file
> is the randomized minute value.
>
> Signed-off-by: Byoungchan Lee <byoungchan.lee@gmx.com>
> ---
>  builtin/gc.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 47 insertions(+), 4 deletions(-)

A few comments on the design.

"ah, the minute part needs to be ignored when comparing with the
existing configuration" smells like a poor strategy for two reasons.

   (1) maybe the part that gets fuzzed would become different over
       time and this new code may need to ignore differently.

   (2) the need to compare with the existing configuration would not
       be limited to macOS, would it?  If anybody wants to avoid
       re-registering with the same configuration again, such a
       selective comparison needs to be reimplemented on every
       backends.

I wonder if we want to tweak get_random_minute() logic to be
deterministic to avoid need for such a fuzzy comparison at its root.

A few possible ideas are to read the value from the existing
configuration and reuse that instead of coming up with a new random
value, or to hash the hostname (or something similar that is
reasonably stable) to use the result as the seed.  Derrick, what do
you think?


As to the patch, as I suspect we may not want a code with the
proposed design, I won't look at it deeply at this point, but please
consult Documentation/CodingGuidelines and/or make sure your patch
will not be whitespace damaged during transit from your repository
to people's mailbox.  For example:

> diff --git a/builtin/gc.c b/builtin/gc.c
> index a9b1c36de2..6405f4d332 100644
> --- a/builtin/gc.c
> +++ b/builtin/gc.c
> @@ -1951,6 +1951,51 @@ static char *launchctl_get_uid(void)
>   return xstrfmt("gui/%d", getuid());
>  }

These two lines are supposed to be what already appear in our
codebase, but we of course do not use a single-space indent.  There
is something funny going on.

Thanks.

Re: [PATCH] builtin/gc: Ignore random minute field when registering macOS services

[Byoungchan Lee]

On 24. 12. 29. 02:09, Junio C Hamano wrote:
> Byoungchan Lee <byoungchan.lee@gmx.com> writes:
>
>> In macOS, `git-maintenance` registers several launchctl services
>> to periodically run Git maintenance tasks by creating plist files
>> in `~/Library/LaunchAgents/`.
>> To avoid re-registering services unnecessarily, we check if a service
>> is already registered by verifying the existence and contents
>> of the corresponding plist file.
>>
>> However, these plist files include a random value in the minute field
>> to distribute maintenance tasks over time. Because this value changes
>> with each registration attempt, a direct comparison of the entire file
>> (via `strbuf_cmp()`) often fails, causing services to be erroneously
>> re-registered. As a result, users may see multiple services registered
>> and receive repeated “Background Items Added” notifications.
>>
>> To resolve this, introduce `launchctl_plist_cmp_ignore_minute()`,
>> which compares the content of the plist file while ignoring
>> the random minute field. This ensures that services are not
>> needlessly re-registered when the only difference in the plist file
>> is the randomized minute value.
>>
>> Signed-off-by: Byoungchan Lee <byoungchan.lee@gmx.com>
>> ---
>>  builtin/gc.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++----
>>  1 file changed, 47 insertions(+), 4 deletions(-)
> A few comments on the design.
>
> "ah, the minute part needs to be ignored when comparing with the
> existing configuration" smells like a poor strategy for two reasons.
>
>    (1) maybe the part that gets fuzzed would become different over
>        time and this new code may need to ignore differently.
>
>    (2) the need to compare with the existing configuration would not
>        be limited to macOS, would it?  If anybody wants to avoid
>        re-registering with the same configuration again, such a
>        selective comparison needs to be reimplemented on every
>        backends.


For justifying my design, I believe we do not need any additional
randomization.

Minute-level randomization is sufficient for tasks repeated on an hourly
basis,

so no further extensibility is necessary.


I also aimed for practicality, because this issue (the repeated
annoyance messages)

only occurred on macOS. I also use Linux with systemd for programming, 

but Linux does not bother me. I am unsure about other operating systems.

> I wonder if we want to tweak get_random_minute() logic to be
> deterministic to avoid need for such a fuzzy comparison at its root.
>
> A few possible ideas are to read the value from the existing
> configuration and reuse that instead of coming up with a new random
> value, or to hash the hostname (or something similar that is
> reasonably stable) to use the result as the seed.  Derrick, what do
> you think?
>
>
> As to the patch, as I suspect we may not want a code with the
> proposed design, I won't look at it deeply at this point, but please
> consult Documentation/CodingGuidelines and/or make sure your patch
> will not be whitespace damaged during transit from your repository
> to people's mailbox.  For example:
>
>> diff --git a/builtin/gc.c b/builtin/gc.c
>> index a9b1c36de2..6405f4d332 100644
>> --- a/builtin/gc.c
>> +++ b/builtin/gc.c
>> @@ -1951,6 +1951,51 @@ static char *launchctl_get_uid(void)
>>   return xstrfmt("gui/%d", getuid());
>>  }
> These two lines are supposed to be what already appear in our
> codebase, but we of course do not use a single-space indent.  There
> is something funny going on.
>
> Thanks.
>