Derrick Stolee <stolee@xxxxxxxxx> 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.
