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

From: Derrick Stolee <stolee@gmail.com>
Date: Nov 26, 2020, 11:21 UTC
Message-ID: <54fa678c-7150-8c48-50e5-b33923a69249@gmail.com>
In-Reply-To: <20201126082255.yyxx2kpskj3td5og@contrib-buster.localdomain>

On 11/26/2020 3:22 AM, Rafael Silva wrote:

Show 27 quoted lines

> 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