[0/3] gc: fix handling of crontab magic markers

Message

Martin Ågren Dec. 21, 2020, 9:26 p.m. UTC

This is a fix for a new bug in the v2.30 cycle:

`git maintenance [start|stop]` add and/or remove entries to/from the
user's cron table. When inserting entries, we add magic "BEGIN" and
"END" markers. When we later removes these entries, we remove everything
from "BEGIN" to ... the end of file. A realistic scenario for hitting
this bug is

  $ git maintenance start
  $ crontab -e # add stuff at the end of the file
  $ git maintenance stop
  $ crontab -l # stuff from above is gone

The second patch is the actual fix. The first patch is just a very minor
fix to the documentation. The third patch future-proofs the magic
markers, so that we can be reasonably sure that all future versions of
Git stick to these strings.

Martin Ågren (3):
  git-maintenance.txt: add missing word
  gc: fix handling of crontab magic markers
  t7900-maintenance: test for magic markers

 Documentation/git-maintenance.txt |  2 +-
 t/t7900-maintenance.sh            | 16 ++++++++++++++++
 builtin/gc.c                      |  7 +++----
 3 files changed, 20 insertions(+), 5 deletions(-)

Comments

Derrick Stolee Dec. 21, 2020, 9:54 p.m. UTC | #1

On 12/21/2020 4:26 PM, Martin Ågren wrote:
> This is a fix for a new bug in the v2.30 cycle:
> 
> `git maintenance [start|stop]` add and/or remove entries to/from the
> user's cron table. When inserting entries, we add magic "BEGIN" and
> "END" markers. When we later removes these entries, we remove everything
> from "BEGIN" to ... the end of file. A realistic scenario for hitting
> this bug is
> 
>   $ git maintenance start
>   $ crontab -e # add stuff at the end of the file
>   $ git maintenance stop
>   $ crontab -l # stuff from above is gone
> 
> The second patch is the actual fix. The first patch is just a very minor
> fix to the documentation. The third patch future-proofs the magic
> markers, so that we can be reasonably sure that all future versions of
> Git stick to these strings.

Thank you for the attention to detail here. I think this series
is succinct and completely correct. Apologies for missing the
obvious "test after the region, too!"

Thanks,
-Stolee