git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: Eric Sunshine <sunshine@sunshineco.com>
To: Derrick Stolee via GitGitGadget <gitgitgadget@gmail.com>
Cc: "Git List" <git@vger.kernel.org>,
	"Jonathan Nieder" <jrnieder@gmail.com>,
	"Jonathan Tan" <jonathantanmy@google.com>,
	"Son Luong Ngoc" <sluongng@gmail.com>,
	"Derrick Stolee" <stolee@gmail.com>,
	"Đoàn Trần Công Danh" <congdanhqx@gmail.com>,
	"Martin Ågren" <martin.agren@gmail.com>,
	"Derrick Stolee" <derrickstolee@github.com>,
	"Derrick Stolee" <dstolee@microsoft.com>
Subject: Re: [PATCH 2/3] maintenance: use launchctl on macOS
Date: Tue, 3 Nov 2020 13:45:36 -0500	[thread overview]
Message-ID: <CAPig+cR1zSqY_y02JBdtjh_+9Pp2dOtGrHHmf-ah5sU6Lhzp6Q@mail.gmail.com> (raw)
In-Reply-To: <832fdf16872cbfee4a5e15b559b2b40dabd545f4.1604412197.git.gitgitgadget@gmail.com>

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

> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> ---
> diff --git a/builtin/gc.c b/builtin/gc.c
> @@ -1491,6 +1491,214 @@ static int maintenance_unregister(void)
> +static int bootout(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, "bootout");
> +       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;
> +}
> +
> +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.

> +       # stop does not remove plist files, but boots them out

Is this desirable? Should git-maintenance do a better job of cleaning
up after itself?

  reply	other threads:[~2020-11-03 18:45 UTC|newest]

