scalar: accept -C and -c options before the subcommand

From: Johannes Schindelin <[email protected]>

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 <[email protected]>

---
scalar: accept -C and -c options

This makes the scalar command a bit more handy by offering the same -c <key>=<value> and -C <directory> options as the git command.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1130%2Fdscho%2Fscalar-c-and-C-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1130/dscho/scalar-c-and-C-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1130

 contrib/scalar/scalar.c   | 22 +++++++++++++++++++++-
 contrib/scalar/scalar.txt | 10 ++++++++++
 2 files changed, 31 insertions(+), 1 deletion(-)

contrib/scalar/scalar.c +9 -0

index 1ce9c2b00e8..7db2a97416e 100644
@@ -808,6 +808,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;

Quoted 50 lines · Johannes Schindelin via GitGitGadget

From: Johannes Schindelin <[email protected]>

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 <[email protected]>

---
scalar: accept -C and -c options

This makes the scalar command a bit more handy by offering the same -c <key>=<value> and -C <directory> options as the git command.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1130%2Fdscho%2Fscalar-c-and-C-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1130/dscho/scalar-c-and-C-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1130

 contrib/scalar/scalar.c   | 22 +++++++++++++++++++++-
 contrib/scalar/scalar.txt | 10 ++++++++++
 2 files changed, 31 insertions(+), 1 deletion(-)

contrib/scalar/scalar.c +14 -0

index 1ce9c2b00e8..7db2a97416e 100644
@@ -808,6 +808,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;

contrib/scalar/scalar.c int cmd_main(int argc, const char **argv) · line 829

Quoted 5 lines

contrib/scalar/scalar.c +5 -0

+			argv += 2;
+		} else
+			break;
+	}
+

All looks right to me based on a cursory scan. It's too bad that we have to copy this code from git.c::handle_options().

