On Sat, Dec 11 2021, Johannes Schindelin wrote:
> Hi Junio,
> [...]
> We do have a very different understanding of "fairly easily" in that case.
> Three iterations, and three weeks time spent on implementing what you
> suggest, only to see broken by the merge of the `ab/ci-updates` patch
> series, suggesting a fixup for the incorrect merge, seeing that fixup
> rejected, and then more discussing, all of that does not strike me as
> "fairly easily". It strikes me as "a lot of time and effort was spent,
> mostly stepping on toes".
I sent you a working path to a fixup in [1] on the 23rd of November
where we won't go from running zero tests in compile-only to running
just the scalar test.
Junio replied[2] ("the above" referring to [1]):
I think the above shows that it is a bug in the topic itself,
You didn't reply further in that fixup thread, and then your v9 re-roll
a week later still had the same issue[3] discussed therein. I again
pointed that out[4]:
Is it intentional that the previously compile-only "pedantic" job is now
running the scalar tests?
You didn't reply, but in your v10 decided to make the current iteration
of this series have no CI testing at all, and cited the interaction with
ab/ci-updates[4]:
because a recent unrelated patch series does not interact well with them.
Which I think is clearly inaccurate, because...
> Granted, if `ab/ci-updates` would not have happened, it would have been
> much easier. Or if `ab/ci-updates` had waited until `js/scalar` advanced
> to `next`. But the way it happened was (unnecessarily?) un-easy.
...your initial patch to run the scalar tests in CI[5] was part of v7, and
had the issue described above. It pre-dates the v1 of ab/ci-updates
being on-list by a couple of days[6].
So yes, I do think it was "easy", as in that was an easy fix-up. You
just didn't follow up on it and submitted re-rolls with the already
noted breakage.
I don't blame you for that, maybe you were busy, it slipped through
etc.
But I don't accept that delays in this topic are my fault, or something
to the effect that that this whole saga represents some failure of the
review process.
Our topics textually/semantically conflicted, it happens. I offered a
fixup & way forward. Fixing it was trivial, and still is. You just
didn't follow-up.
> [...]
>> If "The Scalar Functional Tests" that were designed with Azure Repos in
>> mind is not a good fit to come into contrib/scalar/, it is fine not to
>> have it here---lack of it would not make the test target you have in
>> contrib/scalar/Makefile any less valuable, I would think.
>
> The test target won't go anywhere, no worries. Just like the test target
> in contrib/subtree/ does not go anywhere.
>
> And just like `contrib/subtree/`, it does not have to be run as part of
> Git's CI build.
But unlike contrib/completion, which we do run as part of Git's CI
build[7]?
>> Unless you are saying that "make -C contrib/scalar test" is useless,
>> that is. But I do not think that is the case.
>
> It is as useful as `make -C contrib/subtree test`. Which, as Ævar will
> readily offer, is broken, because it does not ensure that top-level `make
> all` is executed and therefore in a fresh checkout will fail.
Before the scalar topic there was only one "make" entry point to build
libgit.a, contrib/scalar/Makefile makes that two. That was the immediate
prompt for the fixup discussion in [1].
So no, I won't offer that "make -C contrib/subtree test" is broken, it
doesn't try to build libgit.a and errors out right away if git isn't
built.
Your scalar patches do try, get most of the way there, and fail.
Your bicycle isn't broken if it doesn't make coffee, but if your fridge
has a built-in coffee maker and it doesn't work it's broken, at least as
it pertains to its coffee making function.
I think I made that distinction clear in [8], but apparently not clear
enough, as you seem to be under the impression that I was conveying the
opposite of the idea I was trying to get across.
> Of course, I disagree that it is "broken". It works as designed. It is in
> the contrib/ part of the tree, i.e. safely in the realm of "you have to
> build Git first, and then the thing in contrib/". In other words, the idea
> to "fix" this kind of "broken"ness is a solution in search of a problem.
I agree with that, but it's your proposed patches that contain the build
integration you're describing as unnecessary for "contrib/subtree/". In
v8->v8 of the series you changed the CI integration from:
make -C contrib/scalar test
To:
make && make -C contrib/scalar test
While keeping the bits in contrib/scalar/Makefile that made it go most
of the way towards a working "libgit.a" useful for testing, but it
breaks before we get everything we need to run the "test" target.
Which I find to be odd given the above comparison to contib/subtree/. If
you have to build git first at the top level why is it trying and
failing to build git? "contrib/subtree" doesn't.
> [...]
> I would find those things quite a bit more useful than to force regular
> Git contributors who want to change libgit.a (even if it is just pointless
> refactoring) to pay attention to contrib/scalar/ in CI, when there is
> still no clear answer whether Scalar will even become a first-class Git
> command eventually (which I hope it will, of course).
It's in-tree, scalar.c is compiled by default, so they'll have to choice
but to pay attention to it.
The question is whether we should have test and CI coverage for code in
that state.
1. https://lore.kernel.org/git/211123.86ilwjujmd.gmgdl@xxxxxxxxxxxxxxxxxxx/
2. https://lore.kernel.org/git/xmqqo86a92jm.fsf@gitster.g/
3. https://lore.kernel.org/git/pull.1005.v10.git.1638538470.gitgitgadget@xxxxxxxxx/
4. https://lore.kernel.org/git/211130.861r2xelmx.gmgdl@xxxxxxxxxxxxxxxxxxx/
5. https://lore.kernel.org/git/1b0328fa236a35c2427b82f53c32944e513580d3.1637158762.git.gitgitgadget@xxxxxxxxx/
6. https://lore.kernel.org/git/cover-0.2-00000000000-20211119T135343Z-avarab@xxxxxxxxx/
7. https://lore.kernel.org/git/211210.86a6h9duay.gmgdl@xxxxxxxxxxxxxxxxxxx/
8. https://lore.kernel.org/git/211123.86ee77uj18.gmgdl@xxxxxxxxxxxxxxxxxxx/
9. https://lore.kernel.org/git/pull.1005.v9.git.1638273289.gitgitgadget@xxxxxxxxx/
