| Message ID | 689d3150e9822eeccac0e1d07c2ba26dac47b4c9.1608585497.git.martin.agren@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 66dc0a3625e9f3ea8e99e5ae4f4d3dded1d5c8c6 |
| Headers | show |
| Series | gc: fix handling of crontab magic markers | expand |
Martin Ågren <martin.agren@gmail.com> writes: > On `git maintenance start`, we add a few entries to the user's cron > table. We wrap our entries using two magic markers, "# BEGIN GIT > MAINTENANCE SCHEDULE" and "# END GIT MAINTENANCE SCHEDULE". At a later > `git maintenance stop`, we will go through the table and remove these > lines. Or rather, we will remove the "BEGIN" marker, the "END" marker > and everything between them. > > Alas, we have a bug in how we detect the "END" marker: we don't. As we > loop through all the lines of the crontab, if we are in the "old > region", i.e., the region we're aiming to remove, we make an early > `continue` and don't get as far as checking for the "END" marker. Thus, > once we've seen our "BEGIN", we remove everything until the end of the > file. > > Rewrite the logic for identifying these markers. There are four cases > that are mutually exclusive: The current line starts a region or it ends > it, or it's firmly within the region, or it's outside of it (and should > be printed). > > Signed-off-by: Martin Ågren <martin.agren@gmail.com> > --- > t/t7900-maintenance.sh | 7 +++++++ > builtin/gc.c | 7 +++---- > 2 files changed, 10 insertions(+), 4 deletions(-) > > diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh > index d1e0c8f830..4bbfce31e9 100755 > --- a/t/t7900-maintenance.sh > +++ b/t/t7900-maintenance.sh > @@ -446,6 +446,13 @@ test_expect_success 'start preserves existing schedule' ' > grep "Important information!" cron.txt > ' > > +test_expect_success 'stop preserves surrounding schedule' ' > + echo "Crucial information!" >>cron.txt && > + GIT_TEST_CRONTAB="test-tool crontab cron.txt" git maintenance stop && 31345d55 (maintenance: extract platform-specific scheduling, 2020-11-24) in ds/maintenance-part-4 needs to adjust this exported variable for the tests to pass in 'seen' Is it just the matter of replacing it with GIT_TEST_MAINT_SCHEDULER="crontab:test-tool crontab ..." or is there more to it? Thanks
Junio C Hamano <gitster@pobox.com> writes: >> +test_expect_success 'stop preserves surrounding schedule' ' >> + echo "Crucial information!" >>cron.txt && >> + GIT_TEST_CRONTAB="test-tool crontab cron.txt" git maintenance stop && > > 31345d55 (maintenance: extract platform-specific scheduling, > 2020-11-24) in ds/maintenance-part-4 needs to adjust this > exported variable for the tests to pass in 'seen' > > Is it just the matter of replacing it with > > GIT_TEST_MAINT_SCHEDULER="crontab:test-tool crontab ..." > > or is there more to it? We'll see soon enough; I noticed the above while viewing https://github.com/git/git/runs/1597371646#step:6:2093 and since then I pushed another round of 'seen' with a non-textual conflict resolution while merging the ds/maintenance-part-4 topic in.
On Tue, Dec 22, 2020 at 5:47 PM Junio C Hamano <gitster@pobox.com> wrote: > Martin Ågren <martin.agren@gmail.com> writes: > > +test_expect_success 'stop preserves surrounding schedule' ' > > + echo "Crucial information!" >>cron.txt && > > + GIT_TEST_CRONTAB="test-tool crontab cron.txt" git maintenance stop && > > 31345d55 (maintenance: extract platform-specific scheduling, > 2020-11-24) in ds/maintenance-part-4 needs to adjust this > exported variable for the tests to pass in 'seen' > > Is it just the matter of replacing it with > GIT_TEST_MAINT_SCHEDULER="crontab:test-tool crontab ..." > or is there more to it? Yes, renaming GIT_TEST_CRONTAB to GIT_TEST_MAINT_SCHEDULER and prepending "crontab:" to the value should be sufficient (per the proposal by [1] and realization by [2]). [1]: https://lore.kernel.org/git/X6+iJNYEbpQjHCb0@flurp.local/ [2]: https://lore.kernel.org/git/4807342b0019be29bb369ed3403a485f0ce9c15d.1605647598.git.gitgitgadget@gmail.com/
On Wed, 23 Dec 2020 at 04:51, Eric Sunshine <sunshine@sunshineco.com> wrote: > > On Tue, Dec 22, 2020 at 5:47 PM Junio C Hamano <gitster@pobox.com> wrote: > > Martin Ågren <martin.agren@gmail.com> writes: > > > +test_expect_success 'stop preserves surrounding schedule' ' > > > + echo "Crucial information!" >>cron.txt && > > > + GIT_TEST_CRONTAB="test-tool crontab cron.txt" git maintenance stop && > > > > 31345d55 (maintenance: extract platform-specific scheduling, > > 2020-11-24) in ds/maintenance-part-4 needs to adjust this > > exported variable for the tests to pass in 'seen' > > > > Is it just the matter of replacing it with > > GIT_TEST_MAINT_SCHEDULER="crontab:test-tool crontab ..." > > or is there more to it? Oh, I never realized this could be a problem. My merge with seen had a textual conflict, but nothing difficult, and the tests passed, so I didn't even stop to think if there was more to it. I clearly didn't notice the environment variable changed both name and value. > Yes, renaming GIT_TEST_CRONTAB to GIT_TEST_MAINT_SCHEDULER and > prepending "crontab:" to the value should be sufficient (per the > proposal by [1] and realization by [2]). > > [1]: https://lore.kernel.org/git/X6+iJNYEbpQjHCb0@flurp.local/ > [2]: https://lore.kernel.org/git/4807342b0019be29bb369ed3403a485f0ce9c15d.1605647598.git.gitgitgadget@gmail.com/ Thanks Junio and Eric for helping out. Martin
Martin Ågren <martin.agren@gmail.com> writes: > On Wed, 23 Dec 2020 at 04:51, Eric Sunshine <sunshine@sunshineco.com> wrote: >> >> On Tue, Dec 22, 2020 at 5:47 PM Junio C Hamano <gitster@pobox.com> wrote: >> > Martin Ågren <martin.agren@gmail.com> writes: >> > > +test_expect_success 'stop preserves surrounding schedule' ' >> > > + echo "Crucial information!" >>cron.txt && >> > > + GIT_TEST_CRONTAB="test-tool crontab cron.txt" git maintenance stop && >> > >> > 31345d55 (maintenance: extract platform-specific scheduling, >> > 2020-11-24) in ds/maintenance-part-4 needs to adjust this >> > exported variable for the tests to pass in 'seen' >> > >> > Is it just the matter of replacing it with >> > GIT_TEST_MAINT_SCHEDULER="crontab:test-tool crontab ..." >> > or is there more to it? > > Oh, I never realized this could be a problem. My merge with seen had a > textual conflict, but nothing difficult, and the tests passed, so I > didn't even stop to think if there was more to it. I clearly didn't > notice the environment variable changed both name and value. > >> Yes, renaming GIT_TEST_CRONTAB to GIT_TEST_MAINT_SCHEDULER and >> prepending "crontab:" to the value should be sufficient (per the >> proposal by [1] and realization by [2]). >> >> [1]: https://lore.kernel.org/git/X6+iJNYEbpQjHCb0@flurp.local/ >> [2]: https://lore.kernel.org/git/4807342b0019be29bb369ed3403a485f0ce9c15d.1605647598.git.gitgitgadget@gmail.com/ > > Thanks Junio and Eric for helping out. Thanks for coming up with the fix.
diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh index d1e0c8f830..4bbfce31e9 100755 --- a/t/t7900-maintenance.sh +++ b/t/t7900-maintenance.sh @@ -446,6 +446,13 @@ test_expect_success 'start preserves existing schedule' ' grep "Important information!" cron.txt ' +test_expect_success 'stop preserves surrounding schedule' ' + echo "Crucial information!" >>cron.txt && + GIT_TEST_CRONTAB="test-tool crontab cron.txt" git maintenance stop && + grep "Important information!" cron.txt && + grep "Crucial information!" cron.txt +' + test_expect_success 'register preserves existing strategy' ' git config maintenance.strategy none && git maintenance register && diff --git a/builtin/gc.c b/builtin/gc.c index b57fda4924..4c24f41852 100644 --- a/builtin/gc.c +++ b/builtin/gc.c @@ -1554,11 +1554,10 @@ static int update_background_schedule(int run_maintenance) 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)) + else if (in_old_region && !strcmp(line.buf, END_LINE)) in_old_region = 0; + else if (!in_old_region) + fprintf(cron_in, "%s\n", line.buf); } if (run_maintenance) {
On `git maintenance start`, we add a few entries to the user's cron table. We wrap our entries using two magic markers, "# BEGIN GIT MAINTENANCE SCHEDULE" and "# END GIT MAINTENANCE SCHEDULE". At a later `git maintenance stop`, we will go through the table and remove these lines. Or rather, we will remove the "BEGIN" marker, the "END" marker and everything between them. Alas, we have a bug in how we detect the "END" marker: we don't. As we loop through all the lines of the crontab, if we are in the "old region", i.e., the region we're aiming to remove, we make an early `continue` and don't get as far as checking for the "END" marker. Thus, once we've seen our "BEGIN", we remove everything until the end of the file. Rewrite the logic for identifying these markers. There are four cases that are mutually exclusive: The current line starts a region or it ends it, or it's firmly within the region, or it's outside of it (and should be printed). Signed-off-by: Martin Ågren <martin.agren@gmail.com> --- t/t7900-maintenance.sh | 7 +++++++ builtin/gc.c | 7 +++---- 2 files changed, 10 insertions(+), 4 deletions(-)