On 1/28/2022 1:05 PM, Junio C Hamano wrote:

Derrick Stolee [off-list ref] writes:

quoted

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