On 24. 12. 29. 02:09, Junio C Hamano wrote:
> Byoungchan Lee <byoungchan.lee@xxxxxxx> 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@xxxxxxx>
>> ---
>> 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.
>
