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

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

[Johannes Schindelin via GitGitGadget]

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>
---
    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(-)

diff --git a/contrib/scalar/scalar.c b/contrib/scalar/scalar.c
index 1ce9c2b00e8..7db2a97416e 100644
--- a/contrib/scalar/scalar.c
+++ b/contrib/scalar/scalar.c
@@ -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;
+			argv += 2;
+		} else
+			break;
+	}
+
 	if (argc > 1) {
 		argv++;
 		argc--;
@@ -818,7 +837,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 f416d637289..cf4e5b889cc 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
 --------
 

base-commit: ddc35d833dd6f9e8946b09cecd3311b8aa18d295
-- 
gitgitgadget

Re: [PATCH] scalar: accept -C and -c options before the subcommand

[Taylor Blau]

On Wed, Jan 26, 2022 at 11:15:29AM +0000, Johannes Schindelin via GitGitGadget wrote:
> 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>
> ---
>     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(-)
>
> diff --git a/contrib/scalar/scalar.c b/contrib/scalar/scalar.c
> index 1ce9c2b00e8..7db2a97416e 100644
> --- a/contrib/scalar/scalar.c
> +++ b/contrib/scalar/scalar.c
> @@ -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;
> +			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? Like I
said, I'm not necessarily worried about the code you wrote, just that it
seems like this sort of thing *should* be easy enough to test. It's fine
if there isn't, too, but again as somebody new to this area some
explanation would be helpful.

Thanks,
Taylor

Re: [PATCH] scalar: accept -C and -c options before the subcommand

[Ævar Arnfjörð Bjarmason]

On Wed, Jan 26 2022, Johannes Schindelin via GitGitGadget wrote:

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

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/pull.1005.v7.git.1637158762.gitgitgadget@gmail.com/

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.

Re: [PATCH] scalar: accept -C and -c options before the subcommand

[Derrick Stolee]

On 1/26/2022 9:55 PM, Ævar Arnfjörð Bjarmason wrote:
> 
> On Wed, Jan 26 2022, Johannes Schindelin via GitGitGadget wrote:
> 
>> 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>
>> ---
>>     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
> 
> 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/pull.1005.v7.git.1637158762.gitgitgadget@gmail.com/
> 
> 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.

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.

Thanks,
-Stolee

Re: [PATCH] scalar: accept -C and -c options before the subcommand

[Johannes Schindelin]

Hi Stolee,

On Thu, 27 Jan 2022, Derrick Stolee wrote:

> 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:

# 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

  --html-path
  --man-path
  --info-path

	Sure, for `scalar help` (which I implement in a patch series I
	have not yet formally contributed to the Git mailing list), these
	options might even make some sense.

  --paginate
  --no-pager

	There are no Scalar commands that would benefit from using the
	pager. But if we parse those options, we also have to handle them.
	Which would mean extracting _even more_ code from `git.c` just so
	that we can reuse `handle_options()` in Scalar.

  --no-replace-objects
  --git-dir
  --work-tree

	These options would only be relevant to the `scalar run` command,
	no other Scalar command works on an existing Git worktree. And
	even for that command, I doubt that they are actually useful in
	Scalar's context.

  --namespace

	This option seems relevant mostly for repositories that are served
	up for cloning and fetching, which is not what the Scalar command
	supports directly.

  --super-prefix
  --bare

	These options do not make sense in Scalar's context (it's neither
	about bare repositories not about submodules). That is the case
	for most `git` commands, too, of course.

  --literal-pathspecs
  --no-literal-pathspecs
  --glob-pathspecs
  --noglob-pathspecs
  --icase-pathspecs
  --no-optional-locks

	None of Scalar's commands take pathspecs, so these options won't
	have any effect.

  --shallow-file

	This option (which I learned about today) is purposefully not
	documented, as it is merely an internal option for use e.g. by
	`receive-pack`.

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

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

Ciao,
Dscho

Re: [PATCH] scalar: accept -C and -c options before the subcommand

[Johannes Schindelin]

Hi Taylor,

On Wed, 26 Jan 2022, Taylor Blau wrote:

