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

[Derrick Stolee <stolee@gmail.com>]

On 11/26/2020 3:22 AM, Rafael Silva wrote:
> On Tue, Nov 24, 2020 at 12:22:40PM -0500, Derrick Stolee wrote:
>> 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.

Excellent point. It would be good to cover this case with a test, to
demonstrate that as _intended_ behavior. It makes sense to fail instead
of "succeed" when doing nothing.

> 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.

Yes. I would say that changing that behavior aligns it with what it
should be doing. The best news is that the 'register' subcommand does
not exist in a released version of Git, so no one depends on the
current behavior.

Thanks,
-Stolee