[Discussion] The architecture of Scalar (and others) within Git

The discussion happening on "Upstreaming the Scalar command" [1] has gotten pretty deep into some details about builds and code organization. While these are important things to discuss, I’d like to step back and reframe the discussion to focus on how components like Scalar fit into the high-level architecture of Git. The decisions we make for Scalar also affect other components that might show up in the future. It could also clarify some of the components already in Git, both in and out of the `contrib/` directory.

[1] https://lore.kernel.org/git/pull.1005.git.1630359290.gitgitgadget@gmail.com/

I'm starting this discussion thread to create a new area to consider these high-level concepts. Specifically: how should a new component like Scalar be included in the Git codebase?

Johannes put details in his cover letter about why we put it in `contrib/scalar/`, but I feel like code review proceeded at first as if that was a foregone conclusion. Then, discussion shifted to some alternatives, but the reasons were focused on code size and specific tweaks to the `Makefile`. I think there are other concerns for the entire Git community that need to be included in the calculus. In particular, the decisions we make today will become precedents for similar contributions in the future.

I'll start by setting up some goals and guidelines for the decision process, then get into some concrete options before comparing them based on these guidelines. I also try to separate the "final" goal from "how do we get there" in the last section.

All of what I say is up for debate. I would love for others to contribute options that I missed. I will send a reply to this message that summarizes all options that are recommended in this thread, along with some attempt to summarize any consensus that we might be approaching.

Goals -----

Our main goal is to identify a community-supported plan for upstreaming the Scalar CLI.

This comes in two parts:

1. What is the preferred final organization of the Scalar code within
   the Git codebase?

2. What is a realistic approach to contributing the complete Scalar CLI
   with incrementally-reviewable pieces, in order to end up with the
   preferred final organization?

Optimization Factors --------------------

There are multiple factors involved that we wish to optimize. Here is my understanding of the most-important factors and how they interact:

### Value

The main reason to include Scalar within the Git codebase is to make it available to more users. This should have downstream effects based on user feedback about what they like or dislike about the Scalar CLI that can inform future features in core Git that simplify repository management at scale.

### CLI compatibility

The Scalar CLI was developed externally, which informed the choice of commands and arguments. We prefer to maintain the existing CLI design to support our existing customers without disruption.

After the existing CLI is completely contributed, we can consider making changes to the CLI by deprecating certain options and replacing them with synonyms. (For example, my personal regret is that `--full-clone` really means `--no-sparse`.)

### Developer friction

Once the Scalar CLI is in the Git codebase, it becomes something that can disrupt Git developers as they modify Git code, especially within libgit.a. What is the right balance of making developers responsible for breaking the Scalar CLI as they make changes to Git?

### Git Maintainer burden

Similar to developer friction, how much does having Scalar in the Git codebase affect the Git maintainer? Does the Git maintainer become responsible for the maintenance of Scalar? Do contributions to Scalar need the same scrutiny as other contributions? Does the maintainer need to inform contributors when they break Scalar?

### Scalar Maintainer(s) responsibility

At the very last, what are the roles of "the Scalar maintainer(s)"? Here, I'm thinking of Johannes and myself as the accountable parties. An example of a responsibility here could be that around release time we validate that Scalar is still working with the changes that happened during that release cycle.

The biggest reason to include the Scalar maintainers' responsibility is that the options listed below present a converse relationship between these responsibilities and those of contributors and the Git maintainer. The more responsibility that the Git community is willing to accept, the less responsibility on the Scalar maintainers. We are _not_ optimizing to minimize our responsibility, but some options change the balance of responsibility.

Options for preferred end state -------------------------------

Let's get into some concrete proposals for the location of the Scalar CLI within the Git codebase. These are ordered based on increasing responsibility on the Git community: the first option should minimize community responsibility and maximize responsibility on the Scalar maintainers. We will discuss the pros and cons of each option after fully describing each of them.

(Please add any options that I may have missed!)

**Option 1:** One-directional coupling in `contrib/scalar/`

The Scalar CLI exists entirely within `contrib/scalar/`, including all code and test infrastructure. To opt-in to building and testing Scalar, run `make -C contrib/scalar test` to build Git and Scalar and run the Scalar test scripts.

**Option 2:** Loose coupling in a new scalar/ directory

Similar to `git-gui` and `gitk-git`, Scalar could exist outside of `contrib/` and in its own `scalar/` directory. Its code is isolated to this directory, but its tests live in `t/` and its documentation lives in `Documentation/`. The root `Makefile` has special logic to avoid building and testing Scalar unless an `INCLUDE_SCALAR` option is enabled. The `DEVELOPER=1` option enables `INCLUDE_SCALAR` by default. Code in `git.c` is generalized so `scalar.c` could take advantage of features such as `-c`, `-C`, and `--exec-path` options.

**Option 3:** Tight coupling with entire Git project

Scalar is included at the root as `scalar.c` and is compiled at the same priority as the `git` executable. Tests and documentation have the same priority as other Git features. Code in `git.c` is generalized so `scalar.c` could take advantage of features such as `-c`, `-C`, and `--exec-path` options.

Note: there are likely more subtle gradients between these options, but
Option 1 and Option 3 are intended as extreme ends of a spectrum. Option
2 is a potential middle ground, but a lot of the details about it could be
altered.

Benefits and drawbacks of these options ---------------------------------------

Here are some pros and cons for each approach. Please reply with additional items to add to the list, or to debate the correctness of each item. I'll try to summarize these contributions in a reply to this message after the discussion stabilizes.

**Option 1:** One-directional coupling in `contrib/scalar/`

This option was our initial choice because it minimizes the responsibility of the Git community. While `contrib/scalar/scalar.c` depends on code in `libgit.a`, the implementation does not require that code to change to accommodate the needs of Scalar. The test suite and documentation is separate.

This does mean that changes to `libgit.a` could break Scalar without any feedback to the developer that has not compiled Scalar. The Scalar maintainers would then need to watch for this and send separate updates to Scalar that fix these dependency breaks. This reduces developer friction, but might cause some extra burden on the Git maintainer. If these "catch up to dependency breaks" updates happen only after a release candidate is out, then maybe this isn't too much of a burden. We don't typically have release candidates for minor releases, so there is some risk there that minor releases could include breaks.

If we make our CI builds include Scalar by default, then the previous paragraph about developer friction is negated.

Having Scalar in `contrib/` makes it easy to differentiate it as an optional component that distributors could choose to include or leave out.

Code in `contrib/` has a lower barrier to entry. In particular, the Scalar CLI is not intended to be up for debate for historical reasons. If we make an exception for Scalar but want Scalar integrated outside of `contrib/`, then are we setting expectations for other tools that might want to be included?

However, projects within `contrib/` do not currently depend on `libgit.a` the way Scalar does. (This is not historically true, as an older project ad such dependencies, but has since been ejected.) This leads to questions of whether or not this is a desirable pattern to follow. It adds to complexity since changes to the core Git codebase can break something in `contrib/` at compile time.

By keeping the code dependency one-directional, some nice-to-have features can be duplicated within `contrib/scalar/` to avoid disrupting their implementation within the core Git codebase.

This option explicitly mentions that all knowledge of how to build Scalar lives within `contrib/scalar/` to avoid disrupting the core `Makefile`. This has already led to debate about some duplication in the Scalar `Makefile` and the one at root. Some functionality might not be critical to include in both places. One knob that we can use to adjust this option is to be more flexible with the root `Makefile` knowing about code within `contrib/scalar`. This violates existing patterns except for how the `make coccicheck` target consumes scripts from `contrib/coccinelle` to do checks on the Git source code.

**Option 2:** Loose coupling in a `scalar/` directory

The first issue with this option is that the Scalar CLI is established and is not up for modification, yet we would be contributing it in a location that is typically under high scrutiny for things like this.

This option also breaks into new territory, because even `git-gui` and `gitk-git` are not based on C and do not depend on `libgit.a`.

There is some risk that as we refactor some things in the Git build system and codebase to allow Scalar to plug in more tightly that we accidentally break something.

Developer friction increases as changes to `libgit.a` that break Scalar should be fixed within the patches that cause those changes. This means that changes to the Scalar project can happen in otherwise-unrelated topics, increasing the attention of both Git and Scalar maintainers during the full release cycle. However, it does reduce risk that Scalar could be broken for a significant chunk of time.

This approach reduces code duplication.

Not only does the code need to be compiled optionally, we need to make sure that tests only run if Scalar was compiled. We also want to make sure that documentation is compiled with the same options. This could affect things like git-scm.com which hosts the Git documentation: should it include the Scalar docs if it isn't shipped by default? How can we make sure that these docs don't accidentally appear there? (Or, should we make sure the docs clearly state that `scalar` might not be available?)

**Option 3:** Tight coupling with the entire Git project

This option removes the ability to opt-out of building the Scalar CLI. Distributors might need to react to remove the `scalar` executable if they do not want to include it.

This option relies on significant buy-in from the Git community.

This option minimizes code duplication and has the simplest build system.

This option sets a risky precedent that new tools can be added to Git without a rigorous review of the CLI.

Possible approaches to land in our final target -----------------------------------------------

If we agree on option 1 (`contrib/scalar/`), then this discussion is moot, because there is no strategy other than to contribute into `contrib/` from the start.

If we agree on another option, then we could still continue contributing Scalar into `contrib/`, following the currently-proposed organization. There might be benefits to a gradual approach to reaching that target. Here is a possible multi-stage approach to land in our final approach:

**Phase 1.** Keep code in contrib/ while we contribute the Scalar features.

Since the current patches on the mailing list are not feature complete, it can be beneficial to move forward with the code in `contrib/scalar/` until we reach feature parity. At that point, we could move the source into its final resting place.

This leans into the fact that `contrib/` contains "interesting tools... maybe even experimental ones". While Scalar is not feature complete, it can be isolated as experimental here until it is ready for promotion to non-experimental.

Depending on the final goal, we could drop some of the work that is currently built within the `contrib/scalar/` directory, such as `-c`/`-C` parsing or documentation builds. These features would be reimplemented in the new location, so we can prevent that duplicate effort if we have a different final location in mind.

**Phase 2.** Establish community standards on optional components

While the work in `contrib/scalar/` continues to reach feature parity, we can work as a community to set some ground rules about these kinds of optional features (depending on how "optional" we land). This can include standards such as how the files are laid out in the repository and how they interact with the `Makefile`. Whatever we do for Scalar is likely to form an example for a future contribution that we can't predict today. Many of the questions we are asking today can be written for posterity:

* What is an appropriate level of coupling with the Git codebase?
* Where should code custom to the component live?
* How should documentation and tests be organized?
* How do we initialize builds of a component?
* Who is responsible for supporting the component? Who fixes the bugs?
* Who is responsible for reviewing changes to the component?

**Phase 3.** Translate the Scalar code from `contrib/scalar/` to new home

After feature parity is reached _and_ we have established some ground rules for how these kinds of components should fit within the Git codebase, we can start to translate Scalar into its new home. This should include some file renames along with some modifications to fit into the build. The goal of this phase is to establish the new build environment and how Scalar fits within the CI builds. Once this is complete, whatever responsibility was agreed upon by the community to keep Scalar working goes into effect.

**Phase 4.** Integrate features that were delayed until final home

