Message ID | 5194f6b1facbd14cc17eea0337c0cc397a2a51fc.1602782524.git.gitgitgadget@gmail.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | Maintenance III: Background maintenance | expand |
On 2020.10.15 17:22, Derrick Stolee via GitGitGadget wrote: > diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh > index 8f383d01d9..7715e40391 100755 > --- a/t/t7900-maintenance.sh > +++ b/t/t7900-maintenance.sh > @@ -315,4 +315,32 @@ test_expect_success 'register and unregister' ' > test_cmp before actual > ' > > +test_expect_success 'start from empty cron table' ' > + GIT_TEST_CRONTAB="test-tool crontab cron.txt" git maintenance start && > + > + # start registers the repo > + git config --get --global maintenance.repo "$(pwd)" && > + > + grep "for-each-repo --config=maintenance.repo maintenance run --schedule=daily" cron.txt && > + grep "for-each-repo --config=maintenance.repo maintenance run --schedule=hourly" cron.txt && > + grep "for-each-repo --config=maintenance.repo maintenance run --schedule=weekly" cron.txt > +' > + > +test_expect_success 'stop from existing schedule' ' > + GIT_TEST_CRONTAB="test-tool crontab cron.txt" git maintenance stop && > + > + # stop does not unregister the repo > + git config --get --global maintenance.repo "$(pwd)" && > + > + # Operation is idempotent > + GIT_TEST_CRONTAB="test-tool crontab cron.txt" git maintenance stop && > + test_must_be_empty cron.txt > +' > + > +test_expect_success 'start preserves existing schedule' ' > + echo "Important information!" >cron.txt && > + GIT_TEST_CRONTAB="test-tool crontab cron.txt" git maintenance start && > + grep "Important information!" cron.txt > +' > + > test_done These two test cases fail when the paths passed to git-config contain ERE metacharacters [similar to the issue addressed in 483a6d9b5da (maintenance: use 'git config --fixed-value', 2020-11-25)]. Since these are already in next, I'm providing a patch to add '--fixed-value' to the git-config calls here as well. -- >8 -- Subject: [PATCH] t7900: use --fixed-value in git-maintenance tests Use --fixed-value in git-config calls in the git-maintenance tests, so that the tests will continue to work even if the repo path contains shell metacharacters. Signed-off-by: Josh Steadmon <steadmon@google.com> --- t/t7900-maintenance.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh index fab0e01c39..41bf523953 100755 --- a/t/t7900-maintenance.sh +++ b/t/t7900-maintenance.sh @@ -422,7 +422,7 @@ test_expect_success 'start from empty cron table' ' GIT_TEST_CRONTAB="test-tool crontab cron.txt" git maintenance start && # start registers the repo - git config --get --global maintenance.repo "$(pwd)" && + git config --get --global --fixed-value maintenance.repo "$(pwd)" && grep "for-each-repo --config=maintenance.repo maintenance run --schedule=daily" cron.txt && grep "for-each-repo --config=maintenance.repo maintenance run --schedule=hourly" cron.txt && @@ -433,7 +433,7 @@ test_expect_success 'stop from existing schedule' ' GIT_TEST_CRONTAB="test-tool crontab cron.txt" git maintenance stop && # stop does not unregister the repo - git config --get --global maintenance.repo "$(pwd)" && + git config --get --global --fixed-value maintenance.repo "$(pwd)" && # Operation is idempotent GIT_TEST_CRONTAB="test-tool crontab cron.txt" git maintenance stop &&
Whoops, had a small think-o while writing the patch message. Fixed
below.
-- >8 --
Subject: [PATCH] t7900: use --fixed-value in git-maintenance tests
Use --fixed-value in git-config calls in the git-maintenance tests, so
that the tests will continue to work even if the repo path contains
regexp metacharacters.
Signed-off-by: Josh Steadmon <steadmon@google.com>
---
t/t7900-maintenance.sh | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
index fab0e01c39..41bf523953 100755
--- a/t/t7900-maintenance.sh
+++ b/t/t7900-maintenance.sh
@@ -422,7 +422,7 @@ test_expect_success 'start from empty cron table' '
GIT_TEST_CRONTAB="test-tool crontab cron.txt" git maintenance start &&
# start registers the repo
- git config --get --global maintenance.repo "$(pwd)" &&
+ git config --get --global --fixed-value maintenance.repo "$(pwd)" &&
grep "for-each-repo --config=maintenance.repo maintenance run --schedule=daily" cron.txt &&
grep "for-each-repo --config=maintenance.repo maintenance run --schedule=hourly" cron.txt &&
@@ -433,7 +433,7 @@ test_expect_success 'stop from existing schedule' '
GIT_TEST_CRONTAB="test-tool crontab cron.txt" git maintenance stop &&
# stop does not unregister the repo
- git config --get --global maintenance.repo "$(pwd)" &&
+ git config --get --global --fixed-value maintenance.repo "$(pwd)" &&
# Operation is idempotent
GIT_TEST_CRONTAB="test-tool crontab cron.txt" git maintenance stop &&
On 12/9/2020 2:16 PM, Josh Steadmon wrote: > Whoops, had a small think-o while writing the patch message. Fixed > below. > > -- >8 -- > Subject: [PATCH] t7900: use --fixed-value in git-maintenance tests > > Use --fixed-value in git-config calls in the git-maintenance tests, so > that the tests will continue to work even if the repo path contains > regexp metacharacters. > > Signed-off-by: Josh Steadmon <steadmon@google.com> > --- > t/t7900-maintenance.sh | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh > index fab0e01c39..41bf523953 100755 > --- a/t/t7900-maintenance.sh > +++ b/t/t7900-maintenance.sh > @@ -422,7 +422,7 @@ test_expect_success 'start from empty cron table' ' > GIT_TEST_CRONTAB="test-tool crontab cron.txt" git maintenance start && > > # start registers the repo > - git config --get --global maintenance.repo "$(pwd)" && > + git config --get --global --fixed-value maintenance.repo "$(pwd)" && > > grep "for-each-repo --config=maintenance.repo maintenance run --schedule=daily" cron.txt && > grep "for-each-repo --config=maintenance.repo maintenance run --schedule=hourly" cron.txt && > @@ -433,7 +433,7 @@ test_expect_success 'stop from existing schedule' ' > GIT_TEST_CRONTAB="test-tool crontab cron.txt" git maintenance stop && > > # stop does not unregister the repo > - git config --get --global maintenance.repo "$(pwd)" && > + git config --get --global --fixed-value maintenance.repo "$(pwd)" && > > # Operation is idempotent > GIT_TEST_CRONTAB="test-tool crontab cron.txt" git maintenance stop && Thank you for this. While I went to make sure the maintenance builtin worked properly, I forgot to check the rest of the test script worked as well. This is a good way to fix that. Thanks, -Stolee
Josh Steadmon <steadmon@google.com> writes: > # start registers the repo > - git config --get --global maintenance.repo "$(pwd)" && > + git config --get --global --fixed-value maintenance.repo "$(pwd)" && The rewrite makes it better than the original, but I wonder why the original did not do a more obvious git config --get maintenance.repo >actual && pwd >expect && test_cmp expect actual > # stop does not unregister the repo > - git config --get --global maintenance.repo "$(pwd)" && > + git config --get --global --fixed-value maintenance.repo "$(pwd)" && Ditto.
On 12/9/2020 7:13 PM, Junio C Hamano wrote: > Josh Steadmon <steadmon@google.com> writes: > >> # start registers the repo >> - git config --get --global maintenance.repo "$(pwd)" && >> + git config --get --global --fixed-value maintenance.repo "$(pwd)" && > > The rewrite makes it better than the original, but I wonder why the > original did not do a more obvious maintenance.repo is a multi-valued config setting, so it is possible that there are multiple existing values. Hence the reason for needing the value filter. Thanks, -Stolee
Derrick Stolee <stolee@gmail.com> writes: > On 12/9/2020 7:13 PM, Junio C Hamano wrote: >> Josh Steadmon <steadmon@google.com> writes: >> >>> # start registers the repo >>> - git config --get --global maintenance.repo "$(pwd)" && >>> + git config --get --global --fixed-value maintenance.repo "$(pwd)" && >> >> The rewrite makes it better than the original, but I wonder why the >> original did not do a more obvious > > maintenance.repo is a multi-valued config setting, so it is possible > that there are multiple existing values. Hence the reason for needing > the value filter. I do not quite get it. You mean as long as $(pwd) appears, you do not care what other value appear on the variable? Aren't we control of what repositories have been registered to the system at this point in the test sequence? It's not wrong per-se to use "does this value exist for the key?", especially with the --fixed-value option. It somehow just felt a bit unusual to me. In any case, thanks for the metacharacter fix. That is now on 'master' so the previous breakages are all gone with Josh's fix. Thanks, both.
diff --git a/Documentation/git-maintenance.txt b/Documentation/git-maintenance.txt index 1c59fd0cb5..7628a6d157 100644 --- a/Documentation/git-maintenance.txt +++ b/Documentation/git-maintenance.txt @@ -45,6 +45,17 @@ run:: config options are true. By default, only `maintenance.gc.enabled` is true. +start:: + Start running maintenance on the current repository. This performs + the same config updates as the `register` subcommand, then updates + the background scheduler to run `git maintenance run --scheduled` + on an hourly basis. + +stop:: + Halt the background maintenance schedule. The current repository + is not removed from the list of maintained repositories, in case + the background maintenance is restarted later. + unregister:: Remove the current repository from background maintenance. This only removes the repository from the configured list. It does not diff --git a/Makefile b/Makefile index 7c588ff036..c39b39bd7d 100644 --- a/Makefile +++ b/Makefile @@ -690,6 +690,7 @@ TEST_BUILTINS_OBJS += test-advise.o TEST_BUILTINS_OBJS += test-bloom.o TEST_BUILTINS_OBJS += test-chmtime.o TEST_BUILTINS_OBJS += test-config.o +TEST_BUILTINS_OBJS += test-crontab.o TEST_BUILTINS_OBJS += test-ctype.o TEST_BUILTINS_OBJS += test-date.o TEST_BUILTINS_OBJS += test-delta.o diff --git a/builtin/gc.c b/builtin/gc.c index edf1d35ce5..a387f46585 100644 --- a/builtin/gc.c +++ b/builtin/gc.c @@ -31,6 +31,7 @@ #include "refs.h" #include "remote.h" #include "object-store.h" +#include "exec-cmd.h" #define FAILED_RUN "failed to run %s" @@ -1456,6 +1457,125 @@ static int maintenance_unregister(void) return run_command(&config_unset); } +#define BEGIN_LINE "# BEGIN GIT MAINTENANCE SCHEDULE" +#define END_LINE "# END GIT MAINTENANCE SCHEDULE" + +static int update_background_schedule(int run_maintenance) +{ + int result = 0; + int in_old_region = 0; + struct child_process crontab_list = CHILD_PROCESS_INIT; + struct child_process crontab_edit = CHILD_PROCESS_INIT; + FILE *cron_list, *cron_in; + const char *crontab_name; + struct strbuf line = STRBUF_INIT; + struct lock_file lk; + char *lock_path = xstrfmt("%s/schedule", the_repository->objects->odb->path); + + if (hold_lock_file_for_update(&lk, lock_path, LOCK_NO_DEREF) < 0) + return error(_("another process is scheduling background maintenance")); + + crontab_name = getenv("GIT_TEST_CRONTAB"); + if (!crontab_name) + crontab_name = "crontab"; + + strvec_split(&crontab_list.args, crontab_name); + strvec_push(&crontab_list.args, "-l"); + crontab_list.in = -1; + crontab_list.out = dup(lk.tempfile->fd); + crontab_list.git_cmd = 0; + + if (start_command(&crontab_list)) { + result = error(_("failed to run 'crontab -l'; your system might not support 'cron'")); + goto cleanup; + } + + /* Ignore exit code, as an empty crontab will return error. */ + finish_command(&crontab_list); + + /* + * Read from the .lock file, filtering out the old + * schedule while appending the new schedule. + */ + cron_list = fdopen(lk.tempfile->fd, "r"); + rewind(cron_list); + + strvec_split(&crontab_edit.args, crontab_name); + crontab_edit.in = -1; + crontab_edit.git_cmd = 0; + + if (start_command(&crontab_edit)) { + result = error(_("failed to run 'crontab'; your system might not support 'cron'")); + goto cleanup; + } + + cron_in = fdopen(crontab_edit.in, "w"); + if (!cron_in) { + result = error(_("failed to open stdin of 'crontab'")); + goto done_editing; + } + + while (!strbuf_getline_lf(&line, cron_list)) { + if (!in_old_region && !strcmp(line.buf, BEGIN_LINE)) + in_old_region = 1; + if (in_old_region) + continue; + fprintf(cron_in, "%s\n", line.buf); + if (in_old_region && !strcmp(line.buf, END_LINE)) + in_old_region = 0; + } + + if (run_maintenance) { + struct strbuf line_format = STRBUF_INIT; + const char *exec_path = git_exec_path(); + + fprintf(cron_in, "%s\n", BEGIN_LINE); + fprintf(cron_in, + "# The following schedule was created by Git\n"); + fprintf(cron_in, "# Any edits made in this region might be\n"); + fprintf(cron_in, + "# replaced in the future by a Git command.\n\n"); + + strbuf_addf(&line_format, + "%%s %%s * * %%s \"%s/git\" --exec-path=\"%s\" for-each-repo --config=maintenance.repo maintenance run --schedule=%%s\n", + exec_path, exec_path); + fprintf(cron_in, line_format.buf, "0", "1-23", "*", "hourly"); + fprintf(cron_in, line_format.buf, "0", "0", "1-6", "daily"); + fprintf(cron_in, line_format.buf, "0", "0", "0", "weekly"); + strbuf_release(&line_format); + + fprintf(cron_in, "\n%s\n", END_LINE); + } + + fflush(cron_in); + fclose(cron_in); + close(crontab_edit.in); + +done_editing: + if (finish_command(&crontab_edit)) { + result = error(_("'crontab' died")); + goto cleanup; + } + fclose(cron_list); + +cleanup: + rollback_lock_file(&lk); + return result; +} + +static int maintenance_start(void) +{ + if (maintenance_register()) + warning(_("failed to add repo to global config")); + + return update_background_schedule(1); +} + +static int maintenance_stop(void) +{ + return update_background_schedule(0); +} + static const char builtin_maintenance_usage[] = N_("git maintenance <subcommand> [<options>]"); int cmd_maintenance(int argc, const char **argv, const char *prefix) @@ -1466,6 +1586,10 @@ int cmd_maintenance(int argc, const char **argv, const char *prefix) if (!strcmp(argv[1], "run")) return maintenance_run(argc - 1, argv + 1, prefix); + if (!strcmp(argv[1], "start")) + return maintenance_start(); + if (!strcmp(argv[1], "stop")) + return maintenance_stop(); if (!strcmp(argv[1], "register")) return maintenance_register(); if (!strcmp(argv[1], "unregister")) diff --git a/t/helper/test-crontab.c b/t/helper/test-crontab.c new file mode 100644 index 0000000000..e7c0137a47 --- /dev/null +++ b/t/helper/test-crontab.c @@ -0,0 +1,35 @@ +#include "test-tool.h" +#include "cache.h" + +/* + * Usage: test-tool cron <file> [-l] + * + * If -l is specified, then write the contents of <file> to stdout. + * Otherwise, write from stdin into <file>. + */ +int cmd__crontab(int argc, const char **argv) +{ + int a; + FILE *from, *to; + + if (argc == 3 && !strcmp(argv[2], "-l")) { + from = fopen(argv[1], "r"); + if (!from) + return 0; + to = stdout; + } else if (argc == 2) { + from = stdin; + to = fopen(argv[1], "w"); + } else + return error("unknown arguments"); + + while ((a = fgetc(from)) != EOF) + fputc(a, to); + + if (argc == 3) + fclose(from); + else + fclose(to); + + return 0; +} diff --git a/t/helper/test-tool.c b/t/helper/test-tool.c index 590b2efca7..432b49d948 100644 --- a/t/helper/test-tool.c +++ b/t/helper/test-tool.c @@ -18,6 +18,7 @@ static struct test_cmd cmds[] = { { "bloom", cmd__bloom }, { "chmtime", cmd__chmtime }, { "config", cmd__config }, + { "crontab", cmd__crontab }, { "ctype", cmd__ctype }, { "date", cmd__date }, { "delta", cmd__delta }, diff --git a/t/helper/test-tool.h b/t/helper/test-tool.h index ddc8e990e9..7c3281e071 100644 --- a/t/helper/test-tool.h +++ b/t/helper/test-tool.h @@ -8,6 +8,7 @@ int cmd__advise_if_enabled(int argc, const char **argv); int cmd__bloom(int argc, const char **argv); int cmd__chmtime(int argc, const char **argv); int cmd__config(int argc, const char **argv); +int cmd__crontab(int argc, const char **argv); int cmd__ctype(int argc, const char **argv); int cmd__date(int argc, const char **argv); int cmd__delta(int argc, const char **argv); diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh index 8f383d01d9..7715e40391 100755 --- a/t/t7900-maintenance.sh +++ b/t/t7900-maintenance.sh @@ -315,4 +315,32 @@ test_expect_success 'register and unregister' ' test_cmp before actual ' +test_expect_success 'start from empty cron table' ' + GIT_TEST_CRONTAB="test-tool crontab cron.txt" git maintenance start && + + # start registers the repo + git config --get --global maintenance.repo "$(pwd)" && + + grep "for-each-repo --config=maintenance.repo maintenance run --schedule=daily" cron.txt && + grep "for-each-repo --config=maintenance.repo maintenance run --schedule=hourly" cron.txt && + grep "for-each-repo --config=maintenance.repo maintenance run --schedule=weekly" cron.txt +' + +test_expect_success 'stop from existing schedule' ' + GIT_TEST_CRONTAB="test-tool crontab cron.txt" git maintenance stop && + + # stop does not unregister the repo + git config --get --global maintenance.repo "$(pwd)" && + + # Operation is idempotent + GIT_TEST_CRONTAB="test-tool crontab cron.txt" git maintenance stop && + test_must_be_empty cron.txt +' + +test_expect_success 'start preserves existing schedule' ' + echo "Important information!" >cron.txt && + GIT_TEST_CRONTAB="test-tool crontab cron.txt" git maintenance start && + grep "Important information!" cron.txt +' + test_done diff --git a/t/test-lib.sh b/t/test-lib.sh index ef31f40037..4a60d1ed76 100644 --- a/t/test-lib.sh +++ b/t/test-lib.sh @@ -1702,3 +1702,9 @@ test_lazy_prereq SHA1 ' test_lazy_prereq REBASE_P ' test -z "$GIT_TEST_SKIP_REBASE_P" ' + +# Ensure that no test accidentally triggers a Git command +# that runs 'crontab', affecting a user's cron schedule. +# Tests that verify the cron integration must set this locally +# to avoid errors. +GIT_TEST_CRONTAB="exit 1"