Re: [PATCH 1/2] scalar register: add --no-maintenance option

[Derrick Stolee <stolee@gmail.com>]

On 5/2/2025 5:15 AM, Patrick Steinhardt wrote:
> On Wed, Apr 30, 2025 at 10:24:39AM +0000, Derrick Stolee via GitGitGadget wrote:
>> diff --git a/Documentation/scalar.adoc b/Documentation/scalar.adoc
>> index 7e4259c6743f..b2b244a86499 100644
>> --- a/Documentation/scalar.adoc
>> +++ b/Documentation/scalar.adoc
>> @@ -11,7 +11,7 @@ SYNOPSIS
>>  scalar clone [--single-branch] [--branch <main-branch>] [--full-clone]
>>  	[--[no-]src] <url> [<enlistment>]
>>  scalar list
>> -scalar register [<enlistment>]
>> +scalar register [--[no-]maintenance] [<enlistment>]
>>  scalar unregister [<enlistment>]
>>  scalar run ( all | config | commit-graph | fetch | loose-objects | pack-files ) [<enlistment>]
>>  scalar reconfigure [ --all | <enlistment> ]
>> @@ -117,6 +117,12 @@ Note: when this subcommand is called in a worktree that is called `src/`, its
>>  parent directory is considered to be the Scalar enlistment. If the worktree is
>>  _not_ called `src/`, it itself will be considered to be the Scalar enlistment.
>>  
>> +--[no-]maintenance::
>> +	By default, `scalar register` configures the enlistment to use Git's
>> +	background maintenance feature. Use the `--no-maintenance` to skip
>> +	this configuration. This does not disable any maintenance that may
>> +	already be enabled in other ways.

>> -	if (toggle_maintenance(1))
>> +	if (toggle_maintenance(maintenance))
>>  		warning(_("could not turn on maintenance"));
>>  
>>  	if (have_fsmonitor_support() && start_fsmonitor_daemon()) {
> 
> Isn't this change contrary to what the docs say? `toggle_maintenance(0)`
> would cause us to execute `git maintenance unregister --force`, which
> deregisters maintenance for us.

You are right. I've confirmed this using 'test_subcommand' in my test, so I'll
plumb the ability to skip toggle_maintenance() in certain cases. I'll carefully
document this in code as well.

Thanks for the careful eye. 
>> @@ -597,11 +597,14 @@ static int cmd_list(int argc, const char **argv UNUSED)
>>  
>>  static int cmd_register(int argc, const char **argv)
>>  {
>> +	int maintenance = 1;
>>  	struct option options[] = {
>> +		OPT_BOOL(0, "maintenance", &maintenance,
>> +			 N_("specify if background maintenance should be enabled")),
> 
> Maybe s/if/whether/? Might just be me not being a native speaker,
> though.

I like your choice here.

>> diff --git a/t/t9210-scalar.sh b/t/t9210-scalar.sh
>> index a81662713eb8..a488f72de9fe 100755
>> --- a/t/t9210-scalar.sh
>> +++ b/t/t9210-scalar.sh
>> @@ -129,6 +129,13 @@ test_expect_success 'scalar unregister' '
>>  	scalar unregister vanish
>>  '
>>  
>> +test_expect_success 'scalar register --no-maintenance' '
>> +	git init register-no-maint &&
>> +	GIT_TEST_MAINT_SCHEDULER="crontab:false,launchctl:false,schtasks:false" \
>> +		scalar register --no-maintenance register-no-maint 2>err &&
>> +	test_must_be_empty err
>> +'
>> +
> 
> We should probably have a test that verifies that we don't deregister
> maintenance.
Yes, will do. The currently-passing test that confirms the unregister is
happening would look like this:

test_expect_success 'scalar register --no-maintenance' '
	git init register-no-maint &&
	event_log="$(pwd)/no-maint.event" &&
	GIT_TEST_MAINT_SCHEDULER="crontab:false,launchctl:false,schtasks:false" \
	GIT_TRACE2_EVENT="$event_log" \
	GIT_TRACE2_EVENT_DEPTH=100 \
		scalar register --no-maintenance register-no-maint 2>err &&
	test_must_be_empty err &&
	test_subcommand git maintenance unregister --force <no-maint.event
'When using test_subcommand, it's really important to verify that this kindof test passes before changing the behavior and turning the test into a
negation, since it's easier to accidentally "pass" a negative test if the
test is malformed.

Thanks,
-Stolee