diff mbox series

[15/15] scalar: accept -C and -c options before the subcommand

Message ID 6455b18f1b623032b9066c1730dee045fbe7a3f3.1630359290.git.gitgitgadget@gmail.com (mailing list archive)
State Superseded
Headers show
Series Upstreaming the Scalar command | expand

Commit Message

Johannes Schindelin Aug. 30, 2021, 9:34 p.m. UTC 
From: Johannes Schindelin <johannes.schindelin@gmx.de>

The `git` executable has these two very useful options:

-C <directory>:
	switch to the specified directory before performing any actions

-c <key>=<value>:
	temporarily configure this setting for the duration of the
	specified scalar subcommand

With this commit, we teach the `scalar` executable the same trick.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 contrib/scalar/scalar.c   | 22 +++++++++++++++++++++-
 contrib/scalar/scalar.txt | 10 ++++++++++
 2 files changed, 31 insertions(+), 1 deletion(-)

Comments

Ævar Arnfjörð Bjarmason Aug. 31, 2021, 8:32 a.m. UTC | #1 
On Mon, Aug 30 2021, Johannes Schindelin via GitGitGadget wrote:

> The `git` executable has these two very useful options:
>
> -C <directory>:
> 	switch to the specified directory before performing any actions
>
> -c <key>=<value>:
> 	temporarily configure this setting for the duration of the
> 	specified scalar subcommand
>
> With this commit, we teach the `scalar` executable the same trick.
> [...]
> +	while (argc > 1 && *argv[1] == '-') {
> +		if (!strcmp(argv[1], "-C")) {
> +			if (argc < 3)
> +				die(_("-C requires a <directory>"));
> +			if (chdir(argv[2]) < 0)
> +				die_errno(_("could not change to '%s'"),
> +					  argv[2]);
> +			argc -= 2;
> +			argv += 2;
> +		} else if (!strcmp(argv[1], "-c")) {
> +			if (argc < 3)
> +				die(_("-c requires a <key>=<value> argument"));
> +			git_config_push_parameter(argv[2]);
> +			argc -= 2;
> +			argv += 2;
> +		} else
> +			break;
> +	}

This along with my earlier comment about the Makefile copy/pasting makes
me wonder if an easier way to integrate this wouldn't be to refactor
git.c a bit to have it understand either "git" or "scalar", then instead
of "ls-tree" etc. as "git" the subcommands would become "built-ins".

Which would give us both "[git|scalar] [-c ...] <cmd>" for free, and
elimante the need for the inevetable future divergence of wanting -p,
-P, --exec-path etc. in both.
Derrick Stolee Aug. 31, 2021, 2:30 p.m. UTC | #2 
On 8/31/2021 4:32 AM, Ævar Arnfjörð Bjarmason wrote:
> 
> On Mon, Aug 30 2021, Johannes Schindelin via GitGitGadget wrote:
> 
>> The `git` executable has these two very useful options:
>>
>> -C <directory>:
>> 	switch to the specified directory before performing any actions
>>
>> -c <key>=<value>:
>> 	temporarily configure this setting for the duration of the
>> 	specified scalar subcommand
>>
>> With this commit, we teach the `scalar` executable the same trick.
>> [...]
>> +	while (argc > 1 && *argv[1] == '-') {
>> +		if (!strcmp(argv[1], "-C")) {
>> +			if (argc < 3)
>> +				die(_("-C requires a <directory>"));
>> +			if (chdir(argv[2]) < 0)
>> +				die_errno(_("could not change to '%s'"),
>> +					  argv[2]);
>> +			argc -= 2;
>> +			argv += 2;
>> +		} else if (!strcmp(argv[1], "-c")) {
>> +			if (argc < 3)
>> +				die(_("-c requires a <key>=<value> argument"));
>> +			git_config_push_parameter(argv[2]);
>> +			argc -= 2;
>> +			argv += 2;
>> +		} else
>> +			break;
>> +	}
> 
> This along with my earlier comment about the Makefile copy/pasting makes
> me wonder if an easier way to integrate this wouldn't be to refactor
> git.c a bit to have it understand either "git" or "scalar", then instead
> of "ls-tree" etc. as "git" the subcommands would become "built-ins".
> 
> Which would give us both "[git|scalar] [-c ...] <cmd>" for free, and
> elimante the need for the inevetable future divergence of wanting -p,
> -P, --exec-path etc. in both.
 
