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
