On 24. 12. 29. 02:09, Junio C Hamano wrote:

Byoungchan Lee [off-list ref] writes:

quoted

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 <redacted>
---
 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:

quoted

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.