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

[Junio C Hamano <gitster@pobox.com>]

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.