Re: [PATCH v10 00/15] Upstreaming the Scalar command

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Dec 11, 2021, 01:07 UTC
Message-ID: <211211.86mtl7c2b9.gmgdl@evledraar.gmail.com>
In-Reply-To: <nycvar.QRO.7.76.6.2112110044010.90@tvgsbejvaqbjf.bet>

On Sat, Dec 11 2021, Johannes Schindelin wrote:

Show 9 quoted lines

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

Show 11 quoted lines

> [...]
>> 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]?

Show 6 quoted lines

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

Show 6 quoted lines

> [...]
> 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@evledraar.gmail.com/
2. https://lore.kernel.org/git/xmqqo86a92jm.fsf@gitster.g/
3. https://lore.kernel.org/git/pull.1005.v10.git.1638538470.gitgitgadget@gmail.com/
4. https://lore.kernel.org/git/211130.861r2xelmx.gmgdl@evledraar.gmail.com/
5. https://lore.kernel.org/git/1b0328fa236a35c2427b82f53c32944e513580d3.1637158762.git.gitgitgadget@gmail.com/
6. https://lore.kernel.org/git/cover-0.2-00000000000-20211119T135343Z-avarab@gmail.com/
7. https://lore.kernel.org/git/211210.86a6h9duay.gmgdl@evledraar.gmail.com/
8. https://lore.kernel.org/git/211123.86ee77uj18.gmgdl@evledraar.gmail.com/
9. https://lore.kernel.org/git/pull.1005.v9.git.1638273289.gitgitgadget@gmail.com/