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

> 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.
>