mbox series

[v8,00/17] Upstreaming the Scalar command

Message ID pull.1005.v8.git.1637363024.gitgitgadget@gmail.com (mailing list archive)
Headers show
Series Upstreaming the Scalar command | expand

Message

Philippe Blain via GitGitGadget Nov. 19, 2021, 11:03 p.m. UTC
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.

Changes since v7:

 * Clarified in the commit message why we cannot easily encapsulate the
   Scalar part of the CMake configuration in contrib/scalar/.
 * Improved the README.md.

Changes since v6:

 * Rebased on top of v2.34.0.
 * Inserted a commit that adds contrib/scalar/README.md, containing the
   roadmap of what I have planned for Scalar.
 * The Scalar test's definition of GIT_TEST_MAINT_SCHEDULER has been
   adjusted to accommodate for a change in v2.32.0..v2.34.0.
 * The config setting defaults now include fetch.showForcedUpdates=false,
   which has been identified as helping with a performance issue in large
   repositories.
 * To avoid mistaking the current patch series for being feature-complete
   enough to unleash onto end users, I moved the Makefile rules to build
   HTML/manual pages to a later patch series.
 * The patch that adds support for -c <key>=<value> and -C <directory> was
   moved to its own add-on patch series: While it is obvious that those
   options are valuable to have, an open question is whether there are other
   "pre-command" options in git that would be useful, too, and I would like
   to postpone that discussion to that date.
 * I added two patches that I had planned on keeping in an add-on patch
   series for later, to build and test Scalar as part of the CI. I am still
   not 100% certain that it is a good idea to do so already now, but let's
   see what the reviewers have to say.

Changes since v5:

 * Fixed the commit message talking about make -C contrib/scalar/Makefile.
 * Fixed the git ls-tree invocation suggested in the manual for scalar
   clone.
 * Invoking make -C contrib/scalar, then changing a source file of libgit.a
   and then immediately invoking make -C contrib/scalar again will now
   implicitly rebuild libgit.a.

Changes since v4:

 * scalar delete now refuses to delete anything if it was started from
   within the enlistment.
 * scalar delete releases any handles to the object store before deleting
   the enlistment.
 * The OBJECTS list in the Makefile will now include Scalar.
 * scalar register now supports secondary worktrees, in addition to the
   primary worktree.

Changes since v3:

 * Moved the "Changes since" section to the top, to make it easier to see
   what changed.
 * Reworded the commit message of the first patch.
 * Removed the [RFC] prefix because I did not hear any objections against
   putting this into contrib/.

Changes since v2:

 * Adjusted the description of the list command in the manual page , as
   suggested by Bagas.
 * Addressed two style nits in cmd_run().
 * The documentation of git reconfigure -a was improved.

Changes since v1:

 * A couple typos were fixed
 * The code parsing the output of ls-remote was made more readable
 * The indentation used in scalar.txt now consistently uses tabs
 * We no longer hard-code core.bare = false when registering with Scalar


Background
==========

Microsoft invested a lot of effort into scaling Git to the needs of the
Windows operating system source code. Based on the experience of the first
approach, VFS for Git, the Scalar project was started. Scalar specifically
has as its core goal to funnel all improvements into core Git.


The present
===========

The Scalar project provides a completely functional non-virtual experience
for monorepos. But why stop there. The Scalar project was designed to be a
self-destructing vehicle to allow those key concepts to be moved into core
Git itself for the benefit of all. For example, partial clone,
sparse-checkout, and scheduled background maintenance have already been
upstreamed and removed from Scalar proper. This patch series provides a
C-based implementation of the final remaining portions of the Scalar
command. This will make it easier for users to experiment with the Scalar
command. It will also make it substantially easier to experiment with moving
functionality from Scalar into core Git, while maintaining
backwards-compatibility for existing Scalar users.

The C-based Scalar has been shipped to Scalar users, and can be tested by
any interested reader: https://github.com/microsoft/git/releases/ (it offers
a Git for Windows installer, a macOS package and an Ubuntu package, Scalar
has been included since v2.33.0.vfs.0.0).


Next steps
==========

Since there are existing Scalar users, I want to ensure
backwards-compatibility with its existing command-line interface. Keeping
that in mind, everything in this series is up for discussion.

