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

From: Derrick Stolee <stolee@gmail.com>
Date: Jan 28, 2022, 19:37 UTC
Message-ID: <5283a1b2-a31f-1657-1a67-cab6d9fec0ac@gmail.com>
In-Reply-To: <nycvar.QRO.7.76.6.2201281148310.347@tvgsbejvaqbjf.bet>

On 1/28/2022 6:27 AM, Johannes Schindelin wrote:

Show 18 quoted lines

> 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.

Show 15 quoted lines

> 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.

Show 5 quoted lines

>   --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.

Show 18 quoted lines

> 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