On Fri, Jan 28 2022, Derrick Stolee wrote:

On 1/28/2022 1:21 PM, Ævar Arnfjörð Bjarmason wrote:

quoted

On Fri, Jan 28 2022, Johannes Schindelin wrote:

quoted

On Thu, 27 Jan 2022, Derrick Stolee wrote:

quoted

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.

quoted

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.

Fair enough.

I think there's cases where it won't make sense, and cases where it
will.

Maybe it doesn't make sense here, but generally I wouldn't take it being
needed for scalar heavily into account per-se, for the review & i18n
reasons I mentioned.

I.e. the list is already looking at these patches, and translators are
being presented these strings as part of our existing set.

So "this reuses existing tested code" and "you won't need to translate
this new thing" will be benefits whatever the current state of scalar
is.

quoted

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

quoted

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

quoted

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?

Yes, sorry the first 3 hunks are the same. I manually re-copied this
into my editor (I forgot a stray debugging printf) and screwed it up.