After the code, docs, and tests have been moved, we can start the work of adding the features that might have been delayed until the final home. The main example on my mind is the `-c`/`-C`/`--exec-path` parsing, which should be its own series. Features that fit into this category include anything that requires modifying existing code outside of `contrib/` to be consumed by Scalar.

Call to action --------------

Thanks for your attention to this topic. I look forward to any new "big" ideas in this space.

Thanks, -Stolee

Hi Stolee,

On Wed, 27 Oct 2021, Derrick Stolee wrote:

Thank you so much for summarizing all of that!

I have voiced my opinion before, so I won't bore you with a rerun of that show. Now, I am eager to sit back and listen to other informed opinions, opinions I have not heard yet. In particular Junio's... :-)

Ciao, Dscho

Hi Stollee, a few very generic points with my "systems" hat on.

On 27/10/2021 22:51, Derrick Stolee wrote:

We probably should also summarise the status of and relationships with the existing 'integrations' like `git-gui`, `gitk`, `Git for-Windows`, etc.

I'm assuming that 'our' here is the corporate team that has been endeavouring to use Git effectively on it's mega-repo and give back what it has learnt to Git.

Here, 'scalar' is about large code mono-repos, rather than any operating system/vendor issues, or visualisation tools. (may need saying)

How to quantify the size of the scalar _code_ contribution, and how is concept ownership handed over?

Ensuring a separation of concerns (partitioning) will be important if the only mega repo that can really 'battle test' the capability is to be separate from the ownership of the coded concepts.

I'm presuming this (responsibility) is with respect to the *transition* from the independent `scalar` to the Git integrated _scalar_ capability.

This appears to list only end points, rather than transitional approaches (cf. cut & cover, tunnel, bridging) to providing some or all of the scalar mono-repo capability to Git.

Are we talking ('not for debate') about the existing `scalar` (corporate) cli or the potential for a medium term transition to a git cli embedding the _scalar_ concepts. 

I'm minded toward seeing a transitional approach (aka: staged, gated, ..) that would bring capability into Git, but I'm not sure about the libgit.a linkage.

regards Philip

Change how the newly proposed "scalar" command is handle, this builds on top of the proposed patches that add it along with documentation and tests to the "contrib/scalar/" directory[1].

With those patches we unconditionally build scalar.o and link it to libgit.a, but don't build the "scalar" binary itself, and don't run its tests as part of our normal test suite run or CI.

As the changes to "t/t9099-scalar.sh" here shows that sort of arrangement leads to breakages. I.e. the scalar tests would fail on OSX, which our OSX CI jobs will catch, but only if we run the tests. Let's add a prerequisite to test that, which requires moving around some of the setup code.

I think that for a libgit.a-using "scalar" in particular, and for "contrib" in general we're better off by not only compiling the object in question, but also linking it, and running its tests by default. What we won't do is install it by default, unless an "INSTALL_SCALAR=Y" is provided along with "make install".

For context: There's been ongoing discussion about how this command should be integrated in terms of the "closeness" of its integration, and how "contrib" sources within git.git should be organized.

That discussion started with my [2], was followed by a WIP patch[3] to implement the approach being finished up here, and has spawned the "[Discussion] The architecture of Scalar (and others) within Git" thread[4]. See also the large earlier discussion hanging off [3].

I really don't think it's important whether a given source file lives in-tree at "contrib/scalar/scalar.c" or the top-level "scalar.c", but as the diff below shows the latter approach is a smaller overall change in our build system, due to how it's organized, more details on that below under "Build changes".

I think that part of the reason for sticking the new command in "contrib/scalar/" was to implicitly make it clear from the paths that it was different. I do think that would be a worthwhile goal in the abstract.

But given the build simplifications we can attain by moving it to the top level that we should seek to resolve that ambiguity in the minds of any potential git.git code maintainers in some other way.

To do that we now have a new 'OPTIONAL CONTRIB COMMANDS' section in "git help git" explaining our backwards and forwards compatibility non-promises when it comes to this and similar future commands. Our users are the ones most likely to be confused about why their git package has installed a "/usr/bin/scalar" that seemingly duplicates parts of git's own functionality. We're much better off by cross-linking our documentation, and mentioning the state of scalar in git's own documentation, along with the full list of porcelain and plumbing utilities.

= Build changes =

This fixes dependency bugs in the previous "contrib/scalar/Makefile", as well as implementing various missing bits of functionality, most notably "make install" will now install the "scalar" command, but only if INSTALL_SCALAR is defined.

Those and other changes and non-changes, categorized as "Same", "New" and "Fixed" below:

== Same ==

A.: We'll unconditionally build scalar.o, as before it's in the
    $(OBJECTS) list.

B.: "make contrib/scalar/scalar.o" has the same dependency graph as
    before, but is now spelled "make scalar.o", more on the rename below.

== New ==

C.: We'll now unconditionally build and test the scalar command. Before we'd only build scalar.o, but not link it.

    Its test lives in "t/t9099-scalar.sh" now (and take <1s to
    run). We should have test coverage of this in-tree command that's
    linking to libgit.a.

    Previously it had to be manually tested by cd-ing to
    contrib/scalar and running "make test", and it would not benefit
    from the combination of our various CI targets.

D.: We'll unconditionally build scalar's documentation, and it will be
    linted along with the rest, including checking for broken links to
    other documentation.

    The minor change to scalar.txt here is needed due to the lint
    check added in cafd9828e89 (doc lint: lint and fix missing "GIT"
    end sections, 2021-04-09), perhaps we should have a different end
    blurb for this command, but for now let's make it consistent with
    the rest.

    When installed (see below) this command is part of the git suite,
    so the end blurb should say something to that effect.

E.: "make install" will now install the "scalar" binary, but only if
    "INSTALL_SCALAR" is defined.

    Relative to our $(prefix) we'll now have (this is with
    INSTALL_SYMLINKS=Y):

        libexec/git-core/scalar: symbolic link to ../../bin/scalar
        bin/scalar: ELF 64-bit LSB pie executable,

    Putting it into libexec isn't strictly necessary, but as we do it
    with "git" we do that by default, and this will ensure that anyone
    who relies on their path being "$(git --exec-path)" will also be
    able to find "scalar".

    Perhaps we shouldn't put it in "libexec" at all, but for now let's
    just follow the herd.

F.: Likewise "make install-man install-doc install-html" works, and
    will install "scalar" documentation if "INSTALL_SCALAR" is
    defined.

    We'll install these files with those targets (and correctly split
    them by target, e.g. only scalar.1 with "install-man"):

        share/doc/git-doc/scalar.txt: ASCII text
        share/doc/git-doc/scalar.html: XML 1.0 document, ASCII text, with CRLF line terminators
        share/man/man1/scalar.1: troff or preprocessor input, ASCII text, with very long lines

== Fixed ==

G.: The dependency graph of contrib/scalar/Makefile was broken when it
    came to scalar.o, it only depended on ../../libgit.a. So it was a
    requirement to run the top-level Makefile first to get around some
    of its dependency issues:

        make && make -C contrib/scalar

H.: By having the top-level Makefile build scalar.o it'll work as well
    as any other git code, including benefiting from
    COMPUTE_HEADER_DEPENDENCIES.

    Targets such as (and possibly others):

        - "make tags TAGS cscope"
        - "make coccicheck"
	- "make check-docs"

    Wouldn't consider contrib/scalar at all, some of those would have
    been fixable, but as shown in the rest of this patch it's easier
    just to have scalar built like any other program instead.

== Discussion and motivation ==

I've been fixing various dependency issues in git's Makefile infrastructure recently, some of which has been integrated into "master", some of which is currently under review on-list, and some of which isn't submitted yet.

To the extent that we have build dependency issues and cases where we can't build something in parallel it's usually because we're invoking one Makefile from another. That's the case in most of our sub-Makefiles, which will usually need to invoke or depend on something created in the top-level Makefile.

None of {t,Documentation,template}/Makefile etc. are truly independent, as looking at $(git grep -F '../' '*Makefile') will reveal, and we'll sometimes need to duplicate logic between them purely to get around them not being driven by one top-level Makefile logic.

Much has been written on this topic, likely most notably the "Recursive Make Considered Harmful" paper (full PDF link at [5]), and to be fair, the there's also a well-known "Non-recursive Make Considered Harmful" retort at [6]).

Theoretical opinions on the topic clearly differ. From a purely practical perspective I ran into textual and semantic conflicts with the "contrib/scalar/" work.

I think the diffstat of this patch demonstrates that this is a much simpler approach, particularly considering that the code being replaced didn't perform much of the needed integrations that are "new" here. E.g. we now have "make install". The unsubmitted [7] (linked to by [8]) shows the boilerplate we'd need for that with the alternate approach.

It would be more pleasing to not have to hardcode "git" in the pre-image and now "git" and "scalar" in the post-image in several places. My initial WIP [3] looks better, but built on a set of Makefile reorganization patches.

I'd like to not make those patches a dependency of this change, but readers should rest assured that we're really not ending up with as many "scalar" special-cases in the long term as this diff indicates, most or all of those can all be generalized into Makefile refactoring that teaches our build system to build N number of differently named top-level commands.

1. https://lore.kernel.org/git/pull.1005.v6.git.1635323239.gitgitgadget@gmail.com/
2. https://lore.kernel.org/git/87czpuxbwp.fsf@evledraar.gmail.com/
3. https://lore.kernel.org/git/87ilz44kdk.fsf@evledraar.gmail.com/
4. https://lore.kernel.org/git/b67bbef4-e4c3-b6a7-1c7f-7d405902ef8b@gmail.com/
5. https://web.archive.org/web/20150330111905/http://miller.emu.id.au/pmiller/books/rmch/
6. https://www.microsoft.com/en-us/research/wp-content/uploads/2016/03/hadrian.pdf
7. https://github.com/dscho/git/commit/473ca8ae673
8. https://lore.kernel.org/git/nycvar.QRO.7.76.6.2110210947590.56@tvgsbejvaqbjf.bet/

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---

On Wed, Oct 27 2021, Derrick Stolee wrote:

I figured working code was better than continuing discussion on-list with reference to working code. As noted in the patch itself htis goes on top of Johannes's v6 scalar patches. CI run at : https://github.com/avar/git/actions/runs/1395993870

To the extent that the above commit message & below diff don't serve as a reply I'll reply to your E-Mail seperately.

Thanks a lot for getting the ball rolling on this.

 .gitignore                                   |  1 +
 Documentation/Makefile                       |  4 +
 Documentation/cmd-list.perl                  |  2 +-
 Documentation/git.txt                        | 12 +++
 {contrib/scalar => Documentation}/scalar.txt |  4 +-
 Makefile                                     | 96 ++++++++++++++------
 command-list.txt                             |  2 +
 contrib/scalar/.gitignore                    |  5 -
 contrib/scalar/Makefile                      | 57 ------------
 contrib/scalar/t/Makefile                    | 78 ----------------
 contrib/scalar/scalar.c => scalar.c          |  0
 {contrib/scalar/t => t}/t9099-scalar.sh      | 42 ++++++---
 12 files changed, 115 insertions(+), 188 deletions(-)
 rename {contrib/scalar => Documentation}/scalar.txt (99%)
 delete mode 100644 contrib/scalar/.gitignore
 delete mode 100644 contrib/scalar/Makefile
 delete mode 100644 contrib/scalar/t/Makefile
 rename contrib/scalar/scalar.c => scalar.c (100%)
 rename {contrib/scalar/t => t}/t9099-scalar.sh (72%)

