{"thread":{"id":"60077","subject":"[PATCH 0/6] maintenance: schedule maintenance on a random minute","startedAt":"2023-08-07T18:51:47Z","lastAt":"2023-08-14T11:29:51Z","messageCount":47,"participants":["Derrick Stolee via GitGitGadget","Taylor Blau","Junio C Hamano","Derrick Stolee","Phillip Wood"],"isPatch":true,"patchVersion":1,"patchTotal":6},"messages":[{"id":"480242","messageId":"pull.1567.git.1691434300.gitgitgadget@gmail.com","threadId":"60077","inReplyTo":null,"subject":"[PATCH 0/6] maintenance: schedule maintenance on a random minute","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-08-07T18:51:34Z","receivedAt":"2023-08-07T18:51:47Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"When we initially created background maintenance -- with its hourly, daily,\nand weekly schedules -- we considered the effects of all clients launching\nfetches to the server every hour on the hour. The worry of DDoSing server\nhosts was noted, but left as something we would consider for a future\nupdate.\n\nAs background maintenance has gained more adoption over the past three\nyears, our worries about DDoSing the big Git hosts has been unfounded. Those\nsystems, especially those serving public repositories, are already resilient\nto thundering herds of much smaller scale.\n\nHowever, sometimes organizations spin up specific custom server\ninfrastructure either in addition to or on top of their Git host. Some of\nthese technologies are built for a different range of scale, and can hit\nconcurrency limits sooner. Organizations with such custom infrastructures\nare more likely to recommend tools like scalar which furthers their adoption\nof background maintenance.\n\nThis series attempts to help by spreading out background maintenance to a\nrandom minute of the hour. This minute is selected during git maintenance\nstart, and the same minute applies to each of the three schedules.\n\nThis isn't a full solution to this problem, as the custom infrastructure\nneeds to be resilient to bursts of activity, but at least this will help\nsomewhat.\n\nEach of the integrated schedulers needs a different way of integrating the\nrandom minute. The most problematic is systemd, since our integration had a\nclever use of templates to write one schedule that inserted the hourly,\ndaily, and weekly schedules as a string into the template. This needs some\nrefactoring before the custom minute could be inserted.\n\nFor the most part, each scheduler's integration is relatively simple. That\nis, until we get to the systemd integration. That integration made use of a\nclever templating technique that is no longer possible when making this\nadjustment. The last two patches involve systemd, though one is just a move\nof code (without edits) to make the second's diff look a lot simpler.\n\nThanks, -Stolee\n\nDerrick Stolee (6):\n maintenance: add get_random_minute()\n maintenance: use random minute in launchctl scheduler\n maintenance: use random minute in Windows scheduler\n maintenance: use random minute in cron scheduler\n maintenance: swap method locations\n maintenance: use random minute in systemd scheduler\n\n builtin/gc.c | 259 ++++++++++++++++++++++++++---------------\n t/t7900-maintenance.sh | 4 +-\n 2 files changed, 166 insertions(+), 97 deletions(-)\n\n\nbase-commit: fe86abd7511a9a6862d5706c6fa1d9b57a63ba09\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1567%2Fderrickstolee%2Fmaintenance-random-minute-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1567/derrickstolee/maintenance-random-minute-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1567\n-- \ngitgitgadget\n"},{"id":"480243","messageId":"babf62ec6d5181c65dd4b653ba9ae24e62a88830.1691434300.git.gitgitgadget@gmail.com","threadId":"60077","inReplyTo":"pull.1567.git.1691434300.gitgitgadget@gmail.com","subject":"[PATCH 2/6] maintenance: use random minute in launchctl scheduler","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-08-07T18:51:36Z","receivedAt":"2023-08-07T18:51:48Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nThe get_random_minute() method was created to allow maintenance\nschedules to be fixed to a random minute of the hour. This randomness is\nonly intended to spread out the load from a number of clients, but each\nclient should have an hour between each maintenance cycle.\n\nUse get_random_minute() when constructing the schedules for launchctl.\n\nThe format already includes a 'Minute' key which is modified from 0 to\nthe random minute.\n\nSigned-off-by: Derrick Stolee \n---\n builtin/gc.c | 23 ++++++++++++-----------\n 1 file changed, 12 insertions(+), 11 deletions(-)\n\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex 66a972bc292..51d6c7620ff 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -1708,7 +1708,6 @@ static int get_schedule_cmd(const char **cmd, int *is_available)\n \treturn 1;\n }\n \n-MAYBE_UNUSED\n static int get_random_minute(void)\n {\n \tstatic int random_initialized = 0;\n@@ -1837,6 +1836,7 @@ static int launchctl_schedule_plist(const char *exec_path, enum schedule_priorit\n \tstruct strbuf plist = STRBUF_INIT, plist2 = STRBUF_INIT;\n \tstruct stat st;\n \tconst char *cmd = \"launchctl\";\n+\tint minute = get_random_minute();\n \n \tget_schedule_cmd(&cmd, NULL);\n \tpreamble = \"\\n\"\n@@ -1862,29 +1862,30 @@ static int launchctl_schedule_plist(const char *exec_path, enum schedule_priorit\n \tcase SCHEDULE_HOURLY:\n \t\trepeat = \"\\n\"\n \t\t\t \"Hour%d\\n\"\n-\t\t\t \"Minute0\\n\"\n+\t\t\t \"Minute%d\\n\"\n \t\t\t \"\\n\";\n \t\tfor (i = 1; i <= 23; i++)\n-\t\t\tstrbuf_addf(&plist, repeat, i);\n+\t\t\tstrbuf_addf(&plist, repeat, i, minute);\n \t\tbreak;\n \n \tcase SCHEDULE_DAILY:\n \t\trepeat = \"\\n\"\n \t\t\t \"Day%d\\n\"\n \t\t\t \"Hour0\\n\"\n-\t\t\t \"Minute0\\n\"\n+\t\t\t \"Minute%d\\n\"\n \t\t\t \"\\n\";\n \t\tfor (i = 1; i <= 6; i++)\n-\t\t\tstrbuf_addf(&plist, repeat, i);\n+\t\t\tstrbuf_addf(&plist, repeat, i, minute);\n \t\tbreak;\n \n \tcase SCHEDULE_WEEKLY:\n-\t\tstrbuf_addstr(&plist,\n-\t\t\t \"\\n\"\n-\t\t\t \"Day0\\n\"\n-\t\t\t \"Hour0\\n\"\n-\t\t\t \"Minute0\\n\"\n-\t\t\t \"\\n\");\n+\t\tstrbuf_addf(&plist,\n+\t\t\t \"\\n\"\n+\t\t\t \"Day0\\n\"\n+\t\t\t \"Hour0\\n\"\n+\t\t\t \"Minute%d\\n\"\n+\t\t\t \"\\n\",\n+\t\t\t minute);\n \t\tbreak;\n \n \tdefault:\n-- \ngitgitgadget\n\n"},{"id":"480244","messageId":"fefdaa9457948ee5302e7cbfaae250e0b589d752.1691434300.git.gitgitgadget@gmail.com","threadId":"60077","inReplyTo":"pull.1567.git.1691434300.gitgitgadget@gmail.com","subject":"[PATCH 1/6] maintenance: add get_random_minute()","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-08-07T18:51:35Z","receivedAt":"2023-08-07T18:51:50Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nWhen we initially created background maintenance -- with its hourly,\ndaily, and weekly schedules -- we considered the effects of all clients\nlaunching fetches to the server every hour on the hour. The worry of\nDDoSing server hosts was noted, but left as something we would consider\nfor a future update.\n\nAs background maintenance has gained more adoption over the past three\nyears, our worries about DDoSing the big Git hosts has been unfounded.\nThose systems, especially those serving public repositories, are already\nresilient to thundering herds of much smaller scale.\n\nHowever, sometimes organizations spin up specific custom server\ninfrastructure either in addition to or on top of their Git host. Some\nof these technologies are built for a different range of scale, and can\nhit concurrency limits sooner. Organizations with such custom\ninfrastructures are more likely to recommend tools like `scalar` which\nfurthers their adoption of background maintenance.\n\nTo help solve for this, create get_random_minute() as a method to help\nGit select a random minute when creating schedules in the future. The\nintegrations with this method do not yet exist, but will follow in\nfuture changes.\n\nOne thing that is important for testability is that we notice when we\nare under a test scenario and return a predictable result. The schedules\nthemselves are not checked for this value, but at least one launchctl\ntest checks that we do not unnecessarily reboot the schedule if it has\nnot changed from a previous version.\n\nSigned-off-by: Derrick Stolee \n---\n builtin/gc.c | 17 +++++++++++++++++\n 1 file changed, 17 insertions(+)\n\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex f3942188a61..66a972bc292 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -1708,6 +1708,23 @@ static int get_schedule_cmd(const char **cmd, int *is_available)\n \treturn 1;\n }\n \n+MAYBE_UNUSED\n+static int get_random_minute(void)\n+{\n+\tstatic int random_initialized = 0;\n+\n+\t/* Use a static value when under tests. */\n+\tif (!getenv(\"GIT_TEST_MAINTENANCE_SCHEDULER\"))\n+\t\treturn 13;\n+\n+\tif (!random_initialized) {\n+\t\tsrand((unsigned int)getpid());\n+\t\trandom_initialized = 1;\n+\t}\n+\n+\treturn rand() % 60;\n+}\n+\n static int is_launchctl_available(void)\n {\n \tconst char *cmd = \"launchctl\";\n-- \ngitgitgadget\n\n"},{"id":"480245","messageId":"1048dff1d3a241effe7235716b1f5d9c46f7478b.1691434300.git.gitgitgadget@gmail.com","threadId":"60077","inReplyTo":"pull.1567.git.1691434300.gitgitgadget@gmail.com","subject":"[PATCH 3/6] maintenance: use random minute in Windows scheduler","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-08-07T18:51:37Z","receivedAt":"2023-08-07T18:51:54Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nThe get_random_minute() method was created to allow maintenance\nschedules to be fixed to a random minute of the hour. This randomness is\nonly intended to spread out the load from a number of clients, but each\nclient should have an hour between each maintenance cycle.\n\nAdd this random minute to the Windows scheduler integration.\n\nWe need only to modify the minute value for the 'StartBoundary' tag\nacross the three schedules.\n\nSigned-off-by: Derrick Stolee \n---\n builtin/gc.c | 16 ++++++++++------\n 1 file changed, 10 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex 51d6c7620ff..f059c9f89d5 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -2002,6 +2002,7 @@ static int schtasks_schedule_task(const char *exec_path, enum schedule_priority\n \tconst char *frequency = get_frequency(schedule);\n \tchar *name = schtasks_task_name(frequency);\n \tstruct strbuf tfilename = STRBUF_INIT;\n+\tint minute = get_random_minute();\n \n \tget_schedule_cmd(&cmd, NULL);\n \n@@ -2022,7 +2023,7 @@ static int schtasks_schedule_task(const char *exec_path, enum schedule_priority\n \tswitch (schedule) {\n \tcase SCHEDULE_HOURLY:\n \t\tfprintf(tfile->fp,\n-\t\t\t\"2020-01-01T01:00:00\\n\"\n+\t\t\t\"2020-01-01T01:%02d:00\\n\"\n \t\t\t\"true\\n\"\n \t\t\t\"\\n\"\n \t\t\t\"1\\n\"\n@@ -2031,12 +2032,13 @@ static int schtasks_schedule_task(const char *exec_path, enum schedule_priority\n \t\t\t\"PT1H\\n\"\n \t\t\t\"PT23H\\n\"\n \t\t\t\"false\\n\"\n-\t\t\t\"\\n\");\n+\t\t\t\"\\n\",\n+\t\t\tminute);\n \t\tbreak;\n \n \tcase SCHEDULE_DAILY:\n \t\tfprintf(tfile->fp,\n-\t\t\t\"2020-01-01T00:00:00\\n\"\n+\t\t\t\"2020-01-01T00:%02d:00\\n\"\n \t\t\t\"true\\n\"\n \t\t\t\"\\n\"\n \t\t\t\"\\n\"\n@@ -2048,19 +2050,21 @@ static int schtasks_schedule_task(const char *exec_path, enum schedule_priority\n \t\t\t\"\\n\"\n \t\t\t\"\\n\"\n \t\t\t\"1\\n\"\n-\t\t\t\"\\n\");\n+\t\t\t\"\\n\",\n+\t\t\tminute);\n \t\tbreak;\n \n \tcase SCHEDULE_WEEKLY:\n \t\tfprintf(tfile->fp,\n-\t\t\t\"2020-01-01T00:00:00\\n\"\n+\t\t\t\"2020-01-01T00:%02d:00\\n\"\n \t\t\t\"true\\n\"\n \t\t\t\"\\n\"\n \t\t\t\"\\n\"\n \t\t\t\"\\n\"\n \t\t\t\"\\n\"\n \t\t\t\"1\\n\"\n-\t\t\t\"\\n\");\n+\t\t\t\"\\n\",\n+\t\t\tminute);\n \t\tbreak;\n \n \tdefault:\n-- \ngitgitgadget\n\n"},{"id":"480246","messageId":"3ef3cf0861ffcb86dc8dbb075a17265d4c23d3fc.1691434300.git.gitgitgadget@gmail.com","threadId":"60077","inReplyTo":"pull.1567.git.1691434300.gitgitgadget@gmail.com","subject":"[PATCH 4/6] maintenance: use random minute in cron scheduler","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-08-07T18:51:38Z","receivedAt":"2023-08-07T18:51:56Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nThe get_random_minute() method was created to allow maintenance\nschedules to be fixed to a random minute of the hour. This randomness is\nonly intended to spread out the load from a number of clients, but each\nclient should have an hour between each maintenance cycle.\n\nAdd this random minute to the cron integration.\n\nThe cron schedule specification starts with a minute indicator, which\nwas previously inserted as the \"0\" string but now takes the given minute\nas an integer parameter.\n\nSigned-off-by: Derrick Stolee \n---\n builtin/gc.c | 9 +++++----\n 1 file changed, 5 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex f059c9f89d5..be47000ed36 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -2181,6 +2181,7 @@ static int crontab_update_schedule(int run_maintenance, int fd)\n \tFILE *cron_list, *cron_in;\n \tstruct strbuf line = STRBUF_INIT;\n \tstruct tempfile *tmpedit = NULL;\n+\tint minute = get_random_minute();\n \n \tget_schedule_cmd(&cmd, NULL);\n \tstrvec_split(&crontab_list.args, cmd);\n@@ -2235,11 +2236,11 @@ static int crontab_update_schedule(int run_maintenance, int fd)\n \t\t\t\"# replaced in the future by a Git command.\\n\\n\");\n \n \t\tstrbuf_addf(&line_format,\n-\t\t\t \"%%s %%s * * %%s \\\"%s/git\\\" --exec-path=\\\"%s\\\" for-each-repo --config=maintenance.repo maintenance run --schedule=%%s\\n\",\n+\t\t\t \"%%d %%s * * %%s \\\"%s/git\\\" --exec-path=\\\"%s\\\" for-each-repo --config=maintenance.repo maintenance run --schedule=%%s\\n\",\n \t\t\t exec_path, exec_path);\n-\t\tfprintf(cron_in, line_format.buf, \"0\", \"1-23\", \"*\", \"hourly\");\n-\t\tfprintf(cron_in, line_format.buf, \"0\", \"0\", \"1-6\", \"daily\");\n-\t\tfprintf(cron_in, line_format.buf, \"0\", \"0\", \"0\", \"weekly\");\n+\t\tfprintf(cron_in, line_format.buf, minute, \"1-23\", \"*\", \"hourly\");\n+\t\tfprintf(cron_in, line_format.buf, minute, \"0\", \"1-6\", \"daily\");\n+\t\tfprintf(cron_in, line_format.buf, minute, \"0\", \"0\", \"weekly\");\n \t\tstrbuf_release(&line_format);\n \n \t\tfprintf(cron_in, \"\\n%s\\n\", END_LINE);\n-- \ngitgitgadget\n\n"},{"id":"480247","messageId":"37f96b6b619d2d3eecd9c2b35483b2d61083603b.1691434300.git.gitgitgadget@gmail.com","threadId":"60077","inReplyTo":"pull.1567.git.1691434300.gitgitgadget@gmail.com","subject":"[PATCH 5/6] maintenance: swap method locations","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-08-07T18:51:39Z","receivedAt":"2023-08-07T18:52:01Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nThe systemd_timer_write_unit_templates() method writes a single template\nthat is then used to start the hourly, daily, and weekly schedules with\nsystemd.\n\nHowever, in order to schedule systemd maintenance on a given minute,\nthese templates need to be replaced with specific schedules for each of\nthese jobs.\n\nBefore modifying the schedules, move the writing method above the\nsystemd_timer_enable_unit() method, so we can write a specific schedule\nfor each unit.\n\nThe diff is computed smaller by showing systemd_timer_enable_unit() move\ninstead of systemd_timer_write_unit_templates().\n\nSigned-off-by: Derrick Stolee \n---\n builtin/gc.c | 128 +++++++++++++++++++++++++--------------------------\n 1 file changed, 64 insertions(+), 64 deletions(-)\n\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex be47000ed36..b3ef95b10aa 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -2299,70 +2299,6 @@ static char *xdg_config_home_systemd(const char *filename)\n \treturn xdg_config_home_for(\"systemd/user\", filename);\n }\n \n-static int systemd_timer_enable_unit(int enable,\n-\t\t\t\t enum schedule_priority schedule)\n-{\n-\tconst char *cmd = \"systemctl\";\n-\tstruct child_process child = CHILD_PROCESS_INIT;\n-\tconst char *frequency = get_frequency(schedule);\n-\n-\t/*\n-\t * Disabling the systemd unit while it is already disabled makes\n-\t * systemctl print an error.\n-\t * Let's ignore it since it means we already are in the expected state:\n-\t * the unit is disabled.\n-\t *\n-\t * On the other hand, enabling a systemd unit which is already enabled\n-\t * produces no error.\n-\t */\n-\tif (!enable)\n-\t\tchild.no_stderr = 1;\n-\n-\tget_schedule_cmd(&cmd, NULL);\n-\tstrvec_split(&child.args, cmd);\n-\tstrvec_pushl(&child.args, \"--user\", enable ? \"enable\" : \"disable\",\n-\t\t \"--now\", NULL);\n-\tstrvec_pushf(&child.args, \"git-maintenance@%s.timer\", frequency);\n-\n-\tif (start_command(&child))\n-\t\treturn error(_(\"failed to start systemctl\"));\n-\tif (finish_command(&child))\n-\t\t/*\n-\t\t * Disabling an already disabled systemd unit makes\n-\t\t * systemctl fail.\n-\t\t * Let's ignore this failure.\n-\t\t *\n-\t\t * Enabling an enabled systemd unit doesn't fail.\n-\t\t */\n-\t\tif (enable)\n-\t\t\treturn error(_(\"failed to run systemctl\"));\n-\treturn 0;\n-}\n-\n-static int systemd_timer_delete_unit_templates(void)\n-{\n-\tint ret = 0;\n-\tchar *filename = xdg_config_home_systemd(\"git-maintenance@.timer\");\n-\tif (unlink(filename) && !is_missing_file_error(errno))\n-\t\tret = error_errno(_(\"failed to delete '%s'\"), filename);\n-\tFREE_AND_NULL(filename);\n-\n-\tfilename = xdg_config_home_systemd(\"git-maintenance@.service\");\n-\tif (unlink(filename) && !is_missing_file_error(errno))\n-\t\tret = error_errno(_(\"failed to delete '%s'\"), filename);\n-\n-\tfree(filename);\n-\treturn ret;\n-}\n-\n-static int systemd_timer_delete_units(void)\n-{\n-\treturn systemd_timer_enable_unit(0, SCHEDULE_HOURLY) ||\n-\t systemd_timer_enable_unit(0, SCHEDULE_DAILY) ||\n-\t systemd_timer_enable_unit(0, SCHEDULE_WEEKLY) ||\n-\t systemd_timer_delete_unit_templates();\n-}\n-\n static int systemd_timer_write_unit_templates(const char *exec_path)\n {\n \tchar *filename;\n@@ -2444,6 +2380,70 @@ error:\n \treturn -1;\n }\n \n+static int systemd_timer_enable_unit(int enable,\n+\t\t\t\t enum schedule_priority schedule)\n+{\n+\tconst char *cmd = \"systemctl\";\n+\tstruct child_process child = CHILD_PROCESS_INIT;\n+\tconst char *frequency = get_frequency(schedule);\n+\n+\t/*\n+\t * Disabling the systemd unit while it is already disabled makes\n+\t * systemctl print an error.\n+\t * Let's ignore it since it means we already are in the expected state:\n+\t * the unit is disabled.\n+\t *\n+\t * On the other hand, enabling a systemd unit which is already enabled\n+\t * produces no error.\n+\t */\n+\tif (!enable)\n+\t\tchild.no_stderr = 1;\n+\n+\tget_schedule_cmd(&cmd, NULL);\n+\tstrvec_split(&child.args, cmd);\n+\tstrvec_pushl(&child.args, \"--user\", enable ? \"enable\" : \"disable\",\n+\t\t \"--now\", NULL);\n+\tstrvec_pushf(&child.args, \"git-maintenance@%s.timer\", frequency);\n+\n+\tif (start_command(&child))\n+\t\treturn error(_(\"failed to start systemctl\"));\n+\tif (finish_command(&child))\n+\t\t/*\n+\t\t * Disabling an already disabled systemd unit makes\n+\t\t * systemctl fail.\n+\t\t * Let's ignore this failure.\n+\t\t *\n+\t\t * Enabling an enabled systemd unit doesn't fail.\n+\t\t */\n+\t\tif (enable)\n+\t\t\treturn error(_(\"failed to run systemctl\"));\n+\treturn 0;\n+}\n+\n+static int systemd_timer_delete_unit_templates(void)\n+{\n+\tint ret = 0;\n+\tchar *filename = xdg_config_home_systemd(\"git-maintenance@.timer\");\n+\tif (unlink(filename) && !is_missing_file_error(errno))\n+\t\tret = error_errno(_(\"failed to delete '%s'\"), filename);\n+\tFREE_AND_NULL(filename);\n+\n+\tfilename = xdg_config_home_systemd(\"git-maintenance@.service\");\n+\tif (unlink(filename) && !is_missing_file_error(errno))\n+\t\tret = error_errno(_(\"failed to delete '%s'\"), filename);\n+\n+\tfree(filename);\n+\treturn ret;\n+}\n+\n+static int systemd_timer_delete_units(void)\n+{\n+\treturn systemd_timer_enable_unit(0, SCHEDULE_HOURLY) ||\n+\t systemd_timer_enable_unit(0, SCHEDULE_DAILY) ||\n+\t systemd_timer_enable_unit(0, SCHEDULE_WEEKLY) ||\n+\t systemd_timer_delete_unit_templates();\n+}\n+\n static int systemd_timer_setup_units(void)\n {\n \tconst char *exec_path = git_exec_path();\n-- \ngitgitgadget\n\n"},{"id":"480248","messageId":"14e340b75faaa66980479f42fec14c457aea5c74.1691434300.git.gitgitgadget@gmail.com","threadId":"60077","inReplyTo":"pull.1567.git.1691434300.gitgitgadget@gmail.com","subject":"[PATCH 6/6] maintenance: use random minute in systemd scheduler","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-08-07T18:51:40Z","receivedAt":"2023-08-07T18:52:02Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nThe get_random_minute() method was created to allow maintenance\nschedules to be fixed to a random minute of the hour. This randomness is\nonly intended to spread out the load from a number of clients, but each\nclient should have an hour between each maintenance cycle.\n\nAdd this random minute to the systemd integration.\n\nThis integration is more complicated than similar changes for other\nschedulers because of a neat trick that systemd allows: templating.\n\nThe previous implementation generated two template files with names\nof the form 'git-maintenance@.(timer|service)'. The '.timer' or\n'.service' indicates that this is a template that is picked up when we\nlater specify '...@.timer' or '...@.service'. The\n'' string is then used to insert into the template both the\n'OnCalendar' schedule setting and the '--schedule' parameter of the\n'git maintenance run' command.\n\nIn order to set these schedules to a given minute, we can no longer use\nthe 'hourly', 'daily', or 'weekly' strings for '' and instead\nneed to abandon the template model.\n\nModify the template with a custom schedule in the 'OnCalendar' setting.\nThis schedule has some interesting differences from cron-like patterns,\nbut is relatively easy to figure out from context. The one that might be\nconfusing is that '*-*-*' is a date-based pattern, but this must be\nomitted when using 'Mon' to signal that we care about the day of the\nweek. Monday is used since that matches the day used for the 'weekly'\nschedule used previously.\n\nThe rest of the change involves making sure we are writing these .timer\nand .service files before initializing the schedule with 'systemctl' and\ndeleting the files when we are done. Some changes are also made to share\nthe random minute along with a single computation of the execution path\nof the current Git executable.\n\nSigned-off-by: Derrick Stolee \n---\n builtin/gc.c | 82 ++++++++++++++++++++++++++++++++----------\n t/t7900-maintenance.sh | 4 ++-\n 2 files changed, 66 insertions(+), 20 deletions(-)\n\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex b3ef95b10aa..5f5bb95641f 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -2299,13 +2299,20 @@ static char *xdg_config_home_systemd(const char *filename)\n \treturn xdg_config_home_for(\"systemd/user\", filename);\n }\n \n-static int systemd_timer_write_unit_templates(const char *exec_path)\n+static int systemd_timer_write_unit_template(enum schedule_priority schedule,\n+\t\t\t\t\t const char *exec_path,\n+\t\t\t\t\t int minute)\n {\n \tchar *filename;\n \tFILE *file;\n \tconst char *unit;\n+\tchar *schedule_pattern = NULL;\n+\tconst char *frequency = get_frequency(schedule);\n+\tchar *local_timer_name = xstrfmt(\"git-maintenance@%s.timer\", frequency);\n+\tchar *local_service_name = xstrfmt(\"git-maintenance@%s.service\", frequency);\n+\n+\tfilename = xdg_config_home_systemd(local_timer_name);\n \n-\tfilename = xdg_config_home_systemd(\"git-maintenance@.timer\");\n \tif (safe_create_leading_directories(filename)) {\n \t\terror(_(\"failed to create directories for '%s'\"), filename);\n \t\tgoto error;\n@@ -2314,6 +2321,23 @@ static int systemd_timer_write_unit_templates(const char *exec_path)\n \tif (!file)\n \t\tgoto error;\n \n+\tswitch (schedule) {\n+\tcase SCHEDULE_HOURLY:\n+\t\tschedule_pattern = xstrfmt(\"*-*-* *:%02d:00\", minute);\n+\t\tbreak;\n+\n+\tcase SCHEDULE_DAILY:\n+\t\tschedule_pattern = xstrfmt(\"*-*-* 0:%02d:00\", minute);\n+\t\tbreak;\n+\n+\tcase SCHEDULE_WEEKLY:\n+\t\tschedule_pattern = xstrfmt(\"Mon 0:%02d:00\", minute);\n+\t\tbreak;\n+\n+\tdefault:\n+\t\tBUG(\"Unhandled schedule_priority\");\n+\t}\n+\n \tunit = \"# This file was created and is maintained by Git.\\n\"\n \t \"# Any edits made in this file might be replaced in the future\\n\"\n \t \"# by a Git command.\\n\"\n@@ -2322,12 +2346,12 @@ static int systemd_timer_write_unit_templates(const char *exec_path)\n \t \"Description=Optimize Git repositories data\\n\"\n \t \"\\n\"\n \t \"[Timer]\\n\"\n-\t \"OnCalendar=%i\\n\"\n+\t \"OnCalendar=%s\\n\"\n \t \"Persistent=true\\n\"\n \t \"\\n\"\n \t \"[Install]\\n\"\n \t \"WantedBy=timers.target\\n\";\n-\tif (fputs(unit, file) == EOF) {\n+\tif (fprintf(file, unit, schedule_pattern) < 0) {\n \t\terror(_(\"failed to write to '%s'\"), filename);\n \t\tfclose(file);\n \t\tgoto error;\n@@ -2338,7 +2362,7 @@ static int systemd_timer_write_unit_templates(const char *exec_path)\n \t}\n \tfree(filename);\n \n-\tfilename = xdg_config_home_systemd(\"git-maintenance@.service\");\n+\tfilename = xdg_config_home_systemd(local_service_name);\n \tfile = fopen_or_warn(filename, \"w\");\n \tif (!file)\n \t\tgoto error;\n@@ -2352,7 +2376,7 @@ static int systemd_timer_write_unit_templates(const char *exec_path)\n \t \"\\n\"\n \t \"[Service]\\n\"\n \t \"Type=oneshot\\n\"\n-\t \"ExecStart=\\\"%s/git\\\" --exec-path=\\\"%s\\\" for-each-repo --config=maintenance.repo maintenance run --schedule=%%i\\n\"\n+\t \"ExecStart=\\\"%s/git\\\" --exec-path=\\\"%s\\\" for-each-repo --config=maintenance.repo maintenance run --schedule=%s\\n\"\n \t \"LockPersonality=yes\\n\"\n \t \"MemoryDenyWriteExecute=yes\\n\"\n \t \"NoNewPrivileges=yes\\n\"\n@@ -2362,7 +2386,7 @@ static int systemd_timer_write_unit_templates(const char *exec_path)\n \t \"RestrictSUIDSGID=yes\\n\"\n \t \"SystemCallArchitectures=native\\n\"\n \t \"SystemCallFilter=@system-service\\n\";\n-\tif (fprintf(file, unit, exec_path, exec_path) < 0) {\n+\tif (fprintf(file, unit, exec_path, exec_path, frequency) < 0) {\n \t\terror(_(\"failed to write to '%s'\"), filename);\n \t\tfclose(file);\n \t\tgoto error;\n@@ -2375,13 +2399,16 @@ static int systemd_timer_write_unit_templates(const char *exec_path)\n \treturn 0;\n \n error:\n+\tfree(schedule_pattern);\n+\tfree(local_timer_name);\n \tfree(filename);\n-\tsystemd_timer_delete_unit_templates();\n \treturn -1;\n }\n \n static int systemd_timer_enable_unit(int enable,\n-\t\t\t\t enum schedule_priority schedule)\n+\t\t\t\t enum schedule_priority schedule,\n+\t\t\t\t const char *exec_path,\n+\t\t\t\t int minute)\n {\n \tconst char *cmd = \"systemctl\";\n \tstruct child_process child = CHILD_PROCESS_INIT;\n@@ -2398,6 +2425,8 @@ static int systemd_timer_enable_unit(int enable,\n \t */\n \tif (!enable)\n \t\tchild.no_stderr = 1;\n+\telse if (systemd_timer_write_unit_template(schedule, exec_path, minute))\n+\t\treturn -1;\n \n \tget_schedule_cmd(&cmd, NULL);\n \tstrvec_split(&child.args, cmd);\n@@ -2420,38 +2449,53 @@ static int systemd_timer_enable_unit(int enable,\n \treturn 0;\n }\n \n-static int systemd_timer_delete_unit_templates(void)\n+static int systemd_timer_delete_unit_template(enum schedule_priority priority)\n {\n+\tconst char *frequency = get_frequency(priority);\n+\tchar *local_timer_name = xstrfmt(\"git-maintenance@%s.timer\", frequency);\n+\tchar *local_service_name = xstrfmt(\"git-maintenance@%s.service\", frequency);\n \tint ret = 0;\n-\tchar *filename = xdg_config_home_systemd(\"git-maintenance@.timer\");\n+\tchar *filename = xdg_config_home_systemd(local_timer_name);\n \tif (unlink(filename) && !is_missing_file_error(errno))\n \t\tret = error_errno(_(\"failed to delete '%s'\"), filename);\n \tFREE_AND_NULL(filename);\n \n-\tfilename = xdg_config_home_systemd(\"git-maintenance@.service\");\n+\tfilename = xdg_config_home_systemd(local_service_name);\n \tif (unlink(filename) && !is_missing_file_error(errno))\n \t\tret = error_errno(_(\"failed to delete '%s'\"), filename);\n \n \tfree(filename);\n+\tfree(local_timer_name);\n+\tfree(local_service_name);\n \treturn ret;\n }\n \n+static int systemd_timer_delete_unit_templates(void)\n+{\n+\t/* Purposefully not short-circuited to make sure all are called. */\n+\treturn systemd_timer_delete_unit_template(SCHEDULE_HOURLY) |\n+\t systemd_timer_delete_unit_template(SCHEDULE_DAILY) |\n+\t systemd_timer_delete_unit_template(SCHEDULE_WEEKLY);\n+}\n+\n static int systemd_timer_delete_units(void)\n {\n-\treturn systemd_timer_enable_unit(0, SCHEDULE_HOURLY) ||\n-\t systemd_timer_enable_unit(0, SCHEDULE_DAILY) ||\n-\t systemd_timer_enable_unit(0, SCHEDULE_WEEKLY) ||\n+\tint minute = get_random_minute();\n+\tconst char *exec_path = git_exec_path();\n+\treturn systemd_timer_enable_unit(0, SCHEDULE_HOURLY, exec_path, minute) ||\n+\t systemd_timer_enable_unit(0, SCHEDULE_DAILY, exec_path, minute) ||\n+\t systemd_timer_enable_unit(0, SCHEDULE_WEEKLY, exec_path, minute) ||\n \t systemd_timer_delete_unit_templates();\n }\n \n static int systemd_timer_setup_units(void)\n {\n+\tint minute = get_random_minute();\n \tconst char *exec_path = git_exec_path();\n \n-\tint ret = systemd_timer_write_unit_templates(exec_path) ||\n-\t\t systemd_timer_enable_unit(1, SCHEDULE_HOURLY) ||\n-\t\t systemd_timer_enable_unit(1, SCHEDULE_DAILY) ||\n-\t\t systemd_timer_enable_unit(1, SCHEDULE_WEEKLY);\n+\tint ret = systemd_timer_enable_unit(1, SCHEDULE_HOURLY, exec_path, minute) ||\n+\t\t systemd_timer_enable_unit(1, SCHEDULE_DAILY, exec_path, minute) ||\n+\t\t systemd_timer_enable_unit(1, SCHEDULE_WEEKLY, exec_path, minute);\n \tif (ret)\n \t\tsystemd_timer_delete_units();\n \treturn ret;\ndiff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh\nindex 487e326b3fa..f5a93f5aecf 100755\n--- a/t/t7900-maintenance.sh\n+++ b/t/t7900-maintenance.sh\n@@ -744,7 +744,9 @@ test_expect_success 'start and stop Linux/systemd maintenance' '\n \t# start registers the repo\n \tgit config --get --global --fixed-value maintenance.repo \"$(pwd)\" &&\n \n-\ttest_systemd_analyze_verify \"systemd/user/git-maintenance@.service\" &&\n+\ttest_systemd_analyze_verify \"systemd/user/git-maintenance@hourly.service\" &&\n+\ttest_systemd_analyze_verify \"systemd/user/git-maintenance@daily.service\" &&\n+\ttest_systemd_analyze_verify \"systemd/user/git-maintenance@weekly.service\" &&\n \n \tprintf -- \"--user enable --now git-maintenance@%s.timer\\n\" hourly daily weekly >expect &&\n \ttest_cmp expect args &&\n-- \ngitgitgadget\n"},{"id":"480264","messageId":"ZNFgIyuhlNd8I9Y2@nand.local","threadId":"60077","inReplyTo":"fefdaa9457948ee5302e7cbfaae250e0b589d752.1691434300.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/6] maintenance: add get_random_minute()","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-08-07T21:20:35Z","receivedAt":"2023-08-07T21:20:41Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Aug 07, 2023 at 06:51:35PM +0000, Derrick Stolee via GitGitGadget wrote:\n> diff --git a/builtin/gc.c b/builtin/gc.c\n> index f3942188a61..66a972bc292 100644\n> --- a/builtin/gc.c\n> +++ b/builtin/gc.c\n> @@ -1708,6 +1708,23 @@ static int get_schedule_cmd(const char **cmd, int *is_available)\n> \treturn 1;\n> }\n>\n> +MAYBE_UNUSED\n> +static int get_random_minute(void)\n> +{\n> +\tstatic int random_initialized = 0;\n> +\n> +\t/* Use a static value when under tests. */\n> +\tif (!getenv(\"GIT_TEST_MAINTENANCE_SCHEDULER\"))\n> +\t\treturn 13;\n> +\n> +\tif (!random_initialized) {\n> +\t\tsrand((unsigned int)getpid());\n> +\t\trandom_initialized = 1;\n> +\t}\n\nI was wondering where else we call srand() within Git, and it looks like\nthe only other spot is in `lock_file_timeout()`.\n\nI doubt it, but is there a chance that that code depends on only calling\nsrand() once? I think the answer is \"no\", since we only use rand()\nwithin that function to generate a random-ish backoff period, so I think\nthe repeatability of it doesn't matter all that much.\n\nSo I think this is kind of outside the scope of your series, but I\nwonder if we should have a git_rand() that automatically initializes the\nPRNG with the value of getpid()? Then multiple callers can grab random\nvalues at will without reinitializing the PRNG.\n\nThanks,\nTaylor\n"},{"id":"480265","messageId":"ZNFgttgDTVr4sr6I@nand.local","threadId":"60077","inReplyTo":"babf62ec6d5181c65dd4b653ba9ae24e62a88830.1691434300.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/6] maintenance: use random minute in launchctl scheduler","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-08-07T21:23:02Z","receivedAt":"2023-08-07T21:23:08Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Aug 07, 2023 at 06:51:36PM +0000, Derrick Stolee via GitGitGadget wrote:\n> From: Derrick Stolee \n>\n> The get_random_minute() method was created to allow maintenance\n> schedules to be fixed to a random minute of the hour. This randomness is\n> only intended to spread out the load from a number of clients, but each\n> client should have an hour between each maintenance cycle.\n>\n> Use get_random_minute() when constructing the schedules for launchctl.\n>\n> The format already includes a 'Minute' key which is modified from 0 to\n> the random minute.\n\nAll looks good. I was wondering if this is something that we'd want to\nallow users to opt out of, but I think that the built-in schedules are\nsufficiently vague that we can afford to have a fudge factor on the\nexact minute.\n\nThanks,\nTaylor\n"},{"id":"480266","messageId":"ZNFhGIXaQr5YQfLq@nand.local","threadId":"60077","inReplyTo":"37f96b6b619d2d3eecd9c2b35483b2d61083603b.1691434300.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 5/6] maintenance: swap method locations","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-08-07T21:24:40Z","receivedAt":"2023-08-07T21:24:56Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Aug 07, 2023 at 06:51:39PM +0000, Derrick Stolee via GitGitGadget wrote:\n> The diff is computed smaller by showing systemd_timer_enable_unit() move\n> instead of systemd_timer_write_unit_templates().\n>\n> Signed-off-by: Derrick Stolee \n> ---\n> builtin/gc.c | 128 +++++++++++++++++++++++++--------------------------\n> 1 file changed, 64 insertions(+), 64 deletions(-)\n\nThis looks like a straightforward code movement, and diffing with\n`--color-moved` verifies it as such. LGTM.\n\nThanks,\nTaylor\n"},{"id":"480267","messageId":"ZNFinmsgDI7Upw60@nand.local","threadId":"60077","inReplyTo":"14e340b75faaa66980479f42fec14c457aea5c74.1691434300.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 6/6] maintenance: use random minute in systemd scheduler","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-08-07T21:31:10Z","receivedAt":"2023-08-07T21:31:25Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Aug 07, 2023 at 06:51:40PM +0000, Derrick Stolee via GitGitGadget wrote:\n> In order to set these schedules to a given minute, we can no longer use\n> the 'hourly', 'daily', or 'weekly' strings for '' and instead\n> need to abandon the template model.\n\nMakes sense.\n\n> Modify the template with a custom schedule in the 'OnCalendar' setting.\n> This schedule has some interesting differences from cron-like patterns,\n> but is relatively easy to figure out from context. The one that might be\n> confusing is that '*-*-*' is a date-based pattern, but this must be\n> omitted when using 'Mon' to signal that we care about the day of the\n> week. Monday is used since that matches the day used for the 'weekly'\n> schedule used previously.\n\nI think the launchd version (which uses \"0\" for the day of the week)\nruns on Sunday, if I remember correctly. I don't think that these two\nnecessarily need to run on the same day of the week when configured to\nrun weekly.\n\nBut I figured I'd raise the question in case you did mean for them to\nboth run on either Sunday or Monday.\n\n> The rest of the change involves making sure we are writing these .timer\n> and .service files before initializing the schedule with 'systemctl' and\n> deleting the files when we are done. Some changes are also made to share\n> the random minute along with a single computation of the execution path\n> of the current Git executable.\n>\n> Signed-off-by: Derrick Stolee \n> ---\n> builtin/gc.c | 82 ++++++++++++++++++++++++++++++++----------\n> t/t7900-maintenance.sh | 4 ++-\n> 2 files changed, 66 insertions(+), 20 deletions(-)\n>\n> diff --git a/builtin/gc.c b/builtin/gc.c\n> index b3ef95b10aa..5f5bb95641f 100644\n> --- a/builtin/gc.c\n> +++ b/builtin/gc.c\n> @@ -2299,13 +2299,20 @@ static char *xdg_config_home_systemd(const char *filename)\n> \treturn xdg_config_home_for(\"systemd/user\", filename);\n> }\n>\n> -static int systemd_timer_write_unit_templates(const char *exec_path)\n> +static int systemd_timer_write_unit_template(enum schedule_priority schedule,\n> +\t\t\t\t\t const char *exec_path,\n> +\t\t\t\t\t int minute)\n> {\n> \tchar *filename;\n> \tFILE *file;\n> \tconst char *unit;\n> +\tchar *schedule_pattern = NULL;\n\nYou should be able to drop the NULL initialization, since you assign\nthis value unconditionally in the switch statement below (or BUG() on an\nunknown schedule type).\n\n> +\tconst char *frequency = get_frequency(schedule);\n> +\tchar *local_timer_name = xstrfmt(\"git-maintenance@%s.timer\", frequency);\n> +\tchar *local_service_name = xstrfmt(\"git-maintenance@%s.service\", frequency);\n> +\n> +\tfilename = xdg_config_home_systemd(local_timer_name);\n>\n> -\tfilename = xdg_config_home_systemd(\"git-maintenance@.timer\");\n> \tif (safe_create_leading_directories(filename)) {\n> \t\terror(_(\"failed to create directories for '%s'\"), filename);\n> \t\tgoto error;\n> @@ -2314,6 +2321,23 @@ static int systemd_timer_write_unit_templates(const char *exec_path)\n> \tif (!file)\n> \t\tgoto error;\n>\n> +\tswitch (schedule) {\n> +\tcase SCHEDULE_HOURLY:\n> +\t\tschedule_pattern = xstrfmt(\"*-*-* *:%02d:00\", minute);\n> +\t\tbreak;\n> +\n> +\tcase SCHEDULE_DAILY:\n> +\t\tschedule_pattern = xstrfmt(\"*-*-* 0:%02d:00\", minute);\n> +\t\tbreak;\n> +\n> +\tcase SCHEDULE_WEEKLY:\n> +\t\tschedule_pattern = xstrfmt(\"Mon 0:%02d:00\", minute);\n> +\t\tbreak;\n> +\n> +\tdefault:\n> +\t\tBUG(\"Unhandled schedule_priority\");\n> +\t}\n> +\n> \tunit = \"# This file was created and is maintained by Git.\\n\"\n> \t \"# Any edits made in this file might be replaced in the future\\n\"\n> \t \"# by a Git command.\\n\"\n> @@ -2322,12 +2346,12 @@ static int systemd_timer_write_unit_templates(const char *exec_path)\n> \t \"Description=Optimize Git repositories data\\n\"\n> \t \"\\n\"\n> \t \"[Timer]\\n\"\n> -\t \"OnCalendar=%i\\n\"\n> +\t \"OnCalendar=%s\\n\"\n> \t \"Persistent=true\\n\"\n> \t \"\\n\"\n> \t \"[Install]\\n\"\n> \t \"WantedBy=timers.target\\n\";\n> -\tif (fputs(unit, file) == EOF) {\n> +\tif (fprintf(file, unit, schedule_pattern) < 0) {\n\nOK, this is the templating part that you were mentioning earlier. I was\nwondering what we were doing fputs()-ing a string with \"%i\" in it\nwithout a formatting value to fill it in with. But that \"%i\" pertains to\nsystemd's instance value, IIUC.\n\nThe rest all looks good, thanks.\n\nThanks,\nTaylor\n"},{"id":"480275","messageId":"xmqq4jlabea3.fsf@gitster.g","threadId":"60077","inReplyTo":"ZNFgIyuhlNd8I9Y2@nand.local","subject":"Re: [PATCH 1/6] maintenance: add get_random_minute()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-08-07T23:53:24Z","receivedAt":"2023-08-07T23:53:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau writes:\n\n> I was wondering where else we call srand() within Git, and it looks like\n> the only other spot is in `lock_file_timeout()`.\n\nlock_file_timeout() should be updated to match git_mkstemps_mode(),\nwhich was taught to use the csprng_bytes() function with 47efda96\n(wrapper: use a CSPRNG to generate random file names, 2022-01-17),\nand this new caller may want to do so as well, perhaps? I dunno,\nbut the caller then does not have to worry about \"initializing it\njust once\".\n"},{"id":"480276","messageId":"xmqqzg329ycm.fsf@gitster.g","threadId":"60077","inReplyTo":"xmqq4jlabea3.fsf@gitster.g","subject":"Re: [PATCH 1/6] maintenance: add get_random_minute()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-08-08T00:22:49Z","receivedAt":"2023-08-08T00:22:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Taylor Blau writes:\n>\n>> I was wondering where else we call srand() within Git, and it looks like\n>> the only other spot is in `lock_file_timeout()`.\n>\n> lock_file_timeout() should be updated to match git_mkstemps_mode(),\n> which was taught to use the csprng_bytes() function with 47efda96\n> (wrapper: use a CSPRNG to generate random file names, 2022-01-17),\n> and this new caller may want to do so as well, perhaps? I dunno,\n> but the caller then does not have to worry about \"initializing it\n> just once\".\n\nOf course, the obvious downside is that crypto-secure one may be,\nunlike for its use in mkstemps(), way overkill for lockfiles and\ncron dispersion purposes, as these codepaths are not on the target\nsurface.\n"},{"id":"480278","messageId":"8aa7be24-f4a8-4515-8425-dd4bc69e07b1@github.com","threadId":"60077","inReplyTo":"adc0cf80-c2e3-7af2-6d7f-036b95d27f80@gmail.com","subject":"Re: [PATCH 6/6] maintenance: use random minute in systemd scheduler","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2023-08-08T13:56:28Z","receivedAt":"2023-08-08T15:54:35Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 8/8/2023 5:53 AM, Phillip Wood wrote:> Hi Stolee\n> \n> On 07/08/2023 19:51, Derrick Stolee via GitGitGadget wrote:\n>> From: Derrick Stolee \n>> + switch (schedule) {\n>> + case SCHEDULE_HOURLY:\n>> + schedule_pattern = xstrfmt(\"*-*-* *:%02d:00\", minute);\n>> + break;\n>> +\n>> + case SCHEDULE_DAILY:\n>> + schedule_pattern = xstrfmt(\"*-*-* 0:%02d:00\", minute);\n>> + break;\n>> +\n>> + case SCHEDULE_WEEKLY:\n>> + schedule_pattern = xstrfmt(\"Mon 0:%02d:00\", minute);\n>> + break;\n> \n> This is not a new issue with this patch but we run the hourly job even\n> when we want to run the daily job or the weekly job and we run the daily\n> job when we want to run the weekly job.\n\nThis is an excellent point! Thanks for bringing it up.\n\n> So only one of these jobs will succeed. The cron entries are careful to\n> only run one job at a time, I think it would be worth doing the same\n> thing here. I think the using the following format strings would fix this.\n> \n> Hourly: \"Tue..Sun *-*-* 1..23:00:%02d\"\n> Daily: \"Tue..Sun *-*-* 00:00:%02d\"\n> Weekly: \"Mon *-*-* 00:00:%02d\"\n\nI would modify this with dropping the \"Tue..Sun\" from the hourly schedule,\nas we still want 23 runs on Mondays.\n\n> It looks like the launchctl schedule has the same issue.\n\nI will take a look at this and consider some additional patches to correct\nthese issues across both schedulers. Thank you for the attention to detail!\n\n> One thing I've been wondering about which is related to maintenance but\n> totally off-topic for this patch is that I think when auto maintenance\n> is enabled we stop automatically running \"gc\" so how do the reflogs get\n> expired?\n\nThe default maintenance schedule does not include a 'gc' run as it does\nnot intend to remove any data. Reflog expiration could be considered as a\nseparate maintenance task that might be useful in the default maintenance\nschedule.\n\nThanks,\n-Stolee\n"},{"id":"480281","messageId":"ZNJVycLBUIXqZ0Xb@nand.local","threadId":"60077","inReplyTo":"xmqqzg329ycm.fsf@gitster.g","subject":"Re: [PATCH 1/6] maintenance: add get_random_minute()","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-08-08T14:48:41Z","receivedAt":"2023-08-08T16:19:48Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Aug 07, 2023 at 05:22:49PM -0700, Junio C Hamano wrote:\n> Junio C Hamano writes:\n>\n> > Taylor Blau writes:\n> >\n> >> I was wondering where else we call srand() within Git, and it looks like\n> >> the only other spot is in `lock_file_timeout()`.\n> >\n> > lock_file_timeout() should be updated to match git_mkstemps_mode(),\n> > which was taught to use the csprng_bytes() function with 47efda96\n> > (wrapper: use a CSPRNG to generate random file names, 2022-01-17),\n> > and this new caller may want to do so as well, perhaps? I dunno,\n> > but the caller then does not have to worry about \"initializing it\n> > just once\".\n>\n> Of course, the obvious downside is that crypto-secure one may be,\n> unlike for its use in mkstemps(), way overkill for lockfiles and\n> cron dispersion purposes, as these codepaths are not on the target\n> surface.\n\nI think that's an acceptable price to pay here, since we can drop the\ncode to remember whether or not srand() has been called or not. Here's a\npatch that we could take in that direction:\n\n--- 8< ---\nSubject: [PATCH] lockfile.c: use a CSPRNG to generate backoff milliseconds\n\nUpon failing to acquire the lockfile, `lock_file_timeout()` will try\nagain with an exponential backoff. This backoff includes some noise as a\nmultiplier over the default backoff behavior ranging from [0.75, 1.25].\n\nIt generates this noise via rand(3). Using a non-cryptographic source of\nrandomness here is OK, since a more trivial attack vector (holding the\nfile open via an external process for longer than the value of\n`timeout_ms`) is easier to exploit.\n\nThat all said, `lock_file_timeout()` initializes the PRNG with\n`srand()`. This has a couple of downsides:\n\n - lock_file_timeout() needs to remember whether or not the PRNG has\n or hasn't been seeded.\n\n - If a different function also calls `srand()`, the PRNG may start\n generating repeated values (if that caller also initialized the PRNG\n with `getpid()`).\n\nLet's avoid both of these by using `csprng_bytes()`, in a similar spirit\nas 47efda967c (wrapper: use a CSPRNG to generate random file names,\n2022-01-17).\n\nUsing a CSPRNG is definitely overkill for noising a backoff window, but\nit avoids the concerns about calling `srand()`, so let's use it here,\ntoo.\n\nSuggested-by: Junio C Hamano \nSigned-off-by: Taylor Blau \n---\n lockfile.c | 12 +++++-------\n 1 file changed, 5 insertions(+), 7 deletions(-)\n\ndiff --git a/lockfile.c b/lockfile.c\nindex 1d5ed01682..6587d407f4 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -107,22 +107,17 @@ static int lock_file_timeout(struct lock_file *lk, const char *path,\n \tint n = 1;\n \tint multiplier = 1;\n \tlong remaining_ms = 0;\n-\tstatic int random_initialized = 0;\n\n \tif (timeout_ms == 0)\n \t\treturn lock_file(lk, path, flags, mode);\n\n-\tif (!random_initialized) {\n-\t\tsrand((unsigned int)getpid());\n-\t\trandom_initialized = 1;\n-\t}\n-\n \tif (timeout_ms > 0)\n \t\tremaining_ms = timeout_ms;\n\n \twhile (1) {\n \t\tlong backoff_ms, wait_ms;\n \t\tint fd;\n+\t\tuint64_t rand;\n\n \t\tfd = lock_file(lk, path, flags, mode);\n\n@@ -135,7 +130,10 @@ static int lock_file_timeout(struct lock_file *lk, const char *path,\n\n \t\tbackoff_ms = multiplier * INITIAL_BACKOFF_MS;\n \t\t/* back off for between 0.75*backoff_ms and 1.25*backoff_ms */\n-\t\twait_ms = (750 + rand() % 500) * backoff_ms / 1000;\n+\t\tif (csprng_bytes(&rand, sizeof(uint64_t)) < 0)\n+\t\t\treturn error_errno(_(\"unable to get random bytes for\"\n+\t\t\t\t\t \"lockfile backoff\"));\n+\t\twait_ms = (750 + rand % 500) * backoff_ms / 1000;\n \t\tsleep_millisec(wait_ms);\n \t\tremaining_ms -= wait_ms;\n\n--\n2.42.0.rc0.26.g802d811bac.dirty\n\n--- >8 ---\n\nThanks,\nTaylor\n"},{"id":"480282","messageId":"adc0cf80-c2e3-7af2-6d7f-036b95d27f80@gmail.com","threadId":"60077","inReplyTo":"14e340b75faaa66980479f42fec14c457aea5c74.1691434300.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 6/6] maintenance: use random minute in systemd scheduler","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-08-08T09:53:56Z","receivedAt":"2023-08-08T16:23:40Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Stolee\n\nOn 07/08/2023 19:51, Derrick Stolee via GitGitGadget wrote:\n> From: Derrick Stolee \n> +\tswitch (schedule) {\n> +\tcase SCHEDULE_HOURLY:\n> +\t\tschedule_pattern = xstrfmt(\"*-*-* *:%02d:00\", minute);\n> +\t\tbreak;\n> +\n> +\tcase SCHEDULE_DAILY:\n> +\t\tschedule_pattern = xstrfmt(\"*-*-* 0:%02d:00\", minute);\n> +\t\tbreak;\n> +\n> +\tcase SCHEDULE_WEEKLY:\n> +\t\tschedule_pattern = xstrfmt(\"Mon 0:%02d:00\", minute);\n> +\t\tbreak;\n\nThis is not a new issue with this patch but we run the hourly job even \nwhen we want to run the daily job or the weekly job and we run the daily \njob when we want to run the weekly job. maintenance_run_tasks() contains\n\n\tif (hold_lock_file_for_update(&lk, lock_path, LOCK_NO_DEREF) < 0) {\n\t\t/*\n\t\t * Another maintenance command is running.\n\t\t *\n\t\t * If --auto was provided, then it is likely due to a\n\t\t * recursive process stack. Do not report an error in\n\t\t * that case.\n\t\t */\n\t\tif (!opts->auto_flag && !opts->quiet)\n\t\t\twarning(_(\"lock file '%s' exists, skipping maintenance\"),\n\t\t\t\tlock_path);\n\t\tfree(lock_path);\n\t\treturn 0;\n\t}\n\nSo only one of these jobs will succeed. The cron entries are careful to \nonly run one job at a time, I think it would be worth doing the same \nthing here. I think the using the following format strings would fix this.\n\nHourly: \"Tue..Sun *-*-* 1..23:00:%02d\"\nDaily: \"Tue..Sun *-*-* 00:00:%02d\"\nWeekly: \"Mon *-*-* 00:00:%02d\"\n\nIt looks like the launchctl schedule has the same issue.\n\nOne thing I've been wondering about which is related to maintenance but \ntotally off-topic for this patch is that I think when auto maintenance \nis enabled we stop automatically running \"gc\" so how do the reflogs get \nexpired?\n\nBest Wishes\n\nPhillip\n"},{"id":"480285","messageId":"2db943c3-2ec2-4b7e-a161-a7c9b3f04d36@github.com","threadId":"60077","inReplyTo":"ZNFinmsgDI7Upw60@nand.local","subject":"Re: [PATCH 6/6] maintenance: use random minute in systemd scheduler","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2023-08-08T13:49:40Z","receivedAt":"2023-08-08T16:39:38Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 8/7/2023 5:31 PM, Taylor Blau wrote:\n> On Mon, Aug 07, 2023 at 06:51:40PM +0000, Derrick Stolee via GitGitGadget wrote:\n\n>> Modify the template with a custom schedule in the 'OnCalendar' setting.\n>> This schedule has some interesting differences from cron-like patterns,\n>> but is relatively easy to figure out from context. The one that might be\n>> confusing is that '*-*-*' is a date-based pattern, but this must be\n>> omitted when using 'Mon' to signal that we care about the day of the\n>> week. Monday is used since that matches the day used for the 'weekly'\n>> schedule used previously.\n> \n> I think the launchd version (which uses \"0\" for the day of the week)\n> runs on Sunday, if I remember correctly. I don't think that these two\n> necessarily need to run on the same day of the week when configured to\n> run weekly.\n> \n> But I figured I'd raise the question in case you did mean for them to\n> both run on either Sunday or Monday.\n\nI don't intend to change the day that is run as part of this change.\n\nI think all other schedulers run on Sunday, but systemd running on Monday\nis a particular detail of its \"weekly\" schedule.\n\n>> -static int systemd_timer_write_unit_templates(const char *exec_path)\n>> +static int systemd_timer_write_unit_template(enum schedule_priority schedule,\n>> +\t\t\t\t\t const char *exec_path,\n>> +\t\t\t\t\t int minute)\n>> {\n>> \tchar *filename;\n>> \tFILE *file;\n>> \tconst char *unit;\n>> +\tchar *schedule_pattern = NULL;\n> \n> You should be able to drop the NULL initialization, since you assign\n> this value unconditionally in the switch statement below (or BUG() on an\n> unknown schedule type).\n\nUnfortunately, GCC complained about a possibly-unassigned value when I\nleft this unset during development, so this actually is necessary for\nthat compiler.\n\nThanks,\n-Stolee\n"},{"id":"480290","messageId":"c1c569a6-5200-2fb8-5fb8-f8d57253fb16@gmail.com","threadId":"60077","inReplyTo":"adc0cf80-c2e3-7af2-6d7f-036b95d27f80@gmail.com","subject":"Re: [PATCH 6/6] maintenance: use random minute in systemd scheduler","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-08-08T13:03:10Z","receivedAt":"2023-08-08T17:04:46Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 08/08/2023 10:53, Phillip Wood wrote:\n> Hourly: \"Tue..Sun *-*-* 1..23:00:%02d\"\n> Daily:  \"Tue..Sun *-*-* 00:00:%02d\"\n> Weekly: \"Mon      *-*-* 00:00:%02d\"\n\nThinking about it some more, this only fixes the problem if the computer \nis actually on at midnight. If the computer is switched off overnight \nthen we still try to start three maintenance jobs at the same time when \nthe user turns their computer on on Tuesday morning. We could stop \nmarking the hourly job as persistent on the assumption that it will be \nrun soon anyway but that does not solve the problem of the daily and \nweekly jobs running concurrently on a Tuesday morning.\n\nBest Wishes\n\nPhillip\n\n"},{"id":"480291","messageId":"3e254d61-b74a-f419-1a03-dbc557a9fe86@gmail.com","threadId":"60077","inReplyTo":"14e340b75faaa66980479f42fec14c457aea5c74.1691434300.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 6/6] maintenance: use random minute in systemd scheduler","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-08-08T12:08:28Z","receivedAt":"2023-08-08T17:08:00Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 07/08/2023 19:51, Derrick Stolee via GitGitGadget wrote:\n> From: Derrick Stolee \n> \n> The get_random_minute() method was created to allow maintenance\n> schedules to be fixed to a random minute of the hour. This randomness is\n> only intended to spread out the load from a number of clients, but each\n> client should have an hour between each maintenance cycle.\n> \n> Add this random minute to the systemd integration.\n\nI think it makes sense to keep the random minute implementation the same \nacross all the schedulers, but we could use RandomizedDelaySec (possibly \ncombined with FixedRandomDelay) to randomize when the job is actually run.\n\n> This integration is more complicated than similar changes for other\n> schedulers because of a neat trick that systemd allows: templating.\n> \n> The previous implementation generated two template files with names\n> of the form 'git-maintenance@.(timer|service)'. The '.timer' or\n> '.service' indicates that this is a template that is picked up when we\n> later specify '...@.timer' or '...@.service'. The\n> '' string is then used to insert into the template both the\n> 'OnCalendar' schedule setting and the '--schedule' parameter of the\n> 'git maintenance run' command.\n> \n> In order to set these schedules to a given minute, we can no longer use\n> the 'hourly', 'daily', or 'weekly' strings for '' and instead\n> need to abandon the template model.\n\nI've left some comments about this below.\n\n\n> @@ -2299,13 +2299,20 @@ static char *xdg_config_home_systemd(const char *filename)\n> \treturn xdg_config_home_for(\"systemd/user\", filename);\n> }\n> \n> -static int systemd_timer_write_unit_templates(const char *exec_path)\n> +static int systemd_timer_write_unit_template(enum schedule_priority schedule,\n\nAs we're not writing template files any more I think we should rename \nthis to systemd_timer_write_unit_file()\n\n> +\t\t\t\t\t const char *exec_path,\n> +\t\t\t\t\t int minute)\n> {\n> \tchar *filename;\n> \tFILE *file;\n> \tconst char *unit;\n> +\tchar *schedule_pattern = NULL;\n> +\tconst char *frequency = get_frequency(schedule);\n> +\tchar *local_timer_name = xstrfmt(\"git-maintenance@%s.timer\", frequency);\n\nThe \"@\" in the name signifies that it is a template unit which it isn't \nanymore so I think we want to change this to \"git-maintenance-%s.timer\"\n\n> +\tchar *local_service_name = xstrfmt(\"git-maintenance@%s.service\", frequency);\n\nSame change to the name here. I wonder if we could still use a template \nservice file but that would complicate the implementation as we'd need \nto write three timer files but only one service file.\n\n> [...]\n> @@ -2375,13 +2399,16 @@ static int systemd_timer_write_unit_templates(const char *exec_path)\n> \treturn 0;\n> \n> error:\n> +\tfree(schedule_pattern);\n> +\tfree(local_timer_name);\n> \tfree(filename);\n> -\tsystemd_timer_delete_unit_templates();\n\nThis looks like a change in behavior as previously we'd remove any files \nif there was an error rather than leaving behind a timer file without a \ncorresponding unit file.\n\nLooking at maintenance_start() we call maintenance_register() which \ndisables \"gc --auto\" before we get to this point so if we fail to write \nthe files we'll end up disabling any form of gc in the repository.\n\n> [...]\n> -static int systemd_timer_delete_unit_templates(void)\n> +static int systemd_timer_delete_unit_template(enum schedule_priority priority)\n\nSame suggestion as above to rename this to ..._unit_file()\n\n> {\n> +\tconst char *frequency = get_frequency(priority);\n> +\tchar *local_timer_name = xstrfmt(\"git-maintenance@%s.timer\", frequency);\n> +\tchar *local_service_name = xstrfmt(\"git-maintenance@%s.service\", frequency);\n\nI'm not sure it is worth it but we could perhaps\n\n\t#define SYSTEMD_UNIT_FORMAT \"git-maintenance-%s.%s\"\n\nabove and then these lines and the ones in \nsystemd_timer_write_unit_file() would become\n\n\tchar *local_timer_name = xstrfmt(SYSTEMD_UNIT_FORMAT, frequency, \"timer\");\n\tchar *local_service = xstrfmt(SYSTEMD_UNIT_FORMAT, frequency, \"service\");\n\n> [...]\n> +static int systemd_timer_delete_unit_templates(void)\n\nNaming again.\n\nBest Wishes\n\nPhillip\n"},{"id":"480299","messageId":"5bb4313a-01f3-4dea-b724-fb41a8def334@github.com","threadId":"60077","inReplyTo":"8aa7be24-f4a8-4515-8425-dd4bc69e07b1@github.com","subject":"Re: [PATCH 6/6] maintenance: use random minute in systemd scheduler","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2023-08-08T17:24:54Z","receivedAt":"2023-08-08T18:18:07Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 8/8/2023 9:56 AM, Derrick Stolee wrote:\n> On 8/8/2023 5:53 AM, Phillip Wood wrote:> Hi Stolee\n\n>> So only one of these jobs will succeed. The cron entries are careful to\n>> only run one job at a time, I think it would be worth doing the same\n>> thing here. I think the using the following format strings would fix this.\n>>\n>> Hourly: \"Tue..Sun *-*-* 1..23:00:%02d\"\n>> Daily: \"Tue..Sun *-*-* 00:00:%02d\"\n>> Weekly: \"Mon *-*-* 00:00:%02d\"\n> \n> I would modify this with dropping the \"Tue..Sun\" from the hourly schedule,\n> as we still want 23 runs on Mondays.\n> \n>> It looks like the launchctl schedule has the same issue.\n> \n> I will take a look at this and consider some additional patches to correct\n> these issues across both schedulers. Thank you for the attention to detail!\n\nTaking a look, it seems that launchctl does not have this same problem.\n\nThe schedule is set via an of s as follows:\n\n\tcase SCHEDULE_HOURLY:\n\t\trepeat = \"\\n\"\n\t\t\t \"Hour%d\\n\"\n\t\t\t \"Minute%d\\n\"\n\t\t\t \"\\n\";\n\t\tfor (i = 1; i <= 23; i++)\n\t\t\tstrbuf_addf(&plist, repeat, i, minute);\n\t\tbreak;\n\n\tcase SCHEDULE_DAILY:\n\t\trepeat = \"\\n\"\n\t\t\t \"Day%d\\n\"\n\t\t\t \"Hour0\\n\"\n\t\t\t \"Minute%d\\n\"\n\t\t\t \"\\n\";\n\t\tfor (i = 1; i <= 6; i++)\n\t\t\tstrbuf_addf(&plist, repeat, i, minute);\n\t\tbreak;\n\nThis means that we create an hourly schedule for each hour 1..23\n(no 0th hour means no collision with daily or weekly schedule) and\na daily schedule for each day 1..6 (no 0th day means no collision\nwith the weekly schedule).\n\nDoes this match your understanding?\n\nThe overlap _definitely_ exists in systemd, which I will fix.\n\nThanks,\n-Stolee\n\n"},{"id":"480302","messageId":"xmqqbkfha3xu.fsf@gitster.g","threadId":"60077","inReplyTo":"ZNJVycLBUIXqZ0Xb@nand.local","subject":"Re: [PATCH 1/6] maintenance: add get_random_minute()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-08-08T16:34:21Z","receivedAt":"2023-08-08T18:43:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau writes:\n\n> I think that's an acceptable price to pay here, since we can drop the\n> code to remember whether or not srand() has been called or not. Here's a\n> patch that we could take in that direction:\n> ...\n> - If a different function also calls `srand()`, the PRNG may start\n> generating repeated values (if that caller also initialized the PRNG\n> with `getpid()`).\n\nHmph, I didn't think about that one. I do not think it can be a\nviable attack vector to attack _this_ code, but the other function\nmight be. But if the other function is worth attacking and\nattackable, it ought to be using crypto-secure one and not srand(),\nso this argument may not give us a good justification X-<.\n\nProvided if code simplification is a good enough rationale, the\npatch looks sensible, but I find its use of u64 a bit questionable\n(though not wrong). I would have expected that the type of \"rand\"\nwould be the same as backoff_ms and wait_ms, two variables involved\nin the same expression.\n\nThanks.\n\n> diff --git a/lockfile.c b/lockfile.c\n> index 1d5ed01682..6587d407f4 100644\n> --- a/lockfile.c\n> +++ b/lockfile.c\n> @@ -107,22 +107,17 @@ static int lock_file_timeout(struct lock_file *lk, const char *path,\n> \tint n = 1;\n> \tint multiplier = 1;\n> \tlong remaining_ms = 0;\n> -\tstatic int random_initialized = 0;\n>\n> \tif (timeout_ms == 0)\n> \t\treturn lock_file(lk, path, flags, mode);\n>\n> -\tif (!random_initialized) {\n> -\t\tsrand((unsigned int)getpid());\n> -\t\trandom_initialized = 1;\n> -\t}\n> -\n> \tif (timeout_ms > 0)\n> \t\tremaining_ms = timeout_ms;\n>\n> \twhile (1) {\n> \t\tlong backoff_ms, wait_ms;\n> \t\tint fd;\n> +\t\tuint64_t rand;\n>\n> \t\tfd = lock_file(lk, path, flags, mode);\n>\n> @@ -135,7 +130,10 @@ static int lock_file_timeout(struct lock_file *lk, const char *path,\n>\n> \t\tbackoff_ms = multiplier * INITIAL_BACKOFF_MS;\n> \t\t/* back off for between 0.75*backoff_ms and 1.25*backoff_ms */\n> -\t\twait_ms = (750 + rand() % 500) * backoff_ms / 1000;\n> +\t\tif (csprng_bytes(&rand, sizeof(uint64_t)) < 0)\n> +\t\t\treturn error_errno(_(\"unable to get random bytes for\"\n> +\t\t\t\t\t \"lockfile backoff\"));\n> +\t\twait_ms = (750 + rand % 500) * backoff_ms / 1000;\n> \t\tsleep_millisec(wait_ms);\n> \t\tremaining_ms -= wait_ms;\n>\n> --\n> 2.42.0.rc0.26.g802d811bac.dirty\n>\n> --- >8 ---\n>\n> Thanks,\n> Taylor\n"},{"id":"480303","messageId":"d247ce48-e2ba-4915-8d96-1fa2f693909d@github.com","threadId":"60077","inReplyTo":"3e254d61-b74a-f419-1a03-dbc557a9fe86@gmail.com","subject":"Re: [PATCH 6/6] maintenance: use random minute in systemd scheduler","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2023-08-08T17:06:51Z","receivedAt":"2023-08-08T18:53:05Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 8/8/2023 8:08 AM, Phillip Wood wrote:\n> On 07/08/2023 19:51, Derrick Stolee via GitGitGadget wrote:\n>> From: Derrick Stolee \n>>\n>> The get_random_minute() method was created to allow maintenance\n>> schedules to be fixed to a random minute of the hour. This randomness is\n>> only intended to spread out the load from a number of clients, but each\n>> client should have an hour between each maintenance cycle.\n>>\n>> Add this random minute to the systemd integration.\n> \n> I think it makes sense to keep the random minute implementation the same across all the schedulers, but we could use RandomizedDelaySec (possibly combined with FixedRandomDelay) to randomize when the job is actually run.\n\nThat's an interesting suggestion, but I also think it is valuable to\nhave consistent gaps between maintenance activities on the client, but\nRandomizedDelaySec would present the possibility of 60-3540 seconds between\n\"hourly\" maintenance runs (if I understand the option correctly).\n\n>> @@ -2299,13 +2299,20 @@ static char *xdg_config_home_systemd(const char *filename)\n>>       return xdg_config_home_for(\"systemd/user\", filename);\n>>   }\n>>   -static int systemd_timer_write_unit_templates(const char *exec_path)\n>> +static int systemd_timer_write_unit_template(enum schedule_priority schedule,\n> \n> As we're not writing template files any more I think we should rename this to systemd_timer_write_unit_file()\n\nGood point. I have adjusted all the names in my next version.\n\n>> +                         const char *exec_path,\n>> +                         int minute)\n>>   {\n>>       char *filename;\n>>       FILE *file;\n>>       const char *unit;\n>> +    char *schedule_pattern = NULL;\n>> +    const char *frequency = get_frequency(schedule);\n>> +    char *local_timer_name = xstrfmt(\"git-maintenance@%s.timer\", frequency);\n> \n> The \"@\" in the name signifies that it is a template unit which it isn't anymore so I think we want to change this to \"git-maintenance-%s.timer\"\n\nI'll also take your SYSTEMD_UNIT_FORMAT macro suggestion to simplify things.\n \n>> +    char *local_service_name = xstrfmt(\"git-maintenance@%s.service\", frequency);\n> \n> Same change to the name here. I wonder if we could still use a template service file but that would complicate the implementation as we'd need to write three timer files but only one service file.\n\nSince we execute systemctl only passing the .timer filename, I think we'd\nneed to keep the '@' symbol in the name if we wanted to use .schedule\ntemplates. Best to keep things simple and abandon templates completely.\n\n>> [...]\n>> @@ -2375,13 +2399,16 @@ static int systemd_timer_write_unit_templates(const char *exec_path)\n>>       return 0;\n>>     error:\n>> +    free(schedule_pattern);\n>> +    free(local_timer_name);\n>>       free(filename);\n>> -    systemd_timer_delete_unit_templates();\n> \n> This looks like a change in behavior as previously we'd remove any files if there was an error rather than leaving behind a timer file without a corresponding unit file.\n\nThe callers take care of deleting the unit files, but there was one place\nwhere a short-circuit could have avoided this deletion. I'll clean that up.\n\n> Looking at maintenance_start() we call maintenance_register() which disables \"gc --auto\" before we get to this point so if we fail to write the files we'll end up disabling any form of gc in the repository.\n\nAdding this to the list of cleanups at the end. Thanks.\n\nI appreciate the careful review!\n\nThanks,\n-Stolee\n\n"},{"id":"480305","messageId":"2e9b32a8-8905-4a1f-a75b-ca747ae95b96@github.com","threadId":"60077","inReplyTo":"d247ce48-e2ba-4915-8d96-1fa2f693909d@github.com","subject":"Re: [PATCH 6/6] maintenance: use random minute in systemd scheduler","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2023-08-08T17:14:57Z","receivedAt":"2023-08-08T18:56:28Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 8/8/2023 1:06 PM, Derrick Stolee wrote:\n> On 8/8/2023 8:08 AM, Phillip Wood wrote:\n>> On 07/08/2023 19:51, Derrick Stolee via GitGitGadget wrote:\n\n>>> +    char *local_timer_name = xstrfmt(\"git-maintenance@%s.timer\", frequency);\n>>\n>> The \"@\" in the name signifies that it is a template unit which it isn't anymore so I think we want to change this to \"git-maintenance-%s.timer\"\n> \n> I'll also take your SYSTEMD_UNIT_FORMAT macro suggestion to simplify things.\n\nAs I was checking things, it turns out that we _should_ keep the '@' symbol\nif only to make sure that our new schedule overwrites the old schedule.\n\nThe alternative is that we manually try to delete the old schedule, but that\nfeels like an inefficient way to do it, leaving some cruft around long-term.\n\nFor completeness, here is what I did to check:\n\n$ systemctl --user list-timers\nNEXT LEFT LAST PASSED UNIT ACTIVATES \nTue 2023-08-08 13:13:00 EDT 6s left n/a n/a git-maintenance-hourly.timer git-maintenance-hourly.service\nTue 2023-08-08 13:50:00 EDT 37min left Tue 2023-08-08 12:50:10 EDT 22min ago git-maintenance@hourly.timer git-maintenance@hourly.service\nWed 2023-08-09 00:13:00 EDT 11h left n/a n/a git-maintenance-daily.timer git-maintenance-daily.service\nWed 2023-08-09 00:50:00 EDT 11h left Tue 2023-08-08 09:35:31 EDT 3h 37min ago git-maintenance@daily.timer git-maintenance@daily.service\nMon 2023-08-14 00:13:00 EDT 5 days left n/a n/a git-maintenance-weekly.timer git-maintenance-weekly.service\nMon 2023-08-14 00:50:00 EDT 5 days left Mon 2023-08-07 10:28:10 EDT 1 day 2h ago git-maintenance@weekly.timer git-maintenance@weekly.service\n\nDo you have an alternative idea how to handle that?\n\nThanks,\n-Stolee\n"},{"id":"480310","messageId":"8854e369-fabb-4044-a06c-eaf5b9fbde4a@github.com","threadId":"60077","inReplyTo":"ZNFgIyuhlNd8I9Y2@nand.local","subject":"Re: [PATCH 1/6] maintenance: add get_random_minute()","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2023-08-08T17:28:50Z","receivedAt":"2023-08-08T19:25:23Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 8/7/2023 5:20 PM, Taylor Blau wrote:\n> On Mon, Aug 07, 2023 at 06:51:35PM +0000, Derrick Stolee via GitGitGadget wrote:\n\n>> +\tif (!random_initialized) {\n>> +\t\tsrand((unsigned int)getpid());\n>> +\t\trandom_initialized = 1;\n>> +\t}\n> \n> I was wondering where else we call srand() within Git, and it looks like\n> the only other spot is in `lock_file_timeout()`.\n> \n> I doubt it, but is there a chance that that code depends on only calling\n> srand() once? I think the answer is \"no\", since we only use rand()\n> within that function to generate a random-ish backoff period, so I think\n> the repeatability of it doesn't matter all that much.\n\nThe main point would be to avoid the cost of spinning up the number\ngenerator, but there is potential for a bug if we run both initializers:\nthe lockfile would re-use the same filenames after the generator is\nreset.\n\n> So I think this is kind of outside the scope of your series, but I\n> wonder if we should have a git_rand() that automatically initializes the\n> PRNG with the value of getpid()? Then multiple callers can grab random\n> values at will without reinitializing the PRNG.\n\nI see you're moving ahead with removing the srand() from the lockfile code,\nso I'll focus on creating a `git_rand()` that centralizes the use of\nsrand(), but won't touch the code in the lockfile so your patch applies\nindependently.\n\nThanks,\n-Stolee\n"},{"id":"480315","messageId":"xmqqsf8t8oo5.fsf@gitster.g","threadId":"60077","inReplyTo":"xmqqbkfha3xu.fsf@gitster.g","subject":"Re: [PATCH 1/6] maintenance: add get_random_minute()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-08-08T16:49:30Z","receivedAt":"2023-08-08T19:46:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Provided if code simplification is a good enough rationale, the\n> patch looks sensible, but I find its use of u64 a bit questionable\n> (though not wrong). I would have expected that the type of \"rand\"\n> would be the same as backoff_ms and wait_ms, two variables involved\n> in the same expression.\n\nAh, not so fast. The use of modulo means we need to be careful\nabout about the fuzzing factor going negative, and use of unsigned\ntype allows us to forget about it.\n\n(fuzz % 250), when fuzz is of a signed random integral type, ranges\nbetween -250 and +250 and because we want the center of our\ndistribution at 1000, so I think the following is equivalent.\n\n lockfile.c | 7 +++----\n 1 file changed, 3 insertions(+), 4 deletions(-)\n\ndiff --git c/lockfile.c w/lockfile.c\nindex 6587d407f4..7de53526ac 100644\n--- c/lockfile.c\n+++ w/lockfile.c\n@@ -115,9 +115,8 @@ static int lock_file_timeout(struct lock_file *lk, const char *path,\n \t\tremaining_ms = timeout_ms;\n \n \twhile (1) {\n-\t\tlong backoff_ms, wait_ms;\n+\t\tlong backoff_ms, wait_ms, fuzz;\n \t\tint fd;\n-\t\tuint64_t rand;\n \n \t\tfd = lock_file(lk, path, flags, mode);\n \n@@ -130,10 +129,10 @@ static int lock_file_timeout(struct lock_file *lk, const char *path,\n \n \t\tbackoff_ms = multiplier * INITIAL_BACKOFF_MS;\n \t\t/* back off for between 0.75*backoff_ms and 1.25*backoff_ms */\n-\t\tif (csprng_bytes(&rand, sizeof(uint64_t)) < 0)\n+\t\tif (csprng_bytes(&fuzz, sizeof(fuzz)) < 0)\n \t\t\treturn error_errno(_(\"unable to get random bytes for\"\n \t\t\t\t\t \"lockfile backoff\"));\n-\t\twait_ms = (750 + rand % 500) * backoff_ms / 1000;\n+\t\twait_ms = (1000 + fuzz % 250) * backoff_ms / 1000;\n \t\tsleep_millisec(wait_ms);\n \t\tremaining_ms -= wait_ms;\n \n\n\n"},{"id":"480328","messageId":"ZNKfKs1mLQhnybvF@nand.local","threadId":"60077","inReplyTo":"xmqqsf8t8oo5.fsf@gitster.g","subject":"Re: [PATCH 1/6] maintenance: add get_random_minute()","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-08-08T20:01:46Z","receivedAt":"2023-08-08T20:34:57Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Aug 08, 2023 at 09:49:30AM -0700, Junio C Hamano wrote:\n> Junio C Hamano writes:\n>\n> > Provided if code simplification is a good enough rationale, the\n> > patch looks sensible, but I find its use of u64 a bit questionable\n> > (though not wrong). I would have expected that the type of \"rand\"\n> > would be the same as backoff_ms and wait_ms, two variables involved\n> > in the same expression.\n>\n> Ah, not so fast. The use of modulo means we need to be careful\n> about about the fuzzing factor going negative, and use of unsigned\n> type allows us to forget about it.\n>\n> (fuzz % 250), when fuzz is of a signed random integral type, ranges\n> between -250 and +250 and because we want the center of our\n> distribution at 1000, so I think the following is equivalent.\n\n[...]\n\n> -\t\twait_ms = (750 + rand % 500) * backoff_ms / 1000;\n> +\t\twait_ms = (1000 + fuzz % 250) * backoff_ms / 1000;\n\nI was going to say that having rand be an unsigned type was the right\nbehavior, but with this change it can (and should) be signed. Thanks for\nthe simplification :-).\n\nThanks,\nTaylor\n"},{"id":"480329","messageId":"ZNKfxOWJAuJ5DxAN@nand.local","threadId":"60077","inReplyTo":"8854e369-fabb-4044-a06c-eaf5b9fbde4a@github.com","subject":"Re: [PATCH 1/6] maintenance: add get_random_minute()","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-08-08T20:04:20Z","receivedAt":"2023-08-08T20:37:24Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Aug 08, 2023 at 01:28:50PM -0400, Derrick Stolee wrote:\n> > So I think this is kind of outside the scope of your series, but I\n> > wonder if we should have a git_rand() that automatically initializes the\n> > PRNG with the value of getpid()? Then multiple callers can grab random\n> > values at will without reinitializing the PRNG.\n>\n> I see you're moving ahead with removing the srand() from the lockfile code,\n> so I'll focus on creating a `git_rand()` that centralizes the use of\n> srand(), but won't touch the code in the lockfile so your patch applies\n> independently.\n\nThat thread may have progressed a little since you last looked at it.\n\nInstead of using srand() and rand() (which would make sense to wrap with\ngit_rand() as you propose), we can simplify our lives by using a CSPRNG,\nwhich only gets initialized once, as is already the case with\ncsprng_bytes().\n\nI think Junio is picking up a lightly modified version of my patch\nthere, see [1].\n\nThanks,\nTaylor\n\n[1]: https://lore.kernel.org/git/ZNKfKs1mLQhnybvF@nand.local/\n"},{"id":"480330","messageId":"ZNKgBMg+bU4k+9fU@nand.local","threadId":"60077","inReplyTo":"2db943c3-2ec2-4b7e-a161-a7c9b3f04d36@github.com","subject":"Re: [PATCH 6/6] maintenance: use random minute in systemd scheduler","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-08-08T20:05:24Z","receivedAt":"2023-08-08T20:37:39Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Aug 08, 2023 at 09:49:40AM -0400, Derrick Stolee wrote:\n> > But I figured I'd raise the question in case you did mean for them to\n> > both run on either Sunday or Monday.\n>\n> I don't intend to change the day that is run as part of this change.\n>\n> I think all other schedulers run on Sunday, but systemd running on Monday\n> is a particular detail of its \"weekly\" schedule.\n\nNo problem -- I figured that you didn't intend on changing the days\naround here, just wanted to make sure it was known that the systemd\nscheduler picks a different day of the week for its weekly schedule than\nthe others do.\n\n> >> -static int systemd_timer_write_unit_templates(const char *exec_path)\n> >> +static int systemd_timer_write_unit_template(enum schedule_priority schedule,\n> >> +\t\t\t\t\t const char *exec_path,\n> >> +\t\t\t\t\t int minute)\n> >> {\n> >> \tchar *filename;\n> >> \tFILE *file;\n> >> \tconst char *unit;\n> >> +\tchar *schedule_pattern = NULL;\n> >\n> > You should be able to drop the NULL initialization, since you assign\n> > this value unconditionally in the switch statement below (or BUG() on an\n> > unknown schedule type).\n>\n> Unfortunately, GCC complained about a possibly-unassigned value when I\n> left this unset during development, so this actually is necessary for\n> that compiler.\n\nAh, I would have thought that GCC would be smart enough to figure out\nthat schedule_pattern is unconditionally initialized via the switch\nstatement, but I guess not. Makes sense.\n\nThanks,\nTaylor\n"},{"id":"480351","messageId":"6f469118-8a7e-0b8d-33e3-c9ef486a262f@gmail.com","threadId":"60077","inReplyTo":"2e9b32a8-8905-4a1f-a75b-ca747ae95b96@github.com","subject":"Re: [PATCH 6/6] maintenance: use random minute in systemd scheduler","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-08-09T10:00:44Z","receivedAt":"2023-08-09T10:00:49Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 08/08/2023 18:14, Derrick Stolee wrote:\n> On 8/8/2023 1:06 PM, Derrick Stolee wrote:\n>> On 8/8/2023 8:08 AM, Phillip Wood wrote:\n>>> On 07/08/2023 19:51, Derrick Stolee via GitGitGadget wrote:\n> \n>>>> +    char *local_timer_name = xstrfmt(\"git-maintenance@%s.timer\", frequency);\n>>>\n>>> The \"@\" in the name signifies that it is a template unit which it isn't anymore so I think we want to change this to \"git-maintenance-%s.timer\"\n>>\n>> I'll also take your SYSTEMD_UNIT_FORMAT macro suggestion to simplify things.\n> \n> As I was checking things, it turns out that we _should_ keep the '@' symbol\n> if only to make sure that our new schedule overwrites the old schedule.\n\nOh, so if the user already has scheduled maintenance set up then running \n\"git maintenance start\" adds a new set of timers. I'd not though about that.\n\n> The alternative is that we manually try to delete the old schedule, but that\n> feels like an inefficient way to do it, leaving some cruft around long-term.\n\nThis patch still changes the names of the files we write. Currently we write\n\n\t$XDG_CONFIG_HOME/systemd/user/git-maintenance@.service\n\t$XDG_CONFIG_HOME/systemd/user/git-maintenance@.timer\n\nand this patch changes that to\n\n\t$XDG_CONFIG_HOME/systemd/user/git-maintenance@hourly.service\n\t$XDG_CONFIG_HOME/systemd/user/git-maintenance@daily.service\n\t$XDG_CONFIG_HOME/systemd/user/git-maintenance@weekly.service\n\t$XDG_CONFIG_HOME/systemd/user/git-maintenance@hourly.timer\n\t$XDG_CONFIG_HOME/systemd/user/git-maintenance@daily.timer\n\t$XDG_CONFIG_HOME/systemd/user/git-maintenance@weekly.timer\n\nIf the user has already enabled maintenance then\n\n\t$XDG_CONFIG_HOME/systemd/user/timers.target.wants/git-maintenance@hourly.timer\n\t$XDG_CONFIG_HOME/systemd/user/timers.target.wants/git-maintenance@daily.timer\n\t$XDG_CONFIG_HOME/systemd/user/timers.target.wants/git-maintenance@weekly.timer\n\nwill exist and are all symlinks pointing to\n\n\t$XDG_CONFIG_HOME/systemd/user/git-maintenance@.timer\n\nAfter this patch if the user runs \"git maintenance start\" again then \nsystemctl will update the symlinks tot point to the matching unit files \nrather than the old template file. That means the user will pick up the \nnew schedule but we leave behind the original files that are unused.\n\n> For completeness, here is what I did to check:\n> \n> $ systemctl --user list-timers\n> NEXT LEFT LAST PASSED UNIT ACTIVATES\n> Tue 2023-08-08 13:13:00 EDT 6s left n/a n/a git-maintenance-hourly.timer git-maintenance-hourly.service\n> Tue 2023-08-08 13:50:00 EDT 37min left Tue 2023-08-08 12:50:10 EDT 22min ago git-maintenance@hourly.timer git-maintenance@hourly.service\n> Wed 2023-08-09 00:13:00 EDT 11h left n/a n/a git-maintenance-daily.timer git-maintenance-daily.service\n> Wed 2023-08-09 00:50:00 EDT 11h left Tue 2023-08-08 09:35:31 EDT 3h 37min ago git-maintenance@daily.timer git-maintenance@daily.service\n> Mon 2023-08-14 00:13:00 EDT 5 days left n/a n/a git-maintenance-weekly.timer git-maintenance-weekly.service\n> Mon 2023-08-14 00:50:00 EDT 5 days left Mon 2023-08-07 10:28:10 EDT 1 day 2h ago git-maintenance@weekly.timer git-maintenance@weekly.service\n> \n> Do you have an alternative idea how to handle that?\n\nI think we should stick with the names as you have them. It might be \nworth keeping the service file as a template so we only write the new \ntimer files and have a reason to use the \"@\" naming scheme. We could \nupdate systemd_timer_setup_units() to delete git-maintenance@.timer if \nwe successfully enable the new timer unit files.\n\nSorry for the confusion, I should have thought about the user running \n\"git maintenance start\" for a second time.\n\nBest Wishes\n\nPhillip\n"},{"id":"480352","messageId":"44b7c1d7-83f3-c549-aa5f-2f06276ba599@gmail.com","threadId":"60077","inReplyTo":"5bb4313a-01f3-4dea-b724-fb41a8def334@github.com","subject":"Re: [PATCH 6/6] maintenance: use random minute in systemd scheduler","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-08-09T10:03:15Z","receivedAt":"2023-08-09T10:03:20Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 08/08/2023 18:24, Derrick Stolee wrote:\n> On 8/8/2023 9:56 AM, Derrick Stolee wrote:\n>> On 8/8/2023 5:53 AM, Phillip Wood wrote:> Hi Stolee\n> \n>>> So only one of these jobs will succeed. The cron entries are careful to\n>>> only run one job at a time, I think it would be worth doing the same\n>>> thing here. I think the using the following format strings would fix this.\n>>>\n>>> Hourly: \"Tue..Sun *-*-* 1..23:00:%02d\"\n>>> Daily: \"Tue..Sun *-*-* 00:00:%02d\"\n>>> Weekly: \"Mon *-*-* 00:00:%02d\"\n>>\n>> I would modify this with dropping the \"Tue..Sun\" from the hourly schedule,\n>> as we still want 23 runs on Mondays.\n\nOops, well spotted\n\n>>> It looks like the launchctl schedule has the same issue.\n>>\n>> I will take a look at this and consider some additional patches to correct\n>> these issues across both schedulers. Thank you for the attention to detail!\n> \n> Taking a look, it seems that launchctl does not have this same problem.\n> \n> The schedule is set via an of s as follows:\n> \n> \tcase SCHEDULE_HOURLY:\n> \t\trepeat = \"\\n\"\n> \t\t\t \"Hour%d\\n\"\n> \t\t\t \"Minute%d\\n\"\n> \t\t\t \"\\n\";\n> \t\tfor (i = 1; i <= 23; i++)\n> \t\t\tstrbuf_addf(&plist, repeat, i, minute);\n> \t\tbreak;\n> \n> \tcase SCHEDULE_DAILY:\n> \t\trepeat = \"\\n\"\n> \t\t\t \"Day%d\\n\"\n> \t\t\t \"Hour0\\n\"\n> \t\t\t \"Minute%d\\n\"\n> \t\t\t \"\\n\";\n> \t\tfor (i = 1; i <= 6; i++)\n> \t\t\tstrbuf_addf(&plist, repeat, i, minute);\n> \t\tbreak;\n> \n> This means that we create an hourly schedule for each hour 1..23\n> (no 0th hour means no collision with daily or weekly schedule) and\n> a daily schedule for each day 1..6 (no 0th day means no collision\n> with the weekly schedule).\n> \n> Does this match your understanding?\n\nYes, having read it again - sorry I'd misunderstood it yesterday.\n\n> The overlap _definitely_ exists in systemd, which I will fix.\n\nThat's great\n\nBest Wishes\n\nPhillip\n\n"},{"id":"480355","messageId":"a1e7d730-e220-48ec-8393-2d3538b80163@github.com","threadId":"60077","inReplyTo":"ZNKfxOWJAuJ5DxAN@nand.local","subject":"Re: [PATCH 1/6] maintenance: add get_random_minute()","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2023-08-09T12:17:37Z","receivedAt":"2023-08-09T12:17:48Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 8/8/2023 4:04 PM, Taylor Blau wrote:\n> On Tue, Aug 08, 2023 at 01:28:50PM -0400, Derrick Stolee wrote:\n\n>> I see you're moving ahead with removing the srand() from the lockfile code,\n \n> That thread may have progressed a little since you last looked at it.\n\nI think this part of my summary is still correct.\n\n>> so I'll focus on creating a `git_rand()` that centralizes the use of\n>> srand(), but won't touch the code in the lockfile so your patch applies\n>> independently.\n \n> Instead of using srand() and rand() (which would make sense to wrap with\n> git_rand() as you propose), we can simplify our lives by using a CSPRNG,\n> which only gets initialized once, as is already the case with\n> csprng_bytes().\n\nSo the idea is to use csprng_bytes() everywhere instead of srand()/rand().\n\nI can adjust my local patch to still create git_rand(), but base it on\ncsprng_bytes() and not collide with your patch. Mimicking rand()'s behavior\nis a simpler interface to consume.\n\nThanks,\n-Stolee\n"},{"id":"480386","messageId":"xmqq7cq4nj7l.fsf@gitster.g","threadId":"60077","inReplyTo":"a1e7d730-e220-48ec-8393-2d3538b80163@github.com","subject":"Re: [PATCH 1/6] maintenance: add get_random_minute()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-08-09T18:50:38Z","receivedAt":"2023-08-09T18:50:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Derrick Stolee writes:\n\n>> Instead of using srand() and rand() (which would make sense to wrap with\n>> git_rand() as you propose), we can simplify our lives by using a CSPRNG,\n>> which only gets initialized once, as is already the case with\n>> csprng_bytes().\n>\n> So the idea is to use csprng_bytes() everywhere instead of srand()/rand().\n>\n> I can adjust my local patch to still create git_rand(), but base it on\n> csprng_bytes() and not collide with your patch. Mimicking rand()'s behavior\n> is a simpler interface to consume.\n\nI am still ambivalent about wasting entropy for something that\nsrand() would suffice, so git_rand() interface may be an welcome\naddition anyway, that serves an extra layer of indirection to\ninsulate the callers from the implementation.\n\nThanks.\n"},{"id":"480398","messageId":"ZNP4b8/QF73F3ySU@nand.local","threadId":"60077","inReplyTo":"xmqq7cq4nj7l.fsf@gitster.g","subject":"Re: [PATCH 1/6] maintenance: add get_random_minute()","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-08-09T20:34:55Z","receivedAt":"2023-08-09T20:35:01Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Aug 09, 2023 at 11:50:38AM -0700, Junio C Hamano wrote:\n> Derrick Stolee writes:\n>\n> >> Instead of using srand() and rand() (which would make sense to wrap with\n> >> git_rand() as you propose), we can simplify our lives by using a CSPRNG,\n> >> which only gets initialized once, as is already the case with\n> >> csprng_bytes().\n> >\n> > So the idea is to use csprng_bytes() everywhere instead of srand()/rand().\n> >\n> > I can adjust my local patch to still create git_rand(), but base it on\n> > csprng_bytes() and not collide with your patch. Mimicking rand()'s behavior\n> > is a simpler interface to consume.\n>\n> I am still ambivalent about wasting entropy for something that\n> srand() would suffice, so git_rand() interface may be an welcome\n> addition anyway, that serves an extra layer of indirection to\n> insulate the callers from the implementation.\n\nSounds good to me, I'm not particularly attached to one implementation\nover another. Thanks, both.\n\nThanks,\nTaylor\n"},{"id":"480473","messageId":"edc08023ed51890f0390aacf783d7213e82704a7.1691699987.git.gitgitgadget@gmail.com","threadId":"60077","inReplyTo":"pull.1567.v2.git.1691699987.gitgitgadget@gmail.com","subject":"[PATCH v2 1/8] maintenance: add get_random_minute()","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-08-10T20:39:40Z","receivedAt":"2023-08-10T20:39:53Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nWhen we initially created background maintenance -- with its hourly,\ndaily, and weekly schedules -- we considered the effects of all clients\nlaunching fetches to the server every hour on the hour. The worry of\nDDoSing server hosts was noted, but left as something we would consider\nfor a future update.\n\nAs background maintenance has gained more adoption over the past three\nyears, our worries about DDoSing the big Git hosts has been unfounded.\nThose systems, especially those serving public repositories, are already\nresilient to thundering herds of much smaller scale.\n\nHowever, sometimes organizations spin up specific custom server\ninfrastructure either in addition to or on top of their Git host. Some\nof these technologies are built for a different range of scale, and can\nhit concurrency limits sooner. Organizations with such custom\ninfrastructures are more likely to recommend tools like `scalar` which\nfurthers their adoption of background maintenance.\n\nTo help solve for this, create get_random_minute() as a method to help\nGit select a random minute when creating schedules in the future. The\nintegrations with this method do not yet exist, but will follow in\nfuture changes.\n\nTo avoid multiple sources of randomness in the Git codebase, create a\nnew helper function, git_rand(), that returns a random uint32_t. This is\nsimilar to how rand() returns a random nonnegative value, except it is\nbased on csprng_bytes() which is cryptographic and will return values\nlarger than RAND_MAX.\n\nOne thing that is important for testability is that we notice when we\nare under a test scenario and return a predictable result. The schedules\nthemselves are not checked for this value, but at least one launchctl\ntest checks that we do not unnecessarily reboot the schedule if it has\nnot changed from a previous version.\n\nSigned-off-by: Derrick Stolee \n---\n builtin/gc.c | 10 ++++++++++\n wrapper.c | 10 ++++++++++\n wrapper.h | 6 ++++++\n 3 files changed, 26 insertions(+)\n\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex 19d73067aad..2ebae7bc17c 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -1708,6 +1708,16 @@ static int get_schedule_cmd(const char **cmd, int *is_available)\n \treturn 1;\n }\n \n+MAYBE_UNUSED\n+static int get_random_minute(void)\n+{\n+\t/* Use a static value when under tests. */\n+\tif (getenv(\"GIT_TEST_MAINT_SCHEDULER\"))\n+\t\treturn 13;\n+\n+\treturn git_rand() % 60;\n+}\n+\n static int is_launchctl_available(void)\n {\n \tconst char *cmd = \"launchctl\";\ndiff --git a/wrapper.c b/wrapper.c\nindex 5160c9e28de..48065c4f533 100644\n--- a/wrapper.c\n+++ b/wrapper.c\n@@ -819,3 +819,13 @@ int csprng_bytes(void *buf, size_t len)\n \treturn 0;\n #endif\n }\n+\n+uint32_t git_rand(void)\n+{\n+\tuint32_t result;\n+\n+\tif (csprng_bytes(&result, sizeof(result)) < 0)\n+\t\tdie(_(\"unable to get random bytes\"));\n+\n+\treturn result;\n+}\ndiff --git a/wrapper.h b/wrapper.h\nindex 79a9c1b5077..79c7321bb37 100644\n--- a/wrapper.h\n+++ b/wrapper.h\n@@ -139,4 +139,10 @@ void sleep_millisec(int millisec);\n */\n int csprng_bytes(void *buf, size_t len);\n \n+/*\n+ * Returns a random uint32_t, uniformly distributed across all possible\n+ * values.\n+ */\n+uint32_t git_rand(void);\n+\n #endif /* WRAPPER_H */\n-- \ngitgitgadget\n\n"},{"id":"480474","messageId":"72ec86f2f882a575544aef71517b0727f2510a0e.1691699987.git.gitgitgadget@gmail.com","threadId":"60077","inReplyTo":"pull.1567.v2.git.1691699987.gitgitgadget@gmail.com","subject":"[PATCH v2 2/8] maintenance: use random minute in launchctl scheduler","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-08-10T20:39:41Z","receivedAt":"2023-08-10T20:39:55Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nThe get_random_minute() method was created to allow maintenance\nschedules to be fixed to a random minute of the hour. This randomness is\nonly intended to spread out the load from a number of clients, but each\nclient should have an hour between each maintenance cycle.\n\nUse get_random_minute() when constructing the schedules for launchctl.\n\nThe format already includes a 'Minute' key which is modified from 0 to\nthe random minute.\n\nSigned-off-by: Derrick Stolee \n---\n builtin/gc.c | 23 ++++++++++++-----------\n 1 file changed, 12 insertions(+), 11 deletions(-)\n\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex 2ebae7bc17c..286ce131a5c 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -1708,7 +1708,6 @@ static int get_schedule_cmd(const char **cmd, int *is_available)\n \treturn 1;\n }\n \n-MAYBE_UNUSED\n static int get_random_minute(void)\n {\n \t/* Use a static value when under tests. */\n@@ -1830,6 +1829,7 @@ static int launchctl_schedule_plist(const char *exec_path, enum schedule_priorit\n \tstruct strbuf plist = STRBUF_INIT, plist2 = STRBUF_INIT;\n \tstruct stat st;\n \tconst char *cmd = \"launchctl\";\n+\tint minute = get_random_minute();\n \n \tget_schedule_cmd(&cmd, NULL);\n \tpreamble = \"\\n\"\n@@ -1855,29 +1855,30 @@ static int launchctl_schedule_plist(const char *exec_path, enum schedule_priorit\n \tcase SCHEDULE_HOURLY:\n \t\trepeat = \"\\n\"\n \t\t\t \"Hour%d\\n\"\n-\t\t\t \"Minute0\\n\"\n+\t\t\t \"Minute%d\\n\"\n \t\t\t \"\\n\";\n \t\tfor (i = 1; i <= 23; i++)\n-\t\t\tstrbuf_addf(&plist, repeat, i);\n+\t\t\tstrbuf_addf(&plist, repeat, i, minute);\n \t\tbreak;\n \n \tcase SCHEDULE_DAILY:\n \t\trepeat = \"\\n\"\n \t\t\t \"Day%d\\n\"\n \t\t\t \"Hour0\\n\"\n-\t\t\t \"Minute0\\n\"\n+\t\t\t \"Minute%d\\n\"\n \t\t\t \"\\n\";\n \t\tfor (i = 1; i <= 6; i++)\n-\t\t\tstrbuf_addf(&plist, repeat, i);\n+\t\t\tstrbuf_addf(&plist, repeat, i, minute);\n \t\tbreak;\n \n \tcase SCHEDULE_WEEKLY:\n-\t\tstrbuf_addstr(&plist,\n-\t\t\t \"\\n\"\n-\t\t\t \"Day0\\n\"\n-\t\t\t \"Hour0\\n\"\n-\t\t\t \"Minute0\\n\"\n-\t\t\t \"\\n\");\n+\t\tstrbuf_addf(&plist,\n+\t\t\t \"\\n\"\n+\t\t\t \"Day0\\n\"\n+\t\t\t \"Hour0\\n\"\n+\t\t\t \"Minute%d\\n\"\n+\t\t\t \"\\n\",\n+\t\t\t minute);\n \t\tbreak;\n \n \tdefault:\n-- \ngitgitgadget\n\n"},{"id":"480475","messageId":"pull.1567.v2.git.1691699987.gitgitgadget@gmail.com","threadId":"60077","inReplyTo":"pull.1567.git.1691434300.gitgitgadget@gmail.com","subject":"[PATCH v2 0/8] maintenance: schedule maintenance on a random minute","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-08-10T20:39:39Z","receivedAt":"2023-08-10T20:39:56Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"When we initially created background maintenance -- with its hourly, daily,\nand weekly schedules -- we considered the effects of all clients launching\nfetches to the server every hour on the hour. The worry of DDoSing server\nhosts was noted, but left as something we would consider for a future\nupdate.\n\nAs background maintenance has gained more adoption over the past three\nyears, our worries about DDoSing the big Git hosts has been unfounded. Those\nsystems, especially those serving public repositories, are already resilient\nto thundering herds of much smaller scale.\n\nHowever, sometimes organizations spin up specific custom server\ninfrastructure either in addition to or on top of their Git host. Some of\nthese technologies are built for a different range of scale, and can hit\nconcurrency limits sooner. Organizations with such custom infrastructures\nare more likely to recommend tools like scalar which furthers their adoption\nof background maintenance.\n\nThis series attempts to help by spreading out background maintenance to a\nrandom minute of the hour. This minute is selected during git maintenance\nstart, and the same minute applies to each of the three schedules.\n\nThis isn't a full solution to this problem, as the custom infrastructure\nneeds to be resilient to bursts of activity, but at least this will help\nsomewhat.\n\nEach of the integrated schedulers needs a different way of integrating the\nrandom minute. The most problematic is systemd, since our integration had a\nclever use of templates to write one schedule that inserted the hourly,\ndaily, and weekly schedules as a string into the template. This needs some\nrefactoring before the custom minute could be inserted.\n\nFor the most part, each scheduler's integration is relatively simple. That\nis, until we get to the systemd integration. That integration made use of a\nclever templating technique that is no longer possible when making this\nadjustment.\n\nPatches 5-7 involve systemd, though patch 5 is just a move of code (without\nedits) to make the diff in patch 6 somewhat simpler (it's still complicated\ndue to templating changes). Patch 7 fixes an issue where the systemd\nschedules overlap.\n\nPatch 8 fixes an issue where config changes persist even if the scheduler\nfails to initialize. Thanks for noticing, Philip!\n\n\nUpdates in version 2\n====================\n\n * get_random_minute() now uses a new helper, git_rand(), which is itself a\n wrapper around csprng_bytes() for easier use.\n * get_random_minute() also had an error in its use of getenv() which is now\n fixed.\n * Patch 6 has a lot of new changes, including:\n * Keeping the .service template.\n * Deleting the old .timer template when safe to do so.\n * Patch 7 fixes the schedule overlap in systemd.\n * Patch 8 fixes the issue where 'mainteancne.auto=false' would persist even\n if the scheduler failed to initialize.\n\nThanks, -Stolee\n\nDerrick Stolee (8):\n maintenance: add get_random_minute()\n maintenance: use random minute in launchctl scheduler\n maintenance: use random minute in Windows scheduler\n maintenance: use random minute in cron scheduler\n maintenance: swap method locations\n maintenance: use random minute in systemd scheduler\n maintenance: fix systemd schedule overlaps\n maintenance: update schedule before config\n\n builtin/gc.c | 291 +++++++++++++++++++++++++++++------------\n t/t7900-maintenance.sh | 28 +++-\n wrapper.c | 10 ++\n wrapper.h | 6 +\n 4 files changed, 250 insertions(+), 85 deletions(-)\n\n\nbase-commit: a82fb66fed250e16d3010c75404503bea3f0ab61\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1567%2Fderrickstolee%2Fmaintenance-random-minute-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1567/derrickstolee/maintenance-random-minute-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/1567\n\nRange-diff vs v1:\n\n 1: fefdaa94579 ! 1: edc08023ed5 maintenance: add get_random_minute()\n @@ Commit message\n integrations with this method do not yet exist, but will follow in\n future changes.\n \n + To avoid multiple sources of randomness in the Git codebase, create a\n + new helper function, git_rand(), that returns a random uint32_t. This is\n + similar to how rand() returns a random nonnegative value, except it is\n + based on csprng_bytes() which is cryptographic and will return values\n + larger than RAND_MAX.\n +\n One thing that is important for testability is that we notice when we\n are under a test scenario and return a predictable result. The schedules\n themselves are not checked for this value, but at least one launchctl\n @@ builtin/gc.c: static int get_schedule_cmd(const char **cmd, int *is_available)\n +MAYBE_UNUSED\n +static int get_random_minute(void)\n +{\n -+\tstatic int random_initialized = 0;\n -+\n +\t/* Use a static value when under tests. */\n -+\tif (!getenv(\"GIT_TEST_MAINTENANCE_SCHEDULER\"))\n ++\tif (getenv(\"GIT_TEST_MAINT_SCHEDULER\"))\n +\t\treturn 13;\n +\n -+\tif (!random_initialized) {\n -+\t\tsrand((unsigned int)getpid());\n -+\t\trandom_initialized = 1;\n -+\t}\n -+\n -+\treturn rand() % 60;\n ++\treturn git_rand() % 60;\n +}\n +\n static int is_launchctl_available(void)\n {\n \tconst char *cmd = \"launchctl\";\n +\n + ## wrapper.c ##\n +@@ wrapper.c: int csprng_bytes(void *buf, size_t len)\n + \treturn 0;\n + #endif\n + }\n ++\n ++uint32_t git_rand(void)\n ++{\n ++\tuint32_t result;\n ++\n ++\tif (csprng_bytes(&result, sizeof(result)) < 0)\n ++\t\tdie(_(\"unable to get random bytes\"));\n ++\n ++\treturn result;\n ++}\n +\n + ## wrapper.h ##\n +@@ wrapper.h: void sleep_millisec(int millisec);\n + */\n + int csprng_bytes(void *buf, size_t len);\n + \n ++/*\n ++ * Returns a random uint32_t, uniformly distributed across all possible\n ++ * values.\n ++ */\n ++uint32_t git_rand(void);\n ++\n + #endif /* WRAPPER_H */\n 2: babf62ec6d5 ! 2: 72ec86f2f88 maintenance: use random minute in launchctl scheduler\n @@ builtin/gc.c: static int get_schedule_cmd(const char **cmd, int *is_available)\n -MAYBE_UNUSED\n static int get_random_minute(void)\n {\n - \tstatic int random_initialized = 0;\n + \t/* Use a static value when under tests. */\n @@ builtin/gc.c: static int launchctl_schedule_plist(const char *exec_path, enum schedule_priorit\n \tstruct strbuf plist = STRBUF_INIT, plist2 = STRBUF_INIT;\n \tstruct stat st;\n 3: 1048dff1d3a = 3: f6d9c4f3b02 maintenance: use random minute in Windows scheduler\n 4: 3ef3cf0861f = 4: b291e6f7aec maintenance: use random minute in cron scheduler\n 5: 37f96b6b619 ! 5: 88610437b4b maintenance: swap method locations\n @@ Commit message\n systemd_timer_enable_unit() method, so we can write a specific schedule\n for each unit.\n \n - The diff is computed smaller by showing systemd_timer_enable_unit() move\n - instead of systemd_timer_write_unit_templates().\n + The diff is computed smaller by showing systemd_timer_enable_unit() and\n + systemd_timer_delete_units() move instead of\n + systemd_timer_write_unit_templates() and\n + systemd_timer_delete_unit_templates().\n \n Signed-off-by: Derrick Stolee \n \n @@ builtin/gc.c: static char *xdg_config_home_systemd(const char *filename)\n -\treturn 0;\n -}\n -\n --static int systemd_timer_delete_unit_templates(void)\n --{\n --\tint ret = 0;\n --\tchar *filename = xdg_config_home_systemd(\"git-maintenance@.timer\");\n --\tif (unlink(filename) && !is_missing_file_error(errno))\n --\t\tret = error_errno(_(\"failed to delete '%s'\"), filename);\n --\tFREE_AND_NULL(filename);\n --\n --\tfilename = xdg_config_home_systemd(\"git-maintenance@.service\");\n --\tif (unlink(filename) && !is_missing_file_error(errno))\n --\t\tret = error_errno(_(\"failed to delete '%s'\"), filename);\n --\n --\tfree(filename);\n --\treturn ret;\n --}\n --\n + static int systemd_timer_delete_unit_templates(void)\n + {\n + \tint ret = 0;\n +@@ builtin/gc.c: static int systemd_timer_delete_unit_templates(void)\n + \treturn ret;\n + }\n + \n -static int systemd_timer_delete_units(void)\n -{\n -\treturn systemd_timer_enable_unit(0, SCHEDULE_HOURLY) ||\n @@ builtin/gc.c: error:\n +\treturn 0;\n +}\n +\n -+static int systemd_timer_delete_unit_templates(void)\n -+{\n -+\tint ret = 0;\n -+\tchar *filename = xdg_config_home_systemd(\"git-maintenance@.timer\");\n -+\tif (unlink(filename) && !is_missing_file_error(errno))\n -+\t\tret = error_errno(_(\"failed to delete '%s'\"), filename);\n -+\tFREE_AND_NULL(filename);\n -+\n -+\tfilename = xdg_config_home_systemd(\"git-maintenance@.service\");\n -+\tif (unlink(filename) && !is_missing_file_error(errno))\n -+\t\tret = error_errno(_(\"failed to delete '%s'\"), filename);\n -+\n -+\tfree(filename);\n -+\treturn ret;\n -+}\n -+\n +static int systemd_timer_delete_units(void)\n +{\n +\treturn systemd_timer_enable_unit(0, SCHEDULE_HOURLY) ||\n 6: 14e340b75fa ! 6: e43778d3e40 maintenance: use random minute in systemd scheduler\n @@ Commit message\n \n In order to set these schedules to a given minute, we can no longer use\n the 'hourly', 'daily', or 'weekly' strings for '' and instead\n - need to abandon the template model.\n + need to abandon the template model for the .timer files. We can still\n + use templates for the .service files. For this reason, we split these\n + writes into two methods.\n \n Modify the template with a custom schedule in the 'OnCalendar' setting.\n This schedule has some interesting differences from cron-like patterns,\n @@ Commit message\n week. Monday is used since that matches the day used for the 'weekly'\n schedule used previously.\n \n + Now that the timer files are not templates, we might want to abandon the\n + '@' symbol in the file names. However, this would cause users with\n + existing schedules to get two competing schedules due to different\n + names. The work to remove the old schedule name is one thing that we can\n + avoid by keeping the '@' symbol in our unit names. Since we are locked\n + into this name, it makes sense that we keep the template model for the\n + .service files.\n +\n The rest of the change involves making sure we are writing these .timer\n and .service files before initializing the schedule with 'systemctl' and\n deleting the files when we are done. Some changes are also made to share\n the random minute along with a single computation of the execution path\n of the current Git executable.\n \n + In addition, older Git versions may have written a\n + 'git-maintenance@.timer' template file. Be sure to remove this when\n + successfully enabling maintenance (or disabling maintenance).\n +\n Signed-off-by: Derrick Stolee \n \n ## builtin/gc.c ##\n @@ builtin/gc.c: static char *xdg_config_home_systemd(const char *filename)\n \treturn xdg_config_home_for(\"systemd/user\", filename);\n }\n \n +-static int systemd_timer_delete_unit_templates(void)\n ++#define SYSTEMD_UNIT_FORMAT \"git-maintenance@%s.%s\"\n ++\n ++static int systemd_timer_delete_timer_file(enum schedule_priority priority)\n + {\n + \tint ret = 0;\n +-\tchar *filename = xdg_config_home_systemd(\"git-maintenance@.timer\");\n ++\tconst char *frequency = get_frequency(priority);\n ++\tchar *local_timer_name = xstrfmt(SYSTEMD_UNIT_FORMAT, frequency, \"timer\");\n ++\tchar *filename = xdg_config_home_systemd(local_timer_name);\n ++\n + \tif (unlink(filename) && !is_missing_file_error(errno))\n + \t\tret = error_errno(_(\"failed to delete '%s'\"), filename);\n +-\tFREE_AND_NULL(filename);\n + \n +-\tfilename = xdg_config_home_systemd(\"git-maintenance@.service\");\n ++\tfree(filename);\n ++\tfree(local_timer_name);\n ++\treturn ret;\n ++}\n ++\n ++static int systemd_timer_delete_service_template(void)\n ++{\n ++\tint ret = 0;\n ++\tchar *local_service_name = xstrfmt(SYSTEMD_UNIT_FORMAT, \"\", \"service\");\n ++\tchar *filename = xdg_config_home_systemd(local_service_name);\n + \tif (unlink(filename) && !is_missing_file_error(errno))\n + \t\tret = error_errno(_(\"failed to delete '%s'\"), filename);\n + \n + \tfree(filename);\n ++\tfree(local_service_name);\n + \treturn ret;\n + }\n + \n -static int systemd_timer_write_unit_templates(const char *exec_path)\n -+static int systemd_timer_write_unit_template(enum schedule_priority schedule,\n -+\t\t\t\t\t const char *exec_path,\n -+\t\t\t\t\t int minute)\n ++/*\n ++ * Write the schedule information into a git-maintenance@.timer\n ++ * file using a custom minute. This timer file cannot use the templating\n ++ * system, so we generate a specific file for each.\n ++ */\n ++static int systemd_timer_write_timer_file(enum schedule_priority schedule,\n ++\t\t\t\t\t int minute)\n {\n ++\tint res = -1;\n \tchar *filename;\n \tFILE *file;\n \tconst char *unit;\n +\tchar *schedule_pattern = NULL;\n +\tconst char *frequency = get_frequency(schedule);\n -+\tchar *local_timer_name = xstrfmt(\"git-maintenance@%s.timer\", frequency);\n -+\tchar *local_service_name = xstrfmt(\"git-maintenance@%s.service\", frequency);\n ++\tchar *local_timer_name = xstrfmt(SYSTEMD_UNIT_FORMAT, frequency, \"timer\");\n +\n +\tfilename = xdg_config_home_systemd(local_timer_name);\n \n @@ builtin/gc.c: static int systemd_timer_write_unit_templates(const char *exec_pat\n \t\tfclose(file);\n \t\tgoto error;\n @@ builtin/gc.c: static int systemd_timer_write_unit_templates(const char *exec_path)\n + \t\terror_errno(_(\"failed to flush '%s'\"), filename);\n + \t\tgoto error;\n \t}\n ++\n ++\tres = 0;\n ++\n ++error:\n ++\tfree(schedule_pattern);\n ++\tfree(local_timer_name);\n \tfree(filename);\n ++\treturn res;\n ++}\n \n -\tfilename = xdg_config_home_systemd(\"git-maintenance@.service\");\n ++/*\n ++ * No matter the schedule, we use the same service and can make use of the\n ++ * templating system. When installing git-maintenance@.timer,\n ++ * systemd will notice that git-maintenance@.service exists as a template\n ++ * and will use this file and insert the into the template at\n ++ * the position of \"%i\".\n ++ */\n ++static int systemd_timer_write_service_template(const char *exec_path)\n ++{\n ++\tint res = -1;\n ++\tchar *filename;\n ++\tFILE *file;\n ++\tconst char *unit;\n ++\tchar *local_service_name = xstrfmt(SYSTEMD_UNIT_FORMAT, \"\", \"service\");\n ++\n +\tfilename = xdg_config_home_systemd(local_service_name);\n ++\tif (safe_create_leading_directories(filename)) {\n ++\t\terror(_(\"failed to create directories for '%s'\"), filename);\n ++\t\tgoto error;\n ++\t}\n \tfile = fopen_or_warn(filename, \"w\");\n \tif (!file)\n \t\tgoto error;\n @@ builtin/gc.c: static int systemd_timer_write_unit_templates(const char *exec_path)\n - \t \"\\n\"\n - \t \"[Service]\\n\"\n - \t \"Type=oneshot\\n\"\n --\t \"ExecStart=\\\"%s/git\\\" --exec-path=\\\"%s\\\" for-each-repo --config=maintenance.repo maintenance run --schedule=%%i\\n\"\n -+\t \"ExecStart=\\\"%s/git\\\" --exec-path=\\\"%s\\\" for-each-repo --config=maintenance.repo maintenance run --schedule=%s\\n\"\n - \t \"LockPersonality=yes\\n\"\n - \t \"MemoryDenyWriteExecute=yes\\n\"\n - \t \"NoNewPrivileges=yes\\n\"\n -@@ builtin/gc.c: static int systemd_timer_write_unit_templates(const char *exec_path)\n - \t \"RestrictSUIDSGID=yes\\n\"\n - \t \"SystemCallArchitectures=native\\n\"\n - \t \"SystemCallFilter=@system-service\\n\";\n --\tif (fprintf(file, unit, exec_path, exec_path) < 0) {\n -+\tif (fprintf(file, unit, exec_path, exec_path, frequency) < 0) {\n - \t\terror(_(\"failed to write to '%s'\"), filename);\n - \t\tfclose(file);\n + \t\terror_errno(_(\"failed to flush '%s'\"), filename);\n \t\tgoto error;\n -@@ builtin/gc.c: static int systemd_timer_write_unit_templates(const char *exec_path)\n - \treturn 0;\n + \t}\n +-\tfree(filename);\n +-\treturn 0;\n ++\n ++\tres = 0;\n \n error:\n -+\tfree(schedule_pattern);\n -+\tfree(local_timer_name);\n ++\tfree(local_service_name);\n \tfree(filename);\n -\tsystemd_timer_delete_unit_templates();\n - \treturn -1;\n +-\treturn -1;\n ++\treturn res;\n }\n \n static int systemd_timer_enable_unit(int enable,\n -\t\t\t\t enum schedule_priority schedule)\n +\t\t\t\t enum schedule_priority schedule,\n -+\t\t\t\t const char *exec_path,\n +\t\t\t\t int minute)\n {\n \tconst char *cmd = \"systemctl\";\n @@ builtin/gc.c: static int systemd_timer_enable_unit(int enable,\n \t */\n \tif (!enable)\n \t\tchild.no_stderr = 1;\n -+\telse if (systemd_timer_write_unit_template(schedule, exec_path, minute))\n ++\telse if (systemd_timer_write_timer_file(schedule, minute))\n +\t\treturn -1;\n \n \tget_schedule_cmd(&cmd, NULL);\n \tstrvec_split(&child.args, cmd);\n + \tstrvec_pushl(&child.args, \"--user\", enable ? \"enable\" : \"disable\",\n + \t\t \"--now\", NULL);\n +-\tstrvec_pushf(&child.args, \"git-maintenance@%s.timer\", frequency);\n ++\tstrvec_pushf(&child.args, SYSTEMD_UNIT_FORMAT, frequency, \"timer\");\n + \n + \tif (start_command(&child))\n + \t\treturn error(_(\"failed to start systemctl\"));\n @@ builtin/gc.c: static int systemd_timer_enable_unit(int enable,\n \treturn 0;\n }\n \n --static int systemd_timer_delete_unit_templates(void)\n -+static int systemd_timer_delete_unit_template(enum schedule_priority priority)\n - {\n -+\tconst char *frequency = get_frequency(priority);\n -+\tchar *local_timer_name = xstrfmt(\"git-maintenance@%s.timer\", frequency);\n -+\tchar *local_service_name = xstrfmt(\"git-maintenance@%s.service\", frequency);\n - \tint ret = 0;\n --\tchar *filename = xdg_config_home_systemd(\"git-maintenance@.timer\");\n -+\tchar *filename = xdg_config_home_systemd(local_timer_name);\n - \tif (unlink(filename) && !is_missing_file_error(errno))\n - \t\tret = error_errno(_(\"failed to delete '%s'\"), filename);\n - \tFREE_AND_NULL(filename);\n - \n --\tfilename = xdg_config_home_systemd(\"git-maintenance@.service\");\n -+\tfilename = xdg_config_home_systemd(local_service_name);\n - \tif (unlink(filename) && !is_missing_file_error(errno))\n - \t\tret = error_errno(_(\"failed to delete '%s'\"), filename);\n - \n - \tfree(filename);\n -+\tfree(local_timer_name);\n -+\tfree(local_service_name);\n - \treturn ret;\n - }\n - \n -+static int systemd_timer_delete_unit_templates(void)\n ++/*\n ++ * A previous version of Git wrote the timer units as template files.\n ++ * Clean these up, if they exist.\n ++ */\n ++static void systemd_timer_delete_stale_timer_templates(void)\n ++{\n ++\tchar *timer_template_name = xstrfmt(SYSTEMD_UNIT_FORMAT, \"\", \"timer\");\n ++\tchar *filename = xdg_config_home_systemd(timer_template_name);\n ++\n ++\tif (unlink(filename) && !is_missing_file_error(errno))\n ++\t\twarning(_(\"failed to delete '%s'\"), filename);\n ++\n ++\tfree(filename);\n ++\tfree(timer_template_name);\n ++}\n ++\n ++static int systemd_timer_delete_unit_files(void)\n +{\n ++\tsystemd_timer_delete_stale_timer_templates();\n ++\n +\t/* Purposefully not short-circuited to make sure all are called. */\n -+\treturn systemd_timer_delete_unit_template(SCHEDULE_HOURLY) |\n -+\t systemd_timer_delete_unit_template(SCHEDULE_DAILY) |\n -+\t systemd_timer_delete_unit_template(SCHEDULE_WEEKLY);\n ++\treturn systemd_timer_delete_timer_file(SCHEDULE_HOURLY) |\n ++\t systemd_timer_delete_timer_file(SCHEDULE_DAILY) |\n ++\t systemd_timer_delete_timer_file(SCHEDULE_WEEKLY) |\n ++\t systemd_timer_delete_service_template();\n +}\n +\n static int systemd_timer_delete_units(void)\n @@ builtin/gc.c: static int systemd_timer_enable_unit(int enable,\n -\treturn systemd_timer_enable_unit(0, SCHEDULE_HOURLY) ||\n -\t systemd_timer_enable_unit(0, SCHEDULE_DAILY) ||\n -\t systemd_timer_enable_unit(0, SCHEDULE_WEEKLY) ||\n +-\t systemd_timer_delete_unit_templates();\n +\tint minute = get_random_minute();\n -+\tconst char *exec_path = git_exec_path();\n -+\treturn systemd_timer_enable_unit(0, SCHEDULE_HOURLY, exec_path, minute) ||\n -+\t systemd_timer_enable_unit(0, SCHEDULE_DAILY, exec_path, minute) ||\n -+\t systemd_timer_enable_unit(0, SCHEDULE_WEEKLY, exec_path, minute) ||\n - \t systemd_timer_delete_unit_templates();\n ++\t/* Purposefully not short-circuited to make sure all are called. */\n ++\treturn systemd_timer_enable_unit(0, SCHEDULE_HOURLY, minute) |\n ++\t systemd_timer_enable_unit(0, SCHEDULE_DAILY, minute) |\n ++\t systemd_timer_enable_unit(0, SCHEDULE_WEEKLY, minute) |\n ++\t systemd_timer_delete_unit_files();\n }\n \n static int systemd_timer_setup_units(void)\n @@ builtin/gc.c: static int systemd_timer_enable_unit(int enable,\n -\t\t systemd_timer_enable_unit(1, SCHEDULE_HOURLY) ||\n -\t\t systemd_timer_enable_unit(1, SCHEDULE_DAILY) ||\n -\t\t systemd_timer_enable_unit(1, SCHEDULE_WEEKLY);\n -+\tint ret = systemd_timer_enable_unit(1, SCHEDULE_HOURLY, exec_path, minute) ||\n -+\t\t systemd_timer_enable_unit(1, SCHEDULE_DAILY, exec_path, minute) ||\n -+\t\t systemd_timer_enable_unit(1, SCHEDULE_WEEKLY, exec_path, minute);\n ++\tint ret = systemd_timer_write_service_template(exec_path) ||\n ++\t\t systemd_timer_enable_unit(1, SCHEDULE_HOURLY, minute) ||\n ++\t\t systemd_timer_enable_unit(1, SCHEDULE_DAILY, minute) ||\n ++\t\t systemd_timer_enable_unit(1, SCHEDULE_WEEKLY, minute);\n ++\n \tif (ret)\n \t\tsystemd_timer_delete_units();\n ++\telse\n ++\t\tsystemd_timer_delete_stale_timer_templates();\n ++\n \treturn ret;\n + }\n + \n \n ## t/t7900-maintenance.sh ##\n @@ t/t7900-maintenance.sh: test_expect_success 'start and stop Linux/systemd maintenance' '\n @@ t/t7900-maintenance.sh: test_expect_success 'start and stop Linux/systemd mainte\n \tgit config --get --global --fixed-value maintenance.repo \"$(pwd)\" &&\n \n -\ttest_systemd_analyze_verify \"systemd/user/git-maintenance@.service\" &&\n ++\tfor schedule in hourly daily weekly\n ++\tdo\n ++\t\ttest_path_is_file \"systemd/user/git-maintenance@$schedule.timer\" || return 1\n ++\tdone &&\n ++\ttest_path_is_file \"systemd/user/git-maintenance@.service\" &&\n ++\n +\ttest_systemd_analyze_verify \"systemd/user/git-maintenance@hourly.service\" &&\n +\ttest_systemd_analyze_verify \"systemd/user/git-maintenance@daily.service\" &&\n +\ttest_systemd_analyze_verify \"systemd/user/git-maintenance@weekly.service\" &&\n \n \tprintf -- \"--user enable --now git-maintenance@%s.timer\\n\" hourly daily weekly >expect &&\n \ttest_cmp expect args &&\n +@@ t/t7900-maintenance.sh: test_expect_success 'start and stop Linux/systemd maintenance' '\n + \t# stop does not unregister the repo\n + \tgit config --get --global --fixed-value maintenance.repo \"$(pwd)\" &&\n + \n +-\ttest_path_is_missing \"systemd/user/git-maintenance@.timer\" &&\n ++\tfor schedule in hourly daily weekly\n ++\tdo\n ++\t\ttest_path_is_missing \"systemd/user/git-maintenance@$schedule.timer\" || return 1\n ++\tdone &&\n + \ttest_path_is_missing \"systemd/user/git-maintenance@.service\" &&\n + \n + \tprintf -- \"--user disable --now git-maintenance@%s.timer\\n\" hourly daily weekly >expect &&\n -: ----------- > 7: 86c4255d63d maintenance: fix systemd schedule overlaps\n -: ----------- > 8: f0c0f6eff88 maintenance: update schedule before config\n\n-- \ngitgitgadget\n"},{"id":"480476","messageId":"b291e6f7aec3a651f9366fac8eea0903f939d767.1691699987.git.gitgitgadget@gmail.com","threadId":"60077","inReplyTo":"pull.1567.v2.git.1691699987.gitgitgadget@gmail.com","subject":"[PATCH v2 4/8] maintenance: use random minute in cron scheduler","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-08-10T20:39:43Z","receivedAt":"2023-08-10T20:39:58Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nThe get_random_minute() method was created to allow maintenance\nschedules to be fixed to a random minute of the hour. This randomness is\nonly intended to spread out the load from a number of clients, but each\nclient should have an hour between each maintenance cycle.\n\nAdd this random minute to the cron integration.\n\nThe cron schedule specification starts with a minute indicator, which\nwas previously inserted as the \"0\" string but now takes the given minute\nas an integer parameter.\n\nSigned-off-by: Derrick Stolee \n---\n builtin/gc.c | 9 +++++----\n 1 file changed, 5 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex 78924bb32c6..ef8bb772c38 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -2174,6 +2174,7 @@ static int crontab_update_schedule(int run_maintenance, int fd)\n \tFILE *cron_list, *cron_in;\n \tstruct strbuf line = STRBUF_INIT;\n \tstruct tempfile *tmpedit = NULL;\n+\tint minute = get_random_minute();\n \n \tget_schedule_cmd(&cmd, NULL);\n \tstrvec_split(&crontab_list.args, cmd);\n@@ -2228,11 +2229,11 @@ static int crontab_update_schedule(int run_maintenance, int fd)\n \t\t\t\"# replaced in the future by a Git command.\\n\\n\");\n \n \t\tstrbuf_addf(&line_format,\n-\t\t\t \"%%s %%s * * %%s \\\"%s/git\\\" --exec-path=\\\"%s\\\" for-each-repo --config=maintenance.repo maintenance run --schedule=%%s\\n\",\n+\t\t\t \"%%d %%s * * %%s \\\"%s/git\\\" --exec-path=\\\"%s\\\" for-each-repo --config=maintenance.repo maintenance run --schedule=%%s\\n\",\n \t\t\t exec_path, exec_path);\n-\t\tfprintf(cron_in, line_format.buf, \"0\", \"1-23\", \"*\", \"hourly\");\n-\t\tfprintf(cron_in, line_format.buf, \"0\", \"0\", \"1-6\", \"daily\");\n-\t\tfprintf(cron_in, line_format.buf, \"0\", \"0\", \"0\", \"weekly\");\n+\t\tfprintf(cron_in, line_format.buf, minute, \"1-23\", \"*\", \"hourly\");\n+\t\tfprintf(cron_in, line_format.buf, minute, \"0\", \"1-6\", \"daily\");\n+\t\tfprintf(cron_in, line_format.buf, minute, \"0\", \"0\", \"weekly\");\n \t\tstrbuf_release(&line_format);\n \n \t\tfprintf(cron_in, \"\\n%s\\n\", END_LINE);\n-- \ngitgitgadget\n\n"},{"id":"480477","messageId":"f6d9c4f3b02653be7139c5cc0adb90564c6f7b12.1691699987.git.gitgitgadget@gmail.com","threadId":"60077","inReplyTo":"pull.1567.v2.git.1691699987.gitgitgadget@gmail.com","subject":"[PATCH v2 3/8] maintenance: use random minute in Windows scheduler","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-08-10T20:39:42Z","receivedAt":"2023-08-10T20:40:00Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nThe get_random_minute() method was created to allow maintenance\nschedules to be fixed to a random minute of the hour. This randomness is\nonly intended to spread out the load from a number of clients, but each\nclient should have an hour between each maintenance cycle.\n\nAdd this random minute to the Windows scheduler integration.\n\nWe need only to modify the minute value for the 'StartBoundary' tag\nacross the three schedules.\n\nSigned-off-by: Derrick Stolee \n---\n builtin/gc.c | 16 ++++++++++------\n 1 file changed, 10 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex 286ce131a5c..78924bb32c6 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -1995,6 +1995,7 @@ static int schtasks_schedule_task(const char *exec_path, enum schedule_priority\n \tconst char *frequency = get_frequency(schedule);\n \tchar *name = schtasks_task_name(frequency);\n \tstruct strbuf tfilename = STRBUF_INIT;\n+\tint minute = get_random_minute();\n \n \tget_schedule_cmd(&cmd, NULL);\n \n@@ -2015,7 +2016,7 @@ static int schtasks_schedule_task(const char *exec_path, enum schedule_priority\n \tswitch (schedule) {\n \tcase SCHEDULE_HOURLY:\n \t\tfprintf(tfile->fp,\n-\t\t\t\"2020-01-01T01:00:00\\n\"\n+\t\t\t\"2020-01-01T01:%02d:00\\n\"\n \t\t\t\"true\\n\"\n \t\t\t\"\\n\"\n \t\t\t\"1\\n\"\n@@ -2024,12 +2025,13 @@ static int schtasks_schedule_task(const char *exec_path, enum schedule_priority\n \t\t\t\"PT1H\\n\"\n \t\t\t\"PT23H\\n\"\n \t\t\t\"false\\n\"\n-\t\t\t\"\\n\");\n+\t\t\t\"\\n\",\n+\t\t\tminute);\n \t\tbreak;\n \n \tcase SCHEDULE_DAILY:\n \t\tfprintf(tfile->fp,\n-\t\t\t\"2020-01-01T00:00:00\\n\"\n+\t\t\t\"2020-01-01T00:%02d:00\\n\"\n \t\t\t\"true\\n\"\n \t\t\t\"\\n\"\n \t\t\t\"\\n\"\n@@ -2041,19 +2043,21 @@ static int schtasks_schedule_task(const char *exec_path, enum schedule_priority\n \t\t\t\"\\n\"\n \t\t\t\"\\n\"\n \t\t\t\"1\\n\"\n-\t\t\t\"\\n\");\n+\t\t\t\"\\n\",\n+\t\t\tminute);\n \t\tbreak;\n \n \tcase SCHEDULE_WEEKLY:\n \t\tfprintf(tfile->fp,\n-\t\t\t\"2020-01-01T00:00:00\\n\"\n+\t\t\t\"2020-01-01T00:%02d:00\\n\"\n \t\t\t\"true\\n\"\n \t\t\t\"\\n\"\n \t\t\t\"\\n\"\n \t\t\t\"\\n\"\n \t\t\t\"\\n\"\n \t\t\t\"1\\n\"\n-\t\t\t\"\\n\");\n+\t\t\t\"\\n\",\n+\t\t\tminute);\n \t\tbreak;\n \n \tdefault:\n-- \ngitgitgadget\n\n"},{"id":"480478","messageId":"88610437b4b53f584c540aca8ec26f40c0f0a426.1691699987.git.gitgitgadget@gmail.com","threadId":"60077","inReplyTo":"pull.1567.v2.git.1691699987.gitgitgadget@gmail.com","subject":"[PATCH v2 5/8] maintenance: swap method locations","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-08-10T20:39:44Z","receivedAt":"2023-08-10T20:40:04Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nThe systemd_timer_write_unit_templates() method writes a single template\nthat is then used to start the hourly, daily, and weekly schedules with\nsystemd.\n\nHowever, in order to schedule systemd maintenance on a given minute,\nthese templates need to be replaced with specific schedules for each of\nthese jobs.\n\nBefore modifying the schedules, move the writing method above the\nsystemd_timer_enable_unit() method, so we can write a specific schedule\nfor each unit.\n\nThe diff is computed smaller by showing systemd_timer_enable_unit() and\nsystemd_timer_delete_units() move instead of\nsystemd_timer_write_unit_templates() and\nsystemd_timer_delete_unit_templates().\n\nSigned-off-by: Derrick Stolee \n---\n builtin/gc.c | 96 ++++++++++++++++++++++++++--------------------------\n 1 file changed, 48 insertions(+), 48 deletions(-)\n\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex ef8bb772c38..e3819fc285a 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -2292,46 +2292,6 @@ static char *xdg_config_home_systemd(const char *filename)\n \treturn xdg_config_home_for(\"systemd/user\", filename);\n }\n \n-static int systemd_timer_enable_unit(int enable,\n-\t\t\t\t enum schedule_priority schedule)\n-{\n-\tconst char *cmd = \"systemctl\";\n-\tstruct child_process child = CHILD_PROCESS_INIT;\n-\tconst char *frequency = get_frequency(schedule);\n-\n-\t/*\n-\t * Disabling the systemd unit while it is already disabled makes\n-\t * systemctl print an error.\n-\t * Let's ignore it since it means we already are in the expected state:\n-\t * the unit is disabled.\n-\t *\n-\t * On the other hand, enabling a systemd unit which is already enabled\n-\t * produces no error.\n-\t */\n-\tif (!enable)\n-\t\tchild.no_stderr = 1;\n-\n-\tget_schedule_cmd(&cmd, NULL);\n-\tstrvec_split(&child.args, cmd);\n-\tstrvec_pushl(&child.args, \"--user\", enable ? \"enable\" : \"disable\",\n-\t\t \"--now\", NULL);\n-\tstrvec_pushf(&child.args, \"git-maintenance@%s.timer\", frequency);\n-\n-\tif (start_command(&child))\n-\t\treturn error(_(\"failed to start systemctl\"));\n-\tif (finish_command(&child))\n-\t\t/*\n-\t\t * Disabling an already disabled systemd unit makes\n-\t\t * systemctl fail.\n-\t\t * Let's ignore this failure.\n-\t\t *\n-\t\t * Enabling an enabled systemd unit doesn't fail.\n-\t\t */\n-\t\tif (enable)\n-\t\t\treturn error(_(\"failed to run systemctl\"));\n-\treturn 0;\n-}\n-\n static int systemd_timer_delete_unit_templates(void)\n {\n \tint ret = 0;\n@@ -2348,14 +2308,6 @@ static int systemd_timer_delete_unit_templates(void)\n \treturn ret;\n }\n \n-static int systemd_timer_delete_units(void)\n-{\n-\treturn systemd_timer_enable_unit(0, SCHEDULE_HOURLY) ||\n-\t systemd_timer_enable_unit(0, SCHEDULE_DAILY) ||\n-\t systemd_timer_enable_unit(0, SCHEDULE_WEEKLY) ||\n-\t systemd_timer_delete_unit_templates();\n-}\n-\n static int systemd_timer_write_unit_templates(const char *exec_path)\n {\n \tchar *filename;\n@@ -2437,6 +2389,54 @@ error:\n \treturn -1;\n }\n \n+static int systemd_timer_enable_unit(int enable,\n+\t\t\t\t enum schedule_priority schedule)\n+{\n+\tconst char *cmd = \"systemctl\";\n+\tstruct child_process child = CHILD_PROCESS_INIT;\n+\tconst char *frequency = get_frequency(schedule);\n+\n+\t/*\n+\t * Disabling the systemd unit while it is already disabled makes\n+\t * systemctl print an error.\n+\t * Let's ignore it since it means we already are in the expected state:\n+\t * the unit is disabled.\n+\t *\n+\t * On the other hand, enabling a systemd unit which is already enabled\n+\t * produces no error.\n+\t */\n+\tif (!enable)\n+\t\tchild.no_stderr = 1;\n+\n+\tget_schedule_cmd(&cmd, NULL);\n+\tstrvec_split(&child.args, cmd);\n+\tstrvec_pushl(&child.args, \"--user\", enable ? \"enable\" : \"disable\",\n+\t\t \"--now\", NULL);\n+\tstrvec_pushf(&child.args, \"git-maintenance@%s.timer\", frequency);\n+\n+\tif (start_command(&child))\n+\t\treturn error(_(\"failed to start systemctl\"));\n+\tif (finish_command(&child))\n+\t\t/*\n+\t\t * Disabling an already disabled systemd unit makes\n+\t\t * systemctl fail.\n+\t\t * Let's ignore this failure.\n+\t\t *\n+\t\t * Enabling an enabled systemd unit doesn't fail.\n+\t\t */\n+\t\tif (enable)\n+\t\t\treturn error(_(\"failed to run systemctl\"));\n+\treturn 0;\n+}\n+\n+static int systemd_timer_delete_units(void)\n+{\n+\treturn systemd_timer_enable_unit(0, SCHEDULE_HOURLY) ||\n+\t systemd_timer_enable_unit(0, SCHEDULE_DAILY) ||\n+\t systemd_timer_enable_unit(0, SCHEDULE_WEEKLY) ||\n+\t systemd_timer_delete_unit_templates();\n+}\n+\n static int systemd_timer_setup_units(void)\n {\n \tconst char *exec_path = git_exec_path();\n-- \ngitgitgadget\n\n"},{"id":"480479","messageId":"86c4255d63d7f38b7a4811e5667404bce04f77f8.1691699987.git.gitgitgadget@gmail.com","threadId":"60077","inReplyTo":"pull.1567.v2.git.1691699987.gitgitgadget@gmail.com","subject":"[PATCH v2 7/8] maintenance: fix systemd schedule overlaps","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-08-10T20:39:46Z","receivedAt":"2023-08-10T20:40:12Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nThe 'git maintenance run' command prevents concurrent runs in the same\nrepository using a 'maintenance.lock' file. However, when using systemd\nthe hourly maintenance runs the same time as the daily and weekly runs.\n(Similarly, daily maintenance runs at the same time as weekly\nmaintenance.) These competing commands result in some maintenance not\nactually being run.\n\nThis overlap was something we could not fix until we made the recent\nchange to not use the builting 'hourly', 'daily', and 'weekly' schedules\nin systemd. We can adjust the schedules such that:\n\n 1. Hourly runs avoid the 0th hour.\n 2. Daily runs avoid Monday.\n\nThis will keep maintenance runs from colliding when using systemd.\n\nSigned-off-by: Derrick Stolee \n---\n builtin/gc.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex e52129e485c..6f8df366fbe 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -2350,11 +2350,11 @@ static int systemd_timer_write_timer_file(enum schedule_priority schedule,\n \n \tswitch (schedule) {\n \tcase SCHEDULE_HOURLY:\n-\t\tschedule_pattern = xstrfmt(\"*-*-* *:%02d:00\", minute);\n+\t\tschedule_pattern = xstrfmt(\"*-*-* 1..23:%02d:00\", minute);\n \t\tbreak;\n \n \tcase SCHEDULE_DAILY:\n-\t\tschedule_pattern = xstrfmt(\"*-*-* 0:%02d:00\", minute);\n+\t\tschedule_pattern = xstrfmt(\"Tue..Sun *-*-* 0:%02d:00\", minute);\n \t\tbreak;\n \n \tcase SCHEDULE_WEEKLY:\n-- \ngitgitgadget\n\n"},{"id":"480480","messageId":"e43778d3e408f5a77b01bce13df6f8b037473cc3.1691699987.git.gitgitgadget@gmail.com","threadId":"60077","inReplyTo":"pull.1567.v2.git.1691699987.gitgitgadget@gmail.com","subject":"[PATCH v2 6/8] maintenance: use random minute in systemd scheduler","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-08-10T20:39:45Z","receivedAt":"2023-08-10T20:40:12Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nThe get_random_minute() method was created to allow maintenance\nschedules to be fixed to a random minute of the hour. This randomness is\nonly intended to spread out the load from a number of clients, but each\nclient should have an hour between each maintenance cycle.\n\nAdd this random minute to the systemd integration.\n\nThis integration is more complicated than similar changes for other\nschedulers because of a neat trick that systemd allows: templating.\n\nThe previous implementation generated two template files with names\nof the form 'git-maintenance@.(timer|service)'. The '.timer' or\n'.service' indicates that this is a template that is picked up when we\nlater specify '...@.timer' or '...@.service'. The\n'' string is then used to insert into the template both the\n'OnCalendar' schedule setting and the '--schedule' parameter of the\n'git maintenance run' command.\n\nIn order to set these schedules to a given minute, we can no longer use\nthe 'hourly', 'daily', or 'weekly' strings for '' and instead\nneed to abandon the template model for the .timer files. We can still\nuse templates for the .service files. For this reason, we split these\nwrites into two methods.\n\nModify the template with a custom schedule in the 'OnCalendar' setting.\nThis schedule has some interesting differences from cron-like patterns,\nbut is relatively easy to figure out from context. The one that might be\nconfusing is that '*-*-*' is a date-based pattern, but this must be\nomitted when using 'Mon' to signal that we care about the day of the\nweek. Monday is used since that matches the day used for the 'weekly'\nschedule used previously.\n\nNow that the timer files are not templates, we might want to abandon the\n'@' symbol in the file names. However, this would cause users with\nexisting schedules to get two competing schedules due to different\nnames. The work to remove the old schedule name is one thing that we can\navoid by keeping the '@' symbol in our unit names. Since we are locked\ninto this name, it makes sense that we keep the template model for the\n.service files.\n\nThe rest of the change involves making sure we are writing these .timer\nand .service files before initializing the schedule with 'systemctl' and\ndeleting the files when we are done. Some changes are also made to share\nthe random minute along with a single computation of the execution path\nof the current Git executable.\n\nIn addition, older Git versions may have written a\n'git-maintenance@.timer' template file. Be sure to remove this when\nsuccessfully enabling maintenance (or disabling maintenance).\n\nSigned-off-by: Derrick Stolee \n---\n builtin/gc.c | 152 ++++++++++++++++++++++++++++++++++-------\n t/t7900-maintenance.sh | 15 +++-\n 2 files changed, 142 insertions(+), 25 deletions(-)\n\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex e3819fc285a..e52129e485c 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -2292,29 +2292,54 @@ static char *xdg_config_home_systemd(const char *filename)\n \treturn xdg_config_home_for(\"systemd/user\", filename);\n }\n \n-static int systemd_timer_delete_unit_templates(void)\n+#define SYSTEMD_UNIT_FORMAT \"git-maintenance@%s.%s\"\n+\n+static int systemd_timer_delete_timer_file(enum schedule_priority priority)\n {\n \tint ret = 0;\n-\tchar *filename = xdg_config_home_systemd(\"git-maintenance@.timer\");\n+\tconst char *frequency = get_frequency(priority);\n+\tchar *local_timer_name = xstrfmt(SYSTEMD_UNIT_FORMAT, frequency, \"timer\");\n+\tchar *filename = xdg_config_home_systemd(local_timer_name);\n+\n \tif (unlink(filename) && !is_missing_file_error(errno))\n \t\tret = error_errno(_(\"failed to delete '%s'\"), filename);\n-\tFREE_AND_NULL(filename);\n \n-\tfilename = xdg_config_home_systemd(\"git-maintenance@.service\");\n+\tfree(filename);\n+\tfree(local_timer_name);\n+\treturn ret;\n+}\n+\n+static int systemd_timer_delete_service_template(void)\n+{\n+\tint ret = 0;\n+\tchar *local_service_name = xstrfmt(SYSTEMD_UNIT_FORMAT, \"\", \"service\");\n+\tchar *filename = xdg_config_home_systemd(local_service_name);\n \tif (unlink(filename) && !is_missing_file_error(errno))\n \t\tret = error_errno(_(\"failed to delete '%s'\"), filename);\n \n \tfree(filename);\n+\tfree(local_service_name);\n \treturn ret;\n }\n \n-static int systemd_timer_write_unit_templates(const char *exec_path)\n+/*\n+ * Write the schedule information into a git-maintenance@.timer\n+ * file using a custom minute. This timer file cannot use the templating\n+ * system, so we generate a specific file for each.\n+ */\n+static int systemd_timer_write_timer_file(enum schedule_priority schedule,\n+\t\t\t\t\t int minute)\n {\n+\tint res = -1;\n \tchar *filename;\n \tFILE *file;\n \tconst char *unit;\n+\tchar *schedule_pattern = NULL;\n+\tconst char *frequency = get_frequency(schedule);\n+\tchar *local_timer_name = xstrfmt(SYSTEMD_UNIT_FORMAT, frequency, \"timer\");\n+\n+\tfilename = xdg_config_home_systemd(local_timer_name);\n \n-\tfilename = xdg_config_home_systemd(\"git-maintenance@.timer\");\n \tif (safe_create_leading_directories(filename)) {\n \t\terror(_(\"failed to create directories for '%s'\"), filename);\n \t\tgoto error;\n@@ -2323,6 +2348,23 @@ static int systemd_timer_write_unit_templates(const char *exec_path)\n \tif (!file)\n \t\tgoto error;\n \n+\tswitch (schedule) {\n+\tcase SCHEDULE_HOURLY:\n+\t\tschedule_pattern = xstrfmt(\"*-*-* *:%02d:00\", minute);\n+\t\tbreak;\n+\n+\tcase SCHEDULE_DAILY:\n+\t\tschedule_pattern = xstrfmt(\"*-*-* 0:%02d:00\", minute);\n+\t\tbreak;\n+\n+\tcase SCHEDULE_WEEKLY:\n+\t\tschedule_pattern = xstrfmt(\"Mon 0:%02d:00\", minute);\n+\t\tbreak;\n+\n+\tdefault:\n+\t\tBUG(\"Unhandled schedule_priority\");\n+\t}\n+\n \tunit = \"# This file was created and is maintained by Git.\\n\"\n \t \"# Any edits made in this file might be replaced in the future\\n\"\n \t \"# by a Git command.\\n\"\n@@ -2331,12 +2373,12 @@ static int systemd_timer_write_unit_templates(const char *exec_path)\n \t \"Description=Optimize Git repositories data\\n\"\n \t \"\\n\"\n \t \"[Timer]\\n\"\n-\t \"OnCalendar=%i\\n\"\n+\t \"OnCalendar=%s\\n\"\n \t \"Persistent=true\\n\"\n \t \"\\n\"\n \t \"[Install]\\n\"\n \t \"WantedBy=timers.target\\n\";\n-\tif (fputs(unit, file) == EOF) {\n+\tif (fprintf(file, unit, schedule_pattern) < 0) {\n \t\terror(_(\"failed to write to '%s'\"), filename);\n \t\tfclose(file);\n \t\tgoto error;\n@@ -2345,9 +2387,36 @@ static int systemd_timer_write_unit_templates(const char *exec_path)\n \t\terror_errno(_(\"failed to flush '%s'\"), filename);\n \t\tgoto error;\n \t}\n+\n+\tres = 0;\n+\n+error:\n+\tfree(schedule_pattern);\n+\tfree(local_timer_name);\n \tfree(filename);\n+\treturn res;\n+}\n \n-\tfilename = xdg_config_home_systemd(\"git-maintenance@.service\");\n+/*\n+ * No matter the schedule, we use the same service and can make use of the\n+ * templating system. When installing git-maintenance@.timer,\n+ * systemd will notice that git-maintenance@.service exists as a template\n+ * and will use this file and insert the into the template at\n+ * the position of \"%i\".\n+ */\n+static int systemd_timer_write_service_template(const char *exec_path)\n+{\n+\tint res = -1;\n+\tchar *filename;\n+\tFILE *file;\n+\tconst char *unit;\n+\tchar *local_service_name = xstrfmt(SYSTEMD_UNIT_FORMAT, \"\", \"service\");\n+\n+\tfilename = xdg_config_home_systemd(local_service_name);\n+\tif (safe_create_leading_directories(filename)) {\n+\t\terror(_(\"failed to create directories for '%s'\"), filename);\n+\t\tgoto error;\n+\t}\n \tfile = fopen_or_warn(filename, \"w\");\n \tif (!file)\n \t\tgoto error;\n@@ -2380,17 +2449,18 @@ static int systemd_timer_write_unit_templates(const char *exec_path)\n \t\terror_errno(_(\"failed to flush '%s'\"), filename);\n \t\tgoto error;\n \t}\n-\tfree(filename);\n-\treturn 0;\n+\n+\tres = 0;\n \n error:\n+\tfree(local_service_name);\n \tfree(filename);\n-\tsystemd_timer_delete_unit_templates();\n-\treturn -1;\n+\treturn res;\n }\n \n static int systemd_timer_enable_unit(int enable,\n-\t\t\t\t enum schedule_priority schedule)\n+\t\t\t\t enum schedule_priority schedule,\n+\t\t\t\t int minute)\n {\n \tconst char *cmd = \"systemctl\";\n \tstruct child_process child = CHILD_PROCESS_INIT;\n@@ -2407,12 +2477,14 @@ static int systemd_timer_enable_unit(int enable,\n \t */\n \tif (!enable)\n \t\tchild.no_stderr = 1;\n+\telse if (systemd_timer_write_timer_file(schedule, minute))\n+\t\treturn -1;\n \n \tget_schedule_cmd(&cmd, NULL);\n \tstrvec_split(&child.args, cmd);\n \tstrvec_pushl(&child.args, \"--user\", enable ? \"enable\" : \"disable\",\n \t\t \"--now\", NULL);\n-\tstrvec_pushf(&child.args, \"git-maintenance@%s.timer\", frequency);\n+\tstrvec_pushf(&child.args, SYSTEMD_UNIT_FORMAT, frequency, \"timer\");\n \n \tif (start_command(&child))\n \t\treturn error(_(\"failed to start systemctl\"));\n@@ -2429,24 +2501,58 @@ static int systemd_timer_enable_unit(int enable,\n \treturn 0;\n }\n \n+/*\n+ * A previous version of Git wrote the timer units as template files.\n+ * Clean these up, if they exist.\n+ */\n+static void systemd_timer_delete_stale_timer_templates(void)\n+{\n+\tchar *timer_template_name = xstrfmt(SYSTEMD_UNIT_FORMAT, \"\", \"timer\");\n+\tchar *filename = xdg_config_home_systemd(timer_template_name);\n+\n+\tif (unlink(filename) && !is_missing_file_error(errno))\n+\t\twarning(_(\"failed to delete '%s'\"), filename);\n+\n+\tfree(filename);\n+\tfree(timer_template_name);\n+}\n+\n+static int systemd_timer_delete_unit_files(void)\n+{\n+\tsystemd_timer_delete_stale_timer_templates();\n+\n+\t/* Purposefully not short-circuited to make sure all are called. */\n+\treturn systemd_timer_delete_timer_file(SCHEDULE_HOURLY) |\n+\t systemd_timer_delete_timer_file(SCHEDULE_DAILY) |\n+\t systemd_timer_delete_timer_file(SCHEDULE_WEEKLY) |\n+\t systemd_timer_delete_service_template();\n+}\n+\n static int systemd_timer_delete_units(void)\n {\n-\treturn systemd_timer_enable_unit(0, SCHEDULE_HOURLY) ||\n-\t systemd_timer_enable_unit(0, SCHEDULE_DAILY) ||\n-\t systemd_timer_enable_unit(0, SCHEDULE_WEEKLY) ||\n-\t systemd_timer_delete_unit_templates();\n+\tint minute = get_random_minute();\n+\t/* Purposefully not short-circuited to make sure all are called. */\n+\treturn systemd_timer_enable_unit(0, SCHEDULE_HOURLY, minute) |\n+\t systemd_timer_enable_unit(0, SCHEDULE_DAILY, minute) |\n+\t systemd_timer_enable_unit(0, SCHEDULE_WEEKLY, minute) |\n+\t systemd_timer_delete_unit_files();\n }\n \n static int systemd_timer_setup_units(void)\n {\n+\tint minute = get_random_minute();\n \tconst char *exec_path = git_exec_path();\n \n-\tint ret = systemd_timer_write_unit_templates(exec_path) ||\n-\t\t systemd_timer_enable_unit(1, SCHEDULE_HOURLY) ||\n-\t\t systemd_timer_enable_unit(1, SCHEDULE_DAILY) ||\n-\t\t systemd_timer_enable_unit(1, SCHEDULE_WEEKLY);\n+\tint ret = systemd_timer_write_service_template(exec_path) ||\n+\t\t systemd_timer_enable_unit(1, SCHEDULE_HOURLY, minute) ||\n+\t\t systemd_timer_enable_unit(1, SCHEDULE_DAILY, minute) ||\n+\t\t systemd_timer_enable_unit(1, SCHEDULE_WEEKLY, minute);\n+\n \tif (ret)\n \t\tsystemd_timer_delete_units();\n+\telse\n+\t\tsystemd_timer_delete_stale_timer_templates();\n+\n \treturn ret;\n }\n \ndiff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh\nindex 487e326b3fa..9ffe76729e6 100755\n--- a/t/t7900-maintenance.sh\n+++ b/t/t7900-maintenance.sh\n@@ -744,7 +744,15 @@ test_expect_success 'start and stop Linux/systemd maintenance' '\n \t# start registers the repo\n \tgit config --get --global --fixed-value maintenance.repo \"$(pwd)\" &&\n \n-\ttest_systemd_analyze_verify \"systemd/user/git-maintenance@.service\" &&\n+\tfor schedule in hourly daily weekly\n+\tdo\n+\t\ttest_path_is_file \"systemd/user/git-maintenance@$schedule.timer\" || return 1\n+\tdone &&\n+\ttest_path_is_file \"systemd/user/git-maintenance@.service\" &&\n+\n+\ttest_systemd_analyze_verify \"systemd/user/git-maintenance@hourly.service\" &&\n+\ttest_systemd_analyze_verify \"systemd/user/git-maintenance@daily.service\" &&\n+\ttest_systemd_analyze_verify \"systemd/user/git-maintenance@weekly.service\" &&\n \n \tprintf -- \"--user enable --now git-maintenance@%s.timer\\n\" hourly daily weekly >expect &&\n \ttest_cmp expect args &&\n@@ -755,7 +763,10 @@ test_expect_success 'start and stop Linux/systemd maintenance' '\n \t# stop does not unregister the repo\n \tgit config --get --global --fixed-value maintenance.repo \"$(pwd)\" &&\n \n-\ttest_path_is_missing \"systemd/user/git-maintenance@.timer\" &&\n+\tfor schedule in hourly daily weekly\n+\tdo\n+\t\ttest_path_is_missing \"systemd/user/git-maintenance@$schedule.timer\" || return 1\n+\tdone &&\n \ttest_path_is_missing \"systemd/user/git-maintenance@.service\" &&\n \n \tprintf -- \"--user disable --now git-maintenance@%s.timer\\n\" hourly daily weekly >expect &&\n-- \ngitgitgadget\n\n"},{"id":"480481","messageId":"f0c0f6eff883c62f6b07223b5f1da3fd8e462507.1691699987.git.gitgitgadget@gmail.com","threadId":"60077","inReplyTo":"pull.1567.v2.git.1691699987.gitgitgadget@gmail.com","subject":"[PATCH v2 8/8] maintenance: update schedule before config","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-08-10T20:39:47Z","receivedAt":"2023-08-10T20:40:15Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nWhen running 'git maintenance start', the current pattern is to\nconfigure global config settings to enable maintenance on the current\nrepository and set 'maintenance.auto' to false and _then_ to set up the\nschedule with the system scheduler.\n\nThis has a problematic error condition: if the scheduler fails to\ninitialize, the repository still will not use automatic maintenance due\nto the 'maintenance.auto' setting.\n\nFix this gap by swapping the order of operations. If Git fails to\ninitialize maintenance, then the config changes should never happen.\n\nReported-by: Phillip Wood \nSigned-off-by: Derrick Stolee \n---\n builtin/gc.c | 5 ++++-\n t/t7900-maintenance.sh | 13 +++++++++++++\n 2 files changed, 17 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex 6f8df366fbe..fe5f871c493 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -2728,9 +2728,12 @@ static int maintenance_start(int argc, const char **argv, const char *prefix)\n \topts.scheduler = resolve_scheduler(opts.scheduler);\n \tvalidate_scheduler(opts.scheduler);\n \n+\tif (update_background_schedule(&opts, 1))\n+\t\tdie(_(\"failed to set up maintenance schedule\"));\n+\n \tif (maintenance_register(ARRAY_SIZE(register_args)-1, register_args, NULL))\n \t\twarning(_(\"failed to add repo to global config\"));\n-\treturn update_background_schedule(&opts, 1);\n+\treturn 0;\n }\n \n static const char *const builtin_maintenance_stop_usage[] = {\ndiff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh\nindex 9ffe76729e6..e56f5980dc4 100755\n--- a/t/t7900-maintenance.sh\n+++ b/t/t7900-maintenance.sh\n@@ -849,4 +849,17 @@ test_expect_success 'register and unregister bare repo' '\n \t)\n '\n \n+test_expect_success 'failed schedule prevents config change' '\n+\tgit init --bare failcase &&\n+\n+\tfor scheduler in crontab launchctl schtasks systemctl\n+\tdo\n+\t\tGIT_TEST_MAINT_SCHEDULER=\"$scheduler:false\" &&\n+\t\texport GIT_TEST_MAINT_SCHEDULER &&\n+\t\ttest_must_fail \\\n+\t\t\tgit -C failcase maintenance start &&\n+\t\ttest_must_fail git -C failcase config maintenance.auto || return 1\n+\tdone\n+'\n+\n test_done\n-- \ngitgitgadget\n"},{"id":"480503","messageId":"xmqqpm3ur3sn.fsf@gitster.g","threadId":"60077","inReplyTo":"86c4255d63d7f38b7a4811e5667404bce04f77f8.1691699987.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 7/8] maintenance: fix systemd schedule overlaps","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-08-10T21:22:16Z","receivedAt":"2023-08-10T21:22:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Derrick Stolee via GitGitGadget\" writes:\n\n> From: Derrick Stolee \n>\n> The 'git maintenance run' command prevents concurrent runs in the same\n> repository using a 'maintenance.lock' file. However, when using systemd\n> the hourly maintenance runs the same time as the daily and weekly runs.\n> (Similarly, daily maintenance runs at the same time as weekly\n> maintenance.) These competing commands result in some maintenance not\n> actually being run.\n>\n> This overlap was something we could not fix until we made the recent\n> change to not use the builting 'hourly', 'daily', and 'weekly' schedules\n> in systemd. We can adjust the schedules such that:\n>\n> 1. Hourly runs avoid the 0th hour.\n> 2. Daily runs avoid Monday.\n\nSimple and stupid is good. When I read the problem description, I\nwondered if the hour in which daily jobs are run will also be\ndispersed (so that the load of the server that helps daily tasks at\nthe clients will not be concentrated in a single hour out of 24\nhours of a day), but hopefully the load is still spread within that\n60 minutes evenly, which probably is good enough.\n\n\n\n>\n> This will keep maintenance runs from colliding when using systemd.\n>\n> Signed-off-by: Derrick Stolee \n> ---\n> builtin/gc.c | 4 ++--\n> 1 file changed, 2 insertions(+), 2 deletions(-)\n>\n> diff --git a/builtin/gc.c b/builtin/gc.c\n> index e52129e485c..6f8df366fbe 100644\n> --- a/builtin/gc.c\n> +++ b/builtin/gc.c\n> @@ -2350,11 +2350,11 @@ static int systemd_timer_write_timer_file(enum schedule_priority schedule,\n> \n> \tswitch (schedule) {\n> \tcase SCHEDULE_HOURLY:\n> -\t\tschedule_pattern = xstrfmt(\"*-*-* *:%02d:00\", minute);\n> +\t\tschedule_pattern = xstrfmt(\"*-*-* 1..23:%02d:00\", minute);\n> \t\tbreak;\n> \n> \tcase SCHEDULE_DAILY:\n> -\t\tschedule_pattern = xstrfmt(\"*-*-* 0:%02d:00\", minute);\n> +\t\tschedule_pattern = xstrfmt(\"Tue..Sun *-*-* 0:%02d:00\", minute);\n> \t\tbreak;\n> \n> \tcase SCHEDULE_WEEKLY:\n"},{"id":"480504","messageId":"ZNVVsAPfwDNIkjVk@nand.local","threadId":"60077","inReplyTo":"edc08023ed51890f0390aacf783d7213e82704a7.1691699987.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 1/8] maintenance: add get_random_minute()","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-08-10T21:25:04Z","receivedAt":"2023-08-10T21:25:10Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, Aug 10, 2023 at 08:39:40PM +0000, Derrick Stolee via GitGitGadget wrote:\n> ---\n> builtin/gc.c | 10 ++++++++++\n> wrapper.c | 10 ++++++++++\n> wrapper.h | 6 ++++++\n> 3 files changed, 26 insertions(+)\n\nVery nice, this patch LGTM. Thanks for working on it!\n\nThanks,\nTaylor\n"},{"id":"480641","messageId":"5d431d2c-2d2c-4fb5-82ca-b9684523e512@gmail.com","threadId":"60077","inReplyTo":"e43778d3e408f5a77b01bce13df6f8b037473cc3.1691699987.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 6/8] maintenance: use random minute in systemd scheduler","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-08-14T11:26:46Z","receivedAt":"2023-08-14T11:27:43Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Stolee\n\nThis all looks good to me now, I've left a comment on the test but I \nthink it is probably fine to go in as it is.\n\n> -static int systemd_timer_write_unit_templates(const char *exec_path)\n> +/*\n> + * Write the schedule information into a git-maintenance@.timer\n> + * file using a custom minute. This timer file cannot use the templating\n> + * system, so we generate a specific file for each.\n> + */\n\nThanks for adding comments to the functions that write the timer and \nservice files, they really helpful especially for readers who are not \nthat familiar with systemd.\n\n> +static int systemd_timer_delete_unit_files(void)\n> +{\n> +\tsystemd_timer_delete_stale_timer_templates();\n> +\n> +\t/* Purposefully not short-circuited to make sure all are called. */\n> +\treturn systemd_timer_delete_timer_file(SCHEDULE_HOURLY) |\n> +\t systemd_timer_delete_timer_file(SCHEDULE_DAILY) |\n> +\t systemd_timer_delete_timer_file(SCHEDULE_WEEKLY) |\n> +\t systemd_timer_delete_service_template();\n\nUsing \"|\" rather than \"||\" is a nice touch.\n\n> diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh\n> index 487e326b3fa..9ffe76729e6 100755\n> --- a/t/t7900-maintenance.sh\n> +++ b/t/t7900-maintenance.sh\n> @@ -744,7 +744,15 @@ test_expect_success 'start and stop Linux/systemd maintenance' '\n> \t# start registers the repo\n> \tgit config --get --global --fixed-value maintenance.repo \"$(pwd)\" &&\n> \n> -\ttest_systemd_analyze_verify \"systemd/user/git-maintenance@.service\" &&\n> +\tfor schedule in hourly daily weekly\n> +\tdo\n> +\t\ttest_path_is_file \"systemd/user/git-maintenance@$schedule.timer\" || return 1\n> +\tdone &&\n> +\ttest_path_is_file \"systemd/user/git-maintenance@.service\" &&\n> +\n> +\ttest_systemd_analyze_verify \"systemd/user/git-maintenance@hourly.service\" &&\n> +\ttest_systemd_analyze_verify \"systemd/user/git-maintenance@daily.service\" &&\n> +\ttest_systemd_analyze_verify \"systemd/user/git-maintenance@weekly.service\" &&\n\nAs we only write the template service file I'm not sure what we gain by \nthese three checks but they don't seem to be doing any harm.\n\nBest Wishes\n\nPhillip\n"},{"id":"480642","messageId":"055a1132-fe35-4f83-ae7c-16d84ca42f2c@gmail.com","threadId":"60077","inReplyTo":"86c4255d63d7f38b7a4811e5667404bce04f77f8.1691699987.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 7/8] maintenance: fix systemd schedule overlaps","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-08-14T11:27:37Z","receivedAt":"2023-08-14T11:28:47Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Stolee\n\nOn 10/08/2023 21:39, Derrick Stolee via GitGitGadget wrote:\n> From: Derrick Stolee \n> \n> The 'git maintenance run' command prevents concurrent runs in the same\n> repository using a 'maintenance.lock' file. However, when using systemd\n> the hourly maintenance runs the same time as the daily and weekly runs.\n> (Similarly, daily maintenance runs at the same time as weekly\n> maintenance.) These competing commands result in some maintenance not\n> actually being run.\n> \n> This overlap was something we could not fix until we made the recent\n> change to not use the builting 'hourly', 'daily', and 'weekly' schedules\n> in systemd. We can adjust the schedules such that:\n> \n> 1. Hourly runs avoid the 0th hour.\n> 2. Daily runs avoid Monday.\n> \n> This will keep maintenance runs from colliding when using systemd.\n\nThanks for fixing this, the patch looks good\n\nBest Wishes\n\nPhillip\n\n> Signed-off-by: Derrick Stolee \n> ---\n> builtin/gc.c | 4 ++--\n> 1 file changed, 2 insertions(+), 2 deletions(-)\n> \n> diff --git a/builtin/gc.c b/builtin/gc.c\n> index e52129e485c..6f8df366fbe 100644\n> --- a/builtin/gc.c\n> +++ b/builtin/gc.c\n> @@ -2350,11 +2350,11 @@ static int systemd_timer_write_timer_file(enum schedule_priority schedule,\n> \n> \tswitch (schedule) {\n> \tcase SCHEDULE_HOURLY:\n> -\t\tschedule_pattern = xstrfmt(\"*-*-* *:%02d:00\", minute);\n> +\t\tschedule_pattern = xstrfmt(\"*-*-* 1..23:%02d:00\", minute);\n> \t\tbreak;\n> \n> \tcase SCHEDULE_DAILY:\n> -\t\tschedule_pattern = xstrfmt(\"*-*-* 0:%02d:00\", minute);\n> +\t\tschedule_pattern = xstrfmt(\"Tue..Sun *-*-* 0:%02d:00\", minute);\n> \t\tbreak;\n> \n> \tcase SCHEDULE_WEEKLY:\n"},{"id":"480643","messageId":"fee3baa3-f962-45be-9b41-4bb17a5d1475@gmail.com","threadId":"60077","inReplyTo":"f0c0f6eff883c62f6b07223b5f1da3fd8e462507.1691699987.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 8/8] maintenance: update schedule before config","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-08-14T11:28:47Z","receivedAt":"2023-08-14T11:29:51Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Stolee\n\nOn 10/08/2023 21:39, Derrick Stolee via GitGitGadget wrote:\n> From: Derrick Stolee \n> \n> When running 'git maintenance start', the current pattern is to\n> configure global config settings to enable maintenance on the current\n> repository and set 'maintenance.auto' to false and _then_ to set up the\n> schedule with the system scheduler.\n> \n> This has a problematic error condition: if the scheduler fails to\n> initialize, the repository still will not use automatic maintenance due\n> to the 'maintenance.auto' setting.\n> \n> Fix this gap by swapping the order of operations. If Git fails to\n> initialize maintenance, then the config changes should never happen.\n\nThe commit message and patch look good to me\n\nThanks\n\nPhillip\n\n> Reported-by: Phillip Wood \n> Signed-off-by: Derrick Stolee \n> ---\n> builtin/gc.c | 5 ++++-\n> t/t7900-maintenance.sh | 13 +++++++++++++\n> 2 files changed, 17 insertions(+), 1 deletion(-)\n> \n> diff --git a/builtin/gc.c b/builtin/gc.c\n> index 6f8df366fbe..fe5f871c493 100644\n> --- a/builtin/gc.c\n> +++ b/builtin/gc.c\n> @@ -2728,9 +2728,12 @@ static int maintenance_start(int argc, const char **argv, const char *prefix)\n> \topts.scheduler = resolve_scheduler(opts.scheduler);\n> \tvalidate_scheduler(opts.scheduler);\n> \n> +\tif (update_background_schedule(&opts, 1))\n> +\t\tdie(_(\"failed to set up maintenance schedule\"));\n> +\n> \tif (maintenance_register(ARRAY_SIZE(register_args)-1, register_args, NULL))\n> \t\twarning(_(\"failed to add repo to global config\"));\n> -\treturn update_background_schedule(&opts, 1);\n> +\treturn 0;\n> }\n> \n> static const char *const builtin_maintenance_stop_usage[] = {\n> diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh\n> index 9ffe76729e6..e56f5980dc4 100755\n> --- a/t/t7900-maintenance.sh\n> +++ b/t/t7900-maintenance.sh\n> @@ -849,4 +849,17 @@ test_expect_success 'register and unregister bare repo' '\n> \t)\n> '\n> \n> +test_expect_success 'failed schedule prevents config change' '\n> +\tgit init --bare failcase &&\n> +\n> +\tfor scheduler in crontab launchctl schtasks systemctl\n> +\tdo\n> +\t\tGIT_TEST_MAINT_SCHEDULER=\"$scheduler:false\" &&\n> +\t\texport GIT_TEST_MAINT_SCHEDULER &&\n> +\t\ttest_must_fail \\\n> +\t\t\tgit -C failcase maintenance start &&\n> +\t\ttest_must_fail git -C failcase config maintenance.auto || return 1\n> +\tdone\n> +'\n> +\n> test_done\n"}]}