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
