{"thread":{"id":"54865","subject":"[PATCH 1/3] git-maintenance.txt: add missing word","startedAt":"2020-12-21T21:27:51Z","lastAt":"2020-12-23T20:01:50Z","messageCount":10,"participants":["Martin Ågren","Derrick Stolee","Junio C Hamano","Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"412775","messageId":"7bd059244087016d2af47c6329e46191bebb168e.1608585497.git.martin.agren@gmail.com","threadId":"54865","inReplyTo":"cover.1608585497.git.martin.agren@gmail.com","subject":"[PATCH 1/3] git-maintenance.txt: add missing word","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2020-12-21T21:26:31Z","receivedAt":"2020-12-21T21:27:51Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"Add a missing \"a\" before \"bunch\".\n\nSigned-off-by: Martin Ågren \n---\n Documentation/git-maintenance.txt | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/Documentation/git-maintenance.txt b/Documentation/git-maintenance.txt\nindex 6fec1eb8dc..d1f9b5172d 100644\n--- a/Documentation/git-maintenance.txt\n+++ b/Documentation/git-maintenance.txt\n@@ -101,7 +101,7 @@ This is done to avoid disrupting the remote-tracking branches. The end users\n expect these refs to stay unmoved unless they initiate a fetch. With prefetch\n task, however, the objects necessary to complete a later real fetch would\n already be obtained, so the real fetch would go faster. In the ideal case,\n-it will just become an update to bunch of remote-tracking branches without\n+it will just become an update to a bunch of remote-tracking branches without\n any object transfer.\n \n gc::\n-- \n2.30.0.rc1\n\n"},{"id":"412776","messageId":"cover.1608585497.git.martin.agren@gmail.com","threadId":"54865","inReplyTo":null,"subject":"[PATCH 0/3] gc: fix handling of crontab magic markers","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2020-12-21T21:26:30Z","receivedAt":"2020-12-21T21:27:51Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"This is a fix for a new bug in the v2.30 cycle:\n\n`git maintenance [start|stop]` add and/or remove entries to/from the\nuser's cron table. When inserting entries, we add magic \"BEGIN\" and\n\"END\" markers. When we later removes these entries, we remove everything\nfrom \"BEGIN\" to ... the end of file. A realistic scenario for hitting\nthis bug is\n\n $ git maintenance start\n $ crontab -e # add stuff at the end of the file\n $ git maintenance stop\n $ crontab -l # stuff from above is gone\n\nThe second patch is the actual fix. The first patch is just a very minor\nfix to the documentation. The third patch future-proofs the magic\nmarkers, so that we can be reasonably sure that all future versions of\nGit stick to these strings.\n\nMartin Ågren (3):\n git-maintenance.txt: add missing word\n gc: fix handling of crontab magic markers\n t7900-maintenance: test for magic markers\n\n Documentation/git-maintenance.txt | 2 +-\n t/t7900-maintenance.sh | 16 ++++++++++++++++\n builtin/gc.c | 7 +++----\n 3 files changed, 20 insertions(+), 5 deletions(-)\n\n-- \n2.30.0.rc1\n\n"},{"id":"412777","messageId":"938ac9a831bacd70e3c3ab7756ed98afa6338554.1608585497.git.martin.agren@gmail.com","threadId":"54865","inReplyTo":"cover.1608585497.git.martin.agren@gmail.com","subject":"[PATCH 3/3] t7900-maintenance: test for magic markers","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2020-12-21T21:26:33Z","receivedAt":"2020-12-21T21:27:51Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"When we insert our \"BEGIN\" and \"END\" markers into the cron table, it's\nso that a Git version from many years into the future would be able to\nidentify this region in the cron table. Let's add a test to make sure\nthat these markers don't ever change.\n\nSigned-off-by: Martin Ågren \n---\n t/t7900-maintenance.sh | 9 +++++++++\n 1 file changed, 9 insertions(+)\n\ndiff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh\nindex 4bbfce31e9..99bf0c7582 100755\n--- a/t/t7900-maintenance.sh\n+++ b/t/t7900-maintenance.sh\n@@ -446,6 +446,15 @@ test_expect_success 'start preserves existing schedule' '\n \tgrep \"Important information!\" cron.txt\n '\n \n+test_expect_success 'magic markers are correct' '\n+\tgrep \"GIT MAINTENANCE SCHEDULE\" cron.txt >actual &&\n+\tcat >expect <<-\\EOF &&\n+\t# BEGIN GIT MAINTENANCE SCHEDULE\n+\t# END GIT MAINTENANCE SCHEDULE\n+\tEOF\n+\ttest_cmp actual expect\n+'\n+\n test_expect_success 'stop preserves surrounding schedule' '\n \techo \"Crucial information!\" >>cron.txt &&\n \tGIT_TEST_CRONTAB=\"test-tool crontab cron.txt\" git maintenance stop &&\n-- \n2.30.0.rc1\n\n"},{"id":"412778","messageId":"689d3150e9822eeccac0e1d07c2ba26dac47b4c9.1608585497.git.martin.agren@gmail.com","threadId":"54865","inReplyTo":"cover.1608585497.git.martin.agren@gmail.com","subject":"[PATCH 2/3] gc: fix handling of crontab magic markers","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2020-12-21T21:26:32Z","receivedAt":"2020-12-21T21:27:51Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On `git maintenance start`, we add a few entries to the user's cron\ntable. We wrap our entries using two magic markers, \"# BEGIN GIT\nMAINTENANCE SCHEDULE\" and \"# END GIT MAINTENANCE SCHEDULE\". At a later\n`git maintenance stop`, we will go through the table and remove these\nlines. Or rather, we will remove the \"BEGIN\" marker, the \"END\" marker\nand everything between them.\n\nAlas, we have a bug in how we detect the \"END\" marker: we don't. As we\nloop through all the lines of the crontab, if we are in the \"old\nregion\", i.e., the region we're aiming to remove, we make an early\n`continue` and don't get as far as checking for the \"END\" marker. Thus,\nonce we've seen our \"BEGIN\", we remove everything until the end of the\nfile.\n\nRewrite the logic for identifying these markers. There are four cases\nthat are mutually exclusive: The current line starts a region or it ends\nit, or it's firmly within the region, or it's outside of it (and should\nbe printed).\n\nSigned-off-by: Martin Ågren \n---\n t/t7900-maintenance.sh | 7 +++++++\n builtin/gc.c | 7 +++----\n 2 files changed, 10 insertions(+), 4 deletions(-)\n\ndiff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh\nindex d1e0c8f830..4bbfce31e9 100755\n--- a/t/t7900-maintenance.sh\n+++ b/t/t7900-maintenance.sh\n@@ -446,6 +446,13 @@ test_expect_success 'start preserves existing schedule' '\n \tgrep \"Important information!\" cron.txt\n '\n \n+test_expect_success 'stop preserves surrounding schedule' '\n+\techo \"Crucial information!\" >>cron.txt &&\n+\tGIT_TEST_CRONTAB=\"test-tool crontab cron.txt\" git maintenance stop &&\n+\tgrep \"Important information!\" cron.txt &&\n+\tgrep \"Crucial information!\" cron.txt\n+'\n+\n test_expect_success 'register preserves existing strategy' '\n \tgit config maintenance.strategy none &&\n \tgit maintenance register &&\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex b57fda4924..4c24f41852 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -1554,11 +1554,10 @@ static int update_background_schedule(int run_maintenance)\n \twhile (!strbuf_getline_lf(&line, cron_list)) {\n \t\tif (!in_old_region && !strcmp(line.buf, BEGIN_LINE))\n \t\t\tin_old_region = 1;\n-\t\tif (in_old_region)\n-\t\t\tcontinue;\n-\t\tfprintf(cron_in, \"%s\\n\", line.buf);\n-\t\tif (in_old_region && !strcmp(line.buf, END_LINE))\n+\t\telse if (in_old_region && !strcmp(line.buf, END_LINE))\n \t\t\tin_old_region = 0;\n+\t\telse if (!in_old_region)\n+\t\t\tfprintf(cron_in, \"%s\\n\", line.buf);\n \t}\n \n \tif (run_maintenance) {\n-- \n2.30.0.rc1\n\n"},{"id":"412780","messageId":"8ee65ef1-a68b-e881-70af-cb2a492331ca@gmail.com","threadId":"54865","inReplyTo":"cover.1608585497.git.martin.agren@gmail.com","subject":"Re: [PATCH 0/3] gc: fix handling of crontab magic markers","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2020-12-21T21:54:08Z","receivedAt":"2020-12-21T21:54:51Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 12/21/2020 4:26 PM, Martin Ågren wrote:\n> This is a fix for a new bug in the v2.30 cycle:\n> \n> `git maintenance [start|stop]` add and/or remove entries to/from the\n> user's cron table. When inserting entries, we add magic \"BEGIN\" and\n> \"END\" markers. When we later removes these entries, we remove everything\n> from \"BEGIN\" to ... the end of file. A realistic scenario for hitting\n> this bug is\n> \n> $ git maintenance start\n> $ crontab -e # add stuff at the end of the file\n> $ git maintenance stop\n> $ crontab -l # stuff from above is gone\n> \n> The second patch is the actual fix. The first patch is just a very minor\n> fix to the documentation. The third patch future-proofs the magic\n> markers, so that we can be reasonably sure that all future versions of\n> Git stick to these strings.\n\nThank you for the attention to detail here. I think this series\nis succinct and completely correct. Apologies for missing the\nobvious \"test after the region, too!\"\n\nThanks,\n-Stolee\n"},{"id":"412868","messageId":"xmqqsg7xfoj9.fsf@gitster.c.googlers.com","threadId":"54865","inReplyTo":"689d3150e9822eeccac0e1d07c2ba26dac47b4c9.1608585497.git.martin.agren@gmail.com","subject":"Re: [PATCH 2/3] gc: fix handling of crontab magic markers","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-12-22T22:45:30Z","receivedAt":"2020-12-22T22:46:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Martin Ågren writes:\n\n> On `git maintenance start`, we add a few entries to the user's cron\n> table. We wrap our entries using two magic markers, \"# BEGIN GIT\n> MAINTENANCE SCHEDULE\" and \"# END GIT MAINTENANCE SCHEDULE\". At a later\n> `git maintenance stop`, we will go through the table and remove these\n> lines. Or rather, we will remove the \"BEGIN\" marker, the \"END\" marker\n> and everything between them.\n>\n> Alas, we have a bug in how we detect the \"END\" marker: we don't. As we\n> loop through all the lines of the crontab, if we are in the \"old\n> region\", i.e., the region we're aiming to remove, we make an early\n> `continue` and don't get as far as checking for the \"END\" marker. Thus,\n> once we've seen our \"BEGIN\", we remove everything until the end of the\n> file.\n>\n> Rewrite the logic for identifying these markers. There are four cases\n> that are mutually exclusive: The current line starts a region or it ends\n> it, or it's firmly within the region, or it's outside of it (and should\n> be printed).\n>\n> Signed-off-by: Martin Ågren \n> ---\n> t/t7900-maintenance.sh | 7 +++++++\n> builtin/gc.c | 7 +++----\n> 2 files changed, 10 insertions(+), 4 deletions(-)\n>\n> diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh\n> index d1e0c8f830..4bbfce31e9 100755\n> --- a/t/t7900-maintenance.sh\n> +++ b/t/t7900-maintenance.sh\n> @@ -446,6 +446,13 @@ test_expect_success 'start preserves existing schedule' '\n> \tgrep \"Important information!\" cron.txt\n> '\n> \n> +test_expect_success 'stop preserves surrounding schedule' '\n> +\techo \"Crucial information!\" >>cron.txt &&\n> +\tGIT_TEST_CRONTAB=\"test-tool crontab cron.txt\" git maintenance stop &&\n\n31345d55 (maintenance: extract platform-specific scheduling,\n2020-11-24) in ds/maintenance-part-4 needs to adjust this\nexported variable for the tests to pass in 'seen'\n\nIs it just the matter of replacing it with\n\n\tGIT_TEST_MAINT_SCHEDULER=\"crontab:test-tool crontab ...\"\n\nor is there more to it?\n\nThanks\n"},{"id":"412870","messageId":"xmqqo8ilfmty.fsf@gitster.c.googlers.com","threadId":"54865","inReplyTo":"xmqqsg7xfoj9.fsf@gitster.c.googlers.com","subject":"Re: [PATCH 2/3] gc: fix handling of crontab magic markers","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-12-22T23:22:17Z","receivedAt":"2020-12-22T23:23:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n>> +test_expect_success 'stop preserves surrounding schedule' '\n>> +\techo \"Crucial information!\" >>cron.txt &&\n>> +\tGIT_TEST_CRONTAB=\"test-tool crontab cron.txt\" git maintenance stop &&\n>\n> 31345d55 (maintenance: extract platform-specific scheduling,\n> 2020-11-24) in ds/maintenance-part-4 needs to adjust this\n> exported variable for the tests to pass in 'seen'\n>\n> Is it just the matter of replacing it with\n>\n> \tGIT_TEST_MAINT_SCHEDULER=\"crontab:test-tool crontab ...\"\n>\n> or is there more to it?\n\nWe'll see soon enough; I noticed the above while viewing\n\n https://github.com/git/git/runs/1597371646#step:6:2093\n\nand since then I pushed another round of 'seen' with a non-textual\nconflict resolution while merging the ds/maintenance-part-4 topic\nin.\n\n"},{"id":"412904","messageId":"CAPig+cRmatC0Q22XBBx-ouzNSn5uiarTes1tSehGZuBWucSt1g@mail.gmail.com","threadId":"54865","inReplyTo":"xmqqsg7xfoj9.fsf@gitster.c.googlers.com","subject":"Re: [PATCH 2/3] gc: fix handling of crontab magic markers","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-12-23T03:50:52Z","receivedAt":"2020-12-23T03:51:47Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Dec 22, 2020 at 5:47 PM Junio C Hamano wrote:\n> Martin Ågren writes:\n> > +test_expect_success 'stop preserves surrounding schedule' '\n> > + echo \"Crucial information!\" >>cron.txt &&\n> > + GIT_TEST_CRONTAB=\"test-tool crontab cron.txt\" git maintenance stop &&\n>\n> 31345d55 (maintenance: extract platform-specific scheduling,\n> 2020-11-24) in ds/maintenance-part-4 needs to adjust this\n> exported variable for the tests to pass in 'seen'\n>\n> Is it just the matter of replacing it with\n> GIT_TEST_MAINT_SCHEDULER=\"crontab:test-tool crontab ...\"\n> or is there more to it?\n\nYes, renaming GIT_TEST_CRONTAB to GIT_TEST_MAINT_SCHEDULER and\nprepending \"crontab:\" to the value should be sufficient (per the\nproposal by [1] and realization by [2]).\n\n[1]: https://lore.kernel.org/git/X6+iJNYEbpQjHCb0@flurp.local/\n[2]: https://lore.kernel.org/git/4807342b0019be29bb369ed3403a485f0ce9c15d.1605647598.git.gitgitgadget@gmail.com/\n"},{"id":"412916","messageId":"CAN0heSrWjpdcPN7v9kS-A43TQ5RW_8kW9x_S7c70vUW5cX-fUA@mail.gmail.com","threadId":"54865","inReplyTo":"CAPig+cRmatC0Q22XBBx-ouzNSn5uiarTes1tSehGZuBWucSt1g@mail.gmail.com","subject":"Re: [PATCH 2/3] gc: fix handling of crontab magic markers","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2020-12-23T10:06:48Z","receivedAt":"2020-12-23T10:07:58Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On Wed, 23 Dec 2020 at 04:51, Eric Sunshine wrote:\n>\n> On Tue, Dec 22, 2020 at 5:47 PM Junio C Hamano wrote:\n> > Martin Ågren writes:\n> > > +test_expect_success 'stop preserves surrounding schedule' '\n> > > + echo \"Crucial information!\" >>cron.txt &&\n> > > + GIT_TEST_CRONTAB=\"test-tool crontab cron.txt\" git maintenance stop &&\n> >\n> > 31345d55 (maintenance: extract platform-specific scheduling,\n> > 2020-11-24) in ds/maintenance-part-4 needs to adjust this\n> > exported variable for the tests to pass in 'seen'\n> >\n> > Is it just the matter of replacing it with\n> > GIT_TEST_MAINT_SCHEDULER=\"crontab:test-tool crontab ...\"\n> > or is there more to it?\n\nOh, I never realized this could be a problem. My merge with seen had a\ntextual conflict, but nothing difficult, and the tests passed, so I\ndidn't even stop to think if there was more to it. I clearly didn't\nnotice the environment variable changed both name and value.\n\n> Yes, renaming GIT_TEST_CRONTAB to GIT_TEST_MAINT_SCHEDULER and\n> prepending \"crontab:\" to the value should be sufficient (per the\n> proposal by [1] and realization by [2]).\n>\n> [1]: https://lore.kernel.org/git/X6+iJNYEbpQjHCb0@flurp.local/\n> [2]: https://lore.kernel.org/git/4807342b0019be29bb369ed3403a485f0ce9c15d.1605647598.git.gitgitgadget@gmail.com/\n\nThanks Junio and Eric for helping out.\n\nMartin\n"},{"id":"412942","messageId":"xmqqk0t88f82.fsf@gitster.c.googlers.com","threadId":"54865","inReplyTo":"CAN0heSrWjpdcPN7v9kS-A43TQ5RW_8kW9x_S7c70vUW5cX-fUA@mail.gmail.com","subject":"Re: [PATCH 2/3] gc: fix handling of crontab magic markers","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-12-23T20:00:45Z","receivedAt":"2020-12-23T20:01:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Martin Ågren writes:\n\n> On Wed, 23 Dec 2020 at 04:51, Eric Sunshine wrote:\n>>\n>> On Tue, Dec 22, 2020 at 5:47 PM Junio C Hamano wrote:\n>> > Martin Ågren writes:\n>> > > +test_expect_success 'stop preserves surrounding schedule' '\n>> > > + echo \"Crucial information!\" >>cron.txt &&\n>> > > + GIT_TEST_CRONTAB=\"test-tool crontab cron.txt\" git maintenance stop &&\n>> >\n>> > 31345d55 (maintenance: extract platform-specific scheduling,\n>> > 2020-11-24) in ds/maintenance-part-4 needs to adjust this\n>> > exported variable for the tests to pass in 'seen'\n>> >\n>> > Is it just the matter of replacing it with\n>> > GIT_TEST_MAINT_SCHEDULER=\"crontab:test-tool crontab ...\"\n>> > or is there more to it?\n>\n> Oh, I never realized this could be a problem. My merge with seen had a\n> textual conflict, but nothing difficult, and the tests passed, so I\n> didn't even stop to think if there was more to it. I clearly didn't\n> notice the environment variable changed both name and value.\n>\n>> Yes, renaming GIT_TEST_CRONTAB to GIT_TEST_MAINT_SCHEDULER and\n>> prepending \"crontab:\" to the value should be sufficient (per the\n>> proposal by [1] and realization by [2]).\n>>\n>> [1]: https://lore.kernel.org/git/X6+iJNYEbpQjHCb0@flurp.local/\n>> [2]: https://lore.kernel.org/git/4807342b0019be29bb369ed3403a485f0ce9c15d.1605647598.git.gitgitgadget@gmail.com/\n>\n> Thanks Junio and Eric for helping out.\n\nThanks for coming up with the fix.\n"}]}