* Re: [Discussion] The architecture of Scalar (and others) within Git
2021-10-27 21:51 [Discussion] The architecture of Scalar (and others) within Git Derrick Stolee
@ 2021-10-28 6:56 ` Johannes Schindelin
2021-10-28 15:29 ` Philip Oakley
` (3 subsequent siblings)
4 siblings, 0 replies; 16+ messages in thread
From: Johannes Schindelin @ 2021-10-28 6:56 UTC (permalink / raw)
To: Derrick Stolee
Cc: Git Mailing List, Ævar Arnfjörð Bjarmason,
Junio C Hamano, Eric Sunshine, Elijah Newren, Bagas Sanjaya,
Matthew John Cheetham, Victoria Dye, Theodore Ts'o
Hi Stolee,
On Wed, 27 Oct 2021, Derrick Stolee wrote:
> [...]
> 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.
>
> [...]
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
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [Discussion] The architecture of Scalar (and others) within Git
2021-10-27 21:51 [Discussion] The architecture of Scalar (and others) within Git Derrick Stolee
2021-10-28 6:56 ` Johannes Schindelin
@ 2021-10-28 15:29 ` Philip Oakley
2021-10-28 18:56 ` [PATCH] contrib: build & test scalar by default, optionally install Ævar Arnfjörð Bjarmason
` (2 subsequent siblings)
4 siblings, 0 replies; 16+ messages in thread
From: Philip Oakley @ 2021-10-28 15:29 UTC (permalink / raw)
To: Derrick Stolee, Git Mailing List
Cc: Johannes Schindelin, Ævar Arnfjörð Bjarmason,
Junio C Hamano, Eric Sunshine, Elijah Newren, Bagas Sanjaya,
Matthew John Cheetham, Victoria Dye, Theodore Ts'o
Hi Stollee,
a few very generic points with my "systems" hat on.
On 27/10/2021 22:51, Derrick Stolee wrote:
> 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.
We probably should also summarise the status of and relationships with
the existing 'integrations' like `git-gui`, `gitk`, `Git for-Windows`, etc.
>
> 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
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.
> main goal is to identify a community-supported plan for upstreaming the
> Scalar CLI.
Here, 'scalar' is about large code mono-repos, rather than any operating
system/vendor issues, or visualisation tools. (may need saying)
>
> 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?
How to quantify the size of the scalar _code_ contribution, and how is
concept ownership handed over?
>
> ### 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.
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.
>
> 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.
I'm presuming this (responsibility) is with respect to the *transition*
from the independent `scalar` to the Git integrated _scalar_ capability.
>
>
> 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.
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.
>
> 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.
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.
> 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:
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.
>
> **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
>
>
regards
Philip
^ permalink raw reply [flat|nested] 16+ messages in thread* [PATCH] contrib: build & test scalar by default, optionally install
2021-10-27 21:51 [Discussion] The architecture of Scalar (and others) within Git Derrick Stolee
2021-10-28 6:56 ` Johannes Schindelin
2021-10-28 15:29 ` Philip Oakley
@ 2021-10-28 18:56 ` Ævar Arnfjörð Bjarmason
2022-06-23 10:26 ` [PATCH v2 0/1] scalar: move to the top-level, test, CI and "install" support Ævar Arnfjörð Bjarmason
2021-10-28 18:58 ` [Discussion] The architecture of Scalar (and others) within Git Ævar Arnfjörð Bjarmason
2021-11-01 23:20 ` Junio C Hamano
4 siblings, 1 reply; 16+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-10-28 18:56 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Derrick Stolee, Johannes Schindelin, Jeff King,
Eric Sunshine, Elijah Newren, Bagas Sanjaya,
Matthew John Cheetham, Victoria Dye, Theodore T .,
Ævar Arnfjörð Bjarmason
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:
> [...]
> 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.
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%)
diff --git a/.gitignore b/.gitignore
index 054249b20a8..7f590115545 100644
--- a/.gitignore
+++ b/.gitignore
@@ -216,6 +216,7 @@
/configure
/.vscode/
/tags
+/scalar
/TAGS
/cscope*
/compile_commands.json
diff --git a/Documentation/Makefile b/Documentation/Makefile
index 911b6bf79c5..c017619ad2a 100644
--- a/Documentation/Makefile
+++ b/Documentation/Makefile
@@ -18,6 +18,9 @@ MAN1_TXT += $(filter-out \
MAN1_TXT += git.txt
MAN1_TXT += gitk.txt
MAN1_TXT += gitweb.txt
+ifndef NO_INSTALL_SCALAR_DOC
+MAN1_TXT += scalar.txt
+endif
# man5 / man7 guides (note: new guides should also be added to command-list.txt)
MAN5_TXT += gitattributes.txt
@@ -311,6 +314,7 @@ endif
cmds_txt = cmds-ancillaryinterrogators.txt \
cmds-ancillarymanipulators.txt \
cmds-mainporcelain.txt \
+ cmds-optionalcontrib.txt \
cmds-plumbinginterrogators.txt \
cmds-plumbingmanipulators.txt \
cmds-synchingrepositories.txt \
diff --git a/Documentation/cmd-list.perl b/Documentation/cmd-list.perl
index af5da45d287..0f4b1b23cfe 100755
--- a/Documentation/cmd-list.perl
+++ b/Documentation/cmd-list.perl
@@ -10,7 +10,7 @@ sub format_one {
$state = 0;
open I, '<', "$name.txt" or die "No such file $name.txt";
while (<I>) {
- if (/^git[a-z0-9-]*\(([0-9])\)$/) {
+ if (/^[a-z0-9-]*\(([0-9])\)$/) {
$mansection = $1;
next;
}
diff --git a/Documentation/git.txt b/Documentation/git.txt
index d63c65e67d8..d2b1280ff9d 100644
--- a/Documentation/git.txt
+++ b/Documentation/git.txt
@@ -337,6 +337,18 @@ The following documentation pages are guides about Git concepts.
include::cmds-guide.txt[]
+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[]
Configuration Mechanism
-----------------------
diff --git a/contrib/scalar/scalar.txt b/Documentation/scalar.txt
similarity index 99%
rename from contrib/scalar/scalar.txt
rename to Documentation/scalar.txt
index cf4e5b889cc..197e9983a0f 100644
--- a/contrib/scalar/scalar.txt
+++ b/Documentation/scalar.txt
@@ -150,6 +150,6 @@ SEE ALSO
--------
linkgit:git-clone[1], linkgit:git-maintenance[1].
-Scalar
+GIT
---
-Associated with the linkgit:git[1] suite
+Part of the linkgit:git[1] suite
diff --git a/Makefile b/Makefile
index 033a50a8b99..78da6c3661a 100644
--- a/Makefile
+++ b/Makefile
@@ -459,6 +459,11 @@ all::
# INSTALL_STRIP can be set to "-s" to strip binaries during installation,
# if your $(INSTALL) command supports the option.
#
+# Define INSTALL_SCALAR if you would like to install the optional
+# "scalar" command. This command is considered "contrib"-level, see
+# 'Optional "contrib" commands' in the built (with "make man") git(1)
+# manual page.
+#
# Define GENERATE_COMPILATION_DATABASE to "yes" to generate JSON compilation
# database entries during compilation if your compiler supports it, using the
# `-MJ` flag. The JSON entries will be placed in the `compile_commands/`
@@ -579,8 +584,10 @@ GIT_OBJS =
LIB_OBJS =
OBJECTS =
PROGRAM_OBJS =
+OTHER_PROGRAMS =
PROGRAMS =
EXCLUDED_PROGRAMS =
+SCALAR_OBJS =
SCRIPT_PERL =
SCRIPT_PYTHON =
SCRIPT_SH =
@@ -784,7 +791,8 @@ BUILT_INS += git-switch$X
BUILT_INS += git-whatchanged$X
# what 'all' will build but not install in gitexecdir
-OTHER_PROGRAMS = git$X
+OTHER_PROGRAMS += git$X
+OTHER_PROGRAMS += scalar$X
# what test wrappers are needed and 'install' will install, in bindir
BINDIR_PROGRAMS_NEED_X += git
@@ -793,6 +801,18 @@ BINDIR_PROGRAMS_NEED_X += git-shell
BINDIR_PROGRAMS_NEED_X += git-upload-archive
BINDIR_PROGRAMS_NEED_X += git-upload-pack
+# Sometimes we only have a test wrapper, but not a program to
+# install. This isn't so pretty, and we could refactor the
+# bin-wrappers/% and install code to make it unnecessary.
+ifdef INSTALL_SCALAR
+PROGRAMS += scalar$X
+BINDIR_PROGRAMS_NEED_X += scalar
+endif
+TEST_BINDIR_PROGRAMS_NEED_X = $(BINDIR_PROGRAMS_NEED_X)
+ifndef INSTALL_SCALAR
+TEST_BINDIR_PROGRAMS_NEED_X += scalar
+endif
+
BINDIR_PROGRAMS_NO_X += git-cvsserver
# Set paths to tools early so that they can be used for version tests.
@@ -2210,9 +2230,13 @@ 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
@@ -2448,7 +2472,12 @@ 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)
@@ -2457,10 +2486,6 @@ 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)
@@ -2594,10 +2619,6 @@ $(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) $@ $^
@@ -2870,15 +2891,18 @@ GIT-PYTHON-VARS: FORCE
fi
endif
-test_bindir_programs := $(patsubst %,bin-wrappers/%,$(BINDIR_PROGRAMS_NEED_X) $(BINDIR_PROGRAMS_NO_X) $(TEST_PROGRAMS_NEED_X))
+test_bindir_programs := $(patsubst %,bin-wrappers/%,$(TEST_BINDIR_PROGRAMS_NEED_X) $(BINDIR_PROGRAMS_NO_X) $(TEST_PROGRAMS_NEED_X))
all:: $(TEST_PROGRAMS) $(test_bindir_programs)
+# Substitution that's only done on programs named git-* in
+# 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
$(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)))|' < $< > $@ && \
+ -e 's|@@PROG@@|$(patsubst test-%,t/helper/test-%$(X),$(@F))$(patsubst git%,$(X),$(filter $(@F),$(GIT_TEST_BINDIR_PROGRAMS_NEED_X)))|' < $< > $@ && \
chmod +x $@
# GNU make supports exporting all variables by "export" without parameters.
@@ -3113,25 +3137,26 @@ 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
+NO_INSTALL_SCALAR_DOC =
+else
+NO_INSTALL_SCALAR_DOC = NoScalarPlease
+endif
install-man-perl: man-perl
$(INSTALL) -d -m 755 '$(DESTDIR_SQ)$(mandir_SQ)/man3'
(cd perl/build/man/man3 && $(TAR) cf - .) | \
(cd '$(DESTDIR_SQ)$(mandir_SQ)/man3' && umask 022 && $(TAR) xof -)
-install-html:
- $(MAKE) -C Documentation install-html
-
-install-info:
- $(MAKE) -C Documentation install-info
-
-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,))
quick-install-doc:
$(MAKE) -C Documentation quick-install
@@ -3183,8 +3208,19 @@ ifneq ($(INCLUDE_DLLS_IN_ARTIFACTS),)
OTHER_PROGRAMS += $(shell echo *.dll t/helper/*.dll)
endif
-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)'
@@ -3327,7 +3363,7 @@ check-docs::
-e 's/\.txt//'; \
) | while read how cmd; \
do \
- case " $(patsubst %$X,%,$(ALL_COMMANDS) $(BUILT_INS) $(EXCLUDED_PROGRAMS)) " in \
+ case " $(patsubst %$X,%,$(ALL_COMMANDS) $(BUILT_INS) $(EXCLUDED_PROGRAMS) scalar) " in \
*" $$cmd "*) ;; \
*) echo "removed but $$how: $$cmd" ;; \
esac; \
diff --git a/command-list.txt b/command-list.txt
index a289f09ed6f..350f5aec074 100644
--- a/command-list.txt
+++ b/command-list.txt
@@ -16,6 +16,7 @@
# synchingrepositories
# synchelpers
# purehelpers
+# optionalcontrib
#
# 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
@@ -214,3 +215,4 @@ gitsubmodules guide
gittutorial-2 guide
gittutorial guide
gitworkflows guide
+scalar optionalcontrib
diff --git a/contrib/scalar/.gitignore b/contrib/scalar/.gitignore
deleted file mode 100644
index 00441073f59..00000000000
--- a/contrib/scalar/.gitignore
+++ /dev/null
@@ -1,5 +0,0 @@
-/*.xml
-/*.1
-/*.html
-/*.exe
-/scalar
diff --git a/contrib/scalar/Makefile b/contrib/scalar/Makefile
deleted file mode 100644
index 44796572ef4..00000000000
--- a/contrib/scalar/Makefile
+++ /dev/null
@@ -1,57 +0,0 @@
-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
-
-all:
-
-include ../../config.mak.uname
--include ../../config.mak.autogen
--include ../../config.mak
-
-TARGETS = scalar$(X) scalar.o
-GITLIBS = ../../common-main.o ../../libgit.a ../../xdiff/lib.a
-
-all: scalar$(X) ../../bin-wrappers/scalar
-
-$(GITLIBS):
- $(QUIET_SUBDIR0)../.. $(QUIET_SUBDIR1) $(subst ../../,,$@)
-
-$(TARGETS): $(GITLIBS) scalar.c
- $(QUIET_SUBDIR0)../.. $(QUIET_SUBDIR1) $(patsubst %,contrib/scalar/%,$@)
-
-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
- $(QUIET_GEN)sed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \
- -e 's|@@BUILD_DIR@@|$(shell cd ../.. && pwd)|' \
- -e 's|@@PROG@@|contrib/scalar/scalar$(X)|' < $< > $@ && \
- chmod +x $@
-
-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
diff --git a/contrib/scalar/t/Makefile b/contrib/scalar/t/Makefile
deleted file mode 100644
index 6170672bb37..00000000000
--- a/contrib/scalar/t/Makefile
+++ /dev/null
@@ -1,78 +0,0 @@
-# Run scalar tests
-#
-# Copyright (c) 2005,2021 Junio C Hamano, Johannes Schindelin
-#
-
--include ../../../config.mak.autogen
--include ../../../config.mak
-
-SHELL_PATH ?= $(SHELL)
-PERL_PATH ?= /usr/bin/perl
-RM ?= rm -f
-PROVE ?= prove
-DEFAULT_TEST_TARGET ?= test
-TEST_LINT ?= test-lint
-
-ifdef TEST_OUTPUT_DIRECTORY
-TEST_RESULTS_DIRECTORY = $(TEST_OUTPUT_DIRECTORY)/test-results
-else
-TEST_RESULTS_DIRECTORY = ../../../t/test-results
-endif
-
-# Shell quote;
-SHELL_PATH_SQ = $(subst ','\'',$(SHELL_PATH))
-PERL_PATH_SQ = $(subst ','\'',$(PERL_PATH))
-TEST_RESULTS_DIRECTORY_SQ = $(subst ','\'',$(TEST_RESULTS_DIRECTORY))
-
-T = $(sort $(wildcard t[0-9][0-9][0-9][0-9]-*.sh))
-
-all: $(DEFAULT_TEST_TARGET)
-
-test: $(TEST_LINT)
- $(MAKE) aggregate-results-and-cleanup
-
-prove: $(TEST_LINT)
- @echo "*** prove ***"; GIT_CONFIG=.git/config $(PROVE) --exec '$(SHELL_PATH_SQ)' $(GIT_PROVE_OPTS) $(T) :: $(GIT_TEST_OPTS)
- $(MAKE) clean-except-prove-cache
-
-$(T):
- @echo "*** $@ ***"; GIT_CONFIG=.git/config '$(SHELL_PATH_SQ)' $@ $(GIT_TEST_OPTS)
-
-clean-except-prove-cache:
- $(RM) -r 'trash directory'.* '$(TEST_RESULTS_DIRECTORY_SQ)'
- $(RM) -r valgrind/bin
-
-clean: clean-except-prove-cache
- $(RM) .prove
-
-test-lint: test-lint-duplicates test-lint-executable test-lint-shell-syntax
-
-test-lint-duplicates:
- @dups=`echo $(T) | tr ' ' '\n' | sed 's/-.*//' | sort | uniq -d` && \
- test -z "$$dups" || { \
- echo >&2 "duplicate test numbers:" $$dups; exit 1; }
-
-test-lint-executable:
- @bad=`for i in $(T); do test -x "$$i" || echo $$i; done` && \
- test -z "$$bad" || { \
- echo >&2 "non-executable tests:" $$bad; exit 1; }
-
-test-lint-shell-syntax:
- @'$(PERL_PATH_SQ)' ../../../t/check-non-portable-shell.pl $(T)
-
-aggregate-results-and-cleanup: $(T)
- $(MAKE) aggregate-results
- $(MAKE) clean
-
-aggregate-results:
- for f in '$(TEST_RESULTS_DIRECTORY_SQ)'/t*-*.counts; do \
- echo "$$f"; \
- done | '$(SHELL_PATH_SQ)' ../../../t/aggregate-results.sh
-
-valgrind:
- $(MAKE) GIT_TEST_OPTS="$(GIT_TEST_OPTS) --valgrind"
-
-test-results:
- mkdir -p test-results
-
-.PHONY: $(T) aggregate-results clean valgrind
diff --git a/contrib/scalar/scalar.c b/scalar.c
similarity index 100%
rename from contrib/scalar/scalar.c
rename to scalar.c
diff --git a/contrib/scalar/t/t9099-scalar.sh b/t/t9099-scalar.sh
similarity index 72%
rename from contrib/scalar/t/t9099-scalar.sh
rename to t/t9099-scalar.sh
index 7e8771d0eff..788695d857e 100755
--- a/contrib/scalar/t/t9099-scalar.sh
+++ b/t/t9099-scalar.sh
@@ -2,13 +2,13 @@
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
GIT_TEST_MAINT_SCHEDULER="crontab:test-tool crontab ../cron.txt"
export GIT_TEST_MAINT_SCHEDULER
@@ -17,7 +17,23 @@ 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 \
@@ -32,18 +48,14 @@ 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 &&
(
@@ -65,7 +77,7 @@ 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 &&
@@ -80,7 +92,7 @@ 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
'
--
2.33.1.1572.g4991fc4c1bd
^ permalink raw reply related [flat|nested] 16+ messages in thread* [PATCH v2 0/1] scalar: move to the top-level, test, CI and "install" support
2021-10-28 18:56 ` [PATCH] contrib: build & test scalar by default, optionally install Ævar Arnfjörð Bjarmason
@ 2022-06-23 10:26 ` Ævar Arnfjörð Bjarmason
2022-06-23 10:26 ` [PATCH v2 1/1] scalar: reorganize from contrib/, still keep it "a contrib command" Ævar Arnfjörð Bjarmason
2022-06-23 15:02 ` [PATCH v2 0/1] scalar: move to the top-level, test, CI and "install" support Derrick Stolee
0 siblings, 2 replies; 16+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-06-23 10:26 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Johannes Schindelin, Victoria Dye,
Ævar Arnfjörð Bjarmason
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
- '
--
2.36.1.1239.gfba91521d90
^ permalink raw reply [flat|nested] 16+ messages in thread* [PATCH v2 1/1] scalar: reorganize from contrib/, still keep it "a contrib command"
2022-06-23 10:26 ` [PATCH v2 0/1] scalar: move to the top-level, test, CI and "install" support Ævar Arnfjörð Bjarmason
@ 2022-06-23 10:26 ` Ævar Arnfjörð Bjarmason
2022-06-23 15:02 ` [PATCH v2 0/1] scalar: move to the top-level, test, CI and "install" support Derrick Stolee
1 sibling, 0 replies; 16+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-06-23 10:26 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Johannes Schindelin, Victoria Dye,
Ævar Arnfjörð Bjarmason
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%)
diff --git a/.gitignore b/.gitignore
index a4522157641..b06d0923ade 100644
--- a/.gitignore
+++ b/.gitignore
@@ -219,6 +219,7 @@
/configure
/.vscode/
/tags
+/scalar
/TAGS
/cscope*
/compile_commands.json
diff --git a/Documentation/Makefile b/Documentation/Makefile
index f2e7fc1daa5..10fff5976af 100644
--- a/Documentation/Makefile
+++ b/Documentation/Makefile
@@ -21,6 +21,9 @@ MAN1_TXT += $(filter-out \
MAN1_TXT += git.txt
MAN1_TXT += gitk.txt
MAN1_TXT += gitweb.txt
+ifndef NO_INSTALL_SCALAR_DOC
+MAN1_TXT += scalar.txt
+endif
# man5 / man7 guides (note: new guides should also be added to command-list.txt)
MAN5_TXT += gitattributes.txt
@@ -284,6 +287,7 @@ endif
cmds_txt = cmds-ancillaryinterrogators.txt \
cmds-ancillarymanipulators.txt \
cmds-mainporcelain.txt \
+ cmds-optionalcontrib.txt \
cmds-plumbinginterrogators.txt \
cmds-plumbingmanipulators.txt \
cmds-synchingrepositories.txt \
diff --git a/Documentation/cmd-list.perl b/Documentation/cmd-list.perl
index af5da45d287..0f4b1b23cfe 100755
--- a/Documentation/cmd-list.perl
+++ b/Documentation/cmd-list.perl
@@ -10,7 +10,7 @@ sub format_one {
$state = 0;
open I, '<', "$name.txt" or die "No such file $name.txt";
while (<I>) {
- if (/^git[a-z0-9-]*\(([0-9])\)$/) {
+ if (/^[a-z0-9-]*\(([0-9])\)$/) {
$mansection = $1;
next;
}
diff --git a/Documentation/git.txt b/Documentation/git.txt
index 302607a4967..10c1979396b 100644
--- a/Documentation/git.txt
+++ b/Documentation/git.txt
@@ -339,6 +339,18 @@ The following documentation pages are guides about Git concepts.
include::cmds-guide.txt[]
+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[]
Configuration Mechanism
-----------------------
diff --git a/contrib/scalar/scalar.txt b/Documentation/scalar.txt
similarity index 99%
rename from contrib/scalar/scalar.txt
rename to Documentation/scalar.txt
index c0425e06533..b11e9eecf21 100644
--- a/contrib/scalar/scalar.txt
+++ b/Documentation/scalar.txt
@@ -162,6 +162,6 @@ SEE ALSO
--------
linkgit:git-clone[1], linkgit:git-maintenance[1].
-Scalar
+GIT
---
-Associated with the linkgit:git[1] suite
+Part of the linkgit:git[1] suite
diff --git a/Makefile b/Makefile
index 04d0fd1fe60..28294cd3626 100644
--- a/Makefile
+++ b/Makefile
@@ -469,6 +469,11 @@ include shared.mak
# INSTALL_STRIP can be set to "-s" to strip binaries during installation,
# if your $(INSTALL) command supports the option.
#
+# Define INSTALL_SCALAR if you would like to install the optional
+# "scalar" command. This command is considered "contrib"-level, see
+# 'Optional "contrib" commands' in the built (with "make man") git(1)
+# manual page.
+#
# Define GENERATE_COMPILATION_DATABASE to "yes" to generate JSON compilation
# database entries during compilation if your compiler supports it, using the
# `-MJ` flag. The JSON entries will be placed in the `compile_commands/`
@@ -602,6 +607,7 @@ GIT_OBJS =
LIB_OBJS =
OBJECTS =
PROGRAM_OBJS =
+OTHER_PROGRAMS =
PROGRAMS =
EXCLUDED_PROGRAMS =
SCRIPT_PERL =
@@ -812,7 +818,8 @@ BUILT_INS += git-switch$X
BUILT_INS += git-whatchanged$X
# what 'all' will build but not install in gitexecdir
-OTHER_PROGRAMS = git$X
+OTHER_PROGRAMS += git$X
+OTHER_PROGRAMS += scalar$X
# what test wrappers are needed and 'install' will install, in bindir
BINDIR_PROGRAMS_NEED_X += git
@@ -821,6 +828,18 @@ BINDIR_PROGRAMS_NEED_X += git-shell
BINDIR_PROGRAMS_NEED_X += git-upload-archive
BINDIR_PROGRAMS_NEED_X += git-upload-pack
+# Sometimes we only have a test wrapper, but not a program to
+# install. This isn't so pretty, and we could refactor the
+# bin-wrappers/% and install code to make it unnecessary.
+ifdef INSTALL_SCALAR
+PROGRAMS += scalar$X
+BINDIR_PROGRAMS_NEED_X += scalar
+endif
+TEST_BINDIR_PROGRAMS_NEED_X = $(BINDIR_PROGRAMS_NEED_X)
+ifndef INSTALL_SCALAR
+TEST_BINDIR_PROGRAMS_NEED_X += scalar
+endif
+
BINDIR_PROGRAMS_NO_X += git-cvsserver
# Set paths to tools early so that they can be used for version tests.
@@ -2543,7 +2562,7 @@ ifndef NO_CURL
OBJECTS += http.o http-walker.o remote-curl.o
endif
-SCALAR_SOURCES := contrib/scalar/scalar.c
+SCALAR_SOURCES := scalar.c
SCALAR_OBJECTS := $(SCALAR_SOURCES:c=o)
OBJECTS += $(SCALAR_OBJECTS)
@@ -2678,7 +2697,7 @@ $(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)
+scalar$X: $(SCALAR_OBJECTS) GIT-LDFLAGS $(GITLIBS)
$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) \
$(filter %.o,$^) $(LIBS)
@@ -3041,15 +3060,18 @@ GIT-PYTHON-VARS: FORCE
fi
endif
-test_bindir_programs := $(patsubst %,bin-wrappers/%,$(BINDIR_PROGRAMS_NEED_X) $(BINDIR_PROGRAMS_NO_X) $(TEST_PROGRAMS_NEED_X))
+test_bindir_programs := $(patsubst %,bin-wrappers/%,$(TEST_BINDIR_PROGRAMS_NEED_X) $(BINDIR_PROGRAMS_NO_X) $(TEST_PROGRAMS_NEED_X))
all:: $(TEST_PROGRAMS) $(test_bindir_programs)
+# Substitution that's only done on programs named git-* in
+# bin-wrappers/*
+GIT_TEST_BINDIR_PROGRAMS_NEED_X = $(filter-out scalar,$(TEST_BINDIR_PROGRAMS_NEED_X))
bin-wrappers/%: wrap-for-bin.sh
$(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)))|' < $< > $@ && \
+ -e 's|@@PROG@@|$(patsubst test-%,t/helper/test-%$(X),$(@F))$(patsubst git%,$(X),$(filter $(@F),$(GIT_TEST_BINDIR_PROGRAMS_NEED_X)))|' < $< > $@ && \
chmod +x $@
# GNU make supports exporting all variables by "export" without parameters.
@@ -3281,11 +3303,18 @@ endif
install-gitweb:
$(MAKE) -C gitweb install
+# 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 =
+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'
@@ -3293,22 +3322,22 @@ install-man-perl: man-perl
(cd '$(DESTDIR_SQ)$(mandir_SQ)/man3' && umask 022 && $(TAR) xof -)
install-html:
- $(MAKE) -C Documentation install-html
+ $(MAKE) -C Documentation install-html NO_INSTALL_SCALAR_DOC='$(NO_INSTALL_SCALAR_DOC)'
install-info:
- $(MAKE) -C Documentation install-info
+ $(MAKE) -C Documentation install-info NO_INSTALL_SCALAR_DOC='$(NO_INSTALL_SCALAR_DOC)'
install-pdf:
- $(MAKE) -C Documentation install-pdf
+ $(MAKE) -C Documentation install-pdf NO_INSTALL_SCALAR_DOC='$(NO_INSTALL_SCALAR_DOC)'
quick-install-doc:
- $(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)'
@@ -3497,7 +3526,7 @@ check-docs::
-e 's/\.txt//'; \
) | while read how cmd; \
do \
- case " $(patsubst %$X,%,$(ALL_COMMANDS) $(BUILT_INS) $(EXCLUDED_PROGRAMS)) " in \
+ case " $(patsubst %$X,%,$(ALL_COMMANDS) $(BUILT_INS) $(EXCLUDED_PROGRAMS) scalar) " in \
*" $$cmd "*) ;; \
*) echo "removed but $$how: $$cmd" ;; \
esac; \
diff --git a/command-list.txt b/command-list.txt
index 9bd6f3c48f4..721f968b3d1 100644
--- a/command-list.txt
+++ b/command-list.txt
@@ -16,6 +16,7 @@
# synchingrepositories
# synchelpers
# purehelpers
+# optionalcontrib
#
# 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
@@ -215,3 +216,4 @@ gittutorial guide
gittutorial-2 guide
gitweb ancillaryinterrogators
gitworkflows guide
+scalar optionalcontrib
diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt
index 1b23f2440d8..3de83f7f454 100644
--- a/contrib/buildsystems/CMakeLists.txt
+++ b/contrib/buildsystems/CMakeLists.txt
@@ -610,7 +610,8 @@ 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)
@@ -745,6 +746,9 @@ 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)
@@ -902,10 +906,16 @@ 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)
@@ -977,7 +987,8 @@ 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)
diff --git a/contrib/scalar/.gitignore b/contrib/scalar/.gitignore
deleted file mode 100644
index ff3d47e84d0..00000000000
--- a/contrib/scalar/.gitignore
+++ /dev/null
@@ -1,2 +0,0 @@
-/*.exe
-/scalar
diff --git a/contrib/scalar/Makefile b/contrib/scalar/Makefile
deleted file mode 100644
index 37f283f35d7..00000000000
--- a/contrib/scalar/Makefile
+++ /dev/null
@@ -1,35 +0,0 @@
-# The default target of this Makefile is...
-all::
-
-# Import tree-wide shared Makefile behavior and libraries
-include ../../shared.mak
-
-include ../../config.mak.uname
--include ../../config.mak.autogen
--include ../../config.mak
-
-TARGETS = scalar$(X) scalar.o
-GITLIBS = ../../common-main.o ../../libgit.a ../../xdiff/lib.a
-
-all:: scalar$(X) ../../bin-wrappers/scalar
-
-$(GITLIBS):
- $(QUIET_SUBDIR0)../.. $(QUIET_SUBDIR1) $(subst ../../,,$@)
-
-$(TARGETS): $(GITLIBS) scalar.c
- $(QUIET_SUBDIR0)../.. $(QUIET_SUBDIR1) $(patsubst %,contrib/scalar/%,$@)
-
-clean:
- $(RM) $(TARGETS) ../../bin-wrappers/scalar
-
-../../bin-wrappers/scalar: ../../wrap-for-bin.sh Makefile
- @mkdir -p ../../bin-wrappers
- $(QUIET_GEN)sed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \
- -e 's|@@BUILD_DIR@@|$(shell cd ../.. && pwd)|' \
- -e 's|@@PROG@@|contrib/scalar/scalar$(X)|' < $< > $@ && \
- chmod +x $@
-
-test: all
- $(MAKE) -C t
-
-.PHONY: $(GITLIBS) all clean test FORCE
diff --git a/contrib/scalar/t/Makefile b/contrib/scalar/t/Makefile
deleted file mode 100644
index 01e82e56d15..00000000000
--- a/contrib/scalar/t/Makefile
+++ /dev/null
@@ -1,81 +0,0 @@
-# Import tree-wide shared Makefile behavior and libraries
-include ../../../shared.mak
-
-# Run scalar tests
-#
-# Copyright (c) 2005,2021 Junio C Hamano, Johannes Schindelin
-#
-
--include ../../../config.mak.autogen
--include ../../../config.mak
-
-SHELL_PATH ?= $(SHELL)
-PERL_PATH ?= /usr/bin/perl
-RM ?= rm -f
-PROVE ?= prove
-DEFAULT_TEST_TARGET ?= test
-TEST_LINT ?= test-lint
-
-ifdef TEST_OUTPUT_DIRECTORY
-TEST_RESULTS_DIRECTORY = $(TEST_OUTPUT_DIRECTORY)/test-results
-else
-TEST_RESULTS_DIRECTORY = ../../../t/test-results
-endif
-
-# Shell quote;
-SHELL_PATH_SQ = $(subst ','\'',$(SHELL_PATH))
-PERL_PATH_SQ = $(subst ','\'',$(PERL_PATH))
-TEST_RESULTS_DIRECTORY_SQ = $(subst ','\'',$(TEST_RESULTS_DIRECTORY))
-
-T = $(sort $(wildcard t[0-9][0-9][0-9][0-9]-*.sh))
-
-all: $(DEFAULT_TEST_TARGET)
-
-test: $(TEST_LINT)
- $(MAKE) aggregate-results-and-cleanup
-
-prove: $(TEST_LINT)
- @echo "*** prove ***"; GIT_CONFIG=.git/config $(PROVE) --exec '$(SHELL_PATH_SQ)' $(GIT_PROVE_OPTS) $(T) :: $(GIT_TEST_OPTS)
- $(MAKE) clean-except-prove-cache
-
-$(T):
- @echo "*** $@ ***"; GIT_CONFIG=.git/config '$(SHELL_PATH_SQ)' $@ $(GIT_TEST_OPTS)
-
-clean-except-prove-cache:
- $(RM) -r 'trash directory'.* '$(TEST_RESULTS_DIRECTORY_SQ)'
- $(RM) -r valgrind/bin
-
-clean: clean-except-prove-cache
- $(RM) .prove
-
-test-lint: test-lint-duplicates test-lint-executable test-lint-shell-syntax
-
-test-lint-duplicates:
- @dups=`echo $(T) | tr ' ' '\n' | sed 's/-.*//' | sort | uniq -d` && \
- test -z "$$dups" || { \
- echo >&2 "duplicate test numbers:" $$dups; exit 1; }
-
-test-lint-executable:
- @bad=`for i in $(T); do test -x "$$i" || echo $$i; done` && \
- test -z "$$bad" || { \
- echo >&2 "non-executable tests:" $$bad; exit 1; }
-
-test-lint-shell-syntax:
- @'$(PERL_PATH_SQ)' ../../../t/check-non-portable-shell.pl $(T)
-
-aggregate-results-and-cleanup: $(T)
- $(MAKE) aggregate-results
- $(MAKE) clean
-
-aggregate-results:
- for f in '$(TEST_RESULTS_DIRECTORY_SQ)'/t*-*.counts; do \
- echo "$$f"; \
- done | '$(SHELL_PATH_SQ)' ../../../t/aggregate-results.sh
-
-valgrind:
- $(MAKE) GIT_TEST_OPTS="$(GIT_TEST_OPTS) --valgrind"
-
-test-results:
- mkdir -p test-results
-
-.PHONY: $(T) aggregate-results clean valgrind
diff --git a/contrib/scalar/scalar.c b/scalar.c
similarity index 100%
rename from contrib/scalar/scalar.c
rename to scalar.c
diff --git a/contrib/scalar/t/t9099-scalar.sh b/t/t7990-scalar.sh
similarity index 96%
rename from contrib/scalar/t/t9099-scalar.sh
rename to t/t7990-scalar.sh
index 10b1172a8aa..897c8e760ef 100755
--- a/contrib/scalar/t/t9099-scalar.sh
+++ b/t/t7990-scalar.sh
@@ -2,13 +2,7 @@
test_description='test the `scalar` command'
-TEST_DIRECTORY=$PWD/../../../t
-export TEST_DIRECTORY
-
-# Make it work with --no-bin-wrappers
-PATH=$PWD/..:$PATH
-
-. ../../../t/test-lib.sh
+. ./test-lib.sh
GIT_TEST_MAINT_SCHEDULER="crontab:test-tool crontab ../cron.txt,launchctl:true,schtasks:true"
export GIT_TEST_MAINT_SCHEDULER
--
2.36.1.1239.gfba91521d90
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH v2 0/1] scalar: move to the top-level, test, CI and "install" support
2022-06-23 10:26 ` [PATCH v2 0/1] scalar: move to the top-level, test, CI and "install" support Ævar Arnfjörð Bjarmason
2022-06-23 10:26 ` [PATCH v2 1/1] scalar: reorganize from contrib/, still keep it "a contrib command" Ævar Arnfjörð Bjarmason
@ 2022-06-23 15:02 ` Derrick Stolee
2022-06-23 15:30 ` Ævar Arnfjörð Bjarmason
1 sibling, 1 reply; 16+ messages in thread
From: Derrick Stolee @ 2022-06-23 15:02 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason, git
Cc: Junio C Hamano, Johannes Schindelin, Victoria Dye
On 6/23/2022 6:26 AM, Ævar Arnfjörð Bjarmason wrote:
> 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.
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 0/1] scalar: move to the top-level, test, CI and "install" support
2022-06-23 15:02 ` [PATCH v2 0/1] scalar: move to the top-level, test, CI and "install" support Derrick Stolee
@ 2022-06-23 15:30 ` Ævar Arnfjörð Bjarmason
2022-06-23 16:12 ` Derrick Stolee
0 siblings, 1 reply; 16+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-06-23 15:30 UTC (permalink / raw)
To: Derrick Stolee
Cc: git, Junio C Hamano, Johannes Schindelin, Victoria Dye, Jiang Xin
On Thu, Jun 23 2022, Derrick Stolee wrote:
> On 6/23/2022 6:26 AM, Ævar Arnfjörð Bjarmason wrote:
>> 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.
>
> 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.
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.
> 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.
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.
> 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.
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
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH v2 0/1] scalar: move to the top-level, test, CI and "install" support
2022-06-23 15:30 ` Ævar Arnfjörð Bjarmason
@ 2022-06-23 16:12 ` Derrick Stolee
2022-06-24 11:59 ` Ævar Arnfjörð Bjarmason
0 siblings, 1 reply; 16+ messages in thread
From: Derrick Stolee @ 2022-06-23 16:12 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: git, Junio C Hamano, Johannes Schindelin, Victoria Dye, Jiang Xin
On 6/23/2022 11:30 AM, Ævar Arnfjörð Bjarmason wrote:
>
> On Thu, Jun 23 2022, Derrick Stolee wrote:
>
>> On 6/23/2022 6:26 AM, Ævar Arnfjörð Bjarmason wrote:
>>> 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.
>>
>> 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.
>
> 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.
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.
>> 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.
>
> 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.
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.
> FWIW I have this local change queued on top of this v2, it's all
> cosmetic, but probably a good idea.
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.
> The $(SCALAR_SOURCES) bit is something I missed, but which Victoria
> didn't in her WIP patch (I stole it from there).
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 0/1] scalar: move to the top-level, test, CI and "install" support
2022-06-23 16:12 ` Derrick Stolee
@ 2022-06-24 11:59 ` Ævar Arnfjörð Bjarmason
2022-06-24 21:53 ` Junio C Hamano
0 siblings, 1 reply; 16+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-06-24 11:59 UTC (permalink / raw)
To: Derrick Stolee
Cc: git, Junio C Hamano, Johannes Schindelin, Victoria Dye, Jiang Xin
On Thu, Jun 23 2022, Derrick Stolee wrote:
> On 6/23/2022 11:30 AM, Ævar Arnfjörð Bjarmason wrote:
>>
>> On Thu, Jun 23 2022, Derrick Stolee wrote:
>>
>>> On 6/23/2022 6:26 AM, Ævar Arnfjörð Bjarmason wrote:
>>>> 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.
>>>
>>> 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.
>>
>> 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.
>
> 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].
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.
> [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.
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.
>>> 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.
>>
>> 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.
>
> 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.
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!.
>> FWIW I have this local change queued on top of this v2, it's all
>> cosmetic, but probably a good idea.
>
> 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.
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...
>> The $(SCALAR_SOURCES) bit is something I missed, but which Victoria
>> didn't in her WIP patch (I stole it from there).
>
> You also mention this in your cover letter (but is notably absent from
> the patch itself).
...yes, hence the "local change queued on top" above, it's a minor
detail I noticed after submitting the patch.
> I can only attribute your insistence here as a lack of respect for
> your fellow contributors. [...]
I really think that's unhelpful. Let's try to assume some mutual good
intent. I think I've explained my motivations above.
> [...]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.
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.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 0/1] scalar: move to the top-level, test, CI and "install" support
2022-06-24 11:59 ` Ævar Arnfjörð Bjarmason
@ 2022-06-24 21:53 ` Junio C Hamano
0 siblings, 0 replies; 16+ messages in thread
From: Junio C Hamano @ 2022-06-24 21:53 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: Derrick Stolee, git, Johannes Schindelin, Victoria Dye, Jiang Xin
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
> 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.
Careful ...
> 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!.
> ...
> Some of the edge cases in the Makefile integration are subtle. I trust
> that someone looking at it would probably discover those eventually.
... 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.
> 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.
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.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Discussion] The architecture of Scalar (and others) within Git
2021-10-27 21:51 [Discussion] The architecture of Scalar (and others) within Git Derrick Stolee
` (2 preceding siblings ...)
2021-10-28 18:56 ` [PATCH] contrib: build & test scalar by default, optionally install Ævar Arnfjörð Bjarmason
@ 2021-10-28 18:58 ` Ævar Arnfjörð Bjarmason
2021-11-04 17:20 ` Derrick Stolee
2021-11-01 23:20 ` Junio C Hamano
4 siblings, 1 reply; 16+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-10-28 18:58 UTC (permalink / raw)
To: Derrick Stolee
Cc: Git Mailing List, Johannes Schindelin, Junio C Hamano,
Eric Sunshine, Elijah Newren, Bagas Sanjaya,
Matthew John Cheetham, Victoria Dye, Theodore Ts'o
On Wed, Oct 27 2021, Derrick Stolee wrote:
> 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?
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.
> 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!)
I think what I'd like to add is...[continued below]...
> **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.
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.
> [...]
> **Option 2:** Loose coupling in a new scalar/ directory
> [...]
> **Option 3:** Tight coupling with entire Git project
> [...]
...[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:
> [...]
> **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.
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.
> Having Scalar in `contrib/` makes it easy to differentiate it as an
> optional component that distributors could choose to include or leave out.
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.
> 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?
We're not. We can just document the status of individual logical
components, separate from in-tree FS paths.
> 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.).[...]
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.
> 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.
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.
> **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.
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.
> 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`.
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.
> [...]
> Developer friction increases as changes to `libgit.a` that break Scalar
> should be fixed within the patches that cause those changes.
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.
> [...]
> **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.
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.
> This option sets a risky precedent that new tools can be added to Git
> without a rigorous review of the CLI.
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).
> **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.
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?
> [...]
> 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.
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.
> [...]
> Thanks for your attention to this topic. I look forward to any new "big"
> ideas in this space.
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/
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [Discussion] The architecture of Scalar (and others) within Git
2021-10-28 18:58 ` [Discussion] The architecture of Scalar (and others) within Git Ævar Arnfjörð Bjarmason
@ 2021-11-04 17:20 ` Derrick Stolee
0 siblings, 0 replies; 16+ messages in thread
From: Derrick Stolee @ 2021-11-04 17:20 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: Git Mailing List, Johannes Schindelin, Junio C Hamano,
Eric Sunshine, Elijah Newren, Bagas Sanjaya,
Matthew John Cheetham, Victoria Dye, Theodore Ts'o
On 10/28/2021 2:58 PM, Ævar Arnfjörð Bjarmason wrote:
>
> On Wed, Oct 27 2021, Derrick Stolee wrote:
>
>> 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?
Thank you for your thoughts on this topic. I wanted to specifically
reply to a few items while the discussion continues to simmer.
> 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.
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.
>> [...]
>> **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.
>
> 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.
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Discussion] The architecture of Scalar (and others) within Git
2021-10-27 21:51 [Discussion] The architecture of Scalar (and others) within Git Derrick Stolee
` (3 preceding siblings ...)
2021-10-28 18:58 ` [Discussion] The architecture of Scalar (and others) within Git Ævar Arnfjörð Bjarmason
@ 2021-11-01 23:20 ` Junio C Hamano
2021-11-04 10:25 ` Johannes Schindelin
4 siblings, 1 reply; 16+ messages in thread
From: Junio C Hamano @ 2021-11-01 23:20 UTC (permalink / raw)
To: Derrick Stolee
Cc: Git Mailing List, Johannes Schindelin,
Ævar Arnfjörð Bjarmason, Eric Sunshine,
Elijah Newren, Bagas Sanjaya, Matthew John Cheetham, Victoria Dye,
Theodore Ts'o
Derrick Stolee <stolee@gmail.com> writes:
> 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.
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.
> Possible approaches to land in our final target
> -----------------------------------------------
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.
> **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.
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.
> 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?
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.
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [Discussion] The architecture of Scalar (and others) within Git
2021-11-01 23:20 ` Junio C Hamano
@ 2021-11-04 10:25 ` Johannes Schindelin
2021-11-05 4:20 ` Junio C Hamano
0 siblings, 1 reply; 16+ messages in thread
From: Johannes Schindelin @ 2021-11-04 10:25 UTC (permalink / raw)
To: Junio C Hamano
Cc: Derrick Stolee, Git Mailing List,
Ævar Arnfjörð Bjarmason, Eric Sunshine,
Elijah Newren, Bagas Sanjaya, Matthew John Cheetham, Victoria Dye,
Theodore Ts'o
[-- Attachment #1: Type: text/plain, Size: 11783 bytes --]
Hi Junio,
On Mon, 1 Nov 2021, Junio C Hamano wrote:
> Derrick Stolee <stolee@gmail.com> writes:
>
> > 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.
>
> 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.
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.
> 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 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.
> 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).
That's a really good point.
> 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.
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.
> 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.
I am still of the opinion that having the code in the same tree will make
cross pollination much easier.
> > Possible approaches to land in our final target
> > -----------------------------------------------
>
> 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?
I think we are very much in alignment here, and I believe Stolee's goal
was to reach such an alignment.
> 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.
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.
> > **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.
>
> 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.
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.
> 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.
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?
> > 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?
>
> I am afraid that the answer to the first four are "it depends" (but the
> remaining two would fall out naturally out of them).
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.
> 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.
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
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [Discussion] The architecture of Scalar (and others) within Git
2021-11-04 10:25 ` Johannes Schindelin
@ 2021-11-05 4:20 ` Junio C Hamano
0 siblings, 0 replies; 16+ messages in thread
From: Junio C Hamano @ 2021-11-05 4:20 UTC (permalink / raw)
To: Johannes Schindelin
Cc: Derrick Stolee, Git Mailing List,
Ævar Arnfjörð Bjarmason, Eric Sunshine,
Elijah Newren, Bagas Sanjaya, Matthew John Cheetham, Victoria Dye,
Theodore Ts'o
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 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?
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.
> 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.
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 ;-)
^ permalink raw reply [flat|nested] 16+ messages in thread