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

Commit Message

Johannes Schindelin Aug. 31, 2022, 4:02 p.m. UTC

From: Johannes Schindelin <[e-mail]>

It is merely handing off to `git help scalar`.

Signed-off-by: Johannes Schindelin <[e-mail]>
Signed-off-by: Victoria Dye <[e-mail]>
---
 scalar.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

Comments

Ævar Arnfjörð Bjarmason Aug. 31, 2022, 4:48 p.m. UTC | #1

On Wed, Aug 31 2022, Johannes Schindelin via GitGitGadget wrote:

> From: Johannes Schindelin <[e-mail]>
>
> It is merely handing off to `git help scalar`.
>
> Signed-off-by: Johannes Schindelin <[e-mail]>
> Signed-off-by: Victoria Dye <[e-mail]>
> ---
>  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.

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

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

Johannes Schindelin Sept. 1, 2022, 8:51 a.m. UTC | #2

Hi Victoria,

On Wed, 31 Aug 2022, Johannes Schindelin via GitGitGadget wrote:

> From: Johannes Schindelin <[e-mail]>

I probably left that in by mistake. Could I bother you to change this (and
the corresponding Signed-off-by: footer) to use my usual email address?

Thank you,
Dscho

>
> It is merely handing off to `git help scalar`.
>
> Signed-off-by: Johannes Schindelin <[e-mail]>
> Signed-off-by: Victoria Dye <[e-mail]>
> ---
>  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"),
> +		NULL
> +	};
> +
> +	argc = parse_options(argc, argv, NULL, options,
> +			     usage, 0);
> +
> +	if (argc != 0)
> +		usage_with_options(usage, options);
> +
> +	return run_git("help", "scalar", NULL);
> +}
> +
>  static int cmd_version(int argc, const char **argv)
>  {
>  	int verbose = 0, build_options = 0;
> @@ -858,6 +877,7 @@ static struct {
>  	{ "run", cmd_run },
>  	{ "reconfigure", cmd_reconfigure },
>  	{ "delete", cmd_delete },
> +	{ "help", cmd_help },
>  	{ "version", cmd_version },
>  	{ "diagnose", cmd_diagnose },
>  	{ NULL, NULL},
> --
> gitgitgadget
>
>

Johannes Schindelin Sept. 1, 2022, 9:17 a.m. UTC | #3

Hi Victoria,

On Wed, 31 Aug 2022, Johannes Schindelin via GitGitGadget wrote:

> From: Johannes Schindelin <[e-mail]>
>
> It is merely handing off to `git help scalar`.
>
> Signed-off-by: Johannes Schindelin <[e-mail]>
> Signed-off-by: Victoria Dye <[e-mail]>
> ---
>  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"),
> +		NULL
> +	};
> +
> +	argc = parse_options(argc, argv, NULL, options,
> +			     usage, 0);
> +
> +	if (argc != 0)
> +		usage_with_options(usage, options);
> +
> +	return run_git("help", "scalar", NULL);
> +}
> +
>  static int cmd_version(int argc, const char **argv)
>  {
>  	int verbose = 0, build_options = 0;
> @@ -858,6 +877,7 @@ static struct {
>  	{ "run", cmd_run },
>  	{ "reconfigure", cmd_reconfigure },
>  	{ "delete", cmd_delete },
> +	{ "help", cmd_help },

Marking this as a tangent ("optional", as some peeps suggested in the Git
standup on IRC [*1*]) with the suggestion to follow up _after_ this here
patch series is done cooking, i.e. once it hits the main branch:

We probably want to migrate `scalar.c` to use the `OPT_SUBCOMMAND` API.

But as I said, please not in this here patch series, so as to separate
concerns properly.

Thanks,
Dscho

>  	{ "version", cmd_version },
>  	{ "diagnose", cmd_diagnose },
>  	{ NULL, NULL},
> --
> gitgitgadget

Footnote *1*:
https://colabti.org/irclogger/irclogger_log/git-devel?date=2022-08-29#l50

Victoria Dye Sept. 1, 2022, 4:08 p.m. UTC | #4

Ævar Arnfjörð Bjarmason wrote:
> 
> On Wed, Aug 31 2022, Johannes Schindelin via GitGitGadget wrote:
> 
>> From: Johannes Schindelin <[e-mail]>
>>
>> It is merely handing off to `git help scalar`.
>>
>> Signed-off-by: Johannes Schindelin <[e-mail]>
>> Signed-off-by: Victoria Dye <[e-mail]>
>> ---
>>  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.

Patch

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"),
+		NULL
+	};
+
+	argc = parse_options(argc, argv, NULL, options,
+			     usage, 0);
+
+	if (argc != 0)
+		usage_with_options(usage, options);
+
+	return run_git("help", "scalar", NULL);
+}
+
 static int cmd_version(int argc, const char **argv)
 {
 	int verbose = 0, build_options = 0;
@@ -858,6 +877,7 @@  static struct {
 	{ "run", cmd_run },
 	{ "reconfigure", cmd_reconfigure },
 	{ "delete", cmd_delete },
+	{ "help", cmd_help },
 	{ "version", cmd_version },
 	{ "diagnose", cmd_diagnose },
 	{ NULL, NULL},