This one-patch series integrates the "scalar" command to the top-level, meaning we build the "scalar" binary by default, and run its tests on "make test" and in CI. We'll also build and test its documentation. We now also have "install" support, both for the program and its docs, but you'll need to:

    make <install-target> INSTALL_SCALAR=Y

I'm sending this out now to avoid needless duplicate work. A recent series attempted to integrate scalar into the CI[1], but it's now been retracted[2]. As I'd discovered when I submitted the v1 of this[3] integrating something like scalar "halfway" is much harder to do (see e.g. the problems with [1] that I pointed out in [4]).

I then saw that Victoria Dye pushed out a WIP topic [5] with a patch that's basically a subset of this one, but doesn't handle various tough edge cases handled here (and this one fully passes CI [6]). namely:

 * This adjusts the the "cmake" build, so VS build passes
 * This integrates with the documentation build, and targets like
   "check-docs"
 * This implements the optional "make install" support (much of which
   is predicated on getting the Documentation/ integration right)
 * This adds "scalar" to the command-list.txt, which has various
   positive downstream effects.

As in the v1 [3] this change proposed to create an "optionalcontrib" category in the command-list.txt. The "scalar" command is put into that category, and in "man git" we say:

	Optional contrib commands
	-------------------------

	The following commands are included with the git sources, but may not
	be present in your installation.

	These should be considered "contrib"-level when it comes to
	maintenance and stability promises. They might not even be included in
	your installation, and may either drastically change in the future, or
	go away entirely.

	include::cmds-optionalcontrib.txt[]

So I think this change allows us to have our cake and eat it too as far as the "contrib status" of a program like "scalar" is concerned. I.e. we're merely changing the internal source organization here to avoid duplicate work during git.git maintenance, but it will be made clear to users that this isn't a normal command, and it's not installed by default.

I think it would make sense as a follow-up to consider moving contrib/{diff-highlight,mw-to-git,subtree} to a similar structure. I.e. they also carry copy/pasted "Makefile" infrastructure in various ways, resulting in buggy integration with the rest (e.g. try to run "make coverage" in a way that includes "git subtree").

If what we want is to e.g. not install those by default, or even not to run their tests or have them in CI by default as this change (and previous discussion on the scalar topic) shows that's much easier than bridging the other gaps.

1. https://lore.kernel.org/git/pull.1129.git.1654160735.gitgitgadget@gmail.com/
2. https://lore.kernel.org/git/nycvar.QRO.7.76.6.2206132246010.353@tvgsbejvaqbjf.bet/
3. https://lore.kernel.org/git/patch-1.1-86fb8d56307-20211028T185016Z-avarab@gmail.com/
4. https://lore.kernel.org/git/220602.86ee07z6qb.gmgdl@evledraar.gmail.com/
5. https://github.com/vdye/git/tree/feature/scalar-toplevel
6. https://github.com/avar/git/actions/runs/2541684143

Ævar Arnfjörð Bjarmason (1):
  scalar: reorganize from contrib/, still keep it "a contrib command"

 .gitignore                                    |  1 +
 Documentation/Makefile                        |  4 +
 Documentation/cmd-list.perl                   |  2 +-
 Documentation/git.txt                         | 12 +++
 {contrib/scalar => Documentation}/scalar.txt  |  4 +-
 Makefile                                      | 57 +++++++++----
 command-list.txt                              |  2 +
 contrib/buildsystems/CMakeLists.txt           | 15 +++-
 contrib/scalar/.gitignore                     |  2 -
 contrib/scalar/Makefile                       | 35 --------
 contrib/scalar/t/Makefile                     | 81 -------------------
 contrib/scalar/scalar.c => scalar.c           |  0
 .../t/t9099-scalar.sh => t/t7990-scalar.sh    |  8 +-
 13 files changed, 79 insertions(+), 144 deletions(-)
 rename {contrib/scalar => Documentation}/scalar.txt (99%)
 delete mode 100644 contrib/scalar/.gitignore
 delete mode 100644 contrib/scalar/Makefile
 delete mode 100644 contrib/scalar/t/Makefile
 rename contrib/scalar/scalar.c => scalar.c (100%)
 rename contrib/scalar/t/t9099-scalar.sh => t/t7990-scalar.sh (96%)