> On Wed, Jan 26, 2022 at 11:15:29AM +0000, Johannes Schindelin via GitGitGadget wrote:
> > 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>
> > ---
> >     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(-)
> >
> > diff --git a/contrib/scalar/scalar.c b/contrib/scalar/scalar.c
> > index 1ce9c2b00e8..7db2a97416e 100644
> > --- a/contrib/scalar/scalar.c
> > +++ b/contrib/scalar/scalar.c
> > @@ -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;
> > +			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().

It's only a dozen lines, though, and they are pretty stable, so I doubt
that we risk divergent copied code.

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

I just responded to Stolee elsewhere in this thread with a lengthy
analysis of the options, and the conclusion that it would not be worth the
effort to refactor `handle_options()`.

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

It would be pretty easy to test `-C`:

	git init sub &&
	scalar -C sub register &&
	[... verify that `sub/` is now a Scalar repository ...]

For `-c`, we would need to configure something parsed by
`git_default_config()` that would influence what `scalar register` does,
then verify that. Or even better, use a config setting that is in the
"Optional" section of `set_recommended_config()`, i.e. it will refuse to
override an already-configured value. Something like `status.aheadBehind`.

I added this:

-- snip --
diff --git a/contrib/scalar/t/t9099-scalar.sh b/contrib/scalar/t/t9099-scalar.sh
index 2e1502ad45e..89781568f43 100755
--- a/contrib/scalar/t/t9099-scalar.sh
+++ b/contrib/scalar/t/t9099-scalar.sh
@@ -85,4 +85,12 @@ test_expect_success 'scalar delete with enlistment' '
 	test_path_is_missing cloned
 '

+test_expect_success 'scalar supports -c/-C' '
+	test_when_finished "scalar delete sub" &&
+	git init sub &&
+	scalar -C sub -c status.aheadBehind=bogus register &&
+	test -z "$(git -C sub config --local status.aheadBehind)" &&
+	test true = "$(git -C sub config core.preloadIndex)"
+'
+
 test_done
-- snap --

Ciao,
Dscho

[PATCH v2] scalar: accept -C and -c options before the subcommand

[Johannes Schindelin via GitGitGadget]

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.

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.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
    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.
    
    Changes since v1:
    
     * Added a regression test case.
     * Augmented the commit message with a brief analysis why we're choosing
       not to refactor git.c:handle_options() but instead copy-edit the
       dozen or so lines that we want.

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

Range-diff vs v1:

 1:  8f2af8c3ec1 ! 1:  d7ee2d03b04 scalar: accept -C and -c options before the subcommand
     @@ Commit message
      
          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.
     +
          Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
      
       ## contrib/scalar/scalar.c ##
     @@ contrib/scalar/scalar.txt: The `scalar` command implements various subcommands,
       COMMANDS
       --------
       
     +
     + ## contrib/scalar/t/t9099-scalar.sh ##
     +@@ contrib/scalar/t/t9099-scalar.sh: test_expect_success 'scalar delete with enlistment' '
     + 	test_path_is_missing cloned
     + '
     + 
     ++test_expect_success 'scalar supports -c/-C' '
     ++	test_when_finished "scalar delete sub" &&
     ++	git init sub &&
     ++	scalar -C sub -c status.aheadBehind=bogus register &&
     ++	test -z "$(git -C sub config --local status.aheadBehind)" &&
     ++	test true = "$(git -C sub config core.preloadIndex)"
     ++'
     ++
     + test_done


 contrib/scalar/scalar.c          | 22 +++++++++++++++++++++-
 contrib/scalar/scalar.txt        | 10 ++++++++++
 contrib/scalar/t/t9099-scalar.sh |  8 ++++++++
 3 files changed, 39 insertions(+), 1 deletion(-)