Thread overview: 83+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-11-03 14:03 [PATCH 0/3] Maintenance IV: Platform-specific background maintenance Derrick Stolee via GitGitGadget
2020-11-03 14:03 ` [PATCH 1/3] maintenance: extract platform-specific scheduling Derrick Stolee via GitGitGadget
2020-11-03 14:03 ` [PATCH 2/3] maintenance: use launchctl on macOS Derrick Stolee via GitGitGadget
2020-11-03 18:45   ` Eric Sunshine [this message]
2020-11-03 21:21     ` Derrick Stolee
2020-11-03 22:27       ` Eric Sunshine
2020-11-04 13:33         ` Derrick Stolee
2020-11-04 14:17       ` Derrick Stolee
2020-11-03 14:03 ` [PATCH 3/3] maintenance: use Windows scheduled tasks Derrick Stolee via GitGitGadget
2020-11-03 19:06   ` Eric Sunshine
2020-11-03 21:23     ` Derrick Stolee
2020-11-03 20:18 ` [PATCH 0/3] Maintenance IV: Platform-specific background maintenance Junio C Hamano
2020-11-03 20:21 ` Junio C Hamano
2020-11-03 21:09   ` Derrick Stolee
2020-11-03 22:30     ` Junio C Hamano
2020-11-04 13:02       ` Derrick Stolee
2020-11-04 17:00         ` Junio C Hamano
2020-11-04 18:43           ` Derrick Stolee
2020-11-04 20:06 ` [PATCH v2 0/4] " Derrick Stolee via GitGitGadget
2020-11-04 20:06   ` [PATCH v2 1/4] maintenance: extract platform-specific scheduling Derrick Stolee via GitGitGadget
2020-11-04 20:06   ` [PATCH v2 2/4] maintenance: include 'cron' details in docs Derrick Stolee via GitGitGadget
2020-11-11  7:10     ` Eric Sunshine
2020-11-04 20:06   ` [PATCH v2 3/4] maintenance: use launchctl on macOS Derrick Stolee via GitGitGadget
2020-11-11  8:12     ` Eric Sunshine
2020-11-12 13:42       ` Derrick Stolee
2020-11-12 16:43         ` Eric Sunshine
2020-11-04 20:06   ` [PATCH v2 4/4] maintenance: use Windows scheduled tasks Derrick Stolee via GitGitGadget
2020-11-11  8:59     ` Eric Sunshine
2020-11-12 13:56       ` Derrick Stolee
2020-11-13 14:00   ` [PATCH v3 0/4] Maintenance IV: Platform-specific background maintenance Derrick Stolee via GitGitGadget
2020-11-13 14:00     ` [PATCH v3 1/4] maintenance: extract platform-specific scheduling Derrick Stolee via GitGitGadget
2020-11-13 14:00     ` [PATCH v3 2/4] maintenance: include 'cron' details in docs Derrick Stolee via GitGitGadget
2020-11-13 14:00     ` [PATCH v3 3/4] maintenance: use launchctl on macOS Derrick Stolee via GitGitGadget
2020-11-13 20:19       ` Eric Sunshine
2020-11-13 20:42         ` Derrick Stolee
2020-11-13 20:53           ` Eric Sunshine
2020-11-13 20:56             ` Eric Sunshine
2020-11-13 14:00     ` [PATCH v3 4/4] maintenance: use Windows scheduled tasks Derrick Stolee via GitGitGadget
2020-11-13 20:44       ` Eric Sunshine
2020-11-13 21:32         ` Derrick Stolee
2020-11-13 21:40           ` Eric Sunshine
2020-11-16 13:13             ` Derrick Stolee
2020-11-13 20:47     ` [PATCH v3 0/4] Maintenance IV: Platform-specific background maintenance Eric Sunshine
2020-11-14  9:23       ` Eric Sunshine
2020-11-16 13:17         ` Derrick Stolee
2020-11-17 21:13     ` [PATCH v4 " Derrick Stolee via GitGitGadget
2020-11-17 21:13       ` [PATCH v4 1/4] maintenance: extract platform-specific scheduling Derrick Stolee via GitGitGadget
2020-11-17 21:13       ` [PATCH v4 2/4] maintenance: include 'cron' details in docs Derrick Stolee via GitGitGadget
2020-11-18  0:34         ` Eric Sunshine
2020-11-18 18:30           ` Derrick Stolee
2020-11-17 21:13       ` [PATCH v4 3/4] maintenance: use launchctl on macOS Derrick Stolee via GitGitGadget
2020-11-18  6:45         ` Eric Sunshine
2020-11-18 18:22           ` Derrick Stolee
2020-11-17 21:13       ` [PATCH v4 4/4] maintenance: use Windows scheduled tasks Derrick Stolee via GitGitGadget
2020-11-18  7:15         ` Eric Sunshine
2020-11-18 18:30           ` Derrick Stolee
2020-11-18 20:54             ` Eric Sunshine
2020-11-18 21:16               ` Derrick Stolee
2020-11-17 23:36       ` [PATCH v4 0/4] Maintenance IV: Platform-specific background maintenance Eric Sunshine
2020-11-24  2:20         ` Derrick Stolee
2020-11-24  2:59           ` Eric Sunshine
2020-11-17 23:54       ` Eric Sunshine
2020-11-24  4:16       ` [PATCH v5 " Derrick Stolee via GitGitGadget
2020-11-24  4:16         ` [PATCH v5 1/4] maintenance: extract platform-specific scheduling Derrick Stolee via GitGitGadget
2020-11-24  4:16         ` [PATCH v5 2/4] maintenance: include 'cron' details in docs Derrick Stolee via GitGitGadget
2020-11-24  4:16         ` [PATCH v5 3/4] maintenance: use launchctl on macOS Derrick Stolee via GitGitGadget
2020-11-24  4:16         ` [PATCH v5 4/4] maintenance: use Windows scheduled tasks Derrick Stolee via GitGitGadget
2020-11-27  9:08           ` Eric Sunshine
2020-12-09 19:28         ` [PATCH v6 0/4] Maintenance IV: Platform-specific background maintenance Derrick Stolee via GitGitGadget
2020-12-09 19:28           ` [PATCH v6 1/4] maintenance: extract platform-specific scheduling Derrick Stolee via GitGitGadget
2020-12-09 19:29           ` [PATCH v6 2/4] maintenance: include 'cron' details in docs Derrick Stolee via GitGitGadget
2020-12-09 19:29           ` [PATCH v6 3/4] maintenance: use launchctl on macOS Derrick Stolee via GitGitGadget
2020-12-09 19:29           ` [PATCH v6 4/4] maintenance: use Windows scheduled tasks Derrick Stolee via GitGitGadget
2020-12-10  0:32           ` [PATCH v6 0/4] Maintenance IV: Platform-specific background maintenance Junio C Hamano
2020-12-10  0:49             ` Eric Sunshine
2020-12-10  1:04               ` Junio C Hamano
2021-01-05 12:17                 ` Derrick Stolee
2021-01-05 13:08           ` [PATCH v7 " Derrick Stolee via GitGitGadget
2021-01-05 13:08             ` [PATCH v7 1/4] maintenance: extract platform-specific scheduling Derrick Stolee via GitGitGadget
2021-01-05 13:08             ` [PATCH v7 2/4] maintenance: include 'cron' details in docs Derrick Stolee via GitGitGadget
2021-01-05 13:08             ` [PATCH v7 3/4] maintenance: use launchctl on macOS Derrick Stolee via GitGitGadget
2021-01-10  6:34               ` Eric Sunshine
2021-01-05 13:08             ` [PATCH v7 4/4] maintenance: use Windows scheduled tasks Derrick Stolee via GitGitGadget

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAPig+cR1zSqY_y02JBdtjh_+9Pp2dOtGrHHmf-ah5sU6Lhzp6Q@mail.gmail.com \
    --to=sunshine@sunshineco.com \
    --cc=congdanhqx@gmail.com \
    --cc=derrickstolee@github.com \
    --cc=dstolee@microsoft.com \
    --cc=git@vger.kernel.org \
    --cc=gitgitgadget@gmail.com \
    --cc=jonathantanmy@google.com \
    --cc=jrnieder@gmail.com \
    --cc=martin.agren@gmail.com \
    --cc=sluongng@gmail.com \
    --cc=stolee@gmail.com \
    --subject='Re: [PATCH 2/3] maintenance: use launchctl on macOS' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Code repositories for project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).