Range-diff against v1:
1:  86fb8d56307 ! 1:  9743e2a1e6a contrib: build & test scalar by default, optionally install
    @@ Metadata
     Author: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
      ## Commit message ##
    -    contrib: build & test scalar by default, optionally install
    -
    -    Change how the newly proposed "scalar" command is handle, this builds
    -    on top of the proposed patches that add it along with documentation
    -    and tests to the "contrib/scalar/" directory[1].
    -
    -    With those patches we unconditionally build scalar.o and link it to
    -    libgit.a, but don't build the "scalar" binary itself, and don't run
    -    its tests as part of our normal test suite run or CI.
    -
    -    As the changes to "t/t9099-scalar.sh" here shows that sort of
    -    arrangement leads to breakages. I.e. the scalar tests would fail on
    -    OSX, which our OSX CI jobs will catch, but only if we run the
    -    tests. Let's add a prerequisite to test that, which requires moving
    -    around some of the setup code.
    -
    -    I think that for a libgit.a-using "scalar" in particular, and for
    -    "contrib" in general we're better off by not only compiling the object
    -    in question, but also linking it, and running its tests by
    -    default. What we won't do is install it by default, unless an
    -    "INSTALL_SCALAR=Y" is provided along with "make install".
    -
    -    For context: There's been ongoing discussion about how this command
    -    should be integrated in terms of the "closeness" of its integration,
    -    and how "contrib" sources within git.git should be organized.
    -
    -    That discussion started with my [2], was followed by a WIP patch[3] to
    -    implement the approach being finished up here, and has spawned the
    +    scalar: reorganize from contrib/, still keep it "a contrib command"
    +
    +    Change the optional scalar command added in 0a43fb22026 (scalar:
    +    create a rudimentary executable, 2021-12-03) so that it builds, tests
    +    and CI's with our usual build and testing process, but is still kept
    +    "a contrib command" in how we talk about it in our documentation.
    +
    +    Before this change we've been building scalar.o in CI as part of the
    +    "static-analysis" job, we'll now build the full binary, and run its
    +    tests. We won't install it unless "INSTALL_SCALAR=Y" is provided to
    +    the Makefile. Before this change there was no "make install" step for
    +    the "scalar" binary at all.
    +
    +    Before this we needed a significant amount of Makefile boilerplate to
    +    do something that the top-level Makefile could do with relatively
    +    little work, and by doing so we'll get the advantage of testing and
    +    CI-ing the scalar command along with our other commands.
    +
    +    The recent [1] shows a (now retracted, see [2]) series to integrate
    +    scalar with CI while it still lived in contrib/*. For context: There's
    +    been past discussion about how this command should be integrated in
    +    terms of the "closeness" of its integration, and how "contrib" sources
    +    within git.git should be organized.
    +
    +    That discussion started with my [3], was followed by a WIP patch[4] to
    +    implement the approach being finished up here, which prompted the
         "[Discussion] The architecture of Scalar (and others) within Git"
    -    thread[4]. See also the large earlier discussion hanging off [3].
    +    thread[4]. See also the large earlier discussion hanging off [4]. An
    +    earlier version of this change was proposed at [6].
     
    -    I really don't think it's important whether a given source file lives
    -    in-tree at "contrib/scalar/scalar.c" or the top-level "scalar.c", but
    -    as the diff below shows the latter approach is a smaller overall
    -    change in our build system, due to how it's organized, more details on
    -    that below under "Build changes".
    -
    -    I think that part of the reason for sticking the new command in
    -    "contrib/scalar/" was to implicitly make it clear from the paths that
    -    it was different. I do think that would be a worthwhile goal in the
    -    abstract.
    +    Part of the reason for sticking the new command in "contrib/scalar/"
    +    was to implicitly make it clear from the paths that it was
    +    different. I do think that would be a worthwhile goal in the abstract.
     
         But given the build simplifications we can attain by moving it to the
         top level that we should seek to resolve that ambiguity in the minds
    @@ Commit message
         git's own documentation, along with the full list of porcelain and
         plumbing utilities.
     
    -    = Build changes =
    -
    -    This fixes dependency bugs in the previous "contrib/scalar/Makefile", as
    -    well as implementing various missing bits of functionality, most
    -    notably "make install" will now install the "scalar" command, but only
    -    if INSTALL_SCALAR is defined.
    -
    -    Those and other changes and non-changes, categorized as "Same", "New"
    -    and "Fixed" below:
    -
    -    == Same ==
    -
    -    A.: We'll unconditionally build scalar.o, as before it's in the
    -        $(OBJECTS) list.
    -
    -    B.: "make contrib/scalar/scalar.o" has the same dependency graph as
    -        before, but is now spelled "make scalar.o", more on the rename below.
    +    Commentary on changes in specific sub-components:
     
    -    == New ==
    -
    -    C.: We'll now unconditionally build and test the scalar command.
    -    Before we'd only build scalar.o, but not link it.
    -
    -        Its test lives in "t/t9099-scalar.sh" now (and take <1s to
    -        run). We should have test coverage of this in-tree command that's
    -        linking to libgit.a.
    +    A.: The tests now live in "t/t7990-scalar.sh" (per the naming guide in
    +        t/README).
     
             Previously it had to be manually tested by cd-ing to
             contrib/scalar and running "make test", and it would not benefit
             from the combination of our various CI targets.
     
    +    B.: The "Documentation/cmd-list.perl" change is needed because that
    +        command wasn't capable of understanding commands in the
    +        command-list.txt whose name didn't start with "git".
    +
    +        If we don't make this change we won't cross-link to "scalar(1)"
    +        from "OPTIONAL CONTRIB COMMANDS" in "git(1)".
    +
    +    C. We'll always mention scalar in "OPTIONAL CONTRIB COMMANDS" in
    +       "git(1)", but the "scalar(1)" manpage itself depends on whether we
    +       build with "INSTALL_SCALAR=Y".
    +
         D.: We'll unconditionally build scalar's documentation, and it will be
             linted along with the rest, including checking for broken links to
             other documentation.
    @@ Commit message
             been fixable, but as shown in the rest of this patch it's easier
             just to have scalar built like any other program instead.
     
    -    == Discussion and motivation ==
    -
    -    I've been fixing various dependency issues in git's Makefile
    -    infrastructure recently, some of which has been integrated into
    -    "master", some of which is currently under review on-list, and some of
    -    which isn't submitted yet.
    -
    -    To the extent that we have build dependency issues and cases where we
    -    can't build something in parallel it's usually because we're invoking
    -    one Makefile from another. That's the case in most of our
    -    sub-Makefiles, which will usually need to invoke or depend on
    -    something created in the top-level Makefile.
    -
    -    None of {t,Documentation,template}/Makefile etc. are truly
    -    independent, as looking at $(git grep -F '../' '*Makefile') will
    -    reveal, and we'll sometimes need to duplicate logic between them
    -    purely to get around them not being driven by one top-level Makefile
    -    logic.
    -
    -    Much has been written on this topic, likely most notably the
    -    "Recursive Make Considered Harmful" paper (full PDF link at [5]), and
    -    to be fair, the there's also a well-known "Non-recursive Make
    -    Considered Harmful" retort at [6]).
    -
    -    Theoretical opinions on the topic clearly differ. From a purely
    -    practical perspective I ran into textual and semantic conflicts with
    -    the "contrib/scalar/" work.
    -
    -    I think the diffstat of this patch demonstrates that this is a much
    -    simpler approach, particularly considering that the code being
    -    replaced didn't perform much of the needed integrations that are "new"
    -    here. E.g. we now have "make install". The unsubmitted [7] (linked to
    -    by [8]) shows the boilerplate we'd need for that with the alternate
    -    approach.
    -
    -    It would be more pleasing to not have to hardcode "git" in the
    -    pre-image and now "git" and "scalar" in the post-image in several
    -    places. My initial WIP [3] looks better, but built on a set of
    -    Makefile reorganization patches.
    -
    -    I'd like to not make those patches a dependency of this change, but
    -    readers should rest assured that we're really not ending up with as
    -    many "scalar" special-cases in the long term as this diff indicates,
    -    most or all of those can all be generalized into Makefile refactoring
    -    that teaches our build system to build N number of differently named
    -    top-level commands.
    -
    -    1. https://lore.kernel.org/git/pull.1005.v6.git.1635323239.gitgitgadget@gmail.com/
    -    2. https://lore.kernel.org/git/87czpuxbwp.fsf@evledraar.gmail.com/
    -    3. https://lore.kernel.org/git/87ilz44kdk.fsf@evledraar.gmail.com/
    -    4. https://lore.kernel.org/git/b67bbef4-e4c3-b6a7-1c7f-7d405902ef8b@gmail.com/
    -    5. https://web.archive.org/web/20150330111905/http://miller.emu.id.au/pmiller/books/rmch/
    -    6. https://www.microsoft.com/en-us/research/wp-content/uploads/2016/03/hadrian.pdf
    -    7. https://github.com/dscho/git/commit/473ca8ae673
    -    8. https://lore.kernel.org/git/nycvar.QRO.7.76.6.2110210947590.56@tvgsbejvaqbjf.bet/
    +    1. https://lore.kernel.org/git/220602.86ee07z6qb.gmgdl@evledraar.gmail.com/
    +    2. https://lore.kernel.org/git/nycvar.QRO.7.76.6.2206132246010.353@tvgsbejvaqbjf.bet/
    +    3. https://lore.kernel.org/git/87czpuxbwp.fsf@evledraar.gmail.com/
    +    4. https://lore.kernel.org/git/87ilz44kdk.fsf@evledraar.gmail.com/
    +    5. https://lore.kernel.org/git/b67bbef4-e4c3-b6a7-1c7f-7d405902ef8b@gmail.com/
    +    6: https://lore.kernel.org/git/patch-1.1-86fb8d56307-20211028T185016Z-avarab@gmail.com/
     
         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
    @@ Documentation/scalar.txt: SEE ALSO
     +Part of the linkgit:git[1] suite
     
      ## Makefile ##
    -@@ Makefile: all::
    +@@ Makefile: include shared.mak
      # INSTALL_STRIP can be set to "-s" to strip binaries during installation,
      # if your $(INSTALL) command supports the option.
      #
    @@ Makefile: GIT_OBJS =
     +OTHER_PROGRAMS =
      PROGRAMS =
      EXCLUDED_PROGRAMS =
    -+SCALAR_OBJS =
      SCRIPT_PERL =
    - SCRIPT_PYTHON =
    - SCRIPT_SH =
     @@ Makefile: BUILT_INS += git-switch$X
      BUILT_INS += git-whatchanged$X
      
    @@ Makefile: BINDIR_PROGRAMS_NEED_X += git-shell
      BINDIR_PROGRAMS_NO_X += git-cvsserver
      
      # Set paths to tools early so that they can be used for version tests.
    -@@ Makefile: git.sp git.s git.o: EXTRA_CPPFLAGS = \
    - 	'-DGIT_MAN_PATH="$(mandir_relative_SQ)"' \
    - 	'-DGIT_INFO_PATH="$(infodir_relative_SQ)"'
    - 
    --git$X: git.o GIT-LDFLAGS $(BUILTIN_OBJS) $(GITLIBS)
    --	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) \
    --		$(filter %.o,$^) $(LIBS)
    -+define top-level-cmd-rule
    -+$(1)$X: $(1).o GIT-LDFLAGS $$(GITLIBS) $(2)
    -+	$$(QUIET_LINK)$$(CC) $$(ALL_CFLAGS) -o $$@ $$(ALL_LDFLAGS) \
    -+		$$(filter %.o,$$^) $(LIBS)
    -+endef
    -+$(eval $(call top-level-cmd-rule,git,$(BUILTIN_OBJS)))
    -+$(eval $(call top-level-cmd-rule,scalar,))
    - 
    - help.sp help.s help.o: command-list.h
    - hook.sp hook.s hook.o: hook-list.h
    -@@ Makefile: GIT_OBJS += git.o
    - .PHONY: git-objs
    - git-objs: $(GIT_OBJS)
    - 
    -+SCALAR_OBJS += scalar.o
    -+.PHONY: scalar-objs
    -+scalar-objs: $(SCALAR_OBJS)
    -+
    - OBJECTS += $(GIT_OBJS)
    -+OBJECTS += $(SCALAR_OBJS)
    - OBJECTS += $(PROGRAM_OBJS)
    - OBJECTS += $(TEST_OBJS)
    - OBJECTS += $(XDIFF_OBJS)
     @@ Makefile: ifndef NO_CURL
      	OBJECTS += http.o http-walker.o remote-curl.o
      endif
      
     -SCALAR_SOURCES := contrib/scalar/scalar.c
    --SCALAR_OBJECTS := $(SCALAR_SOURCES:c=o)
    --OBJECTS += $(SCALAR_OBJECTS)
    --
    - .PHONY: objects
    - objects: $(OBJECTS)
    ++SCALAR_SOURCES := scalar.c
    + SCALAR_OBJECTS := $(SCALAR_SOURCES:c=o)
    + OBJECTS += $(SCALAR_OBJECTS)
      
     @@ Makefile: $(REMOTE_CURL_PRIMARY): remote-curl.o http.o http-walker.o GIT-LDFLAGS $(GITLIBS
      	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) \
      		$(CURL_LIBCURL) $(EXPAT_LIBEXPAT) $(LIBS)
      
     -contrib/scalar/scalar$X: $(SCALAR_OBJECTS) GIT-LDFLAGS $(GITLIBS)
    --	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) \
    --		$(filter %.o,$^) $(LIBS)
    --
    - $(LIB_FILE): $(LIB_OBJS)
    - 	$(QUIET_AR)$(RM) $@ && $(AR) $(ARFLAGS) $@ $^
    ++scalar$X: $(SCALAR_OBJECTS) GIT-LDFLAGS $(GITLIBS)
    + 	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) \
    + 		$(filter %.o,$^) $(LIBS)
      
     @@ Makefile: GIT-PYTHON-VARS: FORCE
                  fi
    @@ Makefile: GIT-PYTHON-VARS: FORCE
     +# bin-wrappers/*
     +GIT_TEST_BINDIR_PROGRAMS_NEED_X = $(filter-out scalar,$(TEST_BINDIR_PROGRAMS_NEED_X))
      bin-wrappers/%: wrap-for-bin.sh
    - 	@mkdir -p bin-wrappers
    + 	$(call mkdir_p_parent_template)
      	$(QUIET_GEN)sed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \
      	     -e 's|@@BUILD_DIR@@|$(shell pwd)|' \
     -	     -e 's|@@PROG@@|$(patsubst test-%,t/helper/test-%$(X),$(@F))$(patsubst git%,$(X),$(filter $(@F),$(BINDIR_PROGRAMS_NEED_X)))|' < $< > $@ && \
    @@ Makefile: endif
      install-gitweb:
      	$(MAKE) -C gitweb install
      
    --install-doc: install-man-perl
    --	$(MAKE) -C Documentation install
    --
    --install-man: install-man-perl
    --	$(MAKE) -C Documentation install-man
    -+ifdef INSTALL_SCALAR
    ++# We must not "export" this as e.g. "check-docs" needs to know about
    ++# scalar.txt. We only exclude scalar.txt for the "install-*" targets.
     +NO_INSTALL_SCALAR_DOC =
    -+else
    ++ifndef INSTALL_SCALAR
     +NO_INSTALL_SCALAR_DOC = NoScalarPlease
     +endif
    ++
    + install-doc: install-man-perl
    +-	$(MAKE) -C Documentation install
    ++	$(MAKE) -C Documentation install NO_INSTALL_SCALAR_DOC='$(NO_INSTALL_SCALAR_DOC)'
    + 
    + install-man: install-man-perl
    +-	$(MAKE) -C Documentation install-man
    ++	$(MAKE) -C Documentation install-man NO_INSTALL_SCALAR_DOC='$(NO_INSTALL_SCALAR_DOC)'
      
      install-man-perl: man-perl
      	$(INSTALL) -d -m 755 '$(DESTDIR_SQ)$(mandir_SQ)/man3'
    - 	(cd perl/build/man/man3 && $(TAR) cf - .) | \
    +@@ Makefile: install-man-perl: man-perl
      	(cd '$(DESTDIR_SQ)$(mandir_SQ)/man3' && umask 022 && $(TAR) xof -)
      
    --install-html:
    + install-html:
     -	$(MAKE) -C Documentation install-html
    --
    --install-info:
    ++	$(MAKE) -C Documentation install-html NO_INSTALL_SCALAR_DOC='$(NO_INSTALL_SCALAR_DOC)'
    + 
    + install-info:
     -	$(MAKE) -C Documentation install-info
    --
    --install-pdf:
    ++	$(MAKE) -C Documentation install-info NO_INSTALL_SCALAR_DOC='$(NO_INSTALL_SCALAR_DOC)'
    + 
    + install-pdf:
     -	$(MAKE) -C Documentation install-pdf
    -+define install-doc-rule
    -+$(1): $(3)
    -+	$$(MAKE) -C Documentation $(2) NO_INSTALL_SCALAR_DOC=$$(NO_INSTALL_SCALAR_DOC)
    -+endef
    -+$(eval $(call install-doc-rule,install-doc,install,install-man-perl))
    -+$(eval $(call install-doc-rule,install-man,install-man,install-man-perl))
    -+$(eval $(call install-doc-rule,install-html,install-html,))
    -+$(eval $(call install-doc-rule,install-info,install-info,))
    -+$(eval $(call install-doc-rule,install-pdf,install-pdf,))
    ++	$(MAKE) -C Documentation install-pdf NO_INSTALL_SCALAR_DOC='$(NO_INSTALL_SCALAR_DOC)'
      
      quick-install-doc:
    - 	$(MAKE) -C Documentation quick-install
    -@@ Makefile: ifneq ($(INCLUDE_DLLS_IN_ARTIFACTS),)
    - OTHER_PROGRAMS += $(shell echo *.dll t/helper/*.dll)
    - endif
    +-	$(MAKE) -C Documentation quick-install
    ++	$(MAKE) -C Documentation quick-install NO_INSTALL_SCALAR_DOC='$(NO_INSTALL_SCALAR_DOC)'
    + 
    + quick-install-man:
    +-	$(MAKE) -C Documentation quick-install-man
    ++	$(MAKE) -C Documentation quick-install-man NO_INSTALL_SCALAR_DOC='$(NO_INSTALL_SCALAR_DOC)'
    + 
    + quick-install-html:
    +-	$(MAKE) -C Documentation quick-install-html
    ++	$(MAKE) -C Documentation quick-install-html NO_INSTALL_SCALAR_DOC='$(NO_INSTALL_SCALAR_DOC)'
    + 
    + 
      
    --artifacts-tar:: $(ALL_COMMANDS_TO_INSTALL) $(SCRIPT_LIB) $(OTHER_PROGRAMS) \
    --		GIT-BUILD-OPTIONS $(TEST_PROGRAMS) $(test_bindir_programs) \
    -+# No scalar in "cmake" yet, and since 4c2c38e800f (ci: modification of
    -+# main.yml to use cmake for vs-build job, 2020-06-26) the "vs-build"
    -+# job has a hard dependency on it. Let's fail in the tests instead of
    -+# in the "vs-build" job itself.
    -+ifeq ($(wildcard scalar),scalar)
    -+ARCHIVE_OTHER_PROGRAMS = $(OTHER_PROGRAMS)
    -+ARCHIVE_TEST_BINDIR_PROGRAMS = $(test_bindir_programs)
    -+else
    -+ARCHIVE_OTHER_PROGRAMS = $(filter-out scalar%, $(OTHER_PROGRAMS))
    -+ARCHIVE_TEST_BINDIR_PROGRAMS = $(filter-out bin-wrappers/scalar,$(test_bindir_programs))
    -+endif
    -+artifacts-tar:: $(ALL_COMMANDS_TO_INSTALL) $(SCRIPT_LIB) $(ARCHIVE_OTHER_PROGRAMS) \
    -+		GIT-BUILD-OPTIONS $(TEST_PROGRAMS) $(ARCHIVE_TEST_BINDIR_PROGRAMS) \
    - 		$(MOFILES)
    - 	$(QUIET_SUBDIR0)templates $(QUIET_SUBDIR1) \
    - 		SHELL_PATH='$(SHELL_PATH_SQ)' PERL_PATH='$(PERL_PATH_SQ)'
     @@ Makefile: check-docs::
      		    -e 's/\.txt//'; \
      	) | while read how cmd; \
    @@ command-list.txt
      #
      # The type names are self explanatory. But if you want to see what
      # command belongs to what group to get a better picture, have a look
    -@@ command-list.txt: gitsubmodules                           guide
    +@@ command-list.txt: gittutorial                             guide
      gittutorial-2                           guide
    - gittutorial                             guide
    + gitweb                                  ancillaryinterrogators
      gitworkflows                            guide
     +scalar                                  optionalcontrib
     
    + ## contrib/buildsystems/CMakeLists.txt ##
    +@@ contrib/buildsystems/CMakeLists.txt: unset(CMAKE_REQUIRED_INCLUDES)
    + #programs
    + set(PROGRAMS_BUILT
    + 	git git-daemon git-http-backend git-sh-i18n--envsubst
    +-	git-shell)
    ++	git-shell
    ++	scalar)
    + 
    + if(NOT CURL_FOUND)
    + 	list(APPEND excluded_progs git-http-fetch git-http-push)
    +@@ contrib/buildsystems/CMakeLists.txt: list(TRANSFORM git_SOURCES PREPEND "${CMAKE_SOURCE_DIR}/")
    + add_executable(git ${CMAKE_SOURCE_DIR}/git.c ${git_SOURCES})
    + target_link_libraries(git common-main)
    + 
    ++add_executable(scalar ${CMAKE_SOURCE_DIR}/scalar.c)
    ++target_link_libraries(scalar common-main)
    ++
    + add_executable(git-daemon ${CMAKE_SOURCE_DIR}/daemon.c)
    + target_link_libraries(git-daemon common-main)
    + 
    +@@ contrib/buildsystems/CMakeLists.txt: list(TRANSFORM git_shell_scripts PREPEND "${CMAKE_BINARY_DIR}/")
    + list(TRANSFORM git_perl_scripts PREPEND "${CMAKE_BINARY_DIR}/")
    + 
    + #install
    ++option(INSTALL_SCALAR "Install the optional 'scalar' contrib command")
    + foreach(program ${PROGRAMS_BUILT})
    + if(program STREQUAL "git" OR program STREQUAL "git-shell")
    + install(TARGETS ${program}
    + 	RUNTIME DESTINATION bin)
    ++elseif(program STREQUAL "scalar")
    ++if(INSTALL_SCALAR)
    ++install(TARGETS ${program}
    ++	RUNTIME DESTINATION bin)
    ++endif()
    + else()
    + install(TARGETS ${program}
    + 	RUNTIME DESTINATION libexec/git-core)
    +@@ contrib/buildsystems/CMakeLists.txt: endif()
    + 
    + #wrapper scripts
    + set(wrapper_scripts
    +-	git git-upload-pack git-receive-pack git-upload-archive git-shell git-remote-ext)
    ++	git git-upload-pack git-receive-pack git-upload-archive git-shell git-remote-ext
    ++	scalar)
    + 
    + set(wrapper_test_scripts
    + 	test-fake-ssh test-tool)
    +
      ## contrib/scalar/.gitignore (deleted) ##
     @@
    --/*.xml
    --/*.1
    --/*.html
     -/*.exe
     -/scalar
     
      ## contrib/scalar/Makefile (deleted) ##
     @@
    --QUIET_SUBDIR0  = +$(MAKE) -C # space to separate -C and subdir
    --QUIET_SUBDIR1  =
    --
    --ifneq ($(findstring s,$(MAKEFLAGS)),s)
    --ifndef V
    --	QUIET_GEN      = @echo '   ' GEN $@;
    --	QUIET_SUBDIR0  = +@subdir=
    --	QUIET_SUBDIR1  = ;$(NO_SUBDIR) echo '   ' SUBDIR $$subdir; \
    --			 $(MAKE) $(PRINT_DIR) -C $$subdir
    --	QUIET          = @
    --else
    --	export V
    --endif
    --endif
    +-# The default target of this Makefile is...
    +-all::
     -
    --all:
    +-# Import tree-wide shared Makefile behavior and libraries
    +-include ../../shared.mak
     -
     -include ../../config.mak.uname
     --include ../../config.mak.autogen
    @@ contrib/scalar/Makefile (deleted)
     -TARGETS = scalar$(X) scalar.o
     -GITLIBS = ../../common-main.o ../../libgit.a ../../xdiff/lib.a
     -
    --all: scalar$(X) ../../bin-wrappers/scalar
    +-all:: scalar$(X) ../../bin-wrappers/scalar
     -
     -$(GITLIBS):
     -	$(QUIET_SUBDIR0)../.. $(QUIET_SUBDIR1) $(subst ../../,,$@)
    @@ contrib/scalar/Makefile (deleted)
     -
     -clean:
     -	$(RM) $(TARGETS) ../../bin-wrappers/scalar
    --	$(RM) scalar.1 scalar.html scalar.xml
     -
     -../../bin-wrappers/scalar: ../../wrap-for-bin.sh Makefile
     -	@mkdir -p ../../bin-wrappers
    @@ contrib/scalar/Makefile (deleted)
     -test: all
     -	$(MAKE) -C t
     -
    --docs: scalar.html scalar.1
    --
    --scalar.html: | scalar.1 # prevent them from trying to build `doc.dep` in parallel
    --
    --scalar.html scalar.1: scalar.txt
    --	$(QUIET_SUBDIR0)../../Documentation$(QUIET_SUBDIR1) \
    --		MAN_TXT=../contrib/scalar/scalar.txt \
    --		../contrib/scalar/$@
    --	$(QUIET)test scalar.1 != "$@" || mv ../../Documentation/$@ .
    --
    --.PHONY: $(GITLIBS) all clean docs test FORCE
    +-.PHONY: $(GITLIBS) all clean test FORCE
     
      ## contrib/scalar/t/Makefile (deleted) ##
     @@
    +-# Import tree-wide shared Makefile behavior and libraries
    +-include ../../../shared.mak
    +-
     -# Run scalar tests
     -#
     -# Copyright (c) 2005,2021 Junio C Hamano, Johannes Schindelin
    @@ contrib/scalar/t/Makefile (deleted)
     
      ## contrib/scalar/scalar.c => scalar.c ##
     
    - ## contrib/scalar/t/t9099-scalar.sh => t/t9099-scalar.sh ##
    + ## contrib/scalar/t/t9099-scalar.sh => t/t7990-scalar.sh ##
     @@
      
      test_description='test the `scalar` command'
      
     -TEST_DIRECTORY=$PWD/../../../t
     -export TEST_DIRECTORY
    -+. ./test-lib.sh
    - 
    +-
     -# Make it work with --no-bin-wrappers
     -PATH=$PWD/..:$PATH
     -
     -. ../../../t/test-lib.sh
    -+if test_have_prereq WINDOWS && ! scalar list 2>/dev/null
    -+then
    -+	skip_all='the contrib cmake process does not build "scalar" yet'
    -+	test_done
    -+fi
    ++. ./test-lib.sh
      
    - GIT_TEST_MAINT_SCHEDULER="crontab:test-tool crontab ../cron.txt"
    + GIT_TEST_MAINT_SCHEDULER="crontab:test-tool crontab ../cron.txt,launchctl:true,schtasks:true"
      export GIT_TEST_MAINT_SCHEDULER
    -@@ t/t9099-scalar.sh: test_expect_success 'scalar shows a usage' '
    - 	test_expect_code 129 scalar -h
    - '
    - 
    --test_expect_success 'scalar unregister' '
    -+test_expect_success 'setup' '
    -+	test_commit first &&
    -+	test_commit second &&
    -+	test_commit third &&
    -+	git switch -c parallel first
    -+'
    -+
    -+test_lazy_prereq SCALAR_REGISTER '
    -+	git init test/src &&
    -+	scalar register test/src &&
    -+	scalar list >expect &&
    -+	scalar unregister test/src &&
    -+	(cd test/src && echo "$PWD") >actual &&
    -+	test_cmp expect actual
    -+'
    -+
    -+test_expect_success SCALAR_REGISTER 'scalar unregister' '
    - 	git init vanish/src &&
    - 	scalar register vanish/src &&
    - 	git config --get --global --fixed-value \
    -@@ t/t9099-scalar.sh: test_expect_success 'scalar unregister' '
    - 	! grep -F "$(pwd)/vanish/src" scalar.repos
    - '
    - 
    --test_expect_success 'set up repository to clone' '
    --	test_commit first &&
    --	test_commit second &&
    --	test_commit third &&
    --	git switch -c parallel first &&
    -+test_expect_success SCALAR_REGISTER 'set up repository to clone' '
    - 	mkdir -p 1/2 &&
    - 	test_commit 1/2/3 &&
    - 	git config uploadPack.allowFilter true &&
    - 	git config uploadPack.allowAnySHA1InWant true
    - '
    - 
    --test_expect_success 'scalar clone' '
    -+test_expect_success SCALAR_REGISTER 'scalar clone' '
    - 	second=$(git rev-parse --verify second:second.t) &&
    - 	scalar clone "file://$(pwd)" cloned --single-branch &&
    - 	(
    -@@ t/t9099-scalar.sh: test_expect_success 'scalar clone' '
    - 	)
    - '
    - 
    --test_expect_success 'scalar reconfigure' '
    -+test_expect_success SCALAR_REGISTER 'scalar reconfigure' '
    - 	git init one/src &&
    - 	scalar register one &&
    - 	git -C one/src config core.preloadIndex false &&
    -@@ t/t9099-scalar.sh: test_expect_success 'scalar delete without enlistment shows a usage' '
    - 	test_expect_code 129 scalar delete
    - '
    - 
    --test_expect_success 'scalar delete with enlistment' '
    -+test_expect_success SCALAR_REGISTER 'scalar delete with enlistment' '
    - 	scalar delete cloned &&
    - 	test_path_is_missing cloned
    - '

Change the optional scalar command added in 0a43fb22026 (scalar: create a rudimentary executable, 2021-12-03) so that it builds, tests and CI's with our usual build and testing process, but is still kept "a contrib command" in how we talk about it in our documentation.

Before this change we've been building scalar.o in CI as part of the "static-analysis" job, we'll now build the full binary, and run its tests. We won't install it unless "INSTALL_SCALAR=Y" is provided to the Makefile. Before this change there was no "make install" step for the "scalar" binary at all.

Before this we needed a significant amount of Makefile boilerplate to do something that the top-level Makefile could do with relatively little work, and by doing so we'll get the advantage of testing and CI-ing the scalar command along with our other commands.

The recent [1] shows a (now retracted, see [2]) series to integrate scalar with CI while it still lived in contrib/*. For context: There's been past discussion about how this command should be integrated in terms of the "closeness" of its integration, and how "contrib" sources within git.git should be organized.

That discussion started with my [3], was followed by a WIP patch[4] to implement the approach being finished up here, which prompted the "[Discussion] The architecture of Scalar (and others) within Git" thread[4]. See also the large earlier discussion hanging off [4]. An earlier version of this change was proposed at [6].

Part of the reason for sticking the new command in "contrib/scalar/" was to implicitly make it clear from the paths that it was different. I do think that would be a worthwhile goal in the abstract.

But given the build simplifications we can attain by moving it to the top level that we should seek to resolve that ambiguity in the minds of any potential git.git code maintainers in some other way.

To do that we now have a new 'OPTIONAL CONTRIB COMMANDS' section in "git help git" explaining our backwards and forwards compatibility non-promises when it comes to this and similar future commands. Our users are the ones most likely to be confused about why their git package has installed a "/usr/bin/scalar" that seemingly duplicates parts of git's own functionality. We're much better off by cross-linking our documentation, and mentioning the state of scalar in git's own documentation, along with the full list of porcelain and plumbing utilities.

Commentary on changes in specific sub-components:

A.: The tests now live in "t/t7990-scalar.sh" (per the naming guide in
    t/README).

    Previously it had to be manually tested by cd-ing to
    contrib/scalar and running "make test", and it would not benefit
    from the combination of our various CI targets.

B.: The "Documentation/cmd-list.perl" change is needed because that
    command wasn't capable of understanding commands in the
    command-list.txt whose name didn't start with "git".

    If we don't make this change we won't cross-link to "scalar(1)"
    from "OPTIONAL CONTRIB COMMANDS" in "git(1)".

C. We'll always mention scalar in "OPTIONAL CONTRIB COMMANDS" in
   "git(1)", but the "scalar(1)" manpage itself depends on whether we
   build with "INSTALL_SCALAR=Y".

D.: We'll unconditionally build scalar's documentation, and it will be
    linted along with the rest, including checking for broken links to
    other documentation.

    The minor change to scalar.txt here is needed due to the lint
    check added in cafd9828e89 (doc lint: lint and fix missing "GIT"
    end sections, 2021-04-09), perhaps we should have a different end
    blurb for this command, but for now let's make it consistent with
    the rest.

    When installed (see below) this command is part of the git suite,
    so the end blurb should say something to that effect.

E.: "make install" will now install the "scalar" binary, but only if
    "INSTALL_SCALAR" is defined.

    Relative to our $(prefix) we'll now have (this is with
    INSTALL_SYMLINKS=Y):

        libexec/git-core/scalar: symbolic link to ../../bin/scalar
        bin/scalar: ELF 64-bit LSB pie executable,

    Putting it into libexec isn't strictly necessary, but as we do it
    with "git" we do that by default, and this will ensure that anyone
    who relies on their path being "$(git --exec-path)" will also be
    able to find "scalar".

    Perhaps we shouldn't put it in "libexec" at all, but for now let's
    just follow the herd.

F.: Likewise "make install-man install-doc install-html" works, and
    will install "scalar" documentation if "INSTALL_SCALAR" is
    defined.

    We'll install these files with those targets (and correctly split
    them by target, e.g. only scalar.1 with "install-man"):

        share/doc/git-doc/scalar.txt: ASCII text
        share/doc/git-doc/scalar.html: XML 1.0 document, ASCII text, with CRLF line terminators
        share/man/man1/scalar.1: troff or preprocessor input, ASCII text, with very long lines

== Fixed ==

G.: The dependency graph of contrib/scalar/Makefile was broken when it
    came to scalar.o, it only depended on ../../libgit.a. So it was a
    requirement to run the top-level Makefile first to get around some
    of its dependency issues:

        make && make -C contrib/scalar

H.: By having the top-level Makefile build scalar.o it'll work as well
    as any other git code, including benefiting from
    COMPUTE_HEADER_DEPENDENCIES.

    Targets such as (and possibly others):

        - "make tags TAGS cscope"
        - "make coccicheck"
	- "make check-docs"

    Wouldn't consider contrib/scalar at all, some of those would have
    been fixable, but as shown in the rest of this patch it's easier
    just to have scalar built like any other program instead.

1. https://lore.kernel.org/git/220602.86ee07z6qb.gmgdl@evledraar.gmail.com/
2. https://lore.kernel.org/git/nycvar.QRO.7.76.6.2206132246010.353@tvgsbejvaqbjf.bet/
3. https://lore.kernel.org/git/87czpuxbwp.fsf@evledraar.gmail.com/
4. https://lore.kernel.org/git/87ilz44kdk.fsf@evledraar.gmail.com/
5. https://lore.kernel.org/git/b67bbef4-e4c3-b6a7-1c7f-7d405902ef8b@gmail.com/
6: https://lore.kernel.org/git/patch-1.1-86fb8d56307-20211028T185016Z-avarab@gmail.com/

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 .gitignore                                    |  1 +
 Documentation/Makefile                        |  4 +
 Documentation/cmd-list.perl                   |  2 +-
 Documentation/git.txt                         | 12 +++
 {contrib/scalar => Documentation}/scalar.txt  |  4 +-
 Makefile                                      | 57 +++++++++----
 command-list.txt                              |  2 +
 contrib/buildsystems/CMakeLists.txt           | 15 +++-
 contrib/scalar/.gitignore                     |  2 -
 contrib/scalar/Makefile                       | 35 --------
 contrib/scalar/t/Makefile                     | 81 -------------------
 contrib/scalar/scalar.c => scalar.c           |  0
 .../t/t9099-scalar.sh => t/t7990-scalar.sh    |  8 +-
 13 files changed, 79 insertions(+), 144 deletions(-)
 rename {contrib/scalar => Documentation}/scalar.txt (99%)
 delete mode 100644 contrib/scalar/.gitignore
 delete mode 100644 contrib/scalar/Makefile
 delete mode 100644 contrib/scalar/t/Makefile
 rename contrib/scalar/scalar.c => scalar.c (100%)
 rename contrib/scalar/t/t9099-scalar.sh => t/t7990-scalar.sh (96%)

On 6/23/2022 6:26 AM, Ævar Arnfjörð Bjarmason wrote:

As mentioned on the list earlier, Victoria is taking over the remaining work to complete the Scalar project. Nothing has been sent to the list because we didn't want to cause a distraction from the release window.

Victoria is taking time to incorporate your previous thoughts on how Scalar is built and its location in the codebase and create a complete narrative of how to get from our current state to that point.

To that point, this thread is the duplicate work you're trying to avoid. Please instead wait for Victoria to present her plan in July instead of moving forward with this topic.

Thanks, -Stolee

On Thu, Jun 23 2022, Derrick Stolee wrote:

I was on the fence about sending this out, but given that the "CI" thread was going on until the start of that window, and wanting to save her the work of re-discovering the subtle issues with the integration I'd already fixed I thought it was better ot send it out.

I wasn't sure she was even aware of it, and given that the WIP patch I saw in my "git fetch" was pretty much a subset of the upthread v1 it seemed that there was needless duplicate work going on.

It seemed clear that that WIP patch was attempting to head in the same direction, but hadn't yet discovered some of the hurdles with e.g. documentation building & installation that I'd fixed already. There's also the CMake integration, which I finished up for this v2.

By "duplicate work" I mean that back in October of last year I sent a patch that was a more complete version of some WIP code I spotted written in the past few days.

But you're probably talking about duplication in the sense of some larger integration of scalar changes into git.git? Or at least that's the only way I can make sense of the seemingly reversed chronology.

I'm all too happy to leave that to someone else that's more interested, perhaps this will save them some time.

Although (sans release window etc.) I think this change is in a state that's ready for pickup, and it seems that here's a consensus on this direction from everyone involved.

Although perhaps there's some minor disagreement about details, such as whether we should have an "optionalcontrib" documentation section etc.

I do think the current state on "master" is slightly confusing, one oddity I spotted is that we've already asked translators to translate contrib/scalar/scalar.c, but we never install it, and it's "in contrib".

It seems that part was unintentional in 0a43fb22026 (scalar: create a rudimentary executable, 2021-12-03), and later formalized in 9f555783c0b (Makefile: generate "po/git.pot" from stable LOCALIZED_C, 2022-05-26). Maybe it's all water under the bridge at this point, i.e. we'd rather keep it than throw away now-existing translations...

FWIW I have this local change queued on top of this v2, it's all cosmetic, but probably a good idea.

The $(SCALAR_SOURCES) bit is something I missed, but which Victoria didn't in her WIP patch (I stole it from there). That part had been added since October, so I managed to miss it when rebasing, it's the part that's been adding contrib/scalar/scalar.c to po/git.pot (see 9f555783c0b).

1:  9743e2a1e6a ! 1:  11404988785 scalar: reorganize from contrib/, still keep it "a contrib command"
    @@ Commit message
     
      ## .gitignore ##
     @@
    + /config.mak.append
      /configure
      /.vscode/
    - /tags
     +/scalar
    + /tags
      /TAGS
      /cscope*
    - /compile_commands.json
     
      ## Documentation/Makefile ##
     @@ Documentation/Makefile: MAN1_TXT += $(filter-out \
    @@ Makefile: ifndef NO_CURL
      endif
      
     -SCALAR_SOURCES := contrib/scalar/scalar.c
    -+SCALAR_SOURCES := scalar.c
    - SCALAR_OBJECTS := $(SCALAR_SOURCES:c=o)
    +-SCALAR_OBJECTS := $(SCALAR_SOURCES:c=o)
    ++SCALAR_OBJECTS := scalar.o
      OBJECTS += $(SCALAR_OBJECTS)
      
    + .PHONY: objects
     @@ Makefile: $(REMOTE_CURL_PRIMARY): remote-curl.o http.o http-walker.o GIT-LDFLAGS $(GITLIBS
      	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) \
      		$(CURL_LIBCURL) $(EXPAT_LIBEXPAT) $(LIBS)
    @@ Makefile: $(REMOTE_CURL_PRIMARY): remote-curl.o http.o http-walker.o GIT-LDFLAGS
      	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) \
      		$(filter %.o,$^) $(LIBS)
      
    +@@ Makefile: XGETTEXT_FLAGS_SH = $(XGETTEXT_FLAGS) --language=Shell \
    + XGETTEXT_FLAGS_PERL = $(XGETTEXT_FLAGS) --language=Perl \
    + 	--keyword=__ --keyword=N__ --keyword="__n:1,2"
    + MSGMERGE_FLAGS = --add-location --backup=off --update
    +-LOCALIZED_C = $(sort $(FOUND_C_SOURCES) $(FOUND_H_SOURCES) $(SCALAR_SOURCES) \
    +-	        $(GENERATED_H))
    ++LOCALIZED_C = $(sort $(FOUND_C_SOURCES) $(FOUND_H_SOURCES) $(GENERATED_H))
    + LOCALIZED_SH = $(sort $(SCRIPT_SH) git-sh-setup.sh)
    + LOCALIZED_PERL = $(sort $(SCRIPT_PERL))
    + 
     @@ Makefile: GIT-PYTHON-VARS: FORCE
                  fi
      endif

On 6/23/2022 11:30 AM, Ævar Arnfjörð Bjarmason wrote:

The CI thread was halted not just because of the release window, but because of a change in who was doing the work and taking time to revisit the plan of record. This was communicated on-list [1].

[1] https://lore.kernel.org/git/nycvar.QRO.7.76.6.2206132246010.353@tvgsbejvaqbjf.bet/

The point is that the end goal is still "Get Scalar out of contrib/" but the questions that are still being worked out are things like:

1. _When_ do we integrate Scalar into CI?
2. _When_ do we move Scalar out of contrib/?
3. _When_ do we implement the remaining Scalar features?

All three of these questions depend on what the final result will look like, and Victoria is still working on developing her own opinion on that before presenting it to the list in a complete form.

Your patch here is interrupting that process.

Poking around in people's forks to discover what they have as WIP is not a good measurement of what work is being done or not. A lot of mental energy is being spent in this area.

Saying "What I see in this person's fork isn't good enough" is not a reason to move forward on your own. WIP work is WORK IN PROGRESS and is not assumed to be complete or up to expectation for review on the list.

Please stop motivating current patches by work that you have been
playing with in your local tree. You do this frequently but it is
not helpful.
 

You also mention this in your cover letter (but is notably absent from the patch itself).

I can only attribute your insistence here as a lack of respect for your fellow contributors. There is no rush for this to happen immediately, so the only reason is that you don't trust that it would be done correctly without you submitting the work.

Thanks, -Stolee

On Thu, Jun 23 2022, Derrick Stolee wrote:

Part of this is me reading between the lines, but as mentioned in the CL I thought it had more to do with the bug I pointed out on the CI series's 2/2.

Coordination & collaboration can be difficult, but generally speaking we try to use the ML as a means to decide how topics move forward, and for coordination.

In this case I understand that there was off-list discussion about how a part of the codebase should be moving forward among a group of co-workers who contribute to git.

I don't think there's anything wrong with that, but this really seems like an overly hostile response to someone who wasn't part of that process.

Especially as I'm not insisting that I be the one to drive anything forward on the scalar topic, I think it makes much more sense that it's Victoria.

This patch is just offered as a way to help that effort along. Perhaps she'd ack it as-is, find it useful as it reveals some edge cases she didn't know about, or drop it and go for some other approach. Whatever gets us to the end-goal sooner than later is fine by me.

Once you "git rm" the scalar Makefile there's not really a lot of ways you can go other than something approximating the upthread patch. Given that some of the edge cases are tricky I deemed it worthwhile to share it.

I'm not saying that, but "you seem to be trying to do X, and may or may not be aware of a past patch that does X, here is is in case it help!.

After I submit patches to the ML I tend to "bump" the branch I sent it from, that range-diff was against the submitted v2 to local changes I applied afterwards, having missed some spots in the initial submission.

I'm not clear on what part of this you're taking issue with...

...yes, hence the "local change queued on top" above, it's a minor detail I noticed after submitting the patch.

I really think that's unhelpful. Let's try to assume some mutual good intent. I think I've explained my motivations above.

Some of the edge cases in the Makefile integration are subtle. I trust that someone looking at it would probably discover those eventually. I think it took me about a day of poking around, much of which was a slow ping-pong with the CI. So someone with a local Windows box could do it faster.

But if I'd have gotten a patch from my future self with all the learned edge cases beforehand I'd have appreciated, so I figured I'd send it in.

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

Careful ...

... Even though you ask to assume good intent, an attitude shown by repeated "I've done that, I already know *the* solution to the problem you are trying to solve, and you can learn from what I did" gives recipients quite an impression different from what you intend to give.

In any case, if you stop sending *replacement* patches in response to others' work, that alone will reduce the friction people around you are feeling quite a lot. A replacement patch is a horrible and inefficient way to comment on others' work, if your goal is to be understood. The recipient would be forced to read and think like you did, making comparison between the two approaches themselves if they want to see if it is worth to use the replacement.

Making comparison between approaches, arguing for the alternative approach to be better, and proposing the alternative to be taken, is the responsibility of the party who is suggesting alternative approaches, not that of the recipient.

For that reason, if you want to say something to other people's work, you are better off doing that either by commenting in-line (i.e. annotating their code you want to comment on), or if you absolutely need to talk in code, or by giving an incremental update once their work solidifies (i.e. pointing out a specific issue in the existing code, explaining why it is a problem worth addressing, and offering an update---just like when you are fixing a bug).

Thanks.

On Wed, Oct 27 2021, Derrick Stolee wrote:

For context. I've submitted a PATCH form of the back & forth you/me/Johannes have been having on this topic, which hopefully should unambiguously clear up exactly what it is that I've been suggesting: https://lore.kernel.org/git/patch-1.1-86fb8d56307-20211028T185016Z-avarab@gmail.com/

As noted there some inline comments here, to the extent that it's not addressed by the patch in some way.

I think what I'd like to add is...[continued below]...

Aside: I think anyone reading this summary might assume that this 1st
item describes the current state of the scalar patches on-list, but
that's not the case.

I.e. that coupling is thoroughly bi-directional in its integration, and the code is built by default.

...[continued from above]...an "Option 0: Do we care?"

I think I've been the only one who's been pointing out anything about the location of these source files, which prompted this mail. But it's not because I care about the location per-se.

I don't think it matters, and I think arguing about that per-se would just amount to needless bikeshedding.

What I *have* been pointing out is to the effect of "hey, your build/test integration is broken in <xyz> ways, it looks to me like the easiest way out of that happens to be to move these files to the top-level".

E.g. the scalar series is now at v6, but apparently it hasn't been noticed that some of the tests were broken on OSX due to apparent errors with launchctl integration. That's because the patches had the code compiled, but didn't run the tests.

If those and other issues with the series were fixed in a way that puts it at whatever in-tree path you & Johannes would prefer I wouldn't mind at all, and I doubt anyone else would have much of a strong preference either.

I think the main reason there's been resistance from you two to consider moving it away from "contrib/*" (but correct me if I'm wrong, I'm still unclear on this point, even after reading this E-Mail in full) is that you think it's important to communicate a certain maintenance status for "scalar", so that we can freely change it, and even "git rm" it one day when it's served its purpose.

I think the patch I've linked to above should address that in a way
that's primarily aimed at communicating that to users, not just git.git
developers. I.e. we now have it listed in "man git" under a "Optional
contrib commands" section that says:
    
    The following commands are included with the git sources, but may
    not be present in your installation.
    
    These should be considered "contrib"-level when it comes to
    maintenance and stability promises. They might not even be included
    in your installation, and may either drastically change in the
    future, or go away entirely.

So I guess my preferred option is: "Option 0: Make it integrate with git.git in a way that's not buggy". I believe the patch I've submitted gets us to that state.

If you wanted to run with that & move it around in-tree I think that would be fine, as long as we're not introducing bugs by doing so.

Some specific & briefer comments below:

Without going into exhaustive detail I'll just note as above that much of this describes a state that's got little or nothing to do with the contrib/scalar/* patches on-list, which are thoroughly bi-bidirectionally integrated, build (but don't test) by default etc.

Why would the FS path have anything to do with making that easy? E.g. distributors now can:

    make install NO_REGEX=MineIsBroken

That we store the relevant sources in-tree at compat/regex/ doesn't matter to distributors. We could just as well have them at the top-level. The UI we present is just some Makefile knob.

Yes, the currently propoed scalar patches would do the same via a sub-Makefile, and e.g. contrib/subtree works that way, but does it *need* to be like that for distributers? No, they'll just read the release notes, and copy whatever the relevant instructions are.

We're not. We can just document the status of individual logical components, separate from in-tree FS paths.

Well, we've got contrib/buildsystems/, if a thing that builds libgit.a doesn't have an (inter-)dependency on libgit.a I don't know what would :)

But yes, we've got no C code in contrib/ currently that links *to* libgit.a, which is one aspect of the breakages I've pointed out with the scalar patches.

But as contrib/buildsystems/ shows the "contrib/" directory is not some "hands-off" directory for git.git developers, i.e. if you make certain changes in non-contrib the vs-build CI will break, since it has a hard dependency on that contrib component.

So in terms of placing things in that directory unambiguously communicating some "third party" or "top-level doesn't depend on this" intent or state, I don't think it's really doing the work you seem to think it will.

As the instigator of particularl that debate: No that's not accurate at all.

Well, to be fair I haven't been commenting on some hypothetical "contrib/scalar/" that's "one-directional" and that you "opt-in to building".

As noted above I'm rather confused by this "contrib/scalar/" summary describing something that doesn't at all map unto the relevant posted patches.

To the extent that I have been discussing this aspect of the actual posted patches it's been because the opposite of "all knowledge of how to build Scalar lives within `contrib/scalar`" is true, as a look at the removed lines in the patch I've posted above shows.

Most of that build and test infrastructure has a thoroughly hard dependency on Makefiles at the top-level, in a way that causes more work for anyone who's changing any of that top-level build logic, not less.

I don't think anyone's been disputing the fundimental point that the whole reason for having this in-tree scalar is for the convenience of some existing users, and that any proposed changes to the CLI UI need to keep that in mind.

How would just having it in some subdirectory change any of that? Sorry, I just continue not to understand what a "contrib/scalar/", "scalar/" or whatever prefix would have anything to do with that.

If we're running with the idea that a one-level subdirectory of not-really-git.git sources is somehow magical then in-tree "sha1dc/" qualifies for the first part of that.

I.e. we build it by default, it's C based, but it doesn't depend on libgit.a.

So again on the "hypothetical contrib/scalar/*" point: This is also true of Johannes's "contrib/scalar/*" patches on-list.

So having read this far this "scalar/*" distinction here seems to really be describing something like the current state (but under a different path), and patches for "Option 1" have not been seen on-list.

I'm apathetic to whether we actually install it by default, but in my patch above that's optional.

So "tight coupling" in the sense of not only building it, but also running the tests does not need to entail that we install it by default.

I think the changes I had to "man git" should cover this, not that the scalar CLI couldn't do with a bit more review/polishing (some of which I discovered recently when trying to get the CI working).

Why specifically does it need to be at a different in-tree path before it reaches feature parity?

This proposal seems to implicitly assume a lot about the necessity for FS-path-based lifecycle management of components, without really justifying why any of that's needed.

Let's just put things in their "final place", document their current state for human consumption somewhere (commit messages, "man git", etc.) and avoid the path-based churn? Why isn't that OK?

Just on -c & -C parsing: It's already true that scalar uses libgit.a, it's even true that scalar (admittedly small) parts of libgit.a that no other caller uses directly.

So I don't see why having it in a given in-tree location and already needing parts of libgit.a would preclude us from creating APIs specifically for its use.

Particularly in the case of git.c where those APIs would benefit any future "top-level program sort of like a git(1)" caller, and would/could even benefit our own sub-commands such as "git commit-graph" et al.

E.g. if we lifted out the typo correction cmd dispatching from git.git and could use it for those + scalar.c, that would be very nice for existing in-tree users, and would also reduce code duplication in scalar.c.

I think that in this case a series of "small" ideas might cause any preconceived "big" solutions to evaporate once implemented.

E.g. in [1] Johannes noted:

    "Just forget about Scalar's build process. Forget about getting its
    CI to work. I have all that figured out already. It is all working
    as well as needed."[1]

That "as well as needed" is doing a lot of work, as noted above it was clearly broken on the OSX CI at least.

One thing my linked-to does is to fix/change that small aspect, does anyone mind the incremental step of scalar.c not only being built but also tested? If so let them just note their concerns in the releant thread.

Once we address such point-by-point issues will any "big" ones remain? Maybe, maybe not.

1. https://lore.kernel.org/git/nycvar.QRO.7.76.6.2110221734530.62@tvgsbejvaqbjf.bet/

On 10/28/2021 2:58 PM, Ævar Arnfjörð Bjarmason wrote:

Thank you for your thoughts on this topic. I wanted to specifically reply to a few items while the discussion continues to simmer.

These failures are due to changes to Git maintenance during the 2.34 cycle, and we've noticed and fixed them in microsoft/git as we rebased on the release candidates.

This is one of the things that we expect with one extreme side of Git's "ownership" of Scalar: things could break Scalar that we won't notice until manually running the tests or taking upstream changes into our fork, but then we will fix them. Right now, that fix would be in a future iteration of the patch series.

You're right that I am describing a theoretical extreme that is not realized by the patch series in its current form. The extreme I'm describing is where we could revert Scalar out of the Git codebase with a simple 'rm -f contrib/scalar' and as minimal remaining work as possible. Johannes relaxed that condition in an attempt to reduce duplication in the Makefiles, but there is opportunity to redesign and get much closer to this extreme, if that's the consensus we reach.

Thanks, -Stolee

Derrick Stolee <stolee@gmail.com> writes:

In other words, Scalar wants to piggyback on Git's popularity, yet wants to retain total control of how its end-user experience should look like, eating the cake and having it at the same time?

I do not think it is fair to proceed with this discussion without having "Scalar lives in its own tree, uses this mailing list to discuss how it should evolve, drawing expertise from the contributors found here, but otherwise would stay to be a spearate project" as another option and debating its pros-and-cons, next to the spectrum among contrib/scalar, scalar/, and scalar.c approaches. IIUC, cgit has one-directional coupling to depend on libgit.a just like the Scalar wants to link with it---I am not saying Scalar should do the same, but just raising it as an example that you do *not* have to live in my tree anywhere to be able to link with libgit.a; that would probably count as an "**Option 0:** A separate project that borrows from Git by having it as a submodule".

I can think of only one obvious advantage going that route, and that is why I said "I am not saying Scalar should do the same", which is that the Scalar project will then retain total control over how it is built and how its end-user experience should stay the same.

I do not think it is an option to refuse to take any input from others on this list on parts of what you have already done, if you want to be in my tree, anywhere, not even in contrib/.

Just like we say "the plumbing command X has behaved this way forever, and we cannot break the backward compatibility" to some possible changes to any of our tools, however, "Scalar has been used with that subcommand behaving this particular way even before the code was donated to Git project, so we cannot lightly change it" would equally be a good input when we evaluate when some things in it may need to be changed, so I do not think it would pose a great backward compatibility issue (after all, we do not change things lightly just for the sake of changing it).

But approaching our developers with a new piece of code, saying "I want to piggyback on your popularity. Among the stuff given to you, some are out of limit and you may not even discuss about modifying it" upfront, is a problematic attitude.

Now, the goal from purely Git project's point of view would be to improve Git's user experience, not necessarily Scalar's, by exposing more of our users to the way its opinionated (which is *not* necessarily a bad thing) decisions work to help its users, and our developers to the scalar codebase. Some of our developers may not even agree with the design decisions Scalar made and that is expected and is fine; based on such a reaction, the lesson Git learns from Scalar to solve a similar issue may turn out to be solve it differently.

I do not know how much Options 1-3 would help such cross pollination more over the Option 0, offhand, though.

Here I sense the "final target" is somehow Scalar is delivered to end-users through my tree and being maintained there, but from my point of view, that is merely an early step. Wasn't the final target for "git" to learn from Scalar's successes and mistakes and give native support to itches Scalar wanted to scratch?

And if that is indeed the goal, when it is achieved, Scalar would
become a historical curiosity just like Cogito is today.  With
Cogito, we cross-pollinated just fine without having both in the
same tree at all, and when Cogito outlived its usefulness, we did
not have to remove anything from our tree.  With cgit, we did not
cross-pollinate at all, unfortunately, and gitweb more or less stays
the same way as it was years ago, and cgit is still a project with a
useful product.  We can reach both extremes even if we take Option
0.  But it would have been messy for something like Cogito, which
was an early UI and workflow experiment, if we took a more
integrated approach.

I personally have no problem with this approach, but with less scrutiny, being in contrib/ longer, with "some stuff are not subject to discussion" attitude, would inevitably lower the code quality of the product, and it is dubious it would become "ready for promotion" ever. The contrib/ hierarchy, especially in early days when we wanted a way to grow the ecosystem, as a place to host "these are done externally, and we only carry them for convenience", was a successful model, but I do not think of anything that successfully used contrib/ as a nursery to later graduate to the core and to be seen as the same quality as other things that started their life in core. I am pessimistic to the model that uses contrib/ as such a place.

I am afraid that the answer to the first four are "it depends" (but the remaining two would fall out naturally out of them).

Thanks. My brain is fried with the -rc work, rereading the material that went to -rc0 since the last release, so I may have more to add later, but for now, the above is what came to my mind.

Hi Junio,

On Mon, 1 Nov 2021, Junio C Hamano wrote:

If that was indeed the intention with Scalar, I would totally agree.

We don't want Scalar to piggy-back onto Git. In fact, we don't even want it to be separate from Git, which is why we are contributing it here. I firmly believe that Scalar has a lot of value and will benefit the Git project and community. That is my motivation behind contributing these patches.

I did consider keeping Scalar/C in its own repository, and I, too, had cgit in mind (and cinnabar). In fact, Scalar/C does live in its own repository right now, and would remain there if the community doesn’t want it: in the microsoft/git fork of git/git.

However, just like partial clone, scheduled maintenance, sparse-checkout cone mode and sparse-index, the remainder of Scalar can benefit Git by moving its functionality to `libgit.a`, or reimplementing it there.

That's a really good point.

Yes, I agree, that would be a problematic attitude. I apologize for giving you the impression that that was my attitude. I want to contribute Scalar to Git because I believe that Git will benefit from it, not because I want to abuse Git’s popularity to advance Scalar.

I am still of the opinion that having the code in the same tree will make cross pollination much easier.

I think we are very much in alignment here, and I believe Stolee's goal was to reach such an alignment.

Scalar is more similar to cgit than to gitweb or Cogito in this context: gitweb and Cogito are scripted users of Git, whereas Scalar and cgit link to libgit.a, are written in C, and it is easy to refactor code from Scalar into libgit.a. That was never the case with gitweb nor with Cogito.

To be clear, I'm not pushing for promotion of Scalar to a top-level command delivered via the Git project at all. I _might_ push for it in the future, _iff_ a wider audience has used Scalar _and_ finds it useful.

In any case, pushing for Scalar to be such a top-level command would be premature at this stage: to avoid overwhelming reviewers, the Scalar patch series is not yet feature-complete, there are other patch series I intend to contribute on top, when the first patch series has settled down.

After seeing `vcs-svn`'s fate, I understand the trepidation. I, too, would have loved to see it being cooked to perfection and then graduate.

Even so, I targeted `contrib/` with my patch series because `contrib/README` says:

    The intention is to keep interesting tools around git here, maybe
    even experimental ones, to give users an easier access to them,
    and to give tools wider exposure, so that they can be improved
    faster.

Does this README's intent still apply, or has the purpose of contrib/ changed?

Would you like me to organize the code and patches such that they more clearly fall under your maintenance once the patches are merged? If that is your preference, I will gladly put in the work.

When I submitted the Scalar patches, I did not want to burden you with maintaining it, I expected to maintain Scalar going forward, but maybe that is not your preference? I assumed it to be my maintenance burden, we aim to support backward compatibility for existing Scalar users, after all. Absolutely everything else, however, is up for discussion, subject to the full scrutiny of the community and to the same standards of a submission to the core of the project.

And I see your point about backward compatibility: The Git project has plenty of experience with maintaining backward compatibility and _still_ improving on initial designs. I therefore can easily give up my hard “this command-line interface must not be changed” stance.

Thank you for taking the time to respond, especially during the -rc cycle. I look forward to continuing this thread when you have time.

Ciao, Dscho

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

The intent may still be the same, but my suspicion is that the world has changed sufficiently to make contrib/ that is offered with such an intent no longer is useful. In other words, "contrib/" as a nursery may have been our dream, but the dream did not materialize and may have outlived its usefulness.

It depends on how you define "maintenance".

Many parts of the system (e.g. difftool, commit-graph, pack bitmaps, multi-pack index, sparse-index, po, and gitweb) may "fall under my maintenance", but you can see that I am not heavily involved in any of these areas, other than serving mostly as a traffic cop. If a patch that touches some area comes, I may trust input from folks who are more deeply involved in the area (e.g. input from Taylor better than from other folks, including mine, for a patch that touches midx) when deciding if the patch needs revising or if it is ready.

I would expect that is more or less how things would work for anything that is in my tree. And I do not think it would be any different for Scalar. I would expect that any bugfixes or new features would be redirected to you folks. Ehh, not that you would need redirecting---I'd expect you'd jump on them even _before_ I would react ;-)