Re: [PATCH 4/8] scalar: implement the `help` subcommand

[Victoria Dye <vdye@github.com>]

Ævar Arnfjörð Bjarmason wrote:
> 
> On Wed, Aug 31 2022, Johannes Schindelin via GitGitGadget wrote:
> 
>> From: Johannes Schindelin <johasc@microsoft.com>
>>
>> It is merely handing off to `git help scalar`.
>>
>> Signed-off-by: Johannes Schindelin <johasc@microsoft.com>
>> Signed-off-by: Victoria Dye <vdye@github.com>
>> ---
>>  scalar.c | 20 ++++++++++++++++++++
>>  1 file changed, 20 insertions(+)
>>
>> diff --git a/scalar.c b/scalar.c
>> index 642d16124eb..675d7a6b0a9 100644
>> --- a/scalar.c
>> +++ b/scalar.c
>> @@ -819,6 +819,25 @@ static int cmd_delete(int argc, const char **argv)
>>  	return res;
>>  }
>>  
>> +static int cmd_help(int argc, const char **argv)
>> +{
>> +	struct option options[] = {
>> +		OPT_END(),
>> +	};
>> +	const char * const usage[] = {
>> +		N_("scalar help"),
> 
> 
> This should not have N_(), as it's a literal command.

Thanks, will fix. 

> 
>> +		NULL
>> +	};
>> +
>> +	argc = parse_options(argc, argv, NULL, options,
>> +			     usage, 0);
>> +
>> +	if (argc != 0)
> 
> If we're re-rolling anyway we usually just do "if (argc)". We don't need
> to worry about argc < 0 (despite the signed type, which is a historical
> C wart).

Normally I'd agree, but in this case there's a readability benefit to
explicitly comparing 'argc' to 0. 'scalar help' expects exactly zero
positional arguments, and showing the '!= 0' makes that expectation clearer
(likewise, 'scalar delete' checks that 'argc != 1' because it expects
exactly one positional arg). 

> 
>> +		usage_with_options(usage, options);
>> +
>> +	return run_git("help", "scalar", NULL);
> 
> Performance isn't sensitive here, but have you tried just calling
> cmd_help() instead with the appropriate arguments? It would avoid
> spawning another command..

As a matter of design preference, I'd rather avoid invoking builtins via
their 'cmd_*()' entrypoint. Doing so in 'scalar.c' would also introduce some
function name conflicts. While that's an overcomeable problem, precedent
across Git doesn't appear to indicate one approach is better than the other,
so I'm happy with things as they are.