Re: [PATCH 1/1] maintenance: fix a SEGFAULT when no repository

[Rafael Silva <rafaeloliveira.cs@gmail.com>]

On Tue, Nov 24, 2020 at 12:22:40PM -0500, Derrick Stolee wrote:
> On 11/24/2020 11:44 AM, Rafael Silva wrote:
> > The "git maintenance run" and "git maintenance start" commands holds a
> > file-based lock at the .git/maintenance.lock and .git/schedule.lock
> > respectively. These locks are used to ensure only one maintenance process
> > is executed at the time as both operations involves writing data into
> > the git repository.
> > 
> > The path to the lock file is built using the "the_repository->objects->odb->path"
> > that results in SEGFAULT when we have no repository available as
> > "the_repository->objects->odb" is set to NULL.
> > 
> > Let's teach the maintenance_run_tasks() and update_background_schedule() to return
> > an error and fails the command when we have no repository available.
> 
> Thank you for noticing this problem, and for a quick fix.
> 
> While I don't necessarily have a problem with this approach, perhaps
> it would be more robust to change the options in git.c to require a
> GIT_DIR, as in this diff?
> 
> -- >8 --
> 
> diff --git a/git.c b/git.c
> index 1cab64b5d1..c3dabd2553 100644
> --- a/git.c
> +++ b/git.c
> @@ -530,7 +530,7 @@ static struct cmd_struct commands[] = {
>         { "ls-tree", cmd_ls_tree, RUN_SETUP },
>         { "mailinfo", cmd_mailinfo, RUN_SETUP_GENTLY | NO_PARSEOPT },
>         { "mailsplit", cmd_mailsplit, NO_PARSEOPT },
> -       { "maintenance", cmd_maintenance, RUN_SETUP_GENTLY | NO_PARSEOPT },
> +       { "maintenance", cmd_maintenance, RUN_SETUP | NO_PARSEOPT },
>         { "merge", cmd_merge, RUN_SETUP | NEED_WORK_TREE },
>         { "merge-base", cmd_merge_base, RUN_SETUP },
>         { "merge-file", cmd_merge_file, RUN_SETUP_GENTLY },
> 
> -- >8 --
> 

Thank you for the review and the attached patch!

> If the above code change fixes your test (below), then that would
> probably be a safer change.

I agree, switching the maintenance command option to use RUN_SETUP
seems like a nicer approach here. Given the all current operations
requires the command to be executed inside the a git repository this
will make the command consistent across the subcommand. 

Also, it seems this provides an opportunity to cleanup the
register and unregister subcommands that currently implement the
check to ensure the commands are running from a git repository.

> 
> The reason to use RUN_SETUP_GENTLY was probably due to some thought
> of modifying the background maintenance schedule without being in a
> Git repository. However, we currently run the [un]register logic
> inside of the stop|start subcommands, so a GIT_DIR is required there,
> too.
> 

Indeed. Aside from this reason, another concern that I have is that
switching the validation to all subcommands (on this case by switching
the maintenance command option) will change a bit the behaviour of register
subcommand. Currently, the behaviour of "register" subcommand is to return
with 0 without any messages when running outside of repository and switching
will make the command fail instead.

Nevertheless, I am inclined to go with your suggestion given that it seems
better approach to support the automatically and make the behaviour
consistent for all subcommands given that changing the behaviour of
"git maintenance register" command will (hopefully) be okay.

Thanks,
Rafael