Re: [PATCH v8 00/17] Upstreaming the Scalar command

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Mon, Nov 22 2021, Johannes Schindelin wrote:

> On Mon, 22 Nov 2021, Ævar Arnfjörð Bjarmason wrote:
>
>> On Mon, Nov 22 2021, Johannes Schindelin wrote:
>>
>> > On Sat, 20 Nov 2021, Elijah Newren wrote:
>> >
>> >> On Fri, Nov 19, 2021 at 3:03 PM Johannes Schindelin via GitGitGadget
>> >> <gitgitgadget@gmail.com> wrote:
>> >> >
>> >> > tl;dr: This series contributes the core part of the Scalar command to
>> >> > the Git project. This command provides an opinionated way to create
>> >> > and configure Git repositories with a focus on very large
>> >> > repositories.
>> >>
>> >> I thought after
>> >> https://lore.kernel.org/git/nycvar.QRO.7.76.6.2110062241150.395@tvgsbejvaqbjf.bet/
>> >> that you'd update merge.renames to true on what is now patch 7.  Did
>> >> you end up changing your mind, or was this overlooked?
>> >
>> > Oops! Thank you so much for the reminder.
>> >
>> > Will fix. I do not plan on sending out a new iteration for a few more days
>> > because I do not want to send lots of patches to the list right now,
>> > reviewer bandwidth seems to be stretched quite a bit already.
>>
>> Bandwidth which is further stretched by continuing to send updates to
>> this topic while ignoring outstanding feedback.
>
> The feedback you are referring to is probably the repeated demand to
> integrate Scalar deeply into Git's build process.
>
> As I have tired of replying, it is not the time for that yet.
>
> Repeating that demand does not make it more sensible, nor does it
> magically make it the right time.

I'm not repeating that demand. I clearly also think the approach you're
insisting picking isn't a good one, but let's leave that aside.

What I'm referring to with "I've also been pointing out" in the context
you elided is that if you:

 1. Check out your topic
 2. Apply my proposed patch on top (a newer version than on-list is
    in avar/scalar-move-build-from-contrib-2 in my GH fork)
 3. Push both to CI
 4. Diff the two logs of the runs (or just manually click through
    and inspect them)

You'd have seen before sending your version of the CI integration that
the difference in behavior that started with your version of the topic
was particular to the contrib/scalar/ integration, but not the top-level
Makefile integration. I.e. adding the scalar tests to the previously
build-only jobs.

I've been noting that as clearly as I'm able to in numerous past
exchanges. You've either ignored those reports, or like here,
selectivtely replied only to parts of what I've told you.

I.e. something like "I'm not going 100% for the approach you
suggest". Sure, I'm not saying you have to. But I also noted that the
patch with that suggested approach can be considered a bug report
against your series.

The reason that patch isn't split into two things, one fixing all the
issues I noticed, and another implementing some "alternate build"
approach is that I found that to be impossible to do.

Those issues are all particular to emergent effects of the build
integration you're choosing to go with.

E.g. in the case of "seen" being broken the CI simply runs "make test"
as it did before, and scalar integrates into that, and you can run that
target without having built anything already.

> Nor is it credible to call the build "broken" when it does what it is
> supposed to do, thank you very much.

Here's specific commands showing that it's broken.

On your version (I've got v7 locally, but the same is true of v8):

    $ make clean; make -C contrib/scalar test
    [...]
        CC hook.o
        CC version.o
        CC help.o
        AR libgit.a
    make[1]: Leaving directory '/home/avar/g/git'
        SUBDIR ../..
    make[1]: Entering directory '/home/avar/g/git'
        * new link flags
        CC contrib/scalar/scalar.o
        LINK contrib/scalar/scalar
    make[1]: Leaving directory '/home/avar/g/git'
    make -C t
    make[1]: Entering directory '/home/avar/g/git/contrib/scalar/t'
    *** prove ***
    error: GIT-BUILD-OPTIONS missing (has Git been built?).
    t9099-scalar.sh .. Dubious, test returned 1 (wstat 256, 0x100)
    No subtests run 
    
    Test Summary Report
    -------------------
    t9099-scalar.sh (Wstat: 256 Tests: 0 Failed: 0)
      Non-zero exit status: 1
      Parse errors: No plan found in TAP output

On the patch I proposed to apply on top:

    $ make clean; make test T=t9099-scalar.sh
    [...]
        CC t/helper/test-xml-encode.o
        GEN bin-wrappers/git
        GEN bin-wrappers/git-receive-pack
        GEN bin-wrappers/git-shell
        GEN bin-wrappers/git-upload-archive
        GEN bin-wrappers/git-upload-pack
        GEN bin-wrappers/scalar
        GEN bin-wrappers/git-cvsserver
        GEN bin-wrappers/test-fake-ssh
        GEN bin-wrappers/test-tool
        LINK t/helper/test-fake-ssh
        LINK t/helper/test-tool
    make -C t/ all
    make[1]: Entering directory '/home/avar/g/git/t'
    rm -f -r 'test-results'
    *** prove ***
    t9099-scalar.sh .. ok   
    All tests successful.

You might correctly note that this doesn't work either on that version
(or for any other existing test in t/):

    $ make clean >/dev/null; make -C t T=t9099-scalar.sh 
    GIT_VERSION = 2.34.GIT-dev
    make: Entering directory '/home/avar/g/git/t'
    rm -f -r 'test-results'
    *** prove ***
    error: GIT-BUILD-OPTIONS missing (has Git been built?).
    t9099-scalar.sh .. Dubious, test returned 1 (wstat 256, 0x100)
    No subtests run 
    
    Test Summary Report
    -------------------
    t9099-scalar.sh (Wstat: 256 Tests: 0 Failed: 0)
      Non-zero exit status: 1
      Parse errors: No plan found in TAP output

Which is true, but that's not broken because it's not attempting to
build the top-level via some incomplete integration, but your version
is.