Could we call handle_options() (assuming that it was available to Scalar's compilation unit) instead? I'm not sure if that's a naive question or not, but it might be nice to explain it out in the commit message in case other reviewers have the same question that I did.

On a more practical note: is there an easy way to test this?

From: Johannes Schindelin <[email protected]>

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 <[email protected]>

---
scalar: accept -C and -c options

This makes the scalar command a bit more handy by offering the same -c <key>=<value> and -C <directory> options as the git command.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1130%2Fdscho%2Fscalar-c-and-C-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1130/dscho/scalar-c-and-C-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1130

Quoted 17 lines · Johannes Schindelin via GitGitGadget

From: Johannes Schindelin <[email protected]>

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 <[email protected]>

---
scalar: accept -C and -c options

Quoted 6 lines

This makes the scalar command a bit more handy by offering the same -c <key>=<value> and -C <directory> options as the git command.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1130%2Fdscho%2Fscalar-c-and-C-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1130/dscho/scalar-c-and-C-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1130

I think it would help for context to note that this patch had at least 6 submissions on the ML already as part of early versions of the scalar series.

Here's the CL of the iteration that ejected it:
https://lore.kernel.org/git/[email protected]/

Where you summarized:

* The patch that adds support for -c <key>=<value> and -C <directory> was moved to its own add-on patch series: While it is obvious that those options are valuable to have, an open question is whether there are other "pre-command" options in git that would be useful, too, and I would like to postpone that discussion to that date.

Having been involved in those discussions I can't remember what the pre-command options you're referring to there are, but it seems "that date" is probably upon us.

The biggest benefits of using handle_options() is for other pre-command options such as --exec-path, which I use on a regular basis when testing new functionality.

There are other options in handle_options() that might not be appropriate, or might be incorrect if we just make handle_options() non-static. For example, scalar --list-cmds=parseopt wouldn't show the scalar commands and would instead show the git commands.

So my feeling is that we should continue to delay this functionality until Scalar is more stable, perhaps even until after it moves out of contrib/. The need to change handle_options() to work with a new top-level command is novel enough to be worth careful scrutiny, but that effort is only valuable if the Git community is more committed to having Scalar in the tree for the long term.

Quoted 8 lines · Derrick Stolee

The biggest benefits of using handle_options() is for other pre-command options such as --exec-path, which I use on a regular basis when testing new functionality.

There are other options in handle_options() that might not be appropriate, or might be incorrect if we just make handle_options() non-static. For example, scalar --list-cmds=parseopt wouldn't show the scalar commands and would instead show the git commands.

Right, and since handle_options() lives in the same file as git's cmd_main() function, we would not only have to disentangle options that work only for git from those that would also work for scalar, but we would have to extract the handle_options() function into a separate file.

And while at it, a tangent someone with infinite time on their hands might suggest is: why not convert handle_options() to the parse_options() machinery? Which would of course solve one issue by adding several new ones. Don't get me wrong: I would find it useful to convert git.c:handle_options() to a function in libgit.a which uses the parse_options() machinery. It'll just require a lot of time, and I do not see enough benefit that would make it worth embarking on that particular journey.

But since I had a look at handle_options() anyway, I might just as well summarize my insights about how applicable the supported options are for scalar here:
[...]
# Detrimental

--exec-path

Since scalar is tightly coupled to a specific Git version, it would cause much more harm than benefit to encourage users to use a different Git version by offering them this option.

--list-cmds

As you pointed out, this option would produce misleading output.

Given that only the -c and -C options are _actually_ useful in the context of the scalar command, I would argue that I chose the best approach, as the benefit of the intrusive refactorings that would be necessary to share code with git.c is rather small compared with the amount of work.

Quoted 6 lines

So my feeling is that we should continue to delay this functionality until Scalar is more stable, perhaps even until after it moves out of contrib/. The need to change handle_options() to work with a new top-level command is novel enough to be worth careful scrutiny, but that effort is only valuable if the Git community is more committed to having Scalar in the tree for the long term.

I am okay with holding off with this, for now.

On the other hand, as I pointed out above: I do not really see it worth the effort to refactor git.c:handle_options() for the minimal benefit it would give us over the approach I chose in the patch under discussion.

Quoted 32 lines · Johannes Schindelin

Quoted 8 lines · Derrick Stolee

The biggest benefits of using handle_options() is for other pre-command options such as --exec-path, which I use on a regular basis when testing new functionality.

There are other options in handle_options() that might not be appropriate, or might be incorrect if we just make handle_options() non-static. For example, scalar --list-cmds=parseopt wouldn't show the scalar commands and would instead show the git commands.

Right, and since handle_options() lives in the same file as git's cmd_main() function, we would not only have to disentangle options that work only for git from those that would also work for scalar, but we would have to extract the handle_options() function into a separate file.

And while at it, a tangent someone with infinite time on their hands might suggest is: why not convert handle_options() to the parse_options() machinery? Which would of course solve one issue by adding several new ones. Don't get me wrong: I would find it useful to convert git.c:handle_options() to a function in libgit.a which uses the parse_options() machinery. It'll just require a lot of time, and I do not see enough benefit that would make it worth embarking on that particular journey.

But since I had a look at handle_options() anyway, I might just as well summarize my insights about how applicable the supported options are for scalar here:
[...]
# Detrimental

Quoted 5 lines

--exec-path

Since scalar is tightly coupled to a specific Git version, it would cause much more harm than benefit to encourage users to use a different Git version by offering them this option.

So just to clarify, do you and Stolee disagree about scalar supporting --exec-path, per his comments above?

In this case I don't mind much, but speaking generally I see you and Stolee tying yourselves in knots again about scalar being in contrib so we shouldn't use libgit.

It already uses libgit, there's even (last I checked) at least one function in it only used directly by the scalar code.

contrib/scalar/scalar.c +5 -0

+	while (argc > 1 && *argv[0] == '-') {
+		int show_usage = 0;
+
+		if (gity_handle_options(&argv, &argc, NULL, &show_usage)) {
+			(argv)++;

Quoted 35 lines · Ævar Arnfjörð Bjarmason

Quoted 32 lines · Johannes Schindelin

Quoted 8 lines · Derrick Stolee

The biggest benefits of using handle_options() is for other pre-command options such as --exec-path, which I use on a regular basis when testing new functionality.

There are other options in handle_options() that might not be appropriate, or might be incorrect if we just make handle_options() non-static. For example, scalar --list-cmds=parseopt wouldn't show the scalar commands and would instead show the git commands.

Right, and since handle_options() lives in the same file as git's cmd_main() function, we would not only have to disentangle options that work only for git from those that would also work for scalar, but we would have to extract the handle_options() function into a separate file.

And while at it, a tangent someone with infinite time on their hands might suggest is: why not convert handle_options() to the parse_options() machinery? Which would of course solve one issue by adding several new ones. Don't get me wrong: I would find it useful to convert git.c:handle_options() to a function in libgit.a which uses the parse_options() machinery. It'll just require a lot of time, and I do not see enough benefit that would make it worth embarking on that particular journey.

But since I had a look at handle_options() anyway, I might just as well summarize my insights about how applicable the supported options are for scalar here:
[...]
# Detrimental

Quoted 8 lines

Quoted 5 lines

--exec-path

Since scalar is tightly coupled to a specific Git version, it would cause much more harm than benefit to encourage users to use a different Git version by offering them this option.

So just to clarify, do you and Stolee disagree about scalar supporting --exec-path, per his comments above?

I think it would be nice, but it's also not a blocker for me.

Quoted 6 lines

In this case I don't mind much, but speaking generally I see you and Stolee tying yourselves in knots again about scalar being in contrib so we shouldn't use libgit.

It already uses libgit, there's even (last I checked) at least one function in it only used directly by the scalar code.

My concern is not that we shouldn't use libgit (because we do) but that we shouldn't make significant changes to libgit.a only for Scalar's benefit until it is incorporated in a more final way.

In my opinion, well-architected code is code that is easy to delete. Until we have Scalar mostly feature-complete and can make a decision about it living in the Git tree long-term (and _where_ it resides), I would like to have the following property: If I were to revert all commits that touch contrib/scalar/ and squash them into a single commit, then we would have a bunch of file deletions and a very small set of edits to the Makefile. I don't know how much the ship has sailed there already, but keeping the size of that "revert diff" small means that we are keeping the coupling low during this review process.

Quoted 19 lines

I don't remember anyone having any objection to scalar using libgit code, or even that there's libgit code just to help it along. That's a self-imposed limitation you two seem to have invented.

Personally I find a patch like the below much easier to review. It's the parts that aren't easy to review boilerplate are all things that we have in-tree already.

Whereas proposing a new way to parse -c or -C will lead (at least me) to carefully eyeballing that new implementation, looking at how it differs (if at all) from the existing one, wondering why the i18n strings are subtly different etc (I saw one reason is that since the code was copy/pasted initially the git.c version was updated, but your patch wasn't updated to copy it).

contrib/scalar/scalar.c +0 -0

index 1ce9c2b00e8..ee793ff6ccc 100644

contrib/scalar/scalar.c

Quoted 4 lines

contrib/scalar/scalar.c +0 -0

index 1ce9c2b00e8..ee793ff6ccc 100644

Was this diff double-copied or something?

Hi Stolee,

Quoted 8 lines · Derrick Stolee

The biggest benefits of using handle_options() is for other pre-command options such as --exec-path, which I use on a regular basis when testing new functionality.

There are other options in handle_options() that might not be appropriate, or might be incorrect if we just make handle_options() non-static. For example, scalar --list-cmds=parseopt wouldn't show the scalar commands and would instead show the git commands.

Right, and since handle_options() lives in the same file as git's cmd_main() function, we would not only have to disentangle options that work only for git from those that would also work for scalar, but we would have to extract the handle_options() function into a separate file.

But since I had a look at handle_options() anyway, I might just as well summarize my insights about how applicable the supported options are for scalar here:

# Beneficial

-c <key>=<value>
--config-env <key>=<value>
-C <directory>

Since I added support for these (except for the long form --config-env that I actually only learned while researching this email), it is obvious that I'd like scalar to support them.

# Won't hurt

# Detrimental

--exec-path

Since scalar is tightly coupled to a specific Git version, it would cause much more harm than benefit to encourage users to use a different Git version by offering them this option.

--list-cmds

As you pointed out, this option would produce misleading output.

Given that only the -c and -C options are _actually_ useful in the context of the scalar command, I would argue that I chose the best approach, as the benefit of the intrusive refactorings that would be necessary to share code with git.c is rather small compared with the amount of work.

Quoted 6 lines

So my feeling is that we should continue to delay this functionality until Scalar is more stable, perhaps even until after it moves out of contrib/. The need to change handle_options() to work with a new top-level command is novel enough to be worth careful scrutiny, but that effort is only valuable if the Git community is more committed to having Scalar in the tree for the long term.

I am okay with holding off with this, for now.

On the other hand, as I pointed out above: I do not really see it worth the effort to refactor git.c:handle_options() for the minimal benefit it would give us over the approach I chose in the patch under discussion.

My understanding was that this was ejected so we could find the right time to lib-ify handle_options() (as Taylor suggested), but we didn't want to do that while Scalar was still in a tentative state (in contrib/ with a plan to move it out after more is implemented).

The biggest benefits of using handle_options() is for other pre-command options such as --exec-path, which I use on a regular basis when testing new functionality.

There are other options in handle_options() that might not be appropriate, or might be incorrect if we just make handle_options() non-static. For example, scalar --list-cmds=parseopt wouldn't show the scalar commands and would instead show the git commands.

So my feeling is that we should continue to delay this functionality until Scalar is more stable, perhaps even until after it moves out of contrib/. The need to change handle_options() to work with a new top-level command is novel enough to be worth careful scrutiny, but that effort is only valuable if the Git community is more committed to having Scalar in the tree for the long term.

Quoted 14 lines · Derrick Stolee

My understanding was that this was ejected so we could find the right time to lib-ify handle_options() (as Taylor suggested), but we didn't want to do that while Scalar was still in a tentative state (in contrib/ with a plan to move it out after more is implemented).

The biggest benefits of using handle_options() is for other pre-command options such as --exec-path, which I use on a regular basis when testing new functionality.

There are other options in handle_options() that might not be appropriate, or might be incorrect if we just make handle_options() non-static. For example, scalar --list-cmds=parseopt wouldn't show the scalar commands and would instead show the git commands.

Quoted 6 lines

So my feeling is that we should continue to delay this functionality until Scalar is more stable, perhaps even until after it moves out of contrib/. The need to change handle_options() to work with a new top-level command is novel enough to be worth careful scrutiny, but that effort is only valuable if the Git community is more committed to having Scalar in the tree for the long term.

The usual caveat that little things tend to accumulate and you can die from thousands of paper cuts aside, if "run in that directory" and "pretend these configuration variables are set" are useful enough features to Scalar, I think that the straight-forward option parser this patch adds is very much acceptable.

If we need more options (i.e. when we need to add the third thing), that would be the best time to see how handle_options() can be restructured to serve the different set of options git and Scaler need.

And this loop, which is too small to be even called "duplicated implementation", should suffice in the meantime.

From: Johannes Schindelin <[email protected]>

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.

Note: It might look like a good idea to try to reuse the handle_options() function in git.c instead of replicating only the -c/-C part. However, that function is not only not in libgit.a, it is also intricately entangled with the rest of the code in git.c that is necessary e.g. to handle --paginate. Besides, no other option handled by that handle_options() function is relevant to Scalar, therefore the cost of refactoring vastly would outweigh the benefit.