{"thread":{"id":"54708","subject":"[PATCH 0/1] maintenance: Fix a SEGFAULT when no repository when running git maintenance run/start","startedAt":"2020-11-24T16:44:58Z","lastAt":"2020-12-24T14:17:15Z","messageCount":22,"participants":["Rafael Silva","Derrick Stolee","Eric Sunshine","Martin Ågren","SZEDER Gábor","Junio C Hamano","Josh Steadmon","Christian Couder"],"isPatch":true,"patchVersion":1,"patchTotal":1},"messages":[{"id":"410687","messageId":"20201124164405.29327-1-rafaeloliveira.cs@gmail.com","threadId":"54708","inReplyTo":null,"subject":"[PATCH 0/1] maintenance: Fix a SEGFAULT when no repository when running git maintenance run/start","fromName":"Rafael Silva","fromEmail":"rafaeloliveira.cs@gmail.com","sentAt":"2020-11-24T16:44:04Z","receivedAt":"2020-11-24T16:44:58Z","isPatch":true,"sender":{"key":"rafaeloliveira.cs@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5935135?v=4"},"body":"In d7514f6ed5 (maintenance: take a lock on the objects directory, 2020-09-17) [1],\nand 2fec604f8d (maintenance: add start/stop subcommands, 2020-09-11) [2] The\n\"git maintenance run\" and \"git maintenance start\" was taught to hold a file-based\nlock at the .git/maintenance.lock and .git/schedule.lock respectively because these\noperations involves writing data into the .git/repository. \n\nThe lock file path string is built using the the_repository->objects->odb->path,\nin case the_repository->objects->odb is NULL when there is not repository available,\nresulting in a SEGFAULT.\n\n[1] https://lore.kernel.org/git/1a0a3eebb825ac3eabfdd86f82ed7ef6abb454c5.1600366313.git.gitgitgadget@gmail.com/\n[2] https://lore.kernel.org/git/5194f6b1facbd14cc17eea0337c0cc397a2a51fc.1602782524.git.gitgitgadget@gmail.com/\n\nIn order to reproduce the error, one can execute maintenance \"run\" and/or\n\"start\" subcommand with a non valid repository: \n\n $ git -C /tmp maintenance start\n Segmentation fault\n\n $ git -C /tmp maintenance run\n Segmentation fault\n\nThe above test was executed from a git built from commit: faefdd61ec (Sixth batch, 2020-11-18):\n\nFor reference here's the output from GDB when debugging the \"start\" command\n\n\tProgram received signal SIGSEGV, Segmentation fault.\n\t0x00005555555b9b4c in maintenance_run_tasks (opts=0x7fffffffded4) at builtin/gc.c:1268\n\t1268\t\tchar *lock_path = xstrfmt(\"%s/maintenance\", r->objects->odb->path);\n\nThis patch serie adds a check on the maintenance_run_tasks() and update_background_schedule()\nto check if the_repository->git_dir is set to validate if a git repository is available and\nreturn error otherwise. The logic is borrowed from maintenance_unregister() that performs\nthe same validation with different error message.\n\nA new test case is added into t/t7900-maintenance.sh to cover these cases. The test is based on\nthe assumption that the `/tmp` directory is available, readable and is not a git repository\nwhich I hope is fine for the running the test is all platforms. \n\nThis patch is based on faefdd61ec (Sixth batch, 2020-11-18) (master branch) as the both commits\nthat introduced the file-based lock are graduated to master already. Hope this also plays nice\nwith the integration branches maintenance-part-v[1,4]. \n\nRafael Silva (1):\n maintenance: fix a SEGFAULT when no repository\n\n builtin/gc.c | 14 ++++++++++++--\n t/t7900-maintenance.sh | 5 +++++\n 2 files changed, 17 insertions(+), 2 deletions(-)\n\n-- \n2.29.2.505.g04529851e5\n\n"},{"id":"410688","messageId":"20201124164405.29327-2-rafaeloliveira.cs@gmail.com","threadId":"54708","inReplyTo":"20201124164405.29327-1-rafaeloliveira.cs@gmail.com","subject":"[PATCH 1/1] maintenance: fix a SEGFAULT when no repository","fromName":"Rafael Silva","fromEmail":"rafaeloliveira.cs@gmail.com","sentAt":"2020-11-24T16:44:05Z","receivedAt":"2020-11-24T16:44:59Z","isPatch":true,"sender":{"key":"rafaeloliveira.cs@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5935135?v=4"},"body":"The \"git maintenance run\" and \"git maintenance start\" commands holds a\nfile-based lock at the .git/maintenance.lock and .git/schedule.lock\nrespectively. These locks are used to ensure only one maintenance process\nis executed at the time as both operations involves writing data into\nthe git repository.\n\nThe path to the lock file is built using the \"the_repository->objects->odb->path\"\nthat results in SEGFAULT when we have no repository available as\n\"the_repository->objects->odb\" is set to NULL.\n\nLet's teach the maintenance_run_tasks() and update_background_schedule() to return\nan error and fails the command when we have no repository available.\n\nSigned-off-by: Rafael Silva \n\n---\n builtin/gc.c | 14 ++++++++++++--\n t/t7900-maintenance.sh | 5 +++++\n 2 files changed, 17 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex 3d258b60c2..d133d93a86 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -1265,9 +1265,14 @@ static int maintenance_run_tasks(struct maintenance_run_opts *opts)\n {\n \tint i, found_selected = 0;\n \tint result = 0;\n+\tchar *lock_path;\n \tstruct lock_file lk;\n \tstruct repository *r = the_repository;\n-\tchar *lock_path = xstrfmt(\"%s/maintenance\", r->objects->odb->path);\n+\n+\tif (!r || !r->gitdir)\n+\t\treturn error(_(\"not a git repository\"));\n+\n+\tlock_path = xstrfmt(\"%s/maintenance\", the_repository->objects->odb->path);\n \n \tif (hold_lock_file_for_update(&lk, lock_path, LOCK_NO_DEREF) < 0) {\n \t\t/*\n@@ -1513,8 +1518,13 @@ static int update_background_schedule(int run_maintenance)\n \tFILE *cron_list, *cron_in;\n \tconst char *crontab_name;\n \tstruct strbuf line = STRBUF_INIT;\n+\tchar *lock_path;\n \tstruct lock_file lk;\n-\tchar *lock_path = xstrfmt(\"%s/schedule\", the_repository->objects->odb->path);\n+\n+\tif (!the_repository || !the_repository->gitdir)\n+\t\treturn error(_(\"not a git repository\"));\n+\n+\tlock_path = xstrfmt(\"%s/schedule\", the_repository->objects->odb->path);\n \n \tif (hold_lock_file_for_update(&lk, lock_path, LOCK_NO_DEREF) < 0)\n \t\treturn error(_(\"another process is scheduling background maintenance\"));\ndiff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh\nindex d9e68bb2bf..bb3556888d 100755\n--- a/t/t7900-maintenance.sh\n+++ b/t/t7900-maintenance.sh\n@@ -441,4 +441,9 @@ test_expect_success 'register preserves existing strategy' '\n \ttest_config maintenance.strategy incremental\n '\n \n+test_expect_success 'run and start command fails when no git repository' '\n+\ttest_must_fail git -C /tmp/ maintenance run &&\n+\ttest_must_fail git -C /tmp/ maintenance start\n+'\n+\n test_done\n-- \n2.29.2.505.g04529851e5\n\n"},{"id":"410689","messageId":"1bfd84da-5b74-be10-fc2c-dee80111ee2d@gmail.com","threadId":"54708","inReplyTo":"20201124164405.29327-2-rafaeloliveira.cs@gmail.com","subject":"Re: [PATCH 1/1] maintenance: fix a SEGFAULT when no repository","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2020-11-24T17:22:40Z","receivedAt":"2020-11-24T17:22:49Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 11/24/2020 11:44 AM, Rafael Silva wrote:\n> The \"git maintenance run\" and \"git maintenance start\" commands holds a\n> file-based lock at the .git/maintenance.lock and .git/schedule.lock\n> respectively. These locks are used to ensure only one maintenance process\n> is executed at the time as both operations involves writing data into\n> the git repository.\n> \n> The path to the lock file is built using the \"the_repository->objects->odb->path\"\n> that results in SEGFAULT when we have no repository available as\n> \"the_repository->objects->odb\" is set to NULL.\n> \n> Let's teach the maintenance_run_tasks() and update_background_schedule() to return\n> an error and fails the command when we have no repository available.\n\nThank you for noticing this problem, and for a quick fix.\n\nWhile I don't necessarily have a problem with this approach, perhaps\nit would be more robust to change the options in git.c to require a\nGIT_DIR, as in this diff?\n\n-- >8 --\n\ndiff --git a/git.c b/git.c\nindex 1cab64b5d1..c3dabd2553 100644\n--- a/git.c\n+++ b/git.c\n@@ -530,7 +530,7 @@ static struct cmd_struct commands[] = {\n { \"ls-tree\", cmd_ls_tree, RUN_SETUP },\n { \"mailinfo\", cmd_mailinfo, RUN_SETUP_GENTLY | NO_PARSEOPT },\n { \"mailsplit\", cmd_mailsplit, NO_PARSEOPT },\n- { \"maintenance\", cmd_maintenance, RUN_SETUP_GENTLY | NO_PARSEOPT },\n+ { \"maintenance\", cmd_maintenance, RUN_SETUP | NO_PARSEOPT },\n { \"merge\", cmd_merge, RUN_SETUP | NEED_WORK_TREE },\n { \"merge-base\", cmd_merge_base, RUN_SETUP },\n { \"merge-file\", cmd_merge_file, RUN_SETUP_GENTLY },\n\n-- >8 --\n\nIf the above code change fixes your test (below), then that would\nprobably be a safer change.\n\nThe reason to use RUN_SETUP_GENTLY was probably due to some thought\nof modifying the background maintenance schedule without being in a\nGit repository. However, we currently run the [un]register logic\ninside of the stop|start subcommands, so a GIT_DIR is required there,\ntoo.\n\n> diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh\n> index d9e68bb2bf..bb3556888d 100755\n> --- a/t/t7900-maintenance.sh\n> +++ b/t/t7900-maintenance.sh\n> @@ -441,4 +441,9 @@ test_expect_success 'register preserves existing strategy' '\n> \ttest_config maintenance.strategy incremental\n> '\n> \n> +test_expect_success 'run and start command fails when no git repository' '\n> +\ttest_must_fail git -C /tmp/ maintenance run &&\n> +\ttest_must_fail git -C /tmp/ maintenance start\n> +'\n> +\n> test_done\n\nThanks,\n-Stolee\n\n"},{"id":"410690","messageId":"CAPig+cQ-iWVz2Q1PtvbV0hk_HHRFqAFjxAF2DZ6doh2RxpZJhw@mail.gmail.com","threadId":"54708","inReplyTo":"20201124164405.29327-2-rafaeloliveira.cs@gmail.com","subject":"Re: [PATCH 1/1] maintenance: fix a SEGFAULT when no repository","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-11-24T17:24:57Z","receivedAt":"2020-11-24T17:25:12Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Nov 24, 2020 at 11:45 AM Rafael Silva\n wrote:\n> The \"git maintenance run\" and \"git maintenance start\" commands holds a\n> file-based lock at the .git/maintenance.lock and .git/schedule.lock\n> respectively. These locks are used to ensure only one maintenance process\n> is executed at the time as both operations involves writing data into\n> the git repository.\n>\n> The path to the lock file is built using the \"the_repository->objects->odb->path\"\n> that results in SEGFAULT when we have no repository available as\n> \"the_repository->objects->odb\" is set to NULL.\n\nThis issue came up in review recently[1] in an unrelated way.\n\n[1]: https://lore.kernel.org/git/CAPig+cRFQfg-NLx5dO+BjQpYduhOYs-_+ZRd=DhO8ebWjGB0iA@mail.gmail.com/\n\n> Let's teach the maintenance_run_tasks() and update_background_schedule() to return\n> an error and fails the command when we have no repository available.\n>\n> Signed-off-by: Rafael Silva \n> ---\n> diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh\n> @@ -441,4 +441,9 @@ test_expect_success 'register preserves existing strategy' '\n> +test_expect_success 'run and start command fails when no git repository' '\n> + test_must_fail git -C /tmp/ maintenance run &&\n> + test_must_fail git -C /tmp/ maintenance start\n> +'\n\nI wouldn't feel comfortable relying upon existence of /tmp/. It might\nbe sufficient to do this instead:\n\n mv .git save.git &&\n test_when_finished \"mv save.git .git\" &&\n test_must_fail git maintenance run &&\n test_must_fail git maintenance start\n"},{"id":"410691","messageId":"CAN0heSpjYEZbjXcX-XSvwwBKOEQ=Dtk=Nnr3T+BOqB+fd7x4sQ@mail.gmail.com","threadId":"54708","inReplyTo":"20201124164405.29327-2-rafaeloliveira.cs@gmail.com","subject":"Re: [PATCH 1/1] maintenance: fix a SEGFAULT when no repository","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2020-11-24T19:03:33Z","receivedAt":"2020-11-24T19:03:48Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On Tue, 24 Nov 2020 at 17:47, Rafael Silva wrote:\n> @@ -1265,9 +1265,14 @@ static int maintenance_run_tasks(struct maintenance_run_opts *opts)\n> {\n> int i, found_selected = 0;\n> int result = 0;\n> + char *lock_path;\n> struct lock_file lk;\n> struct repository *r = the_repository;\n> - char *lock_path = xstrfmt(\"%s/maintenance\", r->objects->odb->path);\n> +\n> + if (!r || !r->gitdir)\n> + return error(_(\"not a git repository\"));\n> +\n> + lock_path = xstrfmt(\"%s/maintenance\", the_repository->objects->odb->path);\n\ns/the_repository/r/\n\n(The preimage uses \"r\" and you check using \"r\".)\n\n> @@ -1513,8 +1518,13 @@ static int update_background_schedule(int run_maintenance)\n> FILE *cron_list, *cron_in;\n> const char *crontab_name;\n> struct strbuf line = STRBUF_INIT;\n> + char *lock_path;\n> struct lock_file lk;\n> - char *lock_path = xstrfmt(\"%s/schedule\", the_repository->objects->odb->path);\n> +\n> + if (!the_repository || !the_repository->gitdir)\n> + return error(_(\"not a git repository\"));\n> +\n> + lock_path = xstrfmt(\"%s/schedule\", the_repository->objects->odb->path);\n\nHere it's \"the_repository\" both before and after. Ok.\n\nMartin\n"},{"id":"410693","messageId":"20201124191407.GC8396@szeder.dev","threadId":"54708","inReplyTo":"CAPig+cQ-iWVz2Q1PtvbV0hk_HHRFqAFjxAF2DZ6doh2RxpZJhw@mail.gmail.com","subject":"Re: [PATCH 1/1] maintenance: fix a SEGFAULT when no repository","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2020-11-24T19:14:07Z","receivedAt":"2020-11-24T19:14:15Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Tue, Nov 24, 2020 at 12:24:57PM -0500, Eric Sunshine wrote:\n> On Tue, Nov 24, 2020 at 11:45 AM Rafael Silva\n> wrote:\n> > diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh\n> > @@ -441,4 +441,9 @@ test_expect_success 'register preserves existing strategy' '\n> > +test_expect_success 'run and start command fails when no git repository' '\n> > + test_must_fail git -C /tmp/ maintenance run &&\n> > + test_must_fail git -C /tmp/ maintenance start\n> > +'\n> \n> I wouldn't feel comfortable relying upon existence of /tmp/.\n\nIndeed.\n\n> It might\n> be sufficient to do this instead:\n> \n> mv .git save.git &&\n> test_when_finished \"mv save.git .git\" &&\n> test_must_fail git maintenance run &&\n> test_must_fail git maintenance start\n\nOur test library contains the 'nongit' helper function exactly for\nthis purpose:\n\n nongit test_must_fail git maintenance run &&\n nongit test_must_fail git maintenance start\n\n"},{"id":"410696","messageId":"CAPig+cTnz2rqs4PY0uTaOrySZDR9Jpi2fUL-x5SJzbceZhoY1A@mail.gmail.com","threadId":"54708","inReplyTo":"20201124191407.GC8396@szeder.dev","subject":"Re: [PATCH 1/1] maintenance: fix a SEGFAULT when no repository","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-11-24T19:34:18Z","receivedAt":"2020-11-24T19:34:34Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Nov 24, 2020 at 2:14 PM SZEDER Gábor wrote:\n> On Tue, Nov 24, 2020 at 12:24:57PM -0500, Eric Sunshine wrote:\n> > mv .git save.git &&\n> > test_when_finished \"mv save.git .git\" &&\n> > test_must_fail git maintenance run &&\n> > test_must_fail git maintenance start\n>\n> Our test library contains the 'nongit' helper function exactly for\n> this purpose:\n>\n> nongit test_must_fail git maintenance run &&\n> nongit test_must_fail git maintenance start\n\nPerfect. I forgot about nongit(). Thanks.\n\nI had intended on suggesting GIT_CEILING_DIRECTORIES in my response --\nwhich is what nongit() employs -- but couldn't get it to work for some\nreason, so I instead suggested moving .git/ aside temporarily.\n"},{"id":"410717","messageId":"xmqqim9uifz8.fsf@gitster.c.googlers.com","threadId":"54708","inReplyTo":"1bfd84da-5b74-be10-fc2c-dee80111ee2d@gmail.com","subject":"Re: [PATCH 1/1] maintenance: fix a SEGFAULT when no repository","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-11-24T21:48:11Z","receivedAt":"2020-11-24T21:48:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Derrick Stolee writes:\n\n> The reason to use RUN_SETUP_GENTLY was probably due to some thought\n> of modifying the background maintenance schedule without being in a\n> Git repository. However, we currently run the [un]register logic\n> inside of the stop|start subcommands, so a GIT_DIR is required there,\n> too.\n\nMeaning all the operations we currently support requires to be done\nin a repository? If so, that may be acceptable.\n\nWhen a new operation that does not require to be in a repository is\nadded, or when an existing operation is updated not to require to be\nin a repository, reverting the change and then checking in the\nimplementation of each operation if we are in a repository instead\nshould be easy enough---it pretty much should amount to Rafael's\npatch, right?\n\nBut then, being prepared for that future already is also OK, so I\ncan go either way. Please figure it out between you ;-)\n\nThanks, all.\n"},{"id":"410719","messageId":"7fbe28ff-6d0f-f377-d86f-0b32f12ddee6@gmail.com","threadId":"54708","inReplyTo":"xmqqim9uifz8.fsf@gitster.c.googlers.com","subject":"Re: [PATCH 1/1] maintenance: fix a SEGFAULT when no repository","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2020-11-24T21:59:08Z","receivedAt":"2020-11-24T21:59:12Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 11/24/2020 4:48 PM, Junio C Hamano wrote:\n> Derrick Stolee writes:\n> \n>> The reason to use RUN_SETUP_GENTLY was probably due to some thought\n>> of modifying the background maintenance schedule without being in a\n>> Git repository. However, we currently run the [un]register logic\n>> inside of the stop|start subcommands, so a GIT_DIR is required there,\n>> too.\n> \n> Meaning all the operations we currently support requires to be done\n> in a repository? If so, that may be acceptable.\n\nThat is the current case.\n\n> When a new operation that does not require to be in a repository is\n> added, or when an existing operation is updated not to require to be\n> in a repository, reverting the change and then checking in the\n> implementation of each operation if we are in a repository instead\n> should be easy enough---it pretty much should amount to Rafael's\n> patch, right?\n\nYes, I agree.\n\n> But then, being prepared for that future already is also OK, so I\n> can go either way. Please figure it out between you ;-)\n\nThe only thing I can think is that using RUN_SETUP protects all\nsubcommands immediately. It is equally possible that we add a\nnew subcommand that _also_ requires the GIT_DIR and we need to\nbe sure to implement this check at the beginning of that method,\ntoo!\n\nBut again, I'm just happy that this was found and is being fixed.\n\nRafael: please feel free to take or ignore my suggestion at your\ndiscretion. The existing review has also been illuminating.\n\nThanks,\n-Stolee\n"},{"id":"410866","messageId":"20201126070744.4vwesc5dpnnl7u5v@contrib-buster.localdomain","threadId":"54708","inReplyTo":"CAN0heSpjYEZbjXcX-XSvwwBKOEQ=Dtk=Nnr3T+BOqB+fd7x4sQ@mail.gmail.com","subject":"Re: [PATCH 1/1] maintenance: fix a SEGFAULT when no repository","fromName":"Rafael Silva","fromEmail":"rafaeloliveira.cs@gmail.com","sentAt":"2020-11-26T07:07:44Z","receivedAt":"2020-11-26T07:08:05Z","isPatch":true,"sender":{"key":"rafaeloliveira.cs@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5935135?v=4"},"body":"On Tue, Nov 24, 2020 at 08:03:33PM +0100, Martin Ågren wrote:\n> On Tue, 24 Nov 2020 at 17:47, Rafael Silva wrote:\n> > @@ -1265,9 +1265,14 @@ static int maintenance_run_tasks(struct maintenance_run_opts *opts)\n> > {\n> > int i, found_selected = 0;\n> > int result = 0;\n> > + char *lock_path;\n> > struct lock_file lk;\n> > struct repository *r = the_repository;\n> > - char *lock_path = xstrfmt(\"%s/maintenance\", r->objects->odb->path);\n> > +\n> > + if (!r || !r->gitdir)\n> > + return error(_(\"not a git repository\"));\n> > +\n> > + lock_path = xstrfmt(\"%s/maintenance\", the_repository->objects->odb->path);\n> \n> s/the_repository/r/\n> \n> (The preimage uses \"r\" and you check using \"r\".)\n\nThanks. will revise this in the next patch version.\n"},{"id":"410867","messageId":"20201126071308.5237t54bxwueummg@contrib-buster.localdomain","threadId":"54708","inReplyTo":"20201124191407.GC8396@szeder.dev","subject":"Re: [PATCH 1/1] maintenance: fix a SEGFAULT when no repository","fromName":"Rafael Silva","fromEmail":"rafaeloliveira.cs@gmail.com","sentAt":"2020-11-26T07:13:08Z","receivedAt":"2020-11-26T07:13:15Z","isPatch":true,"sender":{"key":"rafaeloliveira.cs@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5935135?v=4"},"body":"On Tue, Nov 24, 2020 at 08:14:07PM +0100, SZEDER Gábor wrote:\n> On Tue, Nov 24, 2020 at 12:24:57PM -0500, Eric Sunshine wrote:\n> > On Tue, Nov 24, 2020 at 11:45 AM Rafael Silva\n> > wrote:\n> > > diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh\n> > > @@ -441,4 +441,9 @@ test_expect_success 'register preserves existing strategy' '\n> > > +test_expect_success 'run and start command fails when no git repository' '\n> > > + test_must_fail git -C /tmp/ maintenance run &&\n> > > + test_must_fail git -C /tmp/ maintenance start\n> > > +'\n> > \n> > I wouldn't feel comfortable relying upon existence of /tmp/.\n> \n> Indeed.\n> \n> > It might\n> > be sufficient to do this instead:\n> > \n> > mv .git save.git &&\n> > test_when_finished \"mv save.git .git\" &&\n> > test_must_fail git maintenance run &&\n> > test_must_fail git maintenance start\n> \n> Our test library contains the 'nongit' helper function exactly for\n> this purpose:\n> \n> nongit test_must_fail git maintenance run &&\n> nongit test_must_fail git maintenance start\n> \n\nI did not know that we have such a test helper and will definitely\nchange on the next revision.\n\nThank you. \n"},{"id":"410873","messageId":"20201126082255.yyxx2kpskj3td5og@contrib-buster.localdomain","threadId":"54708","inReplyTo":"1bfd84da-5b74-be10-fc2c-dee80111ee2d@gmail.com","subject":"Re: [PATCH 1/1] maintenance: fix a SEGFAULT when no repository","fromName":"Rafael Silva","fromEmail":"rafaeloliveira.cs@gmail.com","sentAt":"2020-11-26T08:22:55Z","receivedAt":"2020-11-26T08:23:00Z","isPatch":true,"sender":{"key":"rafaeloliveira.cs@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5935135?v=4"},"body":"On Tue, Nov 24, 2020 at 12:22:40PM -0500, Derrick Stolee wrote:\n> On 11/24/2020 11:44 AM, Rafael Silva wrote:\n> > The \"git maintenance run\" and \"git maintenance start\" commands holds a\n> > file-based lock at the .git/maintenance.lock and .git/schedule.lock\n> > respectively. These locks are used to ensure only one maintenance process\n> > is executed at the time as both operations involves writing data into\n> > the git repository.\n> > \n> > The path to the lock file is built using the \"the_repository->objects->odb->path\"\n> > that results in SEGFAULT when we have no repository available as\n> > \"the_repository->objects->odb\" is set to NULL.\n> > \n> > Let's teach the maintenance_run_tasks() and update_background_schedule() to return\n> > an error and fails the command when we have no repository available.\n> \n> Thank you for noticing this problem, and for a quick fix.\n> \n> While I don't necessarily have a problem with this approach, perhaps\n> it would be more robust to change the options in git.c to require a\n> GIT_DIR, as in this diff?\n> \n> -- >8 --\n> \n> diff --git a/git.c b/git.c\n> index 1cab64b5d1..c3dabd2553 100644\n> --- a/git.c\n> +++ b/git.c\n> @@ -530,7 +530,7 @@ static struct cmd_struct commands[] = {\n> { \"ls-tree\", cmd_ls_tree, RUN_SETUP },\n> { \"mailinfo\", cmd_mailinfo, RUN_SETUP_GENTLY | NO_PARSEOPT },\n> { \"mailsplit\", cmd_mailsplit, NO_PARSEOPT },\n> - { \"maintenance\", cmd_maintenance, RUN_SETUP_GENTLY | NO_PARSEOPT },\n> + { \"maintenance\", cmd_maintenance, RUN_SETUP | NO_PARSEOPT },\n> { \"merge\", cmd_merge, RUN_SETUP | NEED_WORK_TREE },\n> { \"merge-base\", cmd_merge_base, RUN_SETUP },\n> { \"merge-file\", cmd_merge_file, RUN_SETUP_GENTLY },\n> \n> -- >8 --\n> \n\nThank you for the review and the attached patch!\n\n> If the above code change fixes your test (below), then that would\n> probably be a safer change.\n\nI agree, switching the maintenance command option to use RUN_SETUP\nseems like a nicer approach here. Given the all current operations\nrequires the command to be executed inside the a git repository this\nwill make the command consistent across the subcommand. \n\nAlso, it seems this provides an opportunity to cleanup the\nregister and unregister subcommands that currently implement the\ncheck to ensure the commands are running from a git repository.\n\n> \n> The reason to use RUN_SETUP_GENTLY was probably due to some thought\n> of modifying the background maintenance schedule without being in a\n> Git repository. However, we currently run the [un]register logic\n> inside of the stop|start subcommands, so a GIT_DIR is required there,\n> too.\n> \n\nIndeed. Aside from this reason, another concern that I have is that\nswitching the validation to all subcommands (on this case by switching\nthe maintenance command option) will change a bit the behaviour of register\nsubcommand. Currently, the behaviour of \"register\" subcommand is to return\nwith 0 without any messages when running outside of repository and switching\nwill make the command fail instead.\n\nNevertheless, I am inclined to go with your suggestion given that it seems\nbetter approach to support the automatically and make the behaviour\nconsistent for all subcommands given that changing the behaviour of\n\"git maintenance register\" command will (hopefully) be okay.\n\nThanks,\nRafael\n"},{"id":"410878","messageId":"54fa678c-7150-8c48-50e5-b33923a69249@gmail.com","threadId":"54708","inReplyTo":"20201126082255.yyxx2kpskj3td5og@contrib-buster.localdomain","subject":"Re: [PATCH 1/1] maintenance: fix a SEGFAULT when no repository","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2020-11-26T11:21:46Z","receivedAt":"2020-11-26T11:22:20Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 11/26/2020 3:22 AM, Rafael Silva wrote:\n> On Tue, Nov 24, 2020 at 12:22:40PM -0500, Derrick Stolee wrote:\n>> If the above code change fixes your test (below), then that would\n>> probably be a safer change.\n> \n> I agree, switching the maintenance command option to use RUN_SETUP\n> seems like a nicer approach here. Given the all current operations\n> requires the command to be executed inside the a git repository this\n> will make the command consistent across the subcommand. \n> \n> Also, it seems this provides an opportunity to cleanup the\n> register and unregister subcommands that currently implement the\n> check to ensure the commands are running from a git repository.\n> \n>>\n>> The reason to use RUN_SETUP_GENTLY was probably due to some thought\n>> of modifying the background maintenance schedule without being in a\n>> Git repository. However, we currently run the [un]register logic\n>> inside of the stop|start subcommands, so a GIT_DIR is required there,\n>> too.\n>>\n> \n> Indeed. Aside from this reason, another concern that I have is that\n> switching the validation to all subcommands (on this case by switching\n> the maintenance command option) will change a bit the behaviour of register\n> subcommand. Currently, the behaviour of \"register\" subcommand is to return\n> with 0 without any messages when running outside of repository and switching\n> will make the command fail instead.\n\nExcellent point. It would be good to cover this case with a test, to\ndemonstrate that as _intended_ behavior. It makes sense to fail instead\nof \"succeed\" when doing nothing.\n\n> Nevertheless, I am inclined to go with your suggestion given that it seems\n> better approach to support the automatically and make the behaviour\n> consistent for all subcommands given that changing the behaviour of\n> \"git maintenance register\" command will (hopefully) be okay.\n\nYes. I would say that changing that behavior aligns it with what it\nshould be doing. The best news is that the 'register' subcommand does\nnot exist in a released version of Git, so no one depends on the\ncurrent behavior.\n\nThanks,\n-Stolee\n\n"},{"id":"410902","messageId":"20201126204141.1438-1-rafaeloliveira.cs@gmail.com","threadId":"54708","inReplyTo":"20201124164405.29327-1-rafaeloliveira.cs@gmail.com","subject":"[PATCH v2 0/1] maintenance: Fix SEGFAULT when running outside of a repository","fromName":"Rafael Silva","fromEmail":"rafaeloliveira.cs@gmail.com","sentAt":"2020-11-26T20:41:40Z","receivedAt":"2020-11-26T20:44:58Z","isPatch":true,"sender":{"key":"rafaeloliveira.cs@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5935135?v=4"},"body":"In d7514f6ed5 (maintenance: take a lock on the objects directory, 2020-09-17) [1],\nand 2fec604f8d (maintenance: add start/stop subcommands, 2020-09-11) [2] The\n\"git maintenance run\" and \"git maintenance start\" was taught to hold a file-based\nlock at the .git/maintenance.lock and .git/schedule.lock respectively because these\noperations involves writing data into the .git/repository. \n\nThe lock file path string is built using the the_repository->objects->odb->path,\nin case the_repository->objects->odb is NULL when there is not repository available,\nresulting in a SEGFAULT.\n\nIn order to reproduce the error, one can execute maintenance \"run\" and/or\n\"start\" subcommand with a non valid repository: \n\n $ git -C /tmp maintenance start\n Segmentation fault\n\n $ git -C /tmp maintenance run\n Segmentation fault\n\nThe above test was executed from a git built from commit: faefdd61ec (Sixth batch, 2020-11-18):\n\nFor reference here's the output from GDB when debugging the \"start\" command\n\n\tProgram received signal SIGSEGV, Segmentation fault.\n\t0x00005555555b9b4c in maintenance_run_tasks (opts=0x7fffffffded4) at builtin/gc.c:1268\n\t1268\t\tchar *lock_path = xstrfmt(\"%s/maintenance\", r->objects->odb->path);\n\n\nUpdates in v2\n=============\n\n * Instead of implementing the check on the subcommands, a more robust approach\n is taken by replacing the \"maintenance\" command option to use RUN_SETUP\n instead of RUN_SETUP_GENTLY as suggested by Derrick Stolee in [3] as part of\n review cycle from v1.\n\n This provides protection for all maintenance subcommands given that\n currently all the commands are required to be executed inside a repository.\n\n * As the RUN_SETUP will enable protection to all commands, the checks in\n maintenance_register() and maintenance_unregister() are removed as they\n are not required anymore.\n\n * Use the \"nongit\" helper function for testing it instead of relying on the\n `/tmp` directory for a non valid git repository as suggested by\n SZEDER Gábor and Eric Sunshine in [4].\n\n * All \"git maintenance\" subcommands are included on the test to ensure the\n behaviour is consistent for all subcommands. In particular the \"register\"\n command that current exists without a message and code 0 which will be\n changed by this patch to fail when running outside of a repository. \n\n It also worth noting that \"register\" command does not exists in a released\n version of Git as mentioned in [5] which make it easier for changing the\n current behaviour of the command\n\n[1] https://lore.kernel.org/git/1a0a3eebb825ac3eabfdd86f82ed7ef6abb454c5.1600366313.git.gitgitgadget@gmail.com/\n[2] https://lore.kernel.org/git/5194f6b1facbd14cc17eea0337c0cc397a2a51fc.1602782524.git.gitgitgadget@gmail.com/\n[3] https://lore.kernel.org/git/1bfd84da-5b74-be10-fc2c-dee80111ee2d@gmail.com/\n[4] https://lore.kernel.org/git/20201124191407.GC8396@szeder.dev/\n[5] https://lore.kernel.org/git/54fa678c-7150-8c48-50e5-b33923a69249@gmail.com/\n\nThanks everyone for the insightful and helpful feedback.\n\nRafael Silva (1):\n maintenance: fix SEGFAULT when no repository\n\n builtin/gc.c | 7 -------\n git.c | 2 +-\n t/t7900-maintenance.sh | 8 ++++++++\n 3 files changed, 9 insertions(+), 8 deletions(-)\n\n-- \n2.29.2.367.g37477fb670\n\n"},{"id":"410903","messageId":"20201126204141.1438-2-rafaeloliveira.cs@gmail.com","threadId":"54708","inReplyTo":"20201126204141.1438-1-rafaeloliveira.cs@gmail.com","subject":"[PATCH v2 1/1] maintenance: fix SEGFAULT when no repository","fromName":"Rafael Silva","fromEmail":"rafaeloliveira.cs@gmail.com","sentAt":"2020-11-26T20:41:41Z","receivedAt":"2020-11-26T20:45:22Z","isPatch":true,"sender":{"key":"rafaeloliveira.cs@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5935135?v=4"},"body":"The \"git maintenance run\" and \"git maintenance start/stop\" commands\nholds a file-based lock at the .git/maintenance.lock and\n.git/schedule.lock respectively. These locks are used to ensure only\none maintenance process is executed at the time as both operations\ninvolves writing data into the git repository.\n\nThe path to the lock file is built using\n\"the_repository->objects->odb->path\" that results in SEGFAULT when we\nhave no repository available as \"the_repository->objects->odb\" is\nset to NULL.\n\nLet's teach maintenance command to use RUN_SETUP option that will\nprovide the validation and fail when running outside of a repository.\nHence fixing the SEGFAULT for all three operations and making the\nbehaviour consistent across all subcommands.\n\nSetting the RUN_SETUP also provides the same protection for all\nsubcommands given that the \"register\" and \"unregister\" also requires to\nbe executed inside a repository.\n\nFurthermore let's remove the local validation implemented by the\n\"register\" and \"unregister\" as this will not be required anymore with\nthe new option.\n\nSigned-off-by: Rafael Silva \n---\n builtin/gc.c | 7 -------\n git.c | 2 +-\n t/t7900-maintenance.sh | 8 ++++++++\n 3 files changed, 9 insertions(+), 8 deletions(-)\n\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex bc25ad52c7..ebb0158308 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -1446,10 +1446,6 @@ static int maintenance_register(void)\n \tstruct child_process config_set = CHILD_PROCESS_INIT;\n \tstruct child_process config_get = CHILD_PROCESS_INIT;\n \n-\t/* There is no current repository, so skip registering it */\n-\tif (!the_repository || !the_repository->gitdir)\n-\t\treturn 0;\n-\n \t/* Disable foreground maintenance */\n \tgit_config_set(\"maintenance.auto\", \"false\");\n \n@@ -1486,9 +1482,6 @@ static int maintenance_unregister(void)\n {\n \tstruct child_process config_unset = CHILD_PROCESS_INIT;\n \n-\tif (!the_repository || !the_repository->gitdir)\n-\t\treturn error(_(\"no current repository to unregister\"));\n-\n \tconfig_unset.git_cmd = 1;\n \tstrvec_pushl(&config_unset.args, \"config\", \"--global\", \"--unset\",\n \t\t \"maintenance.repo\",\ndiff --git a/git.c b/git.c\nindex 4b7bd77b80..a00a0a4d94 100644\n--- a/git.c\n+++ b/git.c\n@@ -535,7 +535,7 @@ static struct cmd_struct commands[] = {\n \t{ \"ls-tree\", cmd_ls_tree, RUN_SETUP },\n \t{ \"mailinfo\", cmd_mailinfo, RUN_SETUP_GENTLY | NO_PARSEOPT },\n \t{ \"mailsplit\", cmd_mailsplit, NO_PARSEOPT },\n-\t{ \"maintenance\", cmd_maintenance, RUN_SETUP_GENTLY | NO_PARSEOPT },\n+\t{ \"maintenance\", cmd_maintenance, RUN_SETUP | NO_PARSEOPT },\n \t{ \"merge\", cmd_merge, RUN_SETUP | NEED_WORK_TREE },\n \t{ \"merge-base\", cmd_merge_base, RUN_SETUP },\n \t{ \"merge-file\", cmd_merge_file, RUN_SETUP_GENTLY },\ndiff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh\nindex d9e68bb2bf..ae5c29b0ff 100755\n--- a/t/t7900-maintenance.sh\n+++ b/t/t7900-maintenance.sh\n@@ -441,4 +441,12 @@ test_expect_success 'register preserves existing strategy' '\n \ttest_config maintenance.strategy incremental\n '\n \n+test_execpt_success 'fails when running outside of a repository' '\n+\tnongit test_must_fail git maintenance run &&\n+\tnongit test_must_fail git maintenance stop &&\n+\tnongit test_must_fail git maintenance start &&\n+\tnongit test_must_fail git maintenance register &&\n+\tnongit test_must_fail git maintenance unregister\n+'\n+\n test_done\n-- \n2.29.2.367.g37477fb670\n\n"},{"id":"410934","messageId":"712ca8f7-c936-e03c-cbb9-4a0432fc3131@gmail.com","threadId":"54708","inReplyTo":"20201126204141.1438-2-rafaeloliveira.cs@gmail.com","subject":"Re: [PATCH v2 1/1] maintenance: fix SEGFAULT when no repository","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2020-11-27T20:43:12Z","receivedAt":"2020-11-27T20:45:54Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 11/26/2020 3:41 PM, Rafael Silva wrote:\n> The \"git maintenance run\" and \"git maintenance start/stop\" commands\n> holds a file-based lock at the .git/maintenance.lock and\n> .git/schedule.lock respectively. These locks are used to ensure only\n> one maintenance process is executed at the time as both operations\n> involves writing data into the git repository.\n> \n> The path to the lock file is built using\n> \"the_repository->objects->odb->path\" that results in SEGFAULT when we\n> have no repository available as \"the_repository->objects->odb\" is\n> set to NULL.\n> \n> Let's teach maintenance command to use RUN_SETUP option that will\n> provide the validation and fail when running outside of a repository.\n> Hence fixing the SEGFAULT for all three operations and making the\n> behaviour consistent across all subcommands.\n> \n> Setting the RUN_SETUP also provides the same protection for all\n> subcommands given that the \"register\" and \"unregister\" also requires to\n> be executed inside a repository.\n> \n> Furthermore let's remove the local validation implemented by the\n> \"register\" and \"unregister\" as this will not be required anymore with\n> the new option.\n\nThank you for this very clean patch!\n\nReviewed-by: Derrick Stolee \n\n"},{"id":"411732","messageId":"20201208201256.GK36751@google.com","threadId":"54708","inReplyTo":"20201126204141.1438-2-rafaeloliveira.cs@gmail.com","subject":"Re: [PATCH v2 1/1] maintenance: fix SEGFAULT when no repository","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2020-12-08T20:12:56Z","receivedAt":"2020-12-08T20:23:56Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"On 2020.11.26 20:41, Rafael Silva wrote:\n> diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh\n> index d9e68bb2bf..ae5c29b0ff 100755\n> --- a/t/t7900-maintenance.sh\n> +++ b/t/t7900-maintenance.sh\n> @@ -441,4 +441,12 @@ test_expect_success 'register preserves existing strategy' '\n> \ttest_config maintenance.strategy incremental\n> '\n> \n> +test_execpt_success 'fails when running outside of a repository' '\n> +\tnongit test_must_fail git maintenance run &&\n> +\tnongit test_must_fail git maintenance stop &&\n> +\tnongit test_must_fail git maintenance start &&\n> +\tnongit test_must_fail git maintenance register &&\n> +\tnongit test_must_fail git maintenance unregister\n> +'\n> +\n> test_done\n\nCaught a typo here, sending this as a squash patch since it's already in\nnext:\n\n-- >8 --\nSubject: [PATCH] t7900: fix typo: \"test_execpt_success\"\n\nSigned-off-by: Josh Steadmon \n---\n t/t7900-maintenance.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh\nindex 4ca3617173..8c061197a6 100755\n--- a/t/t7900-maintenance.sh\n+++ b/t/t7900-maintenance.sh\n@@ -441,7 +441,7 @@ test_expect_success 'register preserves existing strategy' '\n \ttest_config maintenance.strategy incremental\n '\n \n-test_execpt_success 'fails when running outside of a repository' '\n+test_expect_success 'fails when running outside of a repository' '\n \tnongit test_must_fail git maintenance run &&\n \tnongit test_must_fail git maintenance stop &&\n \tnongit test_must_fail git maintenance start &&\n-- \n2.29.2.576.ga3fc446d84-goog\n\n"},{"id":"411753","messageId":"xmqqsg8g559i.fsf@gitster.c.googlers.com","threadId":"54708","inReplyTo":"20201208201256.GK36751@google.com","subject":"Re: [PATCH v2 1/1] maintenance: fix SEGFAULT when no repository","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-12-08T21:58:49Z","receivedAt":"2020-12-08T22:00:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Josh Steadmon writes:\n\n> Caught a typo here, sending this as a squash patch since it's already in\n> next:\n\nThe breakage and the fix looks obvious to me, but...\n\nHow did CI allow 'next' to pass with such a typo, I wonder?\nMoreover, my pre-push tests of all the integration branches\nI didn't notice this to fail, but I cannot see how it could\nhave been succeeded. Puzzled...\n\nWill queue, thanks.\n\n>\n> -- >8 --\n> Subject: [PATCH] t7900: fix typo: \"test_execpt_success\"\n>\n> Signed-off-by: Josh Steadmon \n> ---\n> t/t7900-maintenance.sh | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh\n> index 4ca3617173..8c061197a6 100755\n> --- a/t/t7900-maintenance.sh\n> +++ b/t/t7900-maintenance.sh\n> @@ -441,7 +441,7 @@ test_expect_success 'register preserves existing strategy' '\n> \ttest_config maintenance.strategy incremental\n> '\n> \n> -test_execpt_success 'fails when running outside of a repository' '\n> +test_expect_success 'fails when running outside of a repository' '\n> \tnongit test_must_fail git maintenance run &&\n> \tnongit test_must_fail git maintenance stop &&\n> \tnongit test_must_fail git maintenance start &&\n"},{"id":"411781","messageId":"xmqqh7ow54eb.fsf@gitster.c.googlers.com","threadId":"54708","inReplyTo":"xmqqsg8g559i.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v2 1/1] maintenance: fix SEGFAULT when no repository","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-12-08T22:17:32Z","receivedAt":"2020-12-08T22:18:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Josh Steadmon writes:\n>\n>> Caught a typo here, sending this as a squash patch since it's already in\n>> next:\n>\n> The breakage and the fix looks obvious to me, but...\n>\n> How did CI allow 'next' to pass with such a typo, I wonder?\n> Moreover, my pre-push tests of all the integration branches\n> I didn't notice this to fail, but I cannot see how it could\n> have been succeeded. Puzzled...\n\nThat is because of this:\n\n $ (sh t7900-maintenance.sh 2>&1; echo $?) | tail -5\n ok 25 - register preserves existing strategy\n t7900-maintenance.sh: line 444: test_execpt_success: command not found\n # passed all 25 test(s)\n 1..25\n 0\n\nThe story is the same with prove.\n\n $ prove t7900-maintenance-sh\n t7900-maintenance.sh .. 24/? t7900-maintenance.sh: line 444: test_execpt_success: command not found\n t7900-maintenance.sh .. ok\n All tests successful.\n Files=1, Tests=25, 2 wallclock secs ( 0.02 usr 0.01 sys + 0.97 cusr 0.97 csys = 1.97 CPU)\n Result: PASS\n\nSince this typo appeared immediately before test_done, we _could_\nimprove test_done to pay attention to $? when it starts (and in a\nsimilar fashion, we _could_ also check $? at the beginning of the\ntest_expect_* for the previous step), but I do not think that is a\ngood approach that would scale well. There are legitimate reasons\nwe have to write things other than test_expect_* at the top level\nof the script (e.g. test helper function may have to be defined to\nbe shared amongst the test pieces in the same script).\n\nI wonder if it is a good direction to go to run the tests with the\n\"set -e\" option on, and accept its peculiarities.\n"},{"id":"411831","messageId":"gohp6kmtynuzdy.fsf@gmail.com","threadId":"54708","inReplyTo":"20201208201256.GK36751@google.com","subject":"Re: [PATCH v2 1/1] maintenance: fix SEGFAULT when no repository","fromName":"Rafael Silva","fromEmail":"rafaeloliveira.cs@gmail.com","sentAt":"2020-12-09T09:29:06Z","receivedAt":"2020-12-09T09:30:29Z","isPatch":true,"sender":{"key":"rafaeloliveira.cs@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5935135?v=4"},"body":"\nJosh Steadmon writes:\n\n> On 2020.11.26 20:41, Rafael Silva wrote:\n>> diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh\n>> index d9e68bb2bf..ae5c29b0ff 100755\n>> --- a/t/t7900-maintenance.sh\n>> +++ b/t/t7900-maintenance.sh\n>> @@ -441,4 +441,12 @@ test_expect_success 'register preserves existing strategy' '\n>> \ttest_config maintenance.strategy incremental\n>> '\n>> \n>> +test_execpt_success 'fails when running outside of a repository' '\n>> +\tnongit test_must_fail git maintenance run &&\n>> +\tnongit test_must_fail git maintenance stop &&\n>> +\tnongit test_must_fail git maintenance start &&\n>> +\tnongit test_must_fail git maintenance register &&\n>> +\tnongit test_must_fail git maintenance unregister\n>> +'\n>> +\n>> test_done\n>\n> Caught a typo here, sending this as a squash patch since it's already in\n> next:\n>\n\nUfff. For some reason I completely missed the test error message when\nworking on the v2.\n\nThank you Josh, for the catch and quick patch.\n\nApologize guys for such mistake.\n\n-- \nThanks, Rafael\n"},{"id":"412990","messageId":"CAP8UFD2mrgympQ0tbhty+cgZ7ow_+bsxE8gm1Wsn_mo+a6sq2Q@mail.gmail.com","threadId":"54708","inReplyTo":"xmqqh7ow54eb.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v2 1/1] maintenance: fix SEGFAULT when no repository","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2020-12-24T08:12:21Z","receivedAt":"2020-12-24T08:14:54Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Wed, Dec 9, 2020 at 2:16 AM Junio C Hamano wrote:\n>\n> Junio C Hamano writes:\n>\n> > Josh Steadmon writes:\n> >\n> >> Caught a typo here, sending this as a squash patch since it's already in\n> >> next:\n> >\n> > The breakage and the fix looks obvious to me, but...\n> >\n> > How did CI allow 'next' to pass with such a typo, I wonder?\n> > Moreover, my pre-push tests of all the integration branches\n> > I didn't notice this to fail, but I cannot see how it could\n> > have been succeeded. Puzzled...\n>\n> That is because of this:\n>\n> $ (sh t7900-maintenance.sh 2>&1; echo $?) | tail -5\n> ok 25 - register preserves existing strategy\n> t7900-maintenance.sh: line 444: test_execpt_success: command not found\n> # passed all 25 test(s)\n> 1..25\n> 0\n>\n> The story is the same with prove.\n>\n> $ prove t7900-maintenance-sh\n> t7900-maintenance.sh .. 24/? t7900-maintenance.sh: line 444: test_execpt_success: command not found\n> t7900-maintenance.sh .. ok\n> All tests successful.\n> Files=1, Tests=25, 2 wallclock secs ( 0.02 usr 0.01 sys + 0.97 cusr 0.97 csys = 1.97 CPU)\n> Result: PASS\n>\n> Since this typo appeared immediately before test_done, we _could_\n> improve test_done to pay attention to $? when it starts (and in a\n> similar fashion, we _could_ also check $? at the beginning of the\n> test_expect_* for the previous step), but I do not think that is a\n> good approach that would scale well. There are legitimate reasons\n> we have to write things other than test_expect_* at the top level\n> of the script (e.g. test helper function may have to be defined to\n> be shared amongst the test pieces in the same script).\n>\n> I wonder if it is a good direction to go to run the tests with the\n> \"set -e\" option on, and accept its peculiarities.\n\nAnother solution could be to define a command_not_found_handle\nfunction as bourne shells should call that.\n\nBy the way it's not the first time we get such an issue, see:\n\nhttps://lore.kernel.org/git/CAP8UFD15+p+xKwJ=B9WVsrc+2TvLHKmu78SBCLUFZVSYoTtbbg@mail.gmail.com/\n"},{"id":"412996","messageId":"xmqqczyz2sw0.fsf@gitster.c.googlers.com","threadId":"54708","inReplyTo":"CAP8UFD2mrgympQ0tbhty+cgZ7ow_+bsxE8gm1Wsn_mo+a6sq2Q@mail.gmail.com","subject":"Re: [PATCH v2 1/1] maintenance: fix SEGFAULT when no repository","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-12-24T14:14:23Z","receivedAt":"2020-12-24T14:17:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder writes:\n\n>> I wonder if it is a good direction to go to run the tests with the\n>> \"set -e\" option on, and accept its peculiarities.\n>\n> Another solution could be to define a command_not_found_handle\n> function as bourne shells should call that.\n\nI am reasonably sure it is bash-ism and a rather stale stackexchange\nquestion seems to say that command_not_found_handler function (note\nthe 'r' at the end) is its equivalent in zsh.\n\nHaving said that, we already have other bash-specific debugging\nsupport in our test harness, and it may not be a bad idea to use the\nfacility to catch these bugs, even if the support is available only\nwhen running the tests under bash and no other shell.\n\n> By the way it's not the first time we get such an issue, see:\n>\n> https://lore.kernel.org/git/CAP8UFD15+p+xKwJ=B9WVsrc+2TvLHKmu78SBCLUFZVSYoTtbbg@mail.gmail.com/\n\nExcellent memory. Thanks.\n"}]}