Re: [PATCH] builtin/gc: fix crash when running `git maintenance start`

[Patrick Steinhardt <ps@pks.im>]

On Tue, Oct 08, 2024 at 10:58:06PM -0400, Derrick Stolee wrote:
> On 10/8/24 2:30 PM, Derrick Stolee wrote:
> > On 10/8/24 8:15 AM, Patrick Steinhardt wrote:
> > > It was reported on the mailing list that running `git maintenance start`
> > > immediately segfaults starting with b6c3f8e12c (builtin/maintenance: fix
> > > leak in `get_schedule_cmd()`, 2024-09-26). And indeed, this segfault is
> > > trivial to reproduce up to a point where one is scratching their head
> > > why we didn't catch this regression in our test suite.
> > 
> > > +test_expect_success 'start without GIT_TEST_MAINT_SCHEDULER' '
> > > +    test_when_finished "rm -rf crontab.log script repo" &&
> > > +    mkdir script &&
> > > +    write_script script/crontab <<-EOF &&
> > > +    echo "\$*" >>"$(pwd)"/crontab.log
> > > +    EOF
> > > +    git init repo &&
> > > +    (
> > > +        cd repo &&
> > > +        sane_unset GIT_TEST_MAINT_SCHEDULER &&
> > > +        PATH="$(pwd)/../script:$PATH" git maintenance start --scheduler=crontab
> > > +    ) &&
> > > +    test_grep -- -l crontab.log &&
> > > +    test_grep -- git_cron_edit_tmp crontab.log
> > > +'
> > > +
> > I see why we didn't catch this immediately. This is a good way to work
> > around this issue of "mocking" the scheduler.
> 
> Unfortunately, this test is broken on macOS and Windows. Those platforms will
> fail when asked for 'crontab' without the test variable.
> 
> Here is a potential fixup that will make your test succeed:

Oh, indeed, thanks for flagging this. The issue is that those platforms
do not make `crontab` available at all.

I think we can land at a better fix though: the systemctl-based
scheduler _is_ available on all platforms if the systemctl binary is
found. So let me adapt the script accordingly.

Patrick