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
