Maintenance IV: Platform-specific background maintenance

[derrickstolee]

This is based on ds/maintenance-part-3.

After sitting with the background maintenance as it has been cooking, I wanted to come back around and implement the background maintenance for Windows. However, I noticed that there were some things bothering me with background maintenance on my macOS machine. These are detailed in PATCH 3, but the tl;dr is that 'cron' is not recommended by Apple and instead 'launchd' satisfies our needs.

This series implements the background scheduling so git maintenance (start|stop) works on those platforms. I've been operating with these schedules for a while now without the problems described in the patches.

There is a particularly annoying case about console windows popping up on Windows, but PATCH 4 describes a plan to get around that.

Update in V7

• I had included an "encoding" string in the XML file for schtasks based on an example using UTF-8. The cross-platform tests then complained (in xmllint) because they wrote in ASCII instead. However, actually testing the situation on Windows (see [1]) against the real schtasks finds that it doesn't like that encoding string. I removed it entirely, and everything seems happier.

• I squashed Eric's two commits making the tests better. He remains a co-author and I kept his Helped-by. I had to rearrange the commit message a bit to point out the care he took for the cross-platform tests without referring to the test doing the wrong thing.

[1] microsoft#304

Thanks,
-Stolee

cc: jrnieder@gmail.com
cc: jonathantanmy@google.com
cc: sluongng@gmail.com
cc: Đoàn Trần Công Danh congdanhqx@gmail.com
cc: Martin Ågren martin.agren@gmail.com
cc: Eric Sunshine sunshine@sunshineco.com
cc: Derrick Stolee stolee@gmail.com

cc: Eric Sunshine sunshine@sunshineco.com
cc: Derrick Stolee stolee@gmail.com

[derrickstolee]

/submit

[gitgitgadget]

Submitted as pull.776.git.1604412196.gitgitgadget@gmail.com

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git pr-776/derrickstolee/maintenance/macOS-v1

To fetch this version to local tag pr-776/derrickstolee/maintenance/macOS-v1:

git fetch --no-tags https://github.com/gitgitgadget/git tag pr-776/derrickstolee/maintenance/macOS-v1

[gitgitgadget]

On the Git mailing list, Eric Sunshine wrote (reply to this):

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?

[gitgitgadget]

On the Git mailing list, Derrick Stolee wrote (reply to this):

On 11/3/2020 1:45 PM, Eric Sunshine wrote:
> 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.)

Yes, this is a strange place to be. How far do we go back to support
as many users as possible? How many users will be simultaneously
stuck on an old version of macOS _and_ interested in updating to this
latest version of Git? Is that worth the extra functionality to detect
the the OS version and change commands?

The good news is that this patch doesn't lock us in to the
boot(strap|out) subcommands too much. We could add in load/unload
subcommands for systems that are too old. However, I did think it
was prudent to take the currently-recommended option for fear that
Apple will completely _delete_ the load/unload options in an
upcoming release.

This makes me realize that I should update the documentation to give
pointers for how to view the schedules for each platform:

- Windows: Open "Task Scheduler"
- macOS: 'launchctl list | grep org.git-scm.git'
- Others: 'crontab -l'

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

Good point. Or a simple boolean value for "add" or "remove".
 
>> +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.

You're right. In an earlier version I thought I needed to add the
name in the XML, but it turns out I did not.

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

Thanks! I'll use that in several places and try to remember next time.