Such a change would likely eliminate the ability to not include Scalar
when building the Git codebase, which we tried to avoid by keeping it
within contrib and have it be compiled via an opt-in flag.

If we want to talk about integrating Scalar into Git in a deeper way,
then that is an interesting discussion to have, but it lives at a much
higher level than Makefile details.

The questions we are really looking to answer in this RFC are:

1. Will the Git project accept Scalar into its codebase?

2. What is the best place for Scalar to live in the Git codebase?

We erred on the side of keeping Scalar as optional as possible. If
the community is more interested in a deeper integration, then that
could be an interesting direction.

In my opinion, I think the current tactic is safest. We could always
decide on a deeper integration later by moving the code around. It
seems harder to do the reverse.

Thanks,
-Stolee
Ævar Arnfjörð Bjarmason Aug. 31, 2021, 2:52 p.m. UTC | #3 
On Tue, Aug 31 2021, Derrick Stolee wrote:

> On 8/31/2021 4:32 AM, Ævar Arnfjörð Bjarmason wrote:
>> 
>> On Mon, Aug 30 2021, Johannes Schindelin via GitGitGadget wrote:
>> 
>>> The `git` executable has these two very useful options:
>>>
>>> -C <directory>:
>>> 	switch to the specified directory before performing any actions
>>>
>>> -c <key>=<value>:
>>> 	temporarily configure this setting for the duration of the
>>> 	specified scalar subcommand
>>>
>>> With this commit, we teach the `scalar` executable the same trick.
>>> [...]
>>> +	while (argc > 1 && *argv[1] == '-') {
>>> +		if (!strcmp(argv[1], "-C")) {
>>> +			if (argc < 3)
>>> +				die(_("-C requires a <directory>"));
>>> +			if (chdir(argv[2]) < 0)
>>> +				die_errno(_("could not change to '%s'"),
>>> +					  argv[2]);
>>> +			argc -= 2;
>>> +			argv += 2;
>>> +		} else if (!strcmp(argv[1], "-c")) {
>>> +			if (argc < 3)
>>> +				die(_("-c requires a <key>=<value> argument"));
>>> +			git_config_push_parameter(argv[2]);
>>> +			argc -= 2;
>>> +			argv += 2;
>>> +		} else
>>> +			break;
>>> +	}
>> 
>> This along with my earlier comment about the Makefile copy/pasting makes
>> me wonder if an easier way to integrate this wouldn't be to refactor
>> git.c a bit to have it understand either "git" or "scalar", then instead
>> of "ls-tree" etc. as "git" the subcommands would become "built-ins".
>> 
>> Which would give us both "[git|scalar] [-c ...] <cmd>" for free, and
>> elimante the need for the inevetable future divergence of wanting -p,
>> -P, --exec-path etc. in both.
>  
> Such a change would likely eliminate the ability to not include Scalar
> when building the Git codebase, which we tried to avoid by keeping it
> within contrib and have it be compiled via an opt-in flag.

I mean to still have it behind a flag, but to handle it similar to how
we handle NO_CURL, EXCLUDED_PROGRAMS and the like, i.e. not requiring
parallel maintenance of copy/pasted Makefile logic in contrib/.

> If we want to talk about integrating Scalar into Git in a deeper way,
> then that is an interesting discussion to have, but it lives at a much
> higher level than Makefile details.

To be clear I'm proposing no change at all in term of what happens when
you run "make install", just commenting on the implementation details of
how we arrange for things to be built and configured before that step.

I realize that this is following some prior art of
e.g. contrib/subtree/Makefile, but IMNSHO that approach is a historical
mistake we should be backing out of. There was some recent discussion of
this here:
https://lore.kernel.org/git/87pmz4ig4o.fsf@evledraar.gmail.com/

E.g. now we have some painful management of the depencency graph between
/Makefile and Documentation/Makefile requiring fixes like 56550ea7180
(Makefile: add missing dependencies of 'config-list.h', 2021-04-08),
adding yet another Makefile into the mix which (to take one example)
depends on doc.dep, which in turn depends on ...; It's all a bunch of
needless complexity we can avoid.

