The discussion happening on "Upstreaming the Scalar command" [1] has
gotten pretty deep into some details about builds and code organization.
While these are important things to discuss, I’d like to step back and
reframe the discussion to focus on how components like Scalar fit into the
high-level architecture of Git. The decisions we make for Scalar also
affect other components that might show up in the future. It could also
clarify some of the components already in Git, both in and out of the
`contrib/` directory.
[1] https://lore.kernel.org/git/pull.1005.git.1630359290.gitgitgadget@gmail.com/
I'm starting this discussion thread to create a new area to consider these
high-level concepts. Specifically: how should a new component like Scalar
be included in the Git codebase?
Johannes put details in his cover letter about why we put it in
`contrib/scalar/`, but I feel like code review proceeded at first as if
that was a foregone conclusion. Then, discussion shifted to some
alternatives, but the reasons were focused on code size and specific
tweaks to the `Makefile`. I think there are other concerns for the entire
Git community that need to be included in the calculus. In particular,
the decisions we make today will become precedents for similar
contributions in the future.
I'll start by setting up some goals and guidelines for the decision
process, then get into some concrete options before comparing them based
on these guidelines. I also try to separate the "final" goal from "how do
we get there" in the last section.
All of what I say is up for debate. I would love for others to contribute
options that I missed. I will send a reply to this message that summarizes
all options that are recommended in this thread, along with some attempt
to summarize any consensus that we might be approaching.
Goals
-----
Our main goal is to identify a community-supported plan for upstreaming the
Scalar CLI.
This comes in two parts:
1. What is the preferred final organization of the Scalar code within
the Git codebase?
2. What is a realistic approach to contributing the complete Scalar CLI
with incrementally-reviewable pieces, in order to end up with the
preferred final organization?
Optimization Factors
--------------------
There are multiple factors involved that we wish to optimize. Here is my
understanding of the most-important factors and how they interact:
### Value
The main reason to include Scalar within the Git codebase is to make it
available to more users. This should have downstream effects based on user
feedback about what they like or dislike about the Scalar CLI that can
inform future features in core Git that simplify repository management at
scale.
### CLI compatibility
The Scalar CLI was developed externally, which informed the choice of
commands and arguments. We prefer to maintain the existing CLI design to
support our existing customers without disruption.
After the existing CLI is completely contributed, we can consider making
changes to the CLI by deprecating certain options and replacing them with
synonyms. (For example, my personal regret is that `--full-clone` really
means `--no-sparse`.)
### Developer friction
Once the Scalar CLI is in the Git codebase, it becomes something that can
disrupt Git developers as they modify Git code, especially within libgit.a.
What is the right balance of making developers responsible for breaking
the Scalar CLI as they make changes to Git?
### Git Maintainer burden
Similar to developer friction, how much does having Scalar in the Git
codebase affect the Git maintainer? Does the Git maintainer become
responsible for the maintenance of Scalar? Do contributions to Scalar need
the same scrutiny as other contributions? Does the maintainer need to
inform contributors when they break Scalar?
### Scalar Maintainer(s) responsibility
At the very last, what are the roles of "the Scalar maintainer(s)"? Here,
I'm thinking of Johannes and myself as the accountable parties. An example
of a responsibility here could be that around release time we validate
that Scalar is still working with the changes that happened during that
release cycle.
The biggest reason to include the Scalar maintainers' responsibility is
that the options listed below present a converse relationship between
these responsibilities and those of contributors and the Git maintainer.
The more responsibility that the Git community is willing to accept, the
less responsibility on the Scalar maintainers. We are _not_ optimizing to
minimize our responsibility, but some options change the balance of
responsibility.
Options for preferred end state
-------------------------------
Let's get into some concrete proposals for the location of the Scalar CLI
within the Git codebase. These are ordered based on increasing
responsibility on the Git community: the first option should minimize
community responsibility and maximize responsibility on the Scalar
maintainers. We will discuss the pros and cons of each option after fully
describing each of them.
(Please add any options that I may have missed!)
**Option 1:** One-directional coupling in `contrib/scalar/`
The Scalar CLI exists entirely within `contrib/scalar/`, including all code
and test infrastructure. To opt-in to building and testing Scalar, run
`make -C contrib/scalar test` to build Git and Scalar and run the Scalar
test scripts.
**Option 2:** Loose coupling in a new scalar/ directory
Similar to `git-gui` and `gitk-git`, Scalar could exist outside of `contrib/`
and in its own `scalar/` directory. Its code is isolated to this directory,
but its tests live in `t/` and its documentation lives in `Documentation/`.
The root `Makefile` has special logic to avoid building and testing Scalar
unless an `INCLUDE_SCALAR` option is enabled. The `DEVELOPER=1` option
enables `INCLUDE_SCALAR` by default. Code in `git.c` is generalized so
`scalar.c` could take advantage of features such as `-c`, `-C`, and
`--exec-path` options.
**Option 3:** Tight coupling with entire Git project
Scalar is included at the root as `scalar.c` and is compiled at the same
priority as the `git` executable. Tests and documentation have the same
priority as other Git features. Code in `git.c` is generalized so
`scalar.c` could take advantage of features such as `-c`, `-C`, and
`--exec-path` options.
Note: there are likely more subtle gradients between these options, but
Option 1 and Option 3 are intended as extreme ends of a spectrum. Option
2 is a potential middle ground, but a lot of the details about it could be
altered.
Benefits and drawbacks of these options
---------------------------------------
Here are some pros and cons for each approach. Please reply with additional
items to add to the list, or to debate the correctness of each item. I'll
try to summarize these contributions in a reply to this message after the
discussion stabilizes.
**Option 1:** One-directional coupling in `contrib/scalar/`
This option was our initial choice because it minimizes the responsibility
of the Git community. While `contrib/scalar/scalar.c` depends on code in
`libgit.a`, the implementation does not require that code to change to
accommodate the needs of Scalar. The test suite and documentation is
separate.
This does mean that changes to `libgit.a` could break Scalar without any
feedback to the developer that has not compiled Scalar. The Scalar
maintainers would then need to watch for this and send separate updates to
Scalar that fix these dependency breaks. This reduces developer friction,
but might cause some extra burden on the Git maintainer. If these "catch
up to dependency breaks" updates happen only after a release candidate is
out, then maybe this isn't too much of a burden. We don't typically have
release candidates for minor releases, so there is some risk there that
minor releases could include breaks.
If we make our CI builds include Scalar by default, then the previous
paragraph about developer friction is negated.
Having Scalar in `contrib/` makes it easy to differentiate it as an
optional component that distributors could choose to include or leave out.
Code in `contrib/` has a lower barrier to entry. In particular, the Scalar
CLI is not intended to be up for debate for historical reasons. If we make
an exception for Scalar but want Scalar integrated outside of `contrib/`,
then are we setting expectations for other tools that might want to be
included?
However, projects within `contrib/` do not currently depend on `libgit.a`
the way Scalar does. (This is not historically true, as an older project
ad such dependencies, but has since been ejected.) This leads to questions
of whether or not this is a desirable pattern to follow. It adds to
complexity since changes to the core Git codebase can break something in
`contrib/` at compile time.
By keeping the code dependency one-directional, some nice-to-have features
can be duplicated within `contrib/scalar/` to avoid disrupting their
implementation within the core Git codebase.
This option explicitly mentions that all knowledge of how to build Scalar
lives within `contrib/scalar/` to avoid disrupting the core `Makefile`.
This has already led to debate about some duplication in the Scalar
`Makefile` and the one at root. Some functionality might not be critical
to include in both places. One knob that we can use to adjust this option
is to be more flexible with the root `Makefile` knowing about code within
`contrib/scalar`. This violates existing patterns except for how the
`make coccicheck` target consumes scripts from `contrib/coccinelle` to do
checks on the Git source code.
**Option 2:** Loose coupling in a `scalar/` directory
The first issue with this option is that the Scalar CLI is established and
is not up for modification, yet we would be contributing it in a location
that is typically under high scrutiny for things like this.
This option also breaks into new territory, because even `git-gui` and
`gitk-git` are not based on C and do not depend on `libgit.a`.
There is some risk that as we refactor some things in the Git build system
and codebase to allow Scalar to plug in more tightly that we accidentally
break something.
Developer friction increases as changes to `libgit.a` that break Scalar
should be fixed within the patches that cause those changes. This means
that changes to the Scalar project can happen in otherwise-unrelated
topics, increasing the attention of both Git and Scalar maintainers during
the full release cycle. However, it does reduce risk that Scalar could be
broken for a significant chunk of time.
This approach reduces code duplication.
Not only does the code need to be compiled optionally, we need to make
sure that tests only run if Scalar was compiled. We also want to make sure
that documentation is compiled with the same options. This could affect
things like git-scm.com which hosts the Git documentation: should it
include the Scalar docs if it isn't shipped by default? How can we make
sure that these docs don't accidentally appear there? (Or, should we
make sure the docs clearly state that `scalar` might not be available?)
**Option 3:** Tight coupling with the entire Git project
This option removes the ability to opt-out of building the Scalar CLI.
Distributors might need to react to remove the `scalar` executable if they
do not want to include it.
This option relies on significant buy-in from the Git community.
This option minimizes code duplication and has the simplest build system.
This option sets a risky precedent that new tools can be added to Git
without a rigorous review of the CLI.
Possible approaches to land in our final target
-----------------------------------------------
If we agree on option 1 (`contrib/scalar/`), then this discussion is moot,
because there is no strategy other than to contribute into `contrib/` from
the start.
If we agree on another option, then we could still continue contributing
Scalar into `contrib/`, following the currently-proposed organization. There
might be benefits to a gradual approach to reaching that target. Here is a
possible multi-stage approach to land in our final approach:
**Phase 1.** Keep code in contrib/ while we contribute the Scalar features.
Since the current patches on the mailing list are not feature complete, it
can be beneficial to move forward with the code in `contrib/scalar/` until
we reach feature parity. At that point, we could move the source into its
final resting place.
This leans into the fact that `contrib/` contains "interesting tools...
maybe even experimental ones". While Scalar is not feature complete, it
can be isolated as experimental here until it is ready for promotion to
non-experimental.
Depending on the final goal, we could drop some of the work that is currently
built within the `contrib/scalar/` directory, such as `-c`/`-C` parsing or
documentation builds. These features would be reimplemented in the new
location, so we can prevent that duplicate effort if we have a different
final location in mind.
**Phase 2.** Establish community standards on optional components
While the work in `contrib/scalar/` continues to reach feature parity, we
can work as a community to set some ground rules about these kinds of
optional features (depending on how "optional" we land). This can include
standards such as how the files are laid out in the repository and how
they interact with the `Makefile`. Whatever we do for Scalar is likely to
form an example for a future contribution that we can't predict today.
Many of the questions we are asking today can be written for posterity:
* What is an appropriate level of coupling with the Git codebase?
* Where should code custom to the component live?
* How should documentation and tests be organized?
* How do we initialize builds of a component?
* Who is responsible for supporting the component? Who fixes the bugs?
* Who is responsible for reviewing changes to the component?
**Phase 3.** Translate the Scalar code from `contrib/scalar/` to new home
After feature parity is reached _and_ we have established some ground rules
for how these kinds of components should fit within the Git codebase, we
can start to translate Scalar into its new home. This should include some
file renames along with some modifications to fit into the build. The goal
of this phase is to establish the new build environment and how Scalar fits
within the CI builds. Once this is complete, whatever responsibility was
agreed upon by the community to keep Scalar working goes into effect.
**Phase 4.** Integrate features that were delayed until final home
After the code, docs, and tests have been moved, we can start the work of
adding the features that might have been delayed until the final home. The
main example on my mind is the `-c`/`-C`/`--exec-path` parsing, which
should be its own series. Features that fit into this category include
anything that requires modifying existing code outside of `contrib/` to be
consumed by Scalar.
Call to action
--------------
Thanks for your attention to this topic. I look forward to any new "big"
ideas in this space.
Thanks,
-Stolee
From: Johannes Schindelin <hidden> Date: 2021-10-28 06:56:22
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
From: Philip Oakley <hidden> Date: 2021-10-28 15:29:42
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
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 <redacted>
---
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%)
@@ -18,6 +18,9 @@ MAN1_TXT += $(filter-out \MAN1_TXT+=git.txtMAN1_TXT+=gitk.txtMAN1_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
@@ -10,7 +10,7 @@ sub format_one {$state=0;openI,'<',"$name.txt"ordie"No such file $name.txt";while(<I>){-if(/^git[a-z0-9-]*\(([0-9])\)$/){+if(/^[a-z0-9-]*\(([0-9])\)$/){$mansection=$1;next;}
@@ -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.txtsimilarity index 99%rename from contrib/scalar/scalar.txtrename to Documentation/scalar.txtindex 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
@@ -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/`
@@ -784,7 +791,8 @@ BUILT_INS += git-switch$XBUILT_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 bindirBINDIR_PROGRAMS_NEED_X+=git
@@ -793,6 +801,18 @@ BINDIR_PROGRAMS_NEED_X += git-shellBINDIR_PROGRAMS_NEED_X+=git-upload-archiveBINDIR_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.
@@ -2870,15 +2891,18 @@ GIT-PYTHON-VARS: FORCEfiendif-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-outscalar,$(TEST_BINDIR_PROGRAMS_NEED_X))bin-wrappers/%:wrap-for-bin.sh@mkdir-pbin-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.
@@ -3183,8 +3208,19 @@ ifneq ($(INCLUDE_DLLS_IN_ARTIFACTS),)OTHER_PROGRAMS+=$(shellecho*.dllt/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-outscalar%,$(OTHER_PROGRAMS))+ARCHIVE_TEST_BINDIR_PROGRAMS=$(filter-outbin-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)'
@@ -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
diff --git a/contrib/scalar/scalar.c b/scalar.csimilarity index 100%rename from contrib/scalar/scalar.crename to scalar.cdiff --git a/contrib/scalar/t/t9099-scalar.sh b/t/t9099-scalar.shsimilarity index 72%rename from contrib/scalar/t/t9099-scalar.shrename to t/t9099-scalar.shindex 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-exportTEST_DIRECTORY+../test-lib.sh-# Make it work with --no-bin-wrappers-PATH=$PWD/..:$PATH--.../../../t/test-lib.sh+iftest_have_prereqWINDOWS&&!scalarlist2>/dev/null+then+skip_all='the contrib cmake process does not build "scalar" yet'+test_done+fiGIT_TEST_MAINT_SCHEDULER="crontab:test-tool crontab ../cron.txt"exportGIT_TEST_MAINT_SCHEDULER
@@ -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_commitfirst&&-test_commitsecond&&-test_committhird&&-gitswitch-cparallelfirst&&+test_expect_successSCALAR_REGISTER'set up repository to clone''mkdir-p1/2&&test_commit1/2/3&&gitconfiguploadPack.allowFiltertrue&&gitconfiguploadPack.allowAnySHA1InWanttrue'-test_expect_success'scalar clone''+test_expect_successSCALAR_REGISTER'scalar clone''second=$(gitrev-parse--verifysecond:second.t)&&scalarclone"file://$(pwd)"cloned--single-branch&&(
@@ -80,7 +92,7 @@ test_expect_success 'scalar delete without enlistment shows a usage' 'test_expect_code129scalardelete'-test_expect_success'scalar delete with enlistment''+test_expect_successSCALAR_REGISTER'scalar delete with enlistment''scalardeletecloned&&test_path_is_missingcloned'
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/
On 10/28/2021 2:58 PM, Ævar Arnfjörð Bjarmason wrote:
On Wed, Oct 27 2021, Derrick Stolee wrote:
quoted
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.
quoted
[...]
**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
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 <redacted>
---
.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%)
@@ -21,6 +21,9 @@ MAN1_TXT += $(filter-out \MAN1_TXT+=git.txtMAN1_TXT+=gitk.txtMAN1_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
@@ -10,7 +10,7 @@ sub format_one {$state=0;openI,'<',"$name.txt"ordie"No such file $name.txt";while(<I>){-if(/^git[a-z0-9-]*\(([0-9])\)$/){+if(/^[a-z0-9-]*\(([0-9])\)$/){$mansection=$1;next;}
@@ -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.txtsimilarity index 99%rename from contrib/scalar/scalar.txtrename to Documentation/scalar.txtindex 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
@@ -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/`
@@ -812,7 +818,8 @@ BUILT_INS += git-switch$XBUILT_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 bindirBINDIR_PROGRAMS_NEED_X+=git
@@ -821,6 +828,18 @@ BINDIR_PROGRAMS_NEED_X += git-shellBINDIR_PROGRAMS_NEED_X+=git-upload-archiveBINDIR_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.
@@ -3041,15 +3060,18 @@ GIT-PYTHON-VARS: FORCEfiendif-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-outscalar,$(TEST_BINDIR_PROGRAMS_NEED_X))bin-wrappers/%:wrap-for-bin.sh$(callmkdir_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 @@ endifinstall-gitweb:$(MAKE)-Cgitwebinstall+# 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)-CDocumentationinstall+$(MAKE)-CDocumentationinstallNO_INSTALL_SCALAR_DOC='$(NO_INSTALL_SCALAR_DOC)'install-man:install-man-perl-$(MAKE)-CDocumentationinstall-man+$(MAKE)-CDocumentationinstall-manNO_INSTALL_SCALAR_DOC='$(NO_INSTALL_SCALAR_DOC)'install-man-perl:man-perl$(INSTALL)-d-m755'$(DESTDIR_SQ)$(mandir_SQ)/man3'
@@ -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
diff --git a/contrib/scalar/scalar.c b/scalar.csimilarity index 100%rename from contrib/scalar/scalar.crename to scalar.cdiff --git a/contrib/scalar/t/t9099-scalar.sh b/t/t7990-scalar.shsimilarity index 96%rename from contrib/scalar/t/t9099-scalar.shrename to t/t7990-scalar.shindex 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-exportTEST_DIRECTORY--# Make it work with --no-bin-wrappers-PATH=$PWD/..:$PATH--.../../../t/test-lib.sh+../test-lib.shGIT_TEST_MAINT_SCHEDULER="crontab:test-tool crontab ../cron.txt,launchctl:true,schtasks:true"exportGIT_TEST_MAINT_SCHEDULER
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 [off-list ref]
## 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 [off-list ref]
@@ 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
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
On 6/23/2022 6:26 AM, Ævar Arnfjörð Bjarmason wrote:
quoted
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
On 6/23/2022 11:30 AM, Ævar Arnfjörð Bjarmason wrote:
On Thu, Jun 23 2022, Derrick Stolee wrote:
quoted
On 6/23/2022 6:26 AM, Ævar Arnfjörð Bjarmason wrote:
quoted
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.
quoted
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
On 6/23/2022 11:30 AM, Ævar Arnfjörð Bjarmason wrote:
quoted
On Thu, Jun 23 2022, Derrick Stolee wrote:
quoted
On 6/23/2022 6:26 AM, Ævar Arnfjörð Bjarmason wrote:
quoted
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.
quoted
quoted
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!.
quoted
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...
quoted
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.