Re: [PATCH 2/3] maintenance: use launchctl on macOS

> On Tue, Nov 3, 2020 at 9:05 AM Derrick Stolee via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
>> maintenance: use launchctl on macOS
> 
> A few comments below (not necessarily worth a re-roll)...
> 
>> The launchctl command needs to be aligned with a user id in order
>> to initialize the command environment. This must be done using
>> the 'launchctl bootstrap' subcommand. This subcommand is new as
>> of macOS 10.11, which was released in September 2015. Before that
>> release the 'launchctl load' subcommand was recommended. The best
>> source of information on this transition I have seen is available
>> at [2].
> 
> It's not clear whether or not this is saying that git-maintenance will
> dynamically adapt to work with modern and older 'launchctl'. A glance
> at the actual code reveals that it knows only about modern
> 'bootstrap'. Perhaps this could be a bit clearer by saying that it
> only supports modern versions, and that support for older versions can
> be added later if needed. (For those of us who are stuck with 10-20
> year old hardware and OS versions, 2015 isn't that long ago.)

>> +static int bootstrap(const char *filename)
>> +{
>> +       int result;
>> +       struct strvec args = STRVEC_INIT;
>> +       char *uid = get_uid();
>> +       const char *launchctl = getenv("GIT_TEST_CRONTAB");
>> +       if (!launchctl)
>> +               launchctl = "/bin/launchctl";
>> +
>> +       strvec_split(&args, launchctl);
>> +       strvec_push(&args, "bootstrap");
>> +       strvec_pushf(&args, "gui/%s", uid);
>> +       strvec_push(&args, filename);
>> +
>> +       result = run_command_v_opt(args.v, 0);
>> +
>> +       strvec_clear(&args);
>> +       free(uid);
>> +       return result;
>> +}
> 
> The bootout() and bootstrap() functions seem to be identical except
> for one string literal. Code could be reduced by refactoring and
> passing that string literal in as an argument.

>> +static int remove_plist(enum schedule_priority schedule)
>> +{
>> +       const char *frequency = get_frequency(schedule);
>> +       char *name = get_service_name(frequency);
>> +       char *filename = get_service_filename(name);
>> +       int result = bootout(filename);
>> +       free(filename);
>> +       free(name);
>> +       return result;
>> +}
> 
> The result of get_service_name() is only ever passed to
> get_service_filename(). If get_service_filename() made the call to
> get_service_name() itself, it would free up callers from having to
> remember to free(name), thus reducing the likelihood of a possible
> leak.

>> +static int schedule_plist(const char *exec_path, enum schedule_priority schedule)
>> +{
>> +       plist = fopen(filename, "w");
>> +
>> +       if (!plist)
>> +               die(_("failed to open '%s'"), filename);
> 
> You can replace the fopen() and die() with a single call to xfopen().

>> +       /* bootout might fail if not already running, so ignore */
>> +       bootout(filename);
>> +       if (bootstrap(filename))
>> +               die(_("failed to bootstrap service %s"), filename);
> 
> I'm guessing that 'launchctl bootout' won't print a confusing and
> unexpected error message if the plist is not presently registered?

>> diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
>> @@ -389,12 +389,58 @@ test_expect_success 'stop from existing schedule' '
>> +test_expect_success MACOS_MAINTENANCE 'start and stop macOS maintenance' '
>> +       echo "#!/bin/sh\necho \$@ >>args" >print-args &&
>> +       chmod a+x print-args &&
> 
> Note that $@ loses its special magic if not surrounded by quotes, thus
> acts just like $*. So, either use "$@" or $* depending upon your
> requirements, but in the case of 'echo', it's just not going to matter
> at all, so $* is fine.
> 
> To construct the script, you can do this instead, which is easier to
> read and handles the 'chmod' for you:
> 
>     write_script print-args <<-\EOF
>     echo $* >>args
>     EOF

>> +       for frequency in hourly daily weekly
>> +       do
>> +               PLIST="$HOME/Library/LaunchAgents/org.git-scm.git.$frequency.plist" &&
>> +               grep schedule=$frequency "$PLIST" &&
>> +               echo "bootout gui/$UID $PLIST" >>expect &&
>> +               echo "bootstrap gui/$UID $PLIST" >>expect || return 1
>> +       done &&
> 
> My gut feeling is that this would be more robust if you manually
> determine UID in the test script the same way as the git-maintenance
> command itself does using '/usr/bin/id -u' rather than relying upon
> inheriting UID from the user's environment.