{"thread":{"id":"62280","subject":"Bug: `git maintenance start` is likely broken in 2.47","startedAt":"2024-10-08T10:50:57Z","lastAt":"2024-10-15T00:36:18Z","messageCount":19,"participants":["Shubham Kanodia","Patrick Steinhardt","Derrick Stolee","Junio C Hamano","Taylor Blau"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"504412","messageId":"CAG=Um+0mJW-oAH+YLC3dWEU64JwS-zMkkTiFWYBe4g6HMbe-iA@mail.gmail.com","threadId":"62280","inReplyTo":null,"subject":"Bug: `git maintenance start` is likely broken in 2.47","fromName":"Shubham Kanodia","fromEmail":"shubham.kanodia10@gmail.com","sentAt":"2024-10-08T10:50:19Z","receivedAt":"2024-10-08T10:50:57Z","isPatch":false,"sender":{"key":"shubham.kanodia10@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8946207?v=4"},"body":"In the most recent release of git (2.47.0), running `git maintenance\nstart` results in a segmentation fault error both on mac & ubuntu.\n\n`b6c3f8e12c` seems to be the first bad commit.\n"},{"id":"504413","messageId":"ZwUbTJS-mg6cCG1e@pks.im","threadId":"62280","inReplyTo":"CAG=Um+0mJW-oAH+YLC3dWEU64JwS-zMkkTiFWYBe4g6HMbe-iA@mail.gmail.com","subject":"Re: Bug: `git maintenance start` is likely broken in 2.47","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-10-08T11:45:23Z","receivedAt":"2024-10-08T11:45:29Z","isPatch":false,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Oct 08, 2024 at 04:20:19PM +0530, Shubham Kanodia wrote:\n> In the most recent release of git (2.47.0), running `git maintenance\n> start` results in a segmentation fault error both on mac & ubuntu.\n> \n> `b6c3f8e12c` seems to be the first bad commit.\n\nThis is embarassingly easy to reproduce. None of our tests catch the\nissue because we are forced to stub out the commands that run, and the\nerror only happens when _not_ stubbing it out. I'll send a patch later\ntoday to address the issue and will also think about a way to test this.\n\nPatrick\n"},{"id":"504414","messageId":"976c97081af7c62960bd71d1b70039657e7cb711.1728389731.git.ps@pks.im","threadId":"62280","inReplyTo":"CAG=Um+0mJW-oAH+YLC3dWEU64JwS-zMkkTiFWYBe4g6HMbe-iA@mail.gmail.com","subject":"[PATCH] builtin/gc: fix crash when running `git maintenance start`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-10-08T12:15:53Z","receivedAt":"2024-10-08T12:15:57Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"It was reported on the mailing list that running `git maintenance start`\nimmediately segfaults starting with b6c3f8e12c (builtin/maintenance: fix\nleak in `get_schedule_cmd()`, 2024-09-26). And indeed, this segfault is\ntrivial to reproduce up to a point where one is scratching their head\nwhy we didn't catch this regression in our test suite.\n\nThe root cause of this error is `get_schedule_cmd()`, which does not\npopulate the `out` parameter in all cases anymore starting with the\nmentioned commit. Callers do assume it to always be populated though and\nwill e.g. call `strvec_split()` on the returned value, which will of\ncourse segfault when the variable is uninitialized.\n\nSo why didn't we catch this trivial regression? The reason is that our\ntests always set up the \"GIT_TEST_MAINT_SCHEDULER\" environment variable\nvia \"t/test-lib.sh\", which allows us to override the scheduler command\nwith a custom one so that we don't accidentally modify the developer's\nsystem. But the faulty code where we don't set the `out` parameter will\nonly get hit in case that environment variable is _not_ set, which is\nnever the case when executing our tests.\n\nFix the regression by again unconditionally allocating the value in the\n`out` parameter, if provided. Add a test that unsets the environment\nvariable to catch future regressions in this area.\n\nReported-by: Shubham Kanodia <shubham.kanodia10@gmail.com>\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n builtin/gc.c           |  7 +++++--\n t/t7900-maintenance.sh | 16 ++++++++++++++++\n 2 files changed, 21 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex 6a7a2da006..d52735354c 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -1832,7 +1832,7 @@ static const char *get_extra_launchctl_strings(void) {\n  *     | Input |                     Output                      |\n  *     | *cmd  | return code |       *out        | *is_available |\n  *     +-------+-------------+-------------------+---------------+\n- *     | \"foo\" |    false    | NULL              |  (unchanged)  |\n+ *     | \"foo\" |    false    | \"foo\" (allocated) |  (unchanged)  |\n  *     +-------+-------------+-------------------+---------------+\n  *\n  *   GIT_TEST_MAINT_SCHEDULER set to “foo:./mock_foo.sh,bar:./mock_bar.sh”\n@@ -1850,8 +1850,11 @@ static int get_schedule_cmd(const char *cmd, int *is_available, char **out)\n \tstruct string_list_item *item;\n \tstruct string_list list = STRING_LIST_INIT_NODUP;\n \n-\tif (!testing)\n+\tif (!testing) {\n+\t\tif (out)\n+\t\t\t*out = xstrdup(cmd);\n \t\treturn 0;\n+\t}\n \n \tif (is_available)\n \t\t*is_available = 0;\ndiff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh\nindex a66d0e089d..4008e4e45e 100755\n--- a/t/t7900-maintenance.sh\n+++ b/t/t7900-maintenance.sh\n@@ -646,6 +646,22 @@ test_expect_success !MINGW 'register and unregister with regex metacharacters' '\n \t\tmaintenance.repo \"$(pwd)/$META\"\n '\n \n+test_expect_success 'start without GIT_TEST_MAINT_SCHEDULER' '\n+\ttest_when_finished \"rm -rf crontab.log script repo\" &&\n+\tmkdir script &&\n+\twrite_script script/crontab <<-EOF &&\n+\techo \"\\$*\" >>\"$(pwd)\"/crontab.log\n+\tEOF\n+\tgit init repo &&\n+\t(\n+\t\tcd repo &&\n+\t\tsane_unset GIT_TEST_MAINT_SCHEDULER &&\n+\t\tPATH=\"$(pwd)/../script:$PATH\" git maintenance start --scheduler=crontab\n+\t) &&\n+\ttest_grep -- -l crontab.log &&\n+\ttest_grep -- git_cron_edit_tmp crontab.log\n+'\n+\n test_expect_success 'start --scheduler=<scheduler>' '\n \ttest_expect_code 129 git maintenance start --scheduler=foo 2>err &&\n \ttest_grep \"unrecognized --scheduler argument\" err &&\n-- \n2.47.0.rc1.33.g90fe3800b9.dirty\n\n"},{"id":"504468","messageId":"c1271bf6-3648-4eca-916f-67a8518fe4e1@gmail.com","threadId":"62280","inReplyTo":"976c97081af7c62960bd71d1b70039657e7cb711.1728389731.git.ps@pks.im","subject":"Re: [PATCH] builtin/gc: fix crash when running `git maintenance start`","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2024-10-08T18:30:42Z","receivedAt":"2024-10-08T18:30:44Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 10/8/24 8:15 AM, Patrick Steinhardt wrote:\n> It was reported on the mailing list that running `git maintenance start`\n> immediately segfaults starting with b6c3f8e12c (builtin/maintenance: fix\n> leak in `get_schedule_cmd()`, 2024-09-26). And indeed, this segfault is\n> trivial to reproduce up to a point where one is scratching their head\n> why we didn't catch this regression in our test suite.\n\n> +test_expect_success 'start without GIT_TEST_MAINT_SCHEDULER' '\n> +\ttest_when_finished \"rm -rf crontab.log script repo\" &&\n> +\tmkdir script &&\n> +\twrite_script script/crontab <<-EOF &&\n> +\techo \"\\$*\" >>\"$(pwd)\"/crontab.log\n> +\tEOF\n> +\tgit init repo &&\n> +\t(\n> +\t\tcd repo &&\n> +\t\tsane_unset GIT_TEST_MAINT_SCHEDULER &&\n> +\t\tPATH=\"$(pwd)/../script:$PATH\" git maintenance start --scheduler=crontab\n> +\t) &&\n> +\ttest_grep -- -l crontab.log &&\n> +\ttest_grep -- git_cron_edit_tmp crontab.log\n> +'\n> +\nI see why we didn't catch this immediately. This is a good way to work\naround this issue of \"mocking\" the scheduler.\n\nThanks for the fast response.\n-Stolee\n\n"},{"id":"504471","messageId":"xmqqbjzu1m2j.fsf@gitster.g","threadId":"62280","inReplyTo":"976c97081af7c62960bd71d1b70039657e7cb711.1728389731.git.ps@pks.im","subject":"Re: [PATCH] builtin/gc: fix crash when running `git maintenance start`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-10-08T18:33:40Z","receivedAt":"2024-10-08T18:33:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n> So why didn't we catch this trivial regression? The reason is that our\n> tests always set up the \"GIT_TEST_MAINT_SCHEDULER\" environment variable\n> via \"t/test-lib.sh\", which allows us to override the scheduler command\n> with a custom one so that we don't accidentally modify the developer's\n> system. But the faulty code where we don't set the `out` parameter will\n> only get hit in case that environment variable is _not_ set, which is\n> never the case when executing our tests.\n>\n> Fix the regression by again unconditionally allocating the value in the\n> `out` parameter, if provided. Add a test that unsets the environment\n> variable to catch future regressions in this area.\n>\n> Reported-by: Shubham Kanodia <shubham.kanodia10@gmail.com>\n> Signed-off-by: Patrick Steinhardt <ps@pks.im>\n> ---\n\nThanks for quickly reporting and addressing this one, both of you.\n\nWill queue.\n"},{"id":"504508","messageId":"b4d75b35-bfe2-4950-b2de-b56d95918b3c@gmail.com","threadId":"62280","inReplyTo":"c1271bf6-3648-4eca-916f-67a8518fe4e1@gmail.com","subject":"Re: [PATCH] builtin/gc: fix crash when running `git maintenance start`","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2024-10-09T02:58:06Z","receivedAt":"2024-10-09T02:58:08Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 10/8/24 2:30 PM, Derrick Stolee wrote:\n> On 10/8/24 8:15 AM, Patrick Steinhardt wrote:\n>> It was reported on the mailing list that running `git maintenance start`\n>> immediately segfaults starting with b6c3f8e12c (builtin/maintenance: fix\n>> leak in `get_schedule_cmd()`, 2024-09-26). And indeed, this segfault is\n>> trivial to reproduce up to a point where one is scratching their head\n>> why we didn't catch this regression in our test suite.\n> \n>> +test_expect_success 'start without GIT_TEST_MAINT_SCHEDULER' '\n>> +    test_when_finished \"rm -rf crontab.log script repo\" &&\n>> +    mkdir script &&\n>> +    write_script script/crontab <<-EOF &&\n>> +    echo \"\\$*\" >>\"$(pwd)\"/crontab.log\n>> +    EOF\n>> +    git init repo &&\n>> +    (\n>> +        cd repo &&\n>> +        sane_unset GIT_TEST_MAINT_SCHEDULER &&\n>> +        PATH=\"$(pwd)/../script:$PATH\" git maintenance start --scheduler=crontab\n>> +    ) &&\n>> +    test_grep -- -l crontab.log &&\n>> +    test_grep -- git_cron_edit_tmp crontab.log\n>> +'\n>> +\n> I see why we didn't catch this immediately. This is a good way to work\n> around this issue of \"mocking\" the scheduler.\n\nUnfortunately, this test is broken on macOS and Windows. Those platforms will\nfail when asked for 'crontab' without the test variable.\n\nHere is a potential fixup that will make your test succeed:\n\n--- >8 ---\n\ndiff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh\nindex 4008e4e45e..86bc77e73f 100755\n--- a/t/t7900-maintenance.sh\n+++ b/t/t7900-maintenance.sh\n@@ -646,7 +646,7 @@ test_expect_success !MINGW 'register and unregister with \nregex metacharacters' '\n  \t\tmaintenance.repo \"$(pwd)/$META\"\n  '\n\n-test_expect_success 'start without GIT_TEST_MAINT_SCHEDULER' '\n+test_expect_success !MINGW,!DARWIN 'start without GIT_TEST_MAINT_SCHEDULER' '\n  \ttest_when_finished \"rm -rf crontab.log script repo\" &&\n  \tmkdir script &&\n  \twrite_script script/crontab <<-EOF &&\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex b1a8ee5c00..f12f3a7609 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -1715,6 +1715,12 @@ case $uname_s in\n  \ttest_set_prereq GREP_STRIPS_CR\n  \ttest_set_prereq WINDOWS\n  \t;;\n+*Darwin*)\n+\ttest_set_prereq POSIXPERM\n+\ttest_set_prereq BSLASHPSPEC\n+\ttest_set_prereq EXECKEEPSPID\n+\ttest_set_prereq DARWIN\n+\t;;\n  *)\n  \ttest_set_prereq POSIXPERM\n  \ttest_set_prereq BSLASHPSPEC\n\n"},{"id":"504518","messageId":"ZwYij4t5eDgy23iw@pks.im","threadId":"62280","inReplyTo":"b4d75b35-bfe2-4950-b2de-b56d95918b3c@gmail.com","subject":"Re: [PATCH] builtin/gc: fix crash when running `git maintenance start`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-10-09T06:28:40Z","receivedAt":"2024-10-09T06:28:45Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Oct 08, 2024 at 10:58:06PM -0400, Derrick Stolee wrote:\n> On 10/8/24 2:30 PM, Derrick Stolee wrote:\n> > On 10/8/24 8:15 AM, Patrick Steinhardt wrote:\n> > > It was reported on the mailing list that running `git maintenance start`\n> > > immediately segfaults starting with b6c3f8e12c (builtin/maintenance: fix\n> > > leak in `get_schedule_cmd()`, 2024-09-26). And indeed, this segfault is\n> > > trivial to reproduce up to a point where one is scratching their head\n> > > why we didn't catch this regression in our test suite.\n> > \n> > > +test_expect_success 'start without GIT_TEST_MAINT_SCHEDULER' '\n> > > +    test_when_finished \"rm -rf crontab.log script repo\" &&\n> > > +    mkdir script &&\n> > > +    write_script script/crontab <<-EOF &&\n> > > +    echo \"\\$*\" >>\"$(pwd)\"/crontab.log\n> > > +    EOF\n> > > +    git init repo &&\n> > > +    (\n> > > +        cd repo &&\n> > > +        sane_unset GIT_TEST_MAINT_SCHEDULER &&\n> > > +        PATH=\"$(pwd)/../script:$PATH\" git maintenance start --scheduler=crontab\n> > > +    ) &&\n> > > +    test_grep -- -l crontab.log &&\n> > > +    test_grep -- git_cron_edit_tmp crontab.log\n> > > +'\n> > > +\n> > I see why we didn't catch this immediately. This is a good way to work\n> > around this issue of \"mocking\" the scheduler.\n> \n> Unfortunately, this test is broken on macOS and Windows. Those platforms will\n> fail when asked for 'crontab' without the test variable.\n> \n> Here is a potential fixup that will make your test succeed:\n\nOh, indeed, thanks for flagging this. The issue is that those platforms\ndo not make `crontab` available at all.\n\nI think we can land at a better fix though: the systemctl-based\nscheduler _is_ available on all platforms if the systemctl binary is\nfound. So let me adapt the script accordingly.\n\nPatrick\n"},{"id":"504522","messageId":"5798c31e1ef9346e7faf73f8c80b32c436937a8a.1728455715.git.ps@pks.im","threadId":"62280","inReplyTo":"CAG=Um+0mJW-oAH+YLC3dWEU64JwS-zMkkTiFWYBe4g6HMbe-iA@mail.gmail.com","subject":"[PATCH v2] builtin/gc: fix crash when running `git maintenance start`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-10-09T07:08:33Z","receivedAt":"2024-10-09T07:08:39Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"It was reported on the mailing list that running `git maintenance start`\nimmediately segfaults starting with b6c3f8e12c (builtin/maintenance: fix\nleak in `get_schedule_cmd()`, 2024-09-26). And indeed, this segfault is\ntrivial to reproduce up to a point where one is scratching their head\nwhy we didn't catch this regression in our test suite.\n\nThe root cause of this error is `get_schedule_cmd()`, which does not\npopulate the `out` parameter in all cases anymore starting with the\nmentioned commit. Callers do assume it to always be populated though and\nwill e.g. call `strvec_split()` on the returned value, which will of\ncourse segfault when the variable is uninitialized.\n\nSo why didn't we catch this trivial regression? The reason is that our\ntests always set up the \"GIT_TEST_MAINT_SCHEDULER\" environment variable\nvia \"t/test-lib.sh\", which allows us to override the scheduler command\nwith a custom one so that we don't accidentally modify the developer's\nsystem. But the faulty code where we don't set the `out` parameter will\nonly get hit in case that environment variable is _not_ set, which is\nnever the case when executing our tests.\n\nFix the regression by again unconditionally allocating the value in the\n`out` parameter, if provided. Add a test that unsets the environment\nvariable to catch future regressions in this area.\n\nReported-by: Shubham Kanodia <shubham.kanodia10@gmail.com>\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n\nThere's a single fix compared to v1, where Stolee mentioned that the\nadded test fails on both Windows and macOS because the \"crontab\"\nscheduler isn't available there. I've adapted the test to instead use\nthe \"systemd\" scheduler, which _is_ available on all platforms when the\nsystemctl(1) binary can be found.\n\nThanks!\n\nPatrick\n\n builtin/gc.c           |  7 +++++--\n t/t7900-maintenance.sh | 16 ++++++++++++++++\n 2 files changed, 21 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex 6a7a2da006..d52735354c 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -1832,7 +1832,7 @@ static const char *get_extra_launchctl_strings(void) {\n  *     | Input |                     Output                      |\n  *     | *cmd  | return code |       *out        | *is_available |\n  *     +-------+-------------+-------------------+---------------+\n- *     | \"foo\" |    false    | NULL              |  (unchanged)  |\n+ *     | \"foo\" |    false    | \"foo\" (allocated) |  (unchanged)  |\n  *     +-------+-------------+-------------------+---------------+\n  *\n  *   GIT_TEST_MAINT_SCHEDULER set to “foo:./mock_foo.sh,bar:./mock_bar.sh”\n@@ -1850,8 +1850,11 @@ static int get_schedule_cmd(const char *cmd, int *is_available, char **out)\n \tstruct string_list_item *item;\n \tstruct string_list list = STRING_LIST_INIT_NODUP;\n \n-\tif (!testing)\n+\tif (!testing) {\n+\t\tif (out)\n+\t\t\t*out = xstrdup(cmd);\n \t\treturn 0;\n+\t}\n \n \tif (is_available)\n \t\t*is_available = 0;\ndiff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh\nindex a66d0e089d..e75b485319 100755\n--- a/t/t7900-maintenance.sh\n+++ b/t/t7900-maintenance.sh\n@@ -646,6 +646,22 @@ test_expect_success !MINGW 'register and unregister with regex metacharacters' '\n \t\tmaintenance.repo \"$(pwd)/$META\"\n '\n \n+test_expect_success 'start without GIT_TEST_MAINT_SCHEDULER' '\n+\ttest_when_finished \"rm -rf systemctl.log script repo\" &&\n+\tmkdir script &&\n+\twrite_script script/systemctl <<-EOF &&\n+\techo \"\\$*\" >>\"$(pwd)\"/systemctl.log\n+\tEOF\n+\tgit init repo &&\n+\t(\n+\t\tcd repo &&\n+\t\tsane_unset GIT_TEST_MAINT_SCHEDULER &&\n+\t\tPATH=\"$(pwd)/../script:$PATH\" git maintenance start --scheduler=systemd\n+\t) &&\n+\ttest_grep -- \"--user list-timers\" systemctl.log &&\n+\ttest_grep -- \"enable --now git-maintenance@\" systemctl.log\n+'\n+\n test_expect_success 'start --scheduler=<scheduler>' '\n \ttest_expect_code 129 git maintenance start --scheduler=foo 2>err &&\n \ttest_grep \"unrecognized --scheduler argument\" err &&\n\nRange-diff against v1:\n1:  976c97081a ! 1:  5798c31e1e builtin/gc: fix crash when running `git maintenance start`\n    @@ t/t7900-maintenance.sh: test_expect_success !MINGW 'register and unregister with\n      '\n      \n     +test_expect_success 'start without GIT_TEST_MAINT_SCHEDULER' '\n    -+\ttest_when_finished \"rm -rf crontab.log script repo\" &&\n    ++\ttest_when_finished \"rm -rf systemctl.log script repo\" &&\n     +\tmkdir script &&\n    -+\twrite_script script/crontab <<-EOF &&\n    -+\techo \"\\$*\" >>\"$(pwd)\"/crontab.log\n    ++\twrite_script script/systemctl <<-EOF &&\n    ++\techo \"\\$*\" >>\"$(pwd)\"/systemctl.log\n     +\tEOF\n     +\tgit init repo &&\n     +\t(\n     +\t\tcd repo &&\n     +\t\tsane_unset GIT_TEST_MAINT_SCHEDULER &&\n    -+\t\tPATH=\"$(pwd)/../script:$PATH\" git maintenance start --scheduler=crontab\n    ++\t\tPATH=\"$(pwd)/../script:$PATH\" git maintenance start --scheduler=systemd\n     +\t) &&\n    -+\ttest_grep -- -l crontab.log &&\n    -+\ttest_grep -- git_cron_edit_tmp crontab.log\n    ++\ttest_grep -- \"--user list-timers\" systemctl.log &&\n    ++\ttest_grep -- \"enable --now git-maintenance@\" systemctl.log\n     +'\n     +\n      test_expect_success 'start --scheduler=<scheduler>' '\n-- \n2.47.0.rc1.33.g90fe3800b9.dirty\n\n"},{"id":"504556","messageId":"26e589c7-7625-409c-9913-9ed4b8ef9808@gmail.com","threadId":"62280","inReplyTo":"5798c31e1ef9346e7faf73f8c80b32c436937a8a.1728455715.git.ps@pks.im","subject":"Re: [PATCH v2] builtin/gc: fix crash when running `git maintenance start`","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2024-10-09T13:13:47Z","receivedAt":"2024-10-09T13:13:49Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 10/9/24 3:08 AM, Patrick Steinhardt wrote:\n\n> There's a single fix compared to v1, where Stolee mentioned that the\n> added test fails on both Windows and macOS because the \"crontab\"\n> scheduler isn't available there. I've adapted the test to instead use\n> the \"systemd\" scheduler, which _is_ available on all platforms when the\n> systemctl(1) binary can be found.\n\nI do appreciate that you are making the test work on all platforms\ninstead of my workaround to avoid the test on mac and Windows.\n\nI was initially confused by your description saying that systemctl is\navailable on these platforms, because it isn't for my machines. But\na way to rephrase what you mean is \"Git has compile-time expectations\nthat crontab doesn't exist for macOS and Windows, but Git checks for\n'systemctl' on the PATH regardless of compiled platform.\"\n\nThus, placing a systemctl script works for these platforms even when\nthey don't have systemctl available normally.\n\nI'm happy with this version.\n\nThanks,\n-Stolee\n\n"},{"id":"504606","messageId":"xmqq8quxw6bi.fsf@gitster.g","threadId":"62280","inReplyTo":"5798c31e1ef9346e7faf73f8c80b32c436937a8a.1728455715.git.ps@pks.im","subject":"Re: [PATCH v2] builtin/gc: fix crash when running `git maintenance start`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-10-09T17:10:25Z","receivedAt":"2024-10-09T17:10:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n> diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh\n> index a66d0e089d..e75b485319 100755\n> --- a/t/t7900-maintenance.sh\n> +++ b/t/t7900-maintenance.sh\n> @@ -646,6 +646,22 @@ test_expect_success !MINGW 'register and unregister with regex metacharacters' '\n>  \t\tmaintenance.repo \"$(pwd)/$META\"\n>  '\n>  \n> +test_expect_success 'start without GIT_TEST_MAINT_SCHEDULER' '\n> +\ttest_when_finished \"rm -rf systemctl.log script repo\" &&\n> +\tmkdir script &&\n> +\twrite_script script/systemctl <<-EOF &&\n> +\techo \"\\$*\" >>\"$(pwd)\"/systemctl.log\n> +\tEOF\n\nWe deliberately include one SP in the name of the trash directory\nwhen we create it under t/ so that we can catch a bug easily when we\nforget quoting \"$TRASH_DIRECTORY\", but otherwise the name under t/\nis safe.  \n\nBut this use of \"$(pwd)\" is a bit iffy.  It is expanded when the\nscript is written, which means the script is designed to write to\nthe systemctl.log file in a directory whose absolute path is known\nand hardcoded in the script.  We assume that the leading directories\ndown to the t/ directory from the root can also safely quoted by\nsimply enclosing inside \"a pair of double quotes\", which might be\niffy (obviously if $(pwd) had a double-quote in it, the quoting\nwould break).\n\nI'll let it pass (and will forget blissfully if nobody screams).\n\nOther than that, looking good.  It certainly is good that there is\nsomething more widely supported than \"cron\" so that we can write\nthis test as a generic one, even if some of us might not want to\ntouch \"systemd\" in the real life ;-)\n\nThanks, will queue.\n"},{"id":"504609","messageId":"xmqqttdluqxb.fsf@gitster.g","threadId":"62280","inReplyTo":"26e589c7-7625-409c-9913-9ed4b8ef9808@gmail.com","subject":"Re: [PATCH v2] builtin/gc: fix crash when running `git maintenance start`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-10-09T17:28:16Z","receivedAt":"2024-10-09T17:28:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Derrick Stolee <stolee@gmail.com> writes:\n\n> ... But\n> a way to rephrase what you mean is \"Git has compile-time expectations\n> that crontab doesn't exist for macOS and Windows, but Git checks for\n> 'systemctl' on the PATH regardless of compiled platform.\"\n>\n> Thus, placing a systemctl script works for these platforms even when\n> they don't have systemctl available normally.\n\nVery nice rewrite.  Thanks for pointing out what your initial\nreading hiccupped, as it is likely the same would happen to others.\n\n"},{"id":"504661","messageId":"xmqqmsjcoom9.fsf@gitster.g","threadId":"62280","inReplyTo":"xmqqbjzu1m2j.fsf@gitster.g","subject":"Re: [PATCH] builtin/gc: fix crash when running `git maintenance start`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-10-09T23:14:38Z","receivedAt":"2024-10-09T23:14:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Patrick Steinhardt <ps@pks.im> writes:\n>\n>> Fix the regression by again unconditionally allocating the value in the\n>> `out` parameter, if provided. Add a test that unsets the environment\n>> variable to catch future regressions in this area.\n>>\n>> Reported-by: Shubham Kanodia <shubham.kanodia10@gmail.com>\n>> Signed-off-by: Patrick Steinhardt <ps@pks.im>\n>> ---\n>\n> Thanks for quickly reporting and addressing this one, both of you.\n>\n> Will queue.\n\nWe seem to be getting:\n\n  git maintenance start --scheduler=systemd\n  D:/a/git/git/git.exe: error while loading shared libraries: ?: cannot open shared object file: No such file or directory\n\nhttps://github.com/git/git/actions/runs/11264159690/job/31323795058#step:5:327\n\n"},{"id":"504668","messageId":"ZwdeFB2-LUuHVm08@pks.im","threadId":"62280","inReplyTo":"xmqqmsjcoom9.fsf@gitster.g","subject":"Re: [PATCH] builtin/gc: fix crash when running `git maintenance start`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-10-10T04:54:53Z","receivedAt":"2024-10-10T04:54:58Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Oct 09, 2024 at 04:14:38PM -0700, Junio C Hamano wrote:\n> Junio C Hamano <gitster@pobox.com> writes:\n> \n> > Patrick Steinhardt <ps@pks.im> writes:\n> >\n> >> Fix the regression by again unconditionally allocating the value in the\n> >> `out` parameter, if provided. Add a test that unsets the environment\n> >> variable to catch future regressions in this area.\n> >>\n> >> Reported-by: Shubham Kanodia <shubham.kanodia10@gmail.com>\n> >> Signed-off-by: Patrick Steinhardt <ps@pks.im>\n> >> ---\n> >\n> > Thanks for quickly reporting and addressing this one, both of you.\n> >\n> > Will queue.\n> \n> We seem to be getting:\n> \n>   git maintenance start --scheduler=systemd\n>   D:/a/git/git/git.exe: error while loading shared libraries: ?: cannot open shared object file: No such file or directory\n> \n> https://github.com/git/git/actions/runs/11264159690/job/31323795058#step:5:327\n\nOh well. Lucky me that I'm spending half of my life in Windows VMs now\nanyway, so I'll have a look.\n\nPatrick\n"},{"id":"504669","messageId":"ZwdhyeVmjIX48k3X@pks.im","threadId":"62280","inReplyTo":"ZwdeFB2-LUuHVm08@pks.im","subject":"Re: [PATCH] builtin/gc: fix crash when running `git maintenance start`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-10-10T05:10:33Z","receivedAt":"2024-10-10T05:10:40Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, Oct 10, 2024 at 06:54:53AM +0200, Patrick Steinhardt wrote:\n> On Wed, Oct 09, 2024 at 04:14:38PM -0700, Junio C Hamano wrote:\n> > Junio C Hamano <gitster@pobox.com> writes:\n> > \n> > > Patrick Steinhardt <ps@pks.im> writes:\n> > >\n> > >> Fix the regression by again unconditionally allocating the value in the\n> > >> `out` parameter, if provided. Add a test that unsets the environment\n> > >> variable to catch future regressions in this area.\n> > >>\n> > >> Reported-by: Shubham Kanodia <shubham.kanodia10@gmail.com>\n> > >> Signed-off-by: Patrick Steinhardt <ps@pks.im>\n> > >> ---\n> > >\n> > > Thanks for quickly reporting and addressing this one, both of you.\n> > >\n> > > Will queue.\n> > \n> > We seem to be getting:\n> > \n> >   git maintenance start --scheduler=systemd\n> >   D:/a/git/git/git.exe: error while loading shared libraries: ?: cannot open shared object file: No such file or directory\n> > \n> > https://github.com/git/git/actions/runs/11264159690/job/31323795058#step:5:327\n> \n> Oh well. Lucky me that I'm spending half of my life in Windows VMs now\n> anyway, so I'll have a look.\n> \n> Patrick\n\nAh, the difference between `pwd` and `$PWD` strikes again: the former\nuses Windows-style paths, the latter uses Cygwin-style ones. And as the\nWindows-style path contains a colon the resulting PATH variable was\nbroken.\n\nWill send v3 in a bit, thanks!\n\nPatrick\n"},{"id":"504672","messageId":"a5b1433abfd84cb627efc17f52e0d644ee207bb0.1728538282.git.ps@pks.im","threadId":"62280","inReplyTo":"CAG=Um+0mJW-oAH+YLC3dWEU64JwS-zMkkTiFWYBe4g6HMbe-iA@mail.gmail.com","subject":"[PATCH v3] builtin/gc: fix crash when running `git maintenance start`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-10-10T05:33:01Z","receivedAt":"2024-10-10T05:33:05Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"It was reported on the mailing list that running `git maintenance start`\nimmediately segfaults starting with b6c3f8e12c (builtin/maintenance: fix\nleak in `get_schedule_cmd()`, 2024-09-26). And indeed, this segfault is\ntrivial to reproduce up to a point where one is scratching their head\nwhy we didn't catch this regression in our test suite.\n\nThe root cause of this error is `get_schedule_cmd()`, which does not\npopulate the `out` parameter in all cases anymore starting with the\nmentioned commit. Callers do assume it to always be populated though and\nwill e.g. call `strvec_split()` on the returned value, which will of\ncourse segfault when the variable is uninitialized.\n\nSo why didn't we catch this trivial regression? The reason is that our\ntests always set up the \"GIT_TEST_MAINT_SCHEDULER\" environment variable\nvia \"t/test-lib.sh\", which allows us to override the scheduler command\nwith a custom one so that we don't accidentally modify the developer's\nsystem. But the faulty code where we don't set the `out` parameter will\nonly get hit in case that environment variable is _not_ set, which is\nnever the case when executing our tests.\n\nFix the regression by again unconditionally allocating the value in the\n`out` parameter, if provided. Add a test that unsets the environment\nvariable to catch future regressions in this area.\n\nReported-by: Shubham Kanodia <shubham.kanodia10@gmail.com>\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n\nTwo changes compared to v2:\n\n  - We do not expand \"$pwd\" in the HERE doc anymore.\n\n  - Fixed \"$(pwd)\" vs \"$PWD\", which broke Windows builds due to a\n    misformatted PATH.\n\nThanks!\n\nPatrick\n\n builtin/gc.c           |  7 +++++--\n t/t7900-maintenance.sh | 16 ++++++++++++++++\n 2 files changed, 21 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex 6a7a2da006..d52735354c 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -1832,7 +1832,7 @@ static const char *get_extra_launchctl_strings(void) {\n  *     | Input |                     Output                      |\n  *     | *cmd  | return code |       *out        | *is_available |\n  *     +-------+-------------+-------------------+---------------+\n- *     | \"foo\" |    false    | NULL              |  (unchanged)  |\n+ *     | \"foo\" |    false    | \"foo\" (allocated) |  (unchanged)  |\n  *     +-------+-------------+-------------------+---------------+\n  *\n  *   GIT_TEST_MAINT_SCHEDULER set to “foo:./mock_foo.sh,bar:./mock_bar.sh”\n@@ -1850,8 +1850,11 @@ static int get_schedule_cmd(const char *cmd, int *is_available, char **out)\n \tstruct string_list_item *item;\n \tstruct string_list list = STRING_LIST_INIT_NODUP;\n \n-\tif (!testing)\n+\tif (!testing) {\n+\t\tif (out)\n+\t\t\t*out = xstrdup(cmd);\n \t\treturn 0;\n+\t}\n \n \tif (is_available)\n \t\t*is_available = 0;\ndiff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh\nindex a66d0e089d..c224c8450c 100755\n--- a/t/t7900-maintenance.sh\n+++ b/t/t7900-maintenance.sh\n@@ -646,6 +646,22 @@ test_expect_success !MINGW 'register and unregister with regex metacharacters' '\n \t\tmaintenance.repo \"$(pwd)/$META\"\n '\n \n+test_expect_success 'start without GIT_TEST_MAINT_SCHEDULER' '\n+\ttest_when_finished \"rm -rf systemctl.log script repo\" &&\n+\tmkdir script &&\n+\twrite_script script/systemctl <<-\\EOF &&\n+\techo \"$*\" >>../systemctl.log\n+\tEOF\n+\tgit init repo &&\n+\t(\n+\t\tcd repo &&\n+\t\tsane_unset GIT_TEST_MAINT_SCHEDULER &&\n+\t\tPATH=\"$PWD/../script:$PATH\" git maintenance start --scheduler=systemd\n+\t) &&\n+\ttest_grep -- \"--user list-timers\" systemctl.log &&\n+\ttest_grep -- \"enable --now git-maintenance@\" systemctl.log\n+'\n+\n test_expect_success 'start --scheduler=<scheduler>' '\n \ttest_expect_code 129 git maintenance start --scheduler=foo 2>err &&\n \ttest_grep \"unrecognized --scheduler argument\" err &&\n\nRange-diff against v2:\n1:  5798c31e1e ! 1:  a5b1433abf builtin/gc: fix crash when running `git maintenance start`\n    @@ t/t7900-maintenance.sh: test_expect_success !MINGW 'register and unregister with\n     +test_expect_success 'start without GIT_TEST_MAINT_SCHEDULER' '\n     +\ttest_when_finished \"rm -rf systemctl.log script repo\" &&\n     +\tmkdir script &&\n    -+\twrite_script script/systemctl <<-EOF &&\n    -+\techo \"\\$*\" >>\"$(pwd)\"/systemctl.log\n    ++\twrite_script script/systemctl <<-\\EOF &&\n    ++\techo \"$*\" >>../systemctl.log\n     +\tEOF\n     +\tgit init repo &&\n     +\t(\n     +\t\tcd repo &&\n     +\t\tsane_unset GIT_TEST_MAINT_SCHEDULER &&\n    -+\t\tPATH=\"$(pwd)/../script:$PATH\" git maintenance start --scheduler=systemd\n    ++\t\tPATH=\"$PWD/../script:$PATH\" git maintenance start --scheduler=systemd\n     +\t) &&\n     +\ttest_grep -- \"--user list-timers\" systemctl.log &&\n     +\ttest_grep -- \"enable --now git-maintenance@\" systemctl.log\n-- \n2.47.0.dirty\n\n"},{"id":"504718","messageId":"xmqqcyk7lwa4.fsf@gitster.g","threadId":"62280","inReplyTo":"a5b1433abfd84cb627efc17f52e0d644ee207bb0.1728538282.git.ps@pks.im","subject":"Re: [PATCH v3] builtin/gc: fix crash when running `git maintenance start`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-10-10T17:09:39Z","receivedAt":"2024-10-10T17:09:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n> +\twrite_script script/systemctl <<-\\EOF &&\n> +\techo \"$*\" >>../systemctl.log\n> +\tEOF\n\nAh, for the purpose of this test, we _know_ in which directory the\n\"systemctl\" will be spawned, so this is good enough for us, of\ncourse.\n\n> +\tgit init repo &&\n> +\t(\n> +\t\tcd repo &&\n> +\t\tsane_unset GIT_TEST_MAINT_SCHEDULER &&\n> +\t\tPATH=\"$PWD/../script:$PATH\" git maintenance start --scheduler=systemd\n\nI suspect we can use the same idea and add a relative path in $PATH\nfor the test, perhaps, even though it is not a good coding\ndiscipline.  If $PWD, instead of $(pwd), works, it is perfectly OK.\n\nWill queue.  Thanks.\n"},{"id":"504966","messageId":"CAG=Um+3=SQ7bZYvr4BNxWgMBNreQHgfs-0VQwkzBq=HW8Zxf=Q@mail.gmail.com","threadId":"62280","inReplyTo":"xmqqcyk7lwa4.fsf@gitster.g","subject":"Re: [PATCH v3] builtin/gc: fix crash when running `git maintenance start`","fromName":"Shubham Kanodia","fromEmail":"shubham.kanodia10@gmail.com","sentAt":"2024-10-14T04:44:53Z","receivedAt":"2024-10-14T04:45:31Z","isPatch":true,"sender":{"key":"shubham.kanodia10@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8946207?v=4"},"body":"On Thu, Oct 10, 2024 at 10:39 PM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Patrick Steinhardt <ps@pks.im> writes:\n>\n> > +     write_script script/systemctl <<-\\EOF &&\n> > +     echo \"$*\" >>../systemctl.log\n> > +     EOF\n>\n> Ah, for the purpose of this test, we _know_ in which directory the\n> \"systemctl\" will be spawned, so this is good enough for us, of\n> course.\n>\n> > +     git init repo &&\n> > +     (\n> > +             cd repo &&\n> > +             sane_unset GIT_TEST_MAINT_SCHEDULER &&\n> > +             PATH=\"$PWD/../script:$PATH\" git maintenance start --scheduler=systemd\n>\n> I suspect we can use the same idea and add a relative path in $PATH\n> for the test, perhaps, even though it is not a good coding\n> discipline.  If $PWD, instead of $(pwd), works, it is perfectly OK.\n>\n> Will queue.  Thanks.\n\nAppreciate for the quick fix, Patrick.\n\nHomebrew upgraded their formulas to 2.47 rather quickly (the next day\nafter release) —\nhttps://github.com/Homebrew/homebrew-core/commit/0435f258701abd3acb9e2f4cd758cc13aa93997c\n\nMac users who do a `brew install git` would now install versions with\na broken maintenance command.\nFortunately, `brew` auto-updates the world every time a user installs\nanything so it's likely they get to a 2.47.1 in the future,\nbut that still might be a while away from when they install the\ncurrent latest (2.47.0).\n\nI'm not sure if Git has a hotfix workflow, but it might make sense to\nprevent more users from getting onto the buggy version\n(especially since repo admins usually set up maintenance in the\nbackground and the error might not be evident to users).\n"},{"id":"504973","messageId":"ZwzYk9dq1H9QGXBG@pks.im","threadId":"62280","inReplyTo":"CAG=Um+3=SQ7bZYvr4BNxWgMBNreQHgfs-0VQwkzBq=HW8Zxf=Q@mail.gmail.com","subject":"Re: [PATCH v3] builtin/gc: fix crash when running `git maintenance start`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-10-14T08:38:43Z","receivedAt":"2024-10-14T08:38:59Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, Oct 14, 2024 at 10:14:53AM +0530, Shubham Kanodia wrote:\n> On Thu, Oct 10, 2024 at 10:39 PM Junio C Hamano <gitster@pobox.com> wrote:\n> >\n> > Patrick Steinhardt <ps@pks.im> writes:\n> >\n> > > +     write_script script/systemctl <<-\\EOF &&\n> > > +     echo \"$*\" >>../systemctl.log\n> > > +     EOF\n> >\n> > Ah, for the purpose of this test, we _know_ in which directory the\n> > \"systemctl\" will be spawned, so this is good enough for us, of\n> > course.\n> >\n> > > +     git init repo &&\n> > > +     (\n> > > +             cd repo &&\n> > > +             sane_unset GIT_TEST_MAINT_SCHEDULER &&\n> > > +             PATH=\"$PWD/../script:$PATH\" git maintenance start --scheduler=systemd\n> >\n> > I suspect we can use the same idea and add a relative path in $PATH\n> > for the test, perhaps, even though it is not a good coding\n> > discipline.  If $PWD, instead of $(pwd), works, it is perfectly OK.\n> >\n> > Will queue.  Thanks.\n> \n> Appreciate for the quick fix, Patrick.\n> \n> Homebrew upgraded their formulas to 2.47 rather quickly (the next day\n> after release) —\n> https://github.com/Homebrew/homebrew-core/commit/0435f258701abd3acb9e2f4cd758cc13aa93997c\n> \n> Mac users who do a `brew install git` would now install versions with\n> a broken maintenance command.\n> Fortunately, `brew` auto-updates the world every time a user installs\n> anything so it's likely they get to a 2.47.1 in the future,\n> but that still might be a while away from when they install the\n> current latest (2.47.0).\n> \n> I'm not sure if Git has a hotfix workflow, but it might make sense to\n> prevent more users from getting onto the buggy version\n> (especially since repo admins usually set up maintenance in the\n> background and the error might not be evident to users).\n\nI'm not sure around the timeline for Git v2.47.1, and Junio is going to\nbe out of office for two weeks, so it may take a while. I'd recommend to\nbackport the patch for now.\n\nPatrick\n"},{"id":"505105","messageId":"Zw25AKpaZO0w4APv@nand.local","threadId":"62280","inReplyTo":"ZwzYk9dq1H9QGXBG@pks.im","subject":"Re: [PATCH v3] builtin/gc: fix crash when running `git maintenance start`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-10-15T00:36:16Z","receivedAt":"2024-10-15T00:36:18Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Oct 14, 2024 at 10:38:43AM +0200, Patrick Steinhardt wrote:\n> I'm not sure around the timeline for Git v2.47.1, and Junio is going to\n> be out of office for two weeks, so it may take a while. I'd recommend to\n> backport the patch for now.\n\nI'm not planning on cutting any release while Junio is gone, so I'd\nexpect that a hypothetical 2.47.1 release wouldn't occur until the\nbeginning of November at the earliest.\n\nThanks,\nTaylor\n"}]}