>> +       /* 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?

You're right, it does. It also returns with a non-zero exit code.
Along with your later suggestion to clear the .plist files, we will
want to have several conditions to not error out during a case where
the task is not scheduled.

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

TIL. thanks.

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

Yeah, you're right. Thanks!

>> +       # 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?

Yes, let's clear up these .plist files.

Thanks!
-Stolee

[gitgitgadget]

On the Git mailing list, Eric Sunshine wrote (reply to this):

On Tue, Nov 3, 2020 at 4:22 PM Derrick Stolee <stolee@gmail.com> wrote:
> On 11/3/2020 1:45 PM, Eric Sunshine wrote:
> > 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.)
>
> Yes, this is a strange place to be. How far do we go back to support
> as many users as possible? How many users will be simultaneously
> stuck on an old version of macOS _and_ interested in updating to this
> latest version of Git? Is that worth the extra functionality to detect
> the the OS version and change commands?

I don't think this patch series needs to answer these questions
provided that it doesn't close the door to someone adding
older-version support down the road. My review comment was more about
the commit message being clearer about the choice -- supporting only
recent 'launchctl' -- being made by this series. (And perhaps the
documentation could mention that it requires a reasonably modern
'launchctl'.)

> This makes me realize that I should update the documentation to give
> pointers for how to view the schedules for each platform:
>
> - Windows: Open "Task Scheduler"
> - macOS: 'launchctl list | grep org.git-scm.git'
> - Others: 'crontab -l'

Good idea.

I haven't looked at the documentation, but if it doesn't already, I
wonder if it should give examples of how to set these up by hand or
how to customize the ones created by git-maintenance itself. I was
also wondering if git-maintenance could have a mode in which it
generates the template file(s) for you but doesn't actually
activate/install it, instead providing instructions for
activation/installation. That way, people could modify the scheduling
file before actually activating it. However, this may all be outside
the scope of the patch series, and could be done later if desired.

[gitgitgadget]

On the Git mailing list, Derrick Stolee wrote (reply to this):

On 11/3/2020 5:27 PM, Eric Sunshine wrote:
> On Tue, Nov 3, 2020 at 4:22 PM Derrick Stolee <stolee@gmail.com> wrote:
>> On 11/3/2020 1:45 PM, Eric Sunshine wrote:
>>> 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.)
>>
>> Yes, this is a strange place to be. How far do we go back to support
>> as many users as possible? How many users will be simultaneously
>> stuck on an old version of macOS _and_ interested in updating to this
>> latest version of Git? Is that worth the extra functionality to detect
>> the the OS version and change commands?
> 
> I don't think this patch series needs to answer these questions
> provided that it doesn't close the door to someone adding
> older-version support down the road. My review comment was more about
> the commit message being clearer about the choice -- supporting only
> recent 'launchctl' -- being made by this series. (And perhaps the
> documentation could mention that it requires a reasonably modern
> 'launchctl'.)

Thanks. I'll be sure to make the commit message more clear.
 
>> This makes me realize that I should update the documentation to give
>> pointers for how to view the schedules for each platform:
>>
>> - Windows: Open "Task Scheduler"
>> - macOS: 'launchctl list | grep org.git-scm.git'
>> - Others: 'crontab -l'
> 
> Good idea.
> 
> I haven't looked at the documentation, but if it doesn't already, I
> wonder if it should give examples of how to set these up by hand or
> how to customize the ones created by git-maintenance itself. I was
> also wondering if git-maintenance could have a mode in which it
> generates the template file(s) for you but doesn't actually
> activate/install it, instead providing instructions for
> activation/installation. That way, people could modify the scheduling
> file before actually activating it. However, this may all be outside
> the scope of the patch series, and could be done later if desired.

Outside of the technical details, the biggest questions I've
tried to handle with the background maintenance feature has
been to balance customization with ease-of-use. My philosophy
is that users fall into a few expertise buckets, and have
different expectations:

* Beginners will never know about background maintenance and
  so will never run "git maintenance start" or set the config
  values.

* Intermediate users might discover "git maintenance start"
  and will appreciate that they don't need to learn cron to
  set up a good default schedule.

* Advanced users will read the documentation and use Git
  config settings to customize their maintenance tasks and
  schedule.

* Expert users might decide the task schedule available by
  "git maintenance start" is too restrictive, so they create
  their own background schedule with custom tasks. They might
  not even run the maintenance builtin and opt instead for
  'git gc' or 'git repack' directly!

My main target has been these "intermediate" users who might
run the command and forget about it. However, I've also tried
to keep the advanced users in mind with the advanced config
options available.

Your comment about documentation demonstrates a way to serve
the advanced and expert users by providing a clear framework
for discovering what Git is doing under the hood and how to
modify or adapt that to their needs. It is also important to
demonstrate how to set up schedules in a similar way without
having them be overwritten by a later "git maintenance start"
command.

I will give this a shot in v2. Thanks.

-Stolee

[gitgitgadget]

On the Git mailing list, Derrick Stolee wrote (reply to this):

On 11/3/2020 4:21 PM, Derrick Stolee wrote:
> On 11/3/2020 1:45 PM, Eric Sunshine wrote:
>> On Tue, Nov 3, 2020 at 9:05 AM Derrick Stolee via GitGitGadget
>>> +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.
> 
> You're right. In an earlier version I thought I needed to add the
> name in the XML, but it turns out I did not.

As I go through the effort to remove get_service_name() I find that
actually the name is used in one place in the XML file:

+		   "<key>Label</key><string>%s</string>\n"

This "Label" should match the filename, I believe. I can still
be more careful about how often this name is actually required.

Thanks,
-Stolee

[gitgitgadget]

User Eric Sunshine <sunshine@sunshineco.com> has been added to the cc: list.

[gitgitgadget]

On the Git mailing list, Eric Sunshine wrote (reply to this):

On Tue, Nov 3, 2020 at 9:05 AM Derrick Stolee via GitGitGadget
<gitgitgadget@gmail.com> wrote:
> There is a deficiency in the current design. Windows has two kinds of
> applications: GUI applications that start by "winmain()" and console
> applications that start by "main()". Console applications are attached
> to a new Console window if they are not already associated with a GUI
> application. This means that every hour the scheudled task launches a
> command window for the scheduled tasks. Not only is this visually
> obtrusive, but it also takes focus from whatever else the user is
> doing!

I wonder if you could use the technique explained in [1] to prevent
the console window from popping up.

[1]: https://pureinfotech.com/prevent-command-window-appearing-scheduled-tasks-windows-10/

> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> ---
> diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
> @@ -441,6 +441,40 @@ test_expect_success MACOS_MAINTENANCE 'start and stop macOS maintenance' '
> +test_expect_success MINGW 'start and stop Windows maintenance' '
> +       echo "echo \$@ >>args" >print-args &&
> +       chmod a+x print-args &&

Same comments as my review of [2/3] regarding $@ and write_script().

> +       rm -f args &&
> +       GIT_TEST_CRONTAB="/bin/sh print-args" git maintenance start &&
> +       cat args &&

Is this 'cat' leftover debugging gunk?

> +       # start registers the repo
> +       git config --get --global maintenance.repo "$(pwd)" &&
> +
> +       rm expect &&
> +       for frequency in hourly daily weekly
> +       do
> +               echo "/create /tn Git Maintenance ($frequency) /f /xml .git/objects/schedule-$frequency.xml" >>expect \
> +                       || return 1
> +       done &&

Rather than using >> within the loop, it's often simpler to capture
the output of the for-loop in its entirety:

    for frequency in hourly daily weekly
    do
        echo "/create ..." || return 1
    done >expect &&

However, in this case 'printf' may be even simpler:

    printf "/create /tn ... .git/objects/schedule-%s.xml\n" \
        hourly daily weekly >expect &&

> +       GIT_TEST_CRONTAB="/bin/sh print-args"  git maintenance stop &&

Too many spaces before the 'git' command.

[gitgitgadget]

On the Git mailing list, Derrick Stolee wrote (reply to this):

On 11/3/2020 2:06 PM, Eric Sunshine wrote:
> On Tue, Nov 3, 2020 at 9:05 AM Derrick Stolee via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
>> There is a deficiency in the current design. Windows has two kinds of
>> applications: GUI applications that start by "winmain()" and console
>> applications that start by "main()". Console applications are attached
>> to a new Console window if they are not already associated with a GUI
>> application. This means that every hour the scheudled task launches a
>> command window for the scheduled tasks. Not only is this visually
>> obtrusive, but it also takes focus from whatever else the user is
>> doing!
> 
> I wonder if you could use the technique explained in [1] to prevent
> the console window from popping up.
> 
> [1]: https://pureinfotech.com/prevent-command-window-appearing-scheduled-tasks-windows-10/

The critical part of that strategy is the "Run whether the user is
logged in or not". The resulting option that triggers causes the
schtasks command to require a password prompt (or a password passed
as a command-line argument). I found that interaction to be too
disruptive.

>> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
>> ---
>> diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
>> @@ -441,6 +441,40 @@ test_expect_success MACOS_MAINTENANCE 'start and stop macOS maintenance' '
>> +test_expect_success MINGW 'start and stop Windows maintenance' '
>> +       echo "echo \$@ >>args" >print-args &&
>> +       chmod a+x print-args &&
> 
> Same comments as my review of [2/3] regarding $@ and write_script().

Noted!

>> +       rm -f args &&
>> +       GIT_TEST_CRONTAB="/bin/sh print-args" git maintenance start &&
>> +       cat args &&
> 
> Is this 'cat' leftover debugging gunk?

Yes. Thanks.

>> +       # start registers the repo
>> +       git config --get --global maintenance.repo "$(pwd)" &&
>> +
>> +       rm expect &&
>> +       for frequency in hourly daily weekly
>> +       do
>> +               echo "/create /tn Git Maintenance ($frequency) /f /xml .git/objects/schedule-$frequency.xml" >>expect \
>> +                       || return 1
>> +       done &&
> 
> Rather than using >> within the loop, it's often simpler to capture
> the output of the for-loop in its entirety:
> 
>     for frequency in hourly daily weekly
>     do
>         echo "/create ..." || return 1
>     done >expect &&
> 
> However, in this case 'printf' may be even simpler:
> 
>     printf "/create /tn ... .git/objects/schedule-%s.xml\n" \
>         hourly daily weekly >expect &&

Excellent.

>> +       GIT_TEST_CRONTAB="/bin/sh print-args"  git maintenance stop &&
> 
> Too many spaces before the 'git' command.

Thanks!
-Stolee

[gitgitgadget]

On the Git mailing list, Junio C Hamano wrote (reply to this):

"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:

> This is based on ds/maintenance-part-3.
>
> After sitting with the background maintenance as it has been cooking, I
> wanted to come back around and implement the background maintenance for
> Windows. However, I noticed that there were some things bothering me with
> background maintenance on my macOS machine. These are detailed in PATCH 2,
> but the tl;dr is that 'cron' is not recommended by Apple and instead
> 'launchd' satisfies our needs.

Nicely done.

> This series implements the background scheduling so git maintenance
> (start|stop) works on those platforms. I've been operating with these
> schedules for a while now without the problems described in the patches.
>
> There is a particularly annoying case about console windows popping up on
> Windows, but PATCH 3 describes a plan to get around that.
>
> Thanks, -Stolee
>
> Derrick Stolee (3):
>   maintenance: extract platform-specific scheduling
>   maintenance: use launchctl on macOS
>   maintenance: use Windows scheduled tasks
>
>  builtin/gc.c           | 428 +++++++++++++++++++++++++++++++++++++++--
>  t/t7900-maintenance.sh |  86 ++++++++-
>  t/test-lib.sh          |   4 +
>  3 files changed, 498 insertions(+), 20 deletions(-)
>
>
> base-commit: 0016b618182f642771dc589cf0090289f9fe1b4f
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-776%2Fderrickstolee%2Fmaintenance%2FmacOS-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-776/derrickstolee/maintenance/macOS-v1
> Pull-Request: https://github.com/gitgitgadget/git/pull/776

[gitgitgadget]

On the Git mailing list, Junio C Hamano wrote (reply to this):

"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:

> This is based on ds/maintenance-part-3.

Ah, I forgot to ask those on CC list how carefully they read the
part 3 of the series, as it's been left on 'seen' for some time, and
I do not know if it is ready to start cooking in 'next'.

Thanks.

[gitgitgadget]

On the Git mailing list, Derrick Stolee wrote (reply to this):

On 11/3/2020 3:21 PM, Junio C Hamano wrote:
> "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
>> This is based on ds/maintenance-part-3.
> 
> Ah, I forgot to ask those on CC list how carefully they read the
> part 3 of the series, as it's been left on 'seen' for some time, and
> I do not know if it is ready to start cooking in 'next'.

It has been a while since anyone has commented, and I've been
running the patches locally for quite a while. I'd be very
happy to see them cook in next. I wasn't quite to the place
to be pushy about it.

I'm hoping that parts 3 and 4 can make it in time for 2.30,
so the feature is universally available for all platforms.
I realize that's not entirely up to just me.

Thanks,
-Stolee

[gitgitgadget]

User Derrick Stolee <stolee@gmail.com> has been added to the cc: list.

[gitgitgadget]

On the Git mailing list, Junio C Hamano wrote (reply to this):

Derrick Stolee <stolee@gmail.com> writes:

> On 11/3/2020 3:21 PM, Junio C Hamano wrote:
>> "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
>> 
>>> This is based on ds/maintenance-part-3.
>> 
>> Ah, I forgot to ask those on CC list how carefully they read the
>> part 3 of the series, as it's been left on 'seen' for some time, and
>> I do not know if it is ready to start cooking in 'next'.
>
> It has been a while since anyone has commented, and I've been
> running the patches locally for quite a while. I'd be very
> happy to see them cook in next. I wasn't quite to the place
> to be pushy about it.
>
> I'm hoping that parts 3 and 4 can make it in time for 2.30,
> so the feature is universally available for all platforms.
> I realize that's not entirely up to just me.

After writing this entry in the What's cooking report:

    * ds/maintenance-part-4 (2020-11-03) 3 commits
     - maintenance: use Windows scheduled tasks
     - maintenance: use launchctl on macOS
     - maintenance: extract platform-specific scheduling
     (this branch uses ds/maintenance-part-3.)

     Follow-up on the "maintenance part-3" which introduced scheduled
     maintenance tasks to support platforms whose native scheduling
     methods are not 'cron'.

I was wondering if I should propose making these two parts into one,
so we may be pretty much on the same page.

[gitgitgadget]

This branch is now known as ds/maintenance-part-4.

[gitgitgadget]

This patch series was integrated into seen via git@d46fe7b.

[gitgitgadget]

On the Git mailing list, Derrick Stolee wrote (reply to this):

On 11/3/2020 5:30 PM, Junio C Hamano wrote:
> Derrick Stolee <stolee@gmail.com> writes:
>> I'm hoping that parts 3 and 4 can make it in time for 2.30,
>> so the feature is universally available for all platforms.
>> I realize that's not entirely up to just me.
> 
> After writing this entry in the What's cooking report:
> 
>     * ds/maintenance-part-4 (2020-11-03) 3 commits
>      - maintenance: use Windows scheduled tasks
>      - maintenance: use launchctl on macOS
>      - maintenance: extract platform-specific scheduling
>      (this branch uses ds/maintenance-part-3.)
> 
>      Follow-up on the "maintenance part-3" which introduced scheduled
>      maintenance tasks to support platforms whose native scheduling
>      methods are not 'cron'.

This is a good summary of this series.

> I was wondering if I should propose making these two parts into one,
> so we may be pretty much on the same page.

I'm happy to have the two topics be merged into one series, but
I'd prefer to only re-roll these three patches during review.
Of course, by keeping them together we have the ability to re-
roll all of the patches, but part-3 has a decent length and has
been stable for a while.

I defer to what is easiest for you.

Thanks,
-Stolee

[gitgitgadget]

On the Git mailing list, Junio C Hamano wrote (reply to this):

Derrick Stolee <stolee@gmail.com> writes:

> On 11/3/2020 5:30 PM, Junio C Hamano wrote:
>> Derrick Stolee <stolee@gmail.com> writes:
>>> I'm hoping that parts 3 and 4 can make it in time for 2.30,
>>> so the feature is universally available for all platforms.
>>> I realize that's not entirely up to just me.
>> ...
>> I was wondering if I should propose making these two parts into one,
>> so we may be pretty much on the same page.
>
> I'm happy to have the two topics be merged into one series, but
> I'd prefer to only re-roll these three patches during review.
> Of course, by keeping them together we have the ability to re-
> roll all of the patches, but part-3 has a decent length and has
> been stable for a while.
>
> I defer to what is easiest for you.

Two topics, one on top of the other, is fine, as long as I can
remember (and you help me by reminding) to keep them together,
and the bottom one is reasonably solid that I do not have to do the
rebasing myself ;-)

[gitgitgadget]

On the Git mailing list, Derrick Stolee wrote (reply to this):

On 11/4/2020 12:00 PM, Junio C Hamano wrote:
> Derrick Stolee <stolee@gmail.com> writes:
> 
>> On 11/3/2020 5:30 PM, Junio C Hamano wrote:
>>> Derrick Stolee <stolee@gmail.com> writes:
>>>> I'm hoping that parts 3 and 4 can make it in time for 2.30,
>>>> so the feature is universally available for all platforms.
>>>> I realize that's not entirely up to just me.
>>> ...
>>> I was wondering if I should propose making these two parts into one,
>>> so we may be pretty much on the same page.
>>
>> I'm happy to have the two topics be merged into one series, but
>> I'd prefer to only re-roll these three patches during review.
>> Of course, by keeping them together we have the ability to re-
>> roll all of the patches, but part-3 has a decent length and has
>> been stable for a while.
>>
>> I defer to what is easiest for you.
> 
> Two topics, one on top of the other, is fine, as long as I can
> remember (and you help me by reminding) to keep them together,
> and the bottom one is reasonably solid that I do not have to do the
> rebasing myself ;-)

Sounds good to me. If I need to re-roll part 3, then
I'll consider replacing both parts with a new series.

Thanks,
-Stolee

[gitgitgadget]

This patch series was integrated into seen via git@0d43881.

[gitgitgadget]

This patch series was integrated into seen via git@2b2b40a.

[gitgitgadget]

This patch series was integrated into seen via git@1a56829.

[gitgitgadget]

This patch series was integrated into seen via git@8bc7b5a.

[gitgitgadget]

This patch series was integrated into seen via git@22514b4.

[gitgitgadget]

This patch series was integrated into seen via git@6939d87.

[gitgitgadget]

This patch series was integrated into seen via git@0a9d855.

[gitgitgadget]

This patch series was integrated into seen via git@19ed21a.

[gitgitgadget]

On the Git mailing list, Derrick Stolee wrote (reply to this):

On 12/9/2020 8:04 PM, Junio C Hamano wrote:
> Eric Sunshine <sunshine@sunshineco.com> writes:
> 
>> On Wed, Dec 9, 2020 at 7:33 PM Junio C Hamano <gitster@pobox.com> wrote:
>>> "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
>>>> Update in V6
>>>> ============
>>>>
>>>>  * The Windows platform uses the tempfile API a bit better, including using
>>>>    the frequency in the filename to make the test simpler.
>>>
>>> Are two fix-up patches from Eric that have been queued near the top
>>> of ds/maintenance-part-4 still relevant?
>>
>> Both of the patches from Sunshine are still relevant atop Stolee's
>> latest (v6), and they should apply cleanly (I would think) since v6
>> didn't change anything related to those patches.
> 
> Yup, I tried rebasing these two and they applied cleanly, so I'll
> include them in today's pushout (which I haven't finished yet).
> 
> I probably would not notice if the updated 4-patch series already
> solved the issue in another way without causing the textual conflict
> with your two fix-up patches, though ;-)

I noticed a subtle issue with the v6 series, so I _will_ reroll the
series squashing in Eric's patches. He will remain a co-author and
I'll add the Helped-by: Ævar along with the details for that patch.

Thanks,
-Stolee

[gitgitgadget]

User Derrick Stolee <stolee@gmail.com> has been added to the cc: list.

[derrickstolee]

/submit

[gitgitgadget]

Submitted as pull.776.v7.git.1609852108.gitgitgadget@gmail.com

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git pr-776/derrickstolee/maintenance/macOS-v7

To fetch this version to local tag pr-776/derrickstolee/maintenance/macOS-v7:

git fetch --no-tags https://github.com/gitgitgadget/git tag pr-776/derrickstolee/maintenance/macOS-v7

[gitgitgadget]

This patch series was integrated into seen via git@0a7befa.

[gitgitgadget]

This patch series was integrated into seen via git@fc181c3.

[gitgitgadget]

This patch series was integrated into seen via git@1c56d4e.

[gitgitgadget]

This patch series was integrated into seen via git@cc69a92.

[gitgitgadget]

This patch series was integrated into seen via git@3d40e3a.

[gitgitgadget]

This patch series was integrated into seen via git@81875d4.

[gitgitgadget]

This patch series was integrated into seen via git@cc69a92.

[gitgitgadget]

This patch series was integrated into next via git@1f98c85.

[gitgitgadget]

This patch series was integrated into seen via git@1f98c85.

[gitgitgadget]

This patch series was integrated into seen via git@7a642c7.

[gitgitgadget]

On the Git mailing list, Eric Sunshine wrote (reply to this):

On Tue, Jan 5, 2021 at 8:08 AM Derrick Stolee via GitGitGadget
<gitgitgadget@gmail.com> wrote:
> [...]
> The need for a user id requires us to run 'id -u' which works on
> POSIX systems but not Windows. Further, the need for fully-qualitifed
> path names including $HOME behaves differently in the Git internals and
> the external test suite. The $HOME variable starts with "C:\..." instead
> of the "/c/..." that is provided by Git in these subcommands. The test
> therefore has a prerequisite that we are not on Windows. The cross-
> platform logic still allows us to test the macOS logic on a Linux
> machine.

You forgot to remove the above paragraph...

> We strive to make these tests work on all platforms, but Windows caused
> some headaches. In particular, the value of getuid() called by the C
> code is not guaranteed to be the same as `$(id -u)` invoked by a test.
> This is because `git.exe` is a native Windows program, whereas the
> utility programs run by the test script mostly utilize the MSYS2 runtime,
> which emulates a POSIX-like environment. Since the purpose of the test
> is to check that the input to the hook is well-formed, the actual user
> ID is immaterial, thus we can work around the problem by making the the
> test UID-agnostic. Another subtle issue is the $HOME environment
> variable being a Windows-style path instead of a Unix-style path. We can
> be more flexible here instead of expecting exact path matches.

...when you added this paragraph from my separate patch which you
folded into this patch for v7. The two paragraphs are at odds with one
another.