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

Hi Stolee,

On Thu, 27 Jan 2022, Derrick Stolee wrote:

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.

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