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

> On 1/28/2022 1:21 PM, Ævar Arnfjörð Bjarmason wrote:
>> 
>> On Fri, Jan 28 2022, Johannes Schindelin wrote:
>> 
>>> 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:
>>> [...]
>>> # 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.
>> 
>> So just to clarify, do you and Stolee disagree about scalar supporting
>> --exec-path, per his comments above?
>
> I think it would be nice, but it's also not a blocker for me.
>
>> In this case I don't mind much, but speaking generally I see you and
>> Stolee tying yourselves in knots again about scalar being in contrib so
>> we shouldn't use libgit.
>> 
>> It already uses libgit, there's even (last I checked) at least one
>> function in it only used directly by the scalar code.
>
> My concern is not that we shouldn't use libgit (because we do) but that
> we shouldn't make significant changes to libgit.a only for Scalar's
> benefit until it is incorporated in a more final way.
>
> In my opinion, well-architected code is code that is easy to delete.
> Until we have Scalar mostly feature-complete and can make a decision
> about it living in the Git tree long-term (and _where_ it resides), I
> would like to have the following property: If I were to revert all
> commits that touch contrib/scalar/ and squash them into a single commit,
> then we would have a bunch of file deletions and a very small set of
> edits to the Makefile. I don't know how much the ship has sailed there
> already, but keeping the size of that "revert diff" small means that we
> are keeping the coupling low during this review process.

>> I don't remember anyone having any objection to scalar using libgit
>> code, or even that there's libgit code just to help it along. That's a
>> self-imposed limitation you two seem to have invented.
>> 
>> Personally I find a patch like the below much easier to review. It's the
>> parts that aren't easy to review boilerplate are all things that we have
>> in-tree already.
>> 
>> Whereas proposing a new way to parse -c or -C will lead (at least me) to
>> carefully eyeballing that new implementation, looking at how it differs
>> (if at all) from the existing one, wondering why the i18n strings are
>> subtly different etc (I saw one reason is that since the code was
>> copy/pasted initially the git.c version was updated, but your patch
>> wasn't updated to copy it).
>
>> diff --git a/contrib/scalar/scalar.c b/contrib/scalar/scalar.c
>> index 1ce9c2b00e8..ee793ff6ccc 100644
>> --- a/contrib/scalar/scalar.c
>> +++ b/contrib/scalar/scalar.c
>
>> diff --git a/contrib/scalar/scalar.c b/contrib/scalar/scalar.c
>> index 1ce9c2b00e8..ee793ff6ccc 100644
>> --- a/contrib/scalar/scalar.c
>> +++ b/contrib/scalar/scalar.c
>
> Was this diff double-copied or something?