I obviously believe that Scalar brings a huge benefit, and think that it
would be ideal for all of Scalar's learnings to end up in git clone/git
init/git maintenance eventually. It is also conceivable, however, that the
scalar command could graduate to be a core part of Git at some stage in the
future (such a decision would probably depend highly on users' feedback).
See also the discussion about the architecture of Scalar
[https://lore.kernel.org/git/b67bbef4-e4c3-b6a7-1c7f-7d405902ef8b@gmail.com/],
kicked off by Stolee.

On top of this patch series, I have lined up a few more:

 1. Implement a scalar diagnose command.
 2. Use the built-in FSMonitor (that patch series obviously needs to wait
    for FSMonitor to be integrated).
 3. Modify the config machinery to be more generous about concurrent writes,
    say, to the user-wide config.
 4. A few patches to optionally build and install scalar as part of a
    regular Git install (also teaching git help scalar to find the Scalar
    documentation

These are included in my vfs-with-scalar branch thicket
[https://github.com/dscho/git/commits/vfs-with-scalar]. On top of that, this
branch thicket also includes patches I do not plan on upstreaming, mainly
because they are too specific either to VFS for Git, or they support Azure
Repos (which does not offer partial clones but speaks the GVFS protocol,
which can be used to emulate partial clones).

One other thing is very interesting about that vfs-with-scalar branch
thicket: it contains a GitHub workflow which will run Scalar's quite
extensive Functional Tests suite. This test suite is quite comprehensive and
caught us a lot of bugs in the past, not only in the Scalar code, but also
core Git.


Epilogue
========

Now, to address some questions that I imagine every reader has who made it
this far:

 * Why not put the Scalar functionality directly into core Git, even a
   built-in? I wanted to provide an easy way for Git contributors to "play
   with" Scalar, without forcing a new top-level command into Git.
 * Why implement the Scalar command in the Git code base? Apart from
   simplifying Scalar maintenance in the Microsoft port of Git, the tight
   version coupling between Git and Scalar reduces the maintenance burden
   even further. Besides, I believe that it will make it much easier to
   shift functionality from Scalar into core Git, once we took the hurdle of
   accepting the Scalar code into the code base.
 * Why contribute Scalar to the Git project? We are biased, of course, yet
   our data-driven approach provides evidence that Scalar helps handling
   huge repositories with ease. By contributing it to the core Git project,
   we are able to share it with more users, especially some users who do not
   want to install Microsoft's fork of Git. We also hope that a lot of
   Scalar (maybe all of it) will end up in core Git, to benefit even more
   users.

Derrick Stolee (4):
  scalar: 'register' sets recommended config and starts maintenance
  scalar: 'unregister' stops background maintenance
  scalar: implement 'scalar list'
  scalar: implement the `run` command

Johannes Schindelin (12):
  scalar: add a README with a roadmap
  scalar: create a rudimentary executable
  scalar: start documenting the command
  scalar: create test infrastructure
  cmake: optionally build `scalar`, too
  ci: also run the `scalar` tests
  scalar: let 'unregister' handle a deleted enlistment directory
    gracefully
  scalar: implement the `clone` subcommand
  scalar: teach 'clone' to support the --single-branch option
  scalar: allow reconfiguring an existing enlistment
  scalar: teach 'reconfigure' to optionally handle all registered
    enlistments
  scalar: implement the `version` command

Matthew John Cheetham (1):
  scalar: implement the `delete` command

 .github/workflows/main.yml          |  15 +
 Makefile                            |   9 +
 ci/run-build-and-tests.sh           |   1 +
 ci/run-test-slice.sh                |   5 +
 contrib/buildsystems/CMakeLists.txt |  14 +
 contrib/scalar/.gitignore           |   2 +
 contrib/scalar/Makefile             |  45 ++
 contrib/scalar/README.md            |  82 +++
 contrib/scalar/scalar.c             | 826 ++++++++++++++++++++++++++++
 contrib/scalar/scalar.txt           | 145 +++++
 contrib/scalar/t/Makefile           |  78 +++
 contrib/scalar/t/t9099-scalar.sh    |  88 +++
 12 files changed, 1310 insertions(+)
 create mode 100644 contrib/scalar/.gitignore
 create mode 100644 contrib/scalar/Makefile
 create mode 100644 contrib/scalar/README.md
 create mode 100644 contrib/scalar/scalar.c
 create mode 100644 contrib/scalar/scalar.txt
 create mode 100644 contrib/scalar/t/Makefile
 create mode 100755 contrib/scalar/t/t9099-scalar.sh


base-commit: cd3e606211bb1cf8bc57f7d76bab98cc17a150bc
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1005%2Fdscho%2Fscalar-the-beginning-v8
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1005/dscho/scalar-the-beginning-v8
Pull-Request: https://github.com/gitgitgadget/git/pull/1005

Range-diff vs v7:

  1:  3aa095dc824 !  1:  3a2e28275f1 scalar: add a README with a roadmap
     @@ contrib/scalar/README.md (new)
      @@
      +# Scalar - an opinionated repository management tool
      +
     -+Scalar is an add-on to Git, helping Git scale to very large repositories and
     -+worktrees. Originally implemented in C# using .NET Core, based on the learnings
     -+from the VFS for Git project, most of the techniques developed by the Scalar
     -+project have been integrated into core Git already:
     ++Scalar is an add-on to Git that helps users take advantage of advanced
     ++performance features in Git. Originally implemented in C# using .NET Core,
     ++based on the learnings from the VFS for Git project, most of the techniques
     ++developed by the Scalar project have been integrated into core Git already:
      +
      +* partial clone,
      +* commit graphs,
     @@ contrib/scalar/README.md (new)
      +- `scalar-diagnose`: The `scalar` command is taught the `diagnose` subcommand.
      +
      +- `scalar-and-builtin-fsmonitor`: The built-in FSMonitor is enabled in `scalar
     -+  init` and in `scalar clone`, for an enormous performance boost when working
     -+  in large worktrees. This patch series necessarily depends on Jeff Hostetler's
     -+  FSMonitor patch series to be integrated into Git.
     ++  register` and in `scalar clone`, for an enormous performance boost when
     ++  working in large worktrees. This patch series necessarily depends on Jeff
     ++  Hostetler's FSMonitor patch series to be integrated into Git.
      +
      +- `scalar-gentler-config-locking`: Scalar enlistments are registered in the
      +  user's Git config. This usually does not represent any problem because it is
     @@ contrib/scalar/README.md (new)
      +- `move-scalar-to-toplevel`: Now that Scalar is complete, let's move it next to
      +  `gitk-git/` and to `git-gui/`, making it a top-level command.
      +
     -+The following two patch series exist, but there is no plan to integrate them
     -+into Git's source tree:
     ++The following two patch series exist in Microsoft's fork of Git and are
     ++publicly available. There is no current plan to upstream them, not because I
     ++want to withhold these patches, but because I don't think the Git community is
     ++interested in these patches.
     ++
     ++There are some interesting ideas there, but the implementation is too specific
     ++to Azure Repos and/or VFS for Git to be of much help in general (and also: my
     ++colleagues tried to upstream some patches already and the enthusiasm for
     ++integrating things related to Azure Repos and VFS for Git can be summarized in
     ++very, very few words).
     ++
     ++These still exist mainly because the GVFS protocol is what Azure Repos has
     ++instead of partial clone, while Git is focused on improving partial clone:
      +
      +- `scalar-with-gvfs`: The primary purpose of this patch series is to support
      +  existing Scalar users whose repositories are hosted in Azure Repos (which
  2:  e0693cc713c =  2:  50160d61a41 scalar: create a rudimentary executable
  3:  d80627615f8 =  3:  74cd6410931 scalar: start documenting the command
  4:  9da1616849e =  4:  37231a4dd07 scalar: create test infrastructure
  5:  dbaad4753c1 !  5:  a39b9c81214 cmake: optionally build `scalar`, too
     @@ Metadata
       ## Commit message ##
          cmake: optionally build `scalar`, too
      
     -    The CMake configuration unfortunately does not let us encapsulate all
     -    (or at least the vast majority) of Scalar's build definition in the
     -    `contrib/scalar/` subdirectory.
     +    The CMake configuration unfortunately does not let us easily encapsulate
     +    Scalar's build definition in the `contrib/scalar/` subdirectory: The
     +    `scalar` executable needs to link in `libgit.a` and `common-main.o`, for
     +    example.
     +
     +    Also, `scalar.c` includes Git's header files, which means that
     +    `scalar.c` needs to be compiled with the very same flags as `libgit.a`
     +    lest `scalar.o` and `libgit.a` have different ideas of, say,
     +    `platform_SHA_CTX`, which would naturally lead to memory corruption.
      
          To alleviate that somewhat, we guard the inclusion of Scalar via the
          `INCLUDE_SCALAR` environment variable.
  6:  1b0328fa236 =  6:  8e3542e43f7 ci: also run the `scalar` tests
  7:  cca604ef326 =  7:  385abdb8d8e scalar: 'register' sets recommended config and starts maintenance
  8:  9fea89cd161 =  8:  64c6a75353e scalar: 'unregister' stops background maintenance
  9:  5e077bf892b =  9:  f7fc1958b9e scalar: let 'unregister' handle a deleted enlistment directory gracefully
 10:  dfa0c470989 = 10:  fd2680bc945 scalar: implement 'scalar list'
 11:  febefe39886 = 11:  4966a43aad9 scalar: implement the `clone` subcommand
 12:  2677bcff335 = 12:  b00d68b37b0 scalar: teach 'clone' to support the --single-branch option
 13:  99affb84284 = 13:  771d826bbb1 scalar: implement the `run` command
 14:  69e2242240b = 14:  a8b2d26a830 scalar: allow reconfiguring an existing enlistment
 15:  0068c18aa62 = 15:  ca284ff34a2 scalar: teach 'reconfigure' to optionally handle all registered enlistments
 16:  d5218523a38 = 16:  9983eb8912c scalar: implement the `delete` command
 17:  96a803416b5 = 17:  889f613ab18 scalar: implement the `version` command

Comments

Elijah Newren Nov. 20, 2021, 5:22 p.m. UTC | #1
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?

Other than that, this round looks good to me.  (I have no opinion on
the build system integration, other than that I like it being optional
and not installed by default.)
Johannes Schindelin Nov. 22, 2021, 12:21 p.m. UTC | #2
Hi Elijah,

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.

> Other than that, this round looks good to me.  (I have no opinion on
> the build system integration, other than that I like it being optional
> and not installed by default.)

Yes, I very much wanted to keep this optional and as well-encapsulated as
possible for the moment. (Hence the way it integrates with Git's build
process.)

Thank you for chiming in!

Ciao,
Dscho
Ævar Arnfjörð Bjarmason Nov. 22, 2021, 4:36 p.m. UTC | #3
On Mon, Nov 22 2021, Johannes Schindelin wrote:

> Hi Elijah,
>
> 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.

I.e. "seen" being broken now due to a merger of this topic and another
topic of mine, which as noted in [1] is really just revealing an
existing breakage in this topic, which I sent you an unresponded-to
patch to fix almost a month ago.

>> Other than that, this round looks good to me.  (I have no opinion on
>> the build system integration, other than that I like it being optional
>> and not installed by default.)
>
> Yes, I very much wanted to keep this optional and as well-encapsulated as
> possible for the moment. (Hence the way it integrates with Git's build
> process.)
>
> Thank you for chiming in!

Whatever disagreement we have about the particulars of how scalar lands
in-tree is one thing, and I'd be the first to admit that some of the
stuff I've been suggesting is just my opinion.

But I've also been pointing out in reviews of this series (all/most of
which you've ignored) that there's specific things that are
categorically broken in this series, and clearly not working as you
intend them to work. And you're simply ignoring those reports.

One of those is that your topic here changes CI in in a way that you
clearly didn't intend, and which is an emergent unintended effect of how
you're approaching this scalar integration.

I.e. the compile-only CI targets now doing tests as a result, which is
broken, and the combination of that and my CI topic revealed that
breakage.

Anyway, as noted in [2] I was hoping to leave this whole scalar thing
behind, since I got tired of those reports/suggestions being
ignored. I'm only replying here again because it's clear that you don't
understand some of the things this scalar topic breaks/changes, and the
only reason you wouldn't understand that is because you've been ignoring
specific reports/patches-on top that address those breakages.

1. https://lore.kernel.org/git/211122.86ee78yxts.gmgdl@evledraar.gmail.com/
2. https://lore.kernel.org/git/211110.86czn8hyis.gmgdl@evledraar.gmail.com/
Johannes Schindelin Nov. 22, 2021, 10:08 p.m. UTC | #4
Hi Ævar,

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.

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

Ciao,
Johannes
Ævar Arnfjörð Bjarmason Nov. 22, 2021, 11:29 p.m. UTC | #5
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.
Johannes Schindelin Nov. 23, 2021, 11:52 a.m. UTC | #6
Hi Ævar,

On Tue, 23 Nov 2021, Ævar Arnfjörð Bjarmason wrote:

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

That's cute. You seem to have missed that this is `contrib/`? The
assumption of pretty much _everything_ in there is that Git was already
built.

Try this at home: `make clean && make -C contrib/subtree/ test`

Yep. It "fails" in the same way. "has Git been built?".

So if that was all the evidence in favor of that misinformation "Scalar's
build is broken! Broken, broken, BROKEN!", I think we can now let it rest.
At last.

Ciao,
Johannes
Ævar Arnfjörð Bjarmason Nov. 23, 2021, 12:45 p.m. UTC | #7
On Tue, Nov 23 2021, Johannes Schindelin wrote:

> Hi Ævar,
>
> On Tue, 23 Nov 2021, Ævar Arnfjörð Bjarmason wrote:
>
>> [...]
>>     $ 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
>
> That's cute. You seem to have missed that this is `contrib/`? The
> assumption of pretty much _everything_ in there is that Git was already
> built.
>
> Try this at home: `make clean && make -C contrib/subtree/ test`
>
> Yep. It "fails" in the same way. "has Git been built?".
>
> So if that was all the evidence in favor of that misinformation "Scalar's
> build is broken! Broken, broken, BROKEN!", I think we can now let it rest.
> At last.

No, it doesn't fail in the same way. Really, it seems like you're either
not fully reading through E-Mails before replying, or entirely
misunderstanding what I'm saying. I highlighted the difference in
"[...]that's not broken because[...]" below the context you're quoting.

I'm specifically pointing out the difference between how these act:

    make clean; make -C contrib/subtree/ test
    make clean; make -C t

Which fail right away without trying to build anything, and how your:

    make clean; make -C contrib/scalar test

Won't fail right away, but get most of the way towards building what it
needs at the top-level.

So yes, I fully agree with your contrib/subtree example, but it's making
my argument for me.

As I pointed out in the just-sent [1] the main issue is that your latest
iteration added "make -C contrib/subtree/ test" in the wrong place, and
we thus ended up running those tests in places we didn't intend. The
main conflict is that semantic conflict.

But that failure also highlighted that contrib/scalar/Makefile will run
a build of the top-level C code, only to error out with
"GIT-BUILD-OPTIONS".

That specifically is what I consider broken. It should either actually
work and properly build its dependency, or not even try.

Either one would be fine in this context, it's the in-between that's
clearly (to me at least) broken. Working software should either attempt
a task and succeed, or not attempt the task at all.

1. https://lore.kernel.org/git/211123.86ilwjujmd.gmgdl@evledraar.gmail.com/
Johannes Schindelin Nov. 23, 2021, 1:05 p.m. UTC | #8
Hi Ævar,

On Tue, 23 Nov 2021, Ævar Arnfjörð Bjarmason wrote:

> On Tue, Nov 23 2021, Johannes Schindelin wrote:
>
> > Hi Ævar,
> >
> > On Tue, 23 Nov 2021, Ævar Arnfjörð Bjarmason wrote:
> >
> >> [...]
> >>     $ 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
> >
> > That's cute. You seem to have missed that this is `contrib/`? The
> > assumption of pretty much _everything_ in there is that Git was already
> > built.
> >
> > Try this at home: `make clean && make -C contrib/subtree/ test`
> >
> > Yep. It "fails" in the same way. "has Git been built?".
> >
> > So if that was all the evidence in favor of that misinformation "Scalar's
> > build is broken! Broken, broken, BROKEN!", I think we can now let it rest.
> > At last.
>
> No, it doesn't fail in the same way. Really, it seems like you're either
> not fully reading through E-Mails before replying, or entirely
> misunderstanding what I'm saying.

That's really... fresh.

I told you half a dozen times that at this point, the build works well
enough, and that what you keep insisting is a problem simply isn't.

Yes, you have to build Git before building Scalar.

Have you actually looked at the design of Scalar? What it does to allow
working with large repositories?

_That_ is what counts.

That you are told to please build Git before running Scalar's tests is
maybe a minor annoyance, but not worth the dozens of mails and the weeks
of delay that you caused over this.

Ciao,
Johannes