diff --git a/contrib/scalar/scalar.c b/contrib/scalar/scalar.c
index 1ce9c2b00e8..7db2a97416e 100644
--- a/contrib/scalar/scalar.c
+++ b/contrib/scalar/scalar.c
@@ -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;
+			argv += 2;
+		} else
+			break;
+	}
+
 	if (argc > 1) {
 		argv++;
 		argc--;
@@ -818,7 +837,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 f416d637289..cf4e5b889cc 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
 --------
 
diff --git a/contrib/scalar/t/t9099-scalar.sh b/contrib/scalar/t/t9099-scalar.sh
index 2e1502ad45e..89781568f43 100755
--- a/contrib/scalar/t/t9099-scalar.sh
+++ b/contrib/scalar/t/t9099-scalar.sh
@@ -85,4 +85,12 @@ test_expect_success 'scalar delete with enlistment' '
 	test_path_is_missing cloned
 '
 
+test_expect_success 'scalar supports -c/-C' '
+	test_when_finished "scalar delete sub" &&
+	git init sub &&
+	scalar -C sub -c status.aheadBehind=bogus register &&
+	test -z "$(git -C sub config --local status.aheadBehind)" &&
+	test true = "$(git -C sub config core.preloadIndex)"
+'
+
 test_done

base-commit: ddc35d833dd6f9e8946b09cecd3311b8aa18d295
-- 
gitgitgadget

Re: [PATCH] scalar: accept -C and -c options before the subcommand

[Junio C Hamano]

Derrick Stolee <stolee@gmail.com> writes:

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

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.

Re: [PATCH] scalar: accept -C and -c options before the subcommand

[Ævar Arnfjörð Bjarmason]

On Fri, Jan 28 2022, Johannes Schindelin wrote:

> On Thu, 27 Jan 2022, Derrick Stolee wrote:
>
>> 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.

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

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

I'm fine with just integrating this patch as-is, I just wanted to chime
in upthread with a question/context about a thing stated in previous
discussion/a link to that discussion.

It does seem to me that you're making a mountain out of a molehill
here. The below quick hack is a lib-ification of -c, -C and --config-env
that took me around 10 minutes. For ease of reading I squashed it into
your patch.

I mean, we're just talking about copy/pasting existing working code to a
new header file, giving it a function parameter, and other small bits of
scaffolding.

One thing we'll be doing by doing that is not wasting the time of
(checks $(ls -l po/*.po|wc -l) ...) around 20 translator on 2 new
strings you introduced, and if you view it with the move detection you
can see it's almost entirely just moving existing code around.

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.

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

diff --git a/Makefile b/Makefile
index 5580859afdb..2d0a6611cd5 100644
--- a/Makefile
+++ b/Makefile
@@ -900,6 +900,7 @@ LIB_OBJS += fmt-merge-msg.o
 LIB_OBJS += fsck.o
 LIB_OBJS += fsmonitor.o
 LIB_OBJS += gettext.o
+LIB_OBJS += gitcmd.o
 LIB_OBJS += gpg-interface.o
 LIB_OBJS += graph.o
 LIB_OBJS += grep.o
diff --git a/contrib/scalar/scalar.c b/contrib/scalar/scalar.c
index 1ce9c2b00e8..ee793ff6ccc 100644
--- a/contrib/scalar/scalar.c
+++ b/contrib/scalar/scalar.c
@@ -11,6 +11,7 @@
 #include "dir.h"
 #include "packfile.h"
 #include "help.h"
+#include "gitcmd.h"
 
 /*
  * Remove the deepest subdirectory in the provided path string. Path must not
@@ -808,17 +809,33 @@ int cmd_main(int argc, const char **argv)
 	struct strbuf scalar_usage = STRBUF_INIT;
 	int i;
 
-	if (argc > 1) {
 	argv++;
 	argc--;
 
+	while (argc > 1 && *argv[0] == '-') {
+		int show_usage = 0;
+
+		if (gity_handle_options(&argv, &argc, NULL, &show_usage)) {
+			(argv)++;
+			(argc)--;
+
+			if (show_usage)
+				goto usage;
+		} else {
+			break;
+		}
+	}
+
+	if (argc) {
 		for (i = 0; builtins[i].name; i++)
 			if (!strcmp(builtins[i].name, argv[0]))
 				return !!builtins[i].fn(argc, argv);
 	}
 
+usage:
 	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/Makefile b/Makefile
index 5580859afdb..2d0a6611cd5 100644
--- a/Makefile
+++ b/Makefile
@@ -900,6 +900,7 @@ LIB_OBJS += fmt-merge-msg.o
 LIB_OBJS += fsck.o
 LIB_OBJS += fsmonitor.o
 LIB_OBJS += gettext.o
+LIB_OBJS += gitcmd.o
 LIB_OBJS += gpg-interface.o
 LIB_OBJS += graph.o
 LIB_OBJS += grep.o
diff --git a/contrib/scalar/scalar.c b/contrib/scalar/scalar.c
index 1ce9c2b00e8..ee793ff6ccc 100644
--- a/contrib/scalar/scalar.c
+++ b/contrib/scalar/scalar.c
@@ -11,6 +11,7 @@
 #include "dir.h"
 #include "packfile.h"
 #include "help.h"
+#include "gitcmd.h"
 
 /*
  * Remove the deepest subdirectory in the provided path string. Path must not
@@ -808,17 +809,33 @@ int cmd_main(int argc, const char **argv)
 	struct strbuf scalar_usage = STRBUF_INIT;
 	int i;
 
-	if (argc > 1) {
 	argv++;
 	argc--;
 
+	while (argc > 1 && *argv[0] == '-') {
+		int show_usage = 0;
+
+		if (gity_handle_options(&argv, &argc, NULL, &show_usage)) {
+			(argv)++;
+			(argc)--;
+
+			if (show_usage)
+				goto usage;
+		} else {
+			break;
+		}
+	}
+
+	if (argc) {
 		for (i = 0; builtins[i].name; i++)
 			if (!strcmp(builtins[i].name, argv[0]))
 				return !!builtins[i].fn(argc, argv);
 	}
 
+usage:
 	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 f416d637289..cf4e5b889cc 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
 --------
 
diff --git a/contrib/scalar/t/t9099-scalar.sh b/contrib/scalar/t/t9099-scalar.sh
index 2e1502ad45e..89781568f43 100755
--- a/contrib/scalar/t/t9099-scalar.sh
+++ b/contrib/scalar/t/t9099-scalar.sh
@@ -85,4 +85,12 @@ test_expect_success 'scalar delete with enlistment' '
 	test_path_is_missing cloned
 '
 
+test_expect_success 'scalar supports -c/-C' '
+	test_when_finished "scalar delete sub" &&
+	git init sub &&
+	scalar -C sub -c status.aheadBehind=bogus register &&
+	test -z "$(git -C sub config --local status.aheadBehind)" &&
+	test true = "$(git -C sub config core.preloadIndex)"
+'
+
 test_done
diff --git a/git.c b/git.c
index edda922ce6d..dc75b3f0294 100644
--- a/git.c
+++ b/git.c
@@ -5,6 +5,7 @@
 #include "run-command.h"
 #include "alias.h"
 #include "shallow.h"
+#include "gitcmd.h"
 
 #define RUN_SETUP		(1<<0)
 #define RUN_SETUP_GENTLY	(1<<1)
@@ -138,6 +139,7 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)
 
 	while (*argc > 0) {
 		const char *cmd = (*argv)[0];
+		int show_usage = 0;
 		if (cmd[0] != '-')
 			break;
 
@@ -247,22 +249,6 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)
 			setenv(GIT_IMPLICIT_WORK_TREE_ENVIRONMENT, "0", 1);
 			if (envchanged)
 				*envchanged = 1;
-		} else if (!strcmp(cmd, "-c")) {
-			if (*argc < 2) {
-				fprintf(stderr, _("-c expects a configuration string\n" ));
-				usage(git_usage_string);
-			}
-			git_config_push_parameter((*argv)[1]);
-			(*argv)++;
-			(*argc)--;
-		} else if (!strcmp(cmd, "--config-env")) {
-			if (*argc < 2) {
-				fprintf(stderr, _("no config key given for --config-env\n" ));
-				usage(git_usage_string);
-			}
-			git_config_push_env((*argv)[1]);
-			(*argv)++;
-			(*argc)--;
 		} else if (skip_prefix(cmd, "--config-env=", &cmd)) {
 			git_config_push_env(cmd);
 		} else if (!strcmp(cmd, "--literal-pathspecs")) {
@@ -295,19 +281,6 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)
 			set_alternate_shallow_file(the_repository, (*argv)[0], 1);
 			if (envchanged)
 				*envchanged = 1;
-		} else if (!strcmp(cmd, "-C")) {
-			if (*argc < 2) {
-				fprintf(stderr, _("no directory given for '%s' option\n" ), "-C");
-				usage(git_usage_string);
-			}
-			if ((*argv)[1][0]) {
-				if (chdir((*argv)[1]))
-					die_errno("cannot change to '%s'", (*argv)[1]);
-				if (envchanged)
-					*envchanged = 1;
-			}
-			(*argv)++;
-			(*argc)--;
 		} else if (skip_prefix(cmd, "--list-cmds=", &cmd)) {
 			trace2_cmd_name("_query_");
 			if (!strcmp(cmd, "parseopt")) {
@@ -322,6 +295,10 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)
 			} else {
 				exit(list_cmds(cmd));
 			}
+		} else if (gity_handle_options(argv, argc, envchanged,
+					       &show_usage)) {
+			if (show_usage)
+				usage(git_usage_string);
 		} else {
 			fprintf(stderr, _("unknown option: %s\n"), cmd);
 			usage(git_usage_string);
diff --git a/gitcmd.c b/gitcmd.c
new file mode 100644
index 00000000000..0d8630ec2a4
--- /dev/null
+++ b/gitcmd.c
@@ -0,0 +1,47 @@
+#include "cache.h"
+#include "gitcmd.h"
+#include "config.h"
+
+int gity_handle_options(const char ***argv, int *argc,
+			int *envchanged, int *show_usage)
+{
+	const char *cmd = (*argv)[0];
+
+	if (!strcmp(cmd, "-c")) {
+		if (*argc < 2) {
+			fprintf(stderr, _("-c expects a configuration string\n" ));
+			goto usage;
+		}
+		git_config_push_parameter((*argv)[1]);
+		(*argv)++;
+		(*argc)--;
+	} else if (!strcmp(cmd, "--config-env")) {
+		if (*argc < 2) {
+			fprintf(stderr, _("no config key given for --config-env\n" ));
+			goto usage;
+		}
+		git_config_push_env((*argv)[1]);
+		(*argv)++;
+		(*argc)--;
+	} else if (!strcmp(cmd, "-C")) {
+		if (*argc < 2) {
+			fprintf(stderr, _("no directory given for '%s' option\n" ), "-C");
+			goto usage;
+		}
+		if ((*argv)[1][0]) {
+			if (chdir((*argv)[1]))
+				die_errno("cannot change to '%s'", (*argv)[1]);
+			if (envchanged)
+				*envchanged = 1;
+		}
+		(*argv)++;
+		(*argc)--;
+	} else {
+		return 0;
+	}
+	return 1;
+usage:
+	*show_usage = 1;
+	return 1;
+}
+
diff --git a/gitcmd.h b/gitcmd.h
new file mode 100644
index 00000000000..95c934b1500
--- /dev/null
+++ b/gitcmd.h
@@ -0,0 +1,11 @@
+#ifndef GITCMD_H
+#define GITCMD_H
+
+/**
+ * Handle options like in a "git"-y way, i.e. to emulate a command
+ * that works like "git" itself. This is the part of handle_options()
+ * that contrib/scalar needs from git.c.
+ */
+int gity_handle_options(const char ***argv, int *argc, int *envchanged,
+			int *show_usage);
+#endif

Re: [PATCH] scalar: accept -C and -c options before the subcommand

[Derrick Stolee]

On 1/28/2022 6:27 AM, Johannes Schindelin wrote:
> Hi Stolee,
> 
> On Thu, 27 Jan 2022, Derrick Stolee wrote:
> 
>> 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.

I agree that these would be necessary steps.

> 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

These "Won't hurt" items look to me as "they probably don't matter, but
it would be nice if "scalar <option>" didn't just fail for users who are
used to "git <option>".

> # Detrimental

I think your "detrimental" choices are actually more useful than any of
your "won't hurt" choices.

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

As mentioned, I use this option in my local development all the time.
This compatibility issue you discuss is something that happens within
Git itself, too, when it calls a subcommand. So, users can already
hurt themselves in this way and should be cautious about using it.

>   --list-cmds
> 
> 	As you pointed out, this option would produce misleading output.

It would, but I also think that a correct implementation would be
helpful to users. It just takes more work than just calling
handle_options() as-is.

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

I was thinking that it would be good to maybe extract just the "-C", "-c"
options from handle_options() and call that from scalar.c, but it wouldn't
work to "just parse "-C" and "-c" and then parse all the other options"
because if someone did "git --exec-path=<X> -C <Y> status" then the "-C"
parser would want to stop after seeing "--exec-path".

So, perhaps the code copy is really the least intrusive approach we have
until we see a need for more of these options.

Thanks,
-Stolee

Re: [PATCH] scalar: accept -C and -c options before the subcommand

[Derrick Stolee]

On 1/28/2022 1:05 PM, Junio C Hamano wrote:
> Derrick Stolee <stolee@gmail.com> writes:
> 
>> 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.
> 
> 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.
 
I have come around to this line of thinking. My response to Johannes'
detailed examination of my idea goes into that more deeply.

Thanks,
-Stolee
 

Re: [PATCH v2] scalar: accept -C and -c options before the subcommand

[Derrick Stolee]

On 1/28/2022 9:31 AM, Johannes Schindelin via GitGitGadget wrote:
> 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.
> 
> 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.

Based on discussions elsewhere in this thread, I have changed my mind
and agree that this approach is the best one to take at the moment.

With this additional context and the new tests, I think this patch is
ready to go.

Thanks,
-Stolee

Re: [PATCH] scalar: accept -C and -c options before the subcommand

[Derrick Stolee]

On 1/28/2022 1:21 PM, Ævar Arnfjörð Bjarmason wrote:
> 
> On Fri, Jan 28 2022, Johannes Schindelin wrote:
> 
>> On Thu, 27 Jan 2022, Derrick Stolee wrote:
>>
>>> 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.
> 
> 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.

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

> diff --git a/contrib/scalar/scalar.c b/contrib/scalar/scalar.c
> index 1ce9c2b00e8..ee793ff6ccc 100644
> --- a/contrib/scalar/scalar.c
> +++ b/contrib/scalar/scalar.c

> diff --git a/contrib/scalar/scalar.c b/contrib/scalar/scalar.c
> index 1ce9c2b00e8..ee793ff6ccc 100644
> --- a/contrib/scalar/scalar.c
> +++ b/contrib/scalar/scalar.c

Was this diff double-copied or something?

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

This loop seemed a tad cumbersome for something calling a helper
method, BUT....

> @@ -322,6 +295,10 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)
>  			} else {
>  				exit(list_cmds(cmd));
>  			}
> +		} else if (gity_handle_options(argv, argc, envchanged,
> +					       &show_usage)) {
> +			if (show_usage)
> +				usage(git_usage_string);

...by checking this at the end of your loop here, you have solved
the concern I was talking about with something like

	git --no-pager -C <X> status

having the -C parsing fail because it doesn't recognize --no-pager.

I think this patch you present here would be a good approach to
start transitioning options out of handle_options(), perhaps one
at a time.

But due to my stated goal of "let's not modify things in libgit.a
for Scalar unless absolutely necessary", this approach should be
delayed until at least Scalar is more final.

Thanks,
-Stolee

Re: [PATCH] scalar: accept -C and -c options before the subcommand

[Ævar Arnfjörð Bjarmason]

On Fri, Jan 28 2022, Derrick Stolee wrote:

> On 1/28/2022 1:21 PM, Ævar Arnfjörð Bjarmason wrote:
>> 
>> On Fri, Jan 28 2022, Johannes Schindelin wrote:
>> 
>>> On Thu, 27 Jan 2022, Derrick Stolee wrote:
>>>
>>>> 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.
>> 
>> 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.
>
>> 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.

Fair enough.

I think there's cases where it won't make sense, and cases where it
will.

Maybe it doesn't make sense here, but generally I wouldn't take it being
needed for scalar heavily into account per-se, for the review & i18n
reasons I mentioned.

I.e. the list is already looking at these patches, and translators are
being presented these strings as part of our existing set.

So "this reuses existing tested code" and "you won't need to translate
this new thing" will be benefits whatever the current state of scalar
is.

>> 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).
>
>> diff --git a/contrib/scalar/scalar.c b/contrib/scalar/scalar.c
>> index 1ce9c2b00e8..ee793ff6ccc 100644
>> --- a/contrib/scalar/scalar.c
>> +++ b/contrib/scalar/scalar.c
>
>> diff --git a/contrib/scalar/scalar.c b/contrib/scalar/scalar.c
>> index 1ce9c2b00e8..ee793ff6ccc 100644
>> --- a/contrib/scalar/scalar.c
>> +++ b/contrib/scalar/scalar.c
>
> Was this diff double-copied or something?

Yes, sorry the first 3 hunks are the same. I manually re-copied this
into my editor (I forgot a stray debugging printf) and screwed it up.