> The questions we are really looking to answer in this RFC are:
>
> 1. Will the Git project accept Scalar into its codebase?
>
> 2. What is the best place for Scalar to live in the Git codebase?
>
> We erred on the side of keeping Scalar as optional as possible. If
> the community is more interested in a deeper integration, then that
> could be an interesting direction.

Indeed, to be clear I realize I'm entirely punting on the real questions
you're interested in. I just gave this an initial cursory skimming for
now, I have not formed an informed opinion on your #1, but just a little
bit of #2.

My initial reaction to #1 without having looked into it deeply is some
combination of "sure, why not?", and that the people/group contributing
major scalability work to git.git should be given the benefit of the
doubt. Maybe we won't keep "scalar" long-term, or change its UI etc.,
all of that can be handled in some carefully worded documentation
somewhere.

Of course all these suggestions I'm making about Makefile arrangement
are rather pointless if there isn't consensus to get past the hurdle of
your #1.

> In my opinion, I think the current tactic is safest. We could always
> decide on a deeper integration later by moving the code around. It
> seems harder to do the reverse.

I think "deeper integration" is the reverse of what you think it is.

I.e. if I'm patching or maintaining part of the Makefile logic to it's
deeper (or perhaps "gnarlier" is the righ word?) integration to need to
duplicate that work in two places, or always take into account that some
not-built-by-default-but-quite-common command's *.txt docs and *.sh
tests live in some unusual place for the purposes of CI, lint, tooling
etc.

In other words, it's a question of how much net complexity is being
added to the (build) system. That complexity doesn't automatically
reduce just because some files live in another directory, sometimes
that's an increase in complexity.

Whereas just conditionally adding it to some list in the top-level
Makefile (or Documentation/Makefile) is relatively maintenance-free, and
to our users / packagers the result should be the same or near enough.
It won't matter to them if building the optional thing is another "make"
command or just a flag to the existing "make" command.
diff mbox series

Patch

diff --git a/contrib/scalar/scalar.c b/contrib/scalar/scalar.c
index be0a49b0d75..41ba4b2f3b1 100644
--- a/contrib/scalar/scalar.c
+++ b/contrib/scalar/scalar.c
@@ -806,6 +806,25 @@  int cmd_main(int argc, const char **argv)
 	struct strbuf scalar_usage = STRBUF_INIT;
 	int i;
 
+	while (argc > 1 && *argv[1] == '-') {
+		if (!strcmp(argv[1], "-C")) {
+			if (argc < 3)
+				die(_("-C requires a <directory>"));
+			if (chdir(argv[2]) < 0)
+				die_errno(_("could not change to '%s'"),
+					  argv[2]);
+			argc -= 2;
+			argv += 2;
+		} else if (!strcmp(argv[1], "-c")) {
+			if (argc < 3)
+				die(_("-c requires a <key>=<value> argument"));
+			git_config_push_parameter(argv[2]);
+			argc -= 2;
+			argv += 2;
+		} else
+			break;
+	}
+
 	if (argc > 1) {
 		argv++;
 		argc--;
@@ -816,7 +835,8 @@  int cmd_main(int argc, const char **argv)
 	}
 
 	strbuf_addstr(&scalar_usage,
-		      N_("scalar <command> [<options>]\n\nCommands:\n"));
+		      N_("scalar [-C <directory>] [-c <key>=<value>] "
+			 "<command> [<options>]\n\nCommands:\n"));
 	for (i = 0; builtins[i].name; i++)
 		strbuf_addf(&scalar_usage, "\t%s\n", builtins[i].name);
 
diff --git a/contrib/scalar/scalar.txt b/contrib/scalar/scalar.txt
index 00923023243..6d85640ef42 100644
--- a/contrib/scalar/scalar.txt
+++ b/contrib/scalar/scalar.txt
@@ -36,6 +36,16 @@  The `scalar` command implements various subcommands, and different options
 depending on the subcommand. With the exception of `clone`, `list` and
 `reconfigure --all`, all subcommands expect to be run in an enlistment.
 
+The following options can be specified _before_ the subcommand:
+
+-C <directory>::
+    Before running the subcommand, change the working directory. This
+    option imitates the same option of linkgit:git[1].
+
+-c <key>=<value>::
+    For the duration of running the specified subcommand, configure this
+    setting. This option imitates the same option of linkgit:git[1].
+
 COMMANDS
 --------