This adds the proptest crate for property-based testing as well as the
proptest-state-machine crate as direct dev dependencies of jj-cli and as
dependencies of the internal testutils crate.
Within testutils, a `proptest` module provides a reference state
machine which models the working copy as a map from path to `DirEntry`.
Directories are not represented explicitly, but are implicit in the
ancestors of entries.
The possible transitions of this state machine are for now limited to
the creation of new files (including replacements of existing files
or directories) and a `Commit` operation which the SUT can use to
snapshot a reference state. Additional transitions (moving files,
modifying file contents incrementally, ...) and states (symlinks,
submodules, conflicts, ...) may be added in the future.
This reference state machine is then applied to the builtin merge-tool's
test suite:
- The initial state is always an empty root directory.
- The `Commit` operation creates `MergedTree` from the current state.
- Each step of the way, the same test logic as in the manual
`test_edit_diff_builtin*` tests is run to check that splitting off
none or all of the changes results in the left or right tree,
respectively. The "right" tree corresponds to the current state,
whereas the "left" tree refers to the last "committed" tree.
Co-authored-by: Waleed Khan <me@waleedkhan.name>
The `Backend::read_file()` method is async but it returns a `Box<dyn
Read>` and reading from that trait is blocking. That's fine with the
local Git backend but it can be slow for remote backends. For example,
our backend at Google reads file chunks 1 MiB at a time from the
server. What that means is that reading lots of small files
concurrently works fine since the whole file contents are returned by
the first `Read::read()` call (it was fetched when
`Backend::read_file()` was issued). However, when reading files that
are larger than one chunk, we end up blocking on the next
`Read::read()` call. I haven't verified that this actually is a
problem at Google, but fixing this blocking is something we should do
eventually anyway.
This patch makes `Backend::read_file()` return a `Pin<Box<dyn
AsyncRead>>` instead, so implementations can be async in the read part
too.
Since `AsyncRead` is not yet standardized, we have to choose between
the one from `futures` and the one from `tokio`. I went with the one
from `tokio`. I picked that because an earlier version of this patch
used `tokio::fs` for some reads. Then I realized that doing that means
that we have to use a tokio runtime, meaning that we can't safely keep
our existing `pollster::FutureExt::block_on()` calls. If we start
depending on tokio's specific runtime, I think we would first want to
remove all the `block_on()` calls. I'll leave that for later. I think
at this point, we could equally well use `futures::io::AsyncRead`, but
I also don't know if there's a reason to prefer that.
The `datatest-stable` crate allows to dynamically instantiate test cases
based on available files. This is applied to `test_config_schema` to
create one test case per config file. As case in point, the test case
for `hints.toml` was missing previously, hence the total number of tests
is up one.
This will become useful when adding more config examples to somewhat
exhaust the schema.
`datatest-stable` uses a custom test harness and thus cannot be used in
the same integration test binary that all of the other test modules run
in. However, if data-driven tests are to be used for other applications,
they can share in the same binary, so the module structure is already
set up to mirror the central "runner" approach.
This uses `zlib-rs`, a native Rust library that is comparable in
performance to `zlib-ng`. Since there’s no complicated C build
and gitoxide only has one hashing backend now, this lets us drop our
`packaging` feature without adding any awkward build requirements.
`zlib-rs` is generally faster at decompression than
`zlib-ng`, and faster at compression on levels 6 and 9; see
<https://trifectatech.org/blog/zlib-rs-is-faster-than-c/>
for details.
I couldn’t get reliable‐looking benchmark results out of my
temperamental laptop; `hyperfine` seemed to think that some random
`jj` workloads I tested might be slightly slower than with `zlib-ng`,
but it wasn’t unambiguously distinguishable from noise, so I’d
like to see measurements from others.
It’s certainly a lot faster than the previous default, and I
think it’s likely that `zlib-rs` will continue to get faster
and that it’s more than worth avoiding the headaches of a native
library with a CMake build dependency. (Though on the other hand,
if distributions move in the direction of shipping `zlib-ng` by
default, maybe there will be more motivation to make `libz-ng-sys`
support system libraries.)
These helpers are going to be needed to port the git2 code in the lib
tests to gitoxide. Since the cli tests already depend on testutils, this
helps with avoiding duplicating the code
The WTFPL license is added to the allow list. I've never heard about this
license, but it's basically the same as public domain according to wikipedia.
There are some experiments to try and compile `jj` to WebAssembly, so that we
might be able to do things like interactive web tutorials. One step for that
is making `git` support in `jj-cli` optional, because we can stub it out for
something more appropriate and it's otherwise a lot of porting annoyance for
both gitoxide and libgit2.
(On top of that, it might be a useful build configuration for other experiments
of mine where removing the need for the large libgit2 depchain is useful.)
As part of this, we need to mark `jj-lib` as having `default-features = false`
in the workspace dependency configuration; otherwise, the default behavior
for Cargo is to compile with all its default features, i.e. with git support
enabled, ignoring the `jj-cli` features clauses.
Other than that, it is fairly straightforward junk; it largely just sprinkles
some `#[cfg]` around liberally in order to make things work. It also adjusts the
CI pipeline so this is tested there, too, so we can progressively clean it up.
Signed-off-by: Austin Seipp <aseipp@pobox.com>
Using `gix/max-performance` requires `cmake` as a build-time dependency, which could be a significant barrier for contributors (including existing ones, like me, who already work on jj but didn't have `cmake` installed thus far).
This commit switches back to using `gix/max-performance-safe`, which doesn't have the `cmake` dependency, and adds `gix/max-performance` behind the `gix-max-performance` feature for `jj-lib`.
It also adds `gix-max-performance` to the `packaging` feature group, since I'm assuming that packagers will want maximum performance, and are more likely to have `cmake` at hand.
Tested with
```
$ cargo build --workspace
$ cargo build --workspace --features packaging
```
(and the `--features packaging` build failed until I installed `cmake`)
If there are multiple files in a subdirectory that are candidates for
completion, only complete the common directory prefix to reduce the number of
completion candidates shown at once.
This matches the normal shell completion of file paths.
There are two known limitations right now:
- Only statically known keys are suggested.
- Keys that the user did not set are still suggested for `jj config get`.
Running that suggestion may result in an error. The error message will be
appropriate though and there is some value in letting the user know that
this config value theoretically exists. Some users may try to explore what
configurations are available via the completions.
There is now an updated version of `esl01-renderdag` published from
the Sapling repo, so let's use that. That lets us remove the
`bitflags` 1.x dependency and the `itertools` 0.10.x non-dev
dependency.
I think `cargo publish` will currently fail because of the
`include_str!()` in `cli/src/commands/help.rs` pointing to
`../../../docs/`, i.e. outside of the crate directory. This patch
attempts to fix that creating a `cli/docs` symlink to `docs` and makes
the `include_str!` use that symlink. I hope the symlink will be
resolved at `cargo publish` time so it also works in the published
crate.
Because symlinks don't work well on Windows, I updated `cli/build.rs`
to include the original path (the one pointing outside the crate) if
`cli/docs` is not a symlink, so the regular build still should work on
Windows (but `cargo publish` won't).
Thanks to Yuya for proposing this solution.
When building this project with [Nix/Crane](https://github.com/ipetkov/crane/discussions/693), if the `jj-cli` dependency is specified in `Cargo.toml` as a git-based crate, `cargo vendor` splits this workspace up into sub-crate directories, which causes `cargo metadata` to fail when searching for relative deps in the workspace root.
This commit simply changes how the crate version is determined, using Cargo's built-in environment variable [`CARGO_PKG_VERSION`](https://doc.rust-lang.org/cargo/reference/environment-variables.html)
Running `cargo publish` from a non-colocated repo (such as my usual
repo) is currently quite scary because it uploads all non-hidden
files, even if they're ignored by `.gitignore`
(https://github.com/rust-lang/cargo/issues/2063). I noticed this a
while ago and have always run the command from a fresh clone since
then. To avoid the need for that, let's use the workaround mentioned
on the bug, which is to explicitly list patterns we want to publish.
Apart from (IMO) looking nicer, this will also sidestep the potential problem
that if the file contains actual jj conflict markers (`>>>>>>>` in the beginning
of a line, for example), jj would currently have trouble materializing and
subsequently parsing conflicts in the file if it actually became conflicted.
I'll demo this bug in either this or a subsequent PR. It's the kind of bug that
sounds serious in theory but might never cause a problem in practice.
After this PR, only `docs/tutorial.md` has a conflict marker that's not indented.
There's only one there, so hopefully it won't be too much of a pain to deal with.
I also indented other strings in `test_conflicts.rs`. IMO, this looks nice and
more consistent with the `insta::assert_snapshot` output. I didn't spend the
time to do the same for `test_resolve_command`.
The translation from method error to keyword error can go wrong if the context
object had n-ary methods (n > 0), which isn't the case as of now. For
simplicity, arguments error is mapped to "self.<name>(..)" suggestion.
Local variables and "self" could be merged without using extra method, but
we'll need extend_*_candidates() to merge in symbol/function aliases anyway.
this greatly speeds up the time to run all tests, at the cost of slightly larger recompile times for individual tests.
this unfortunately adds the requirement that all tests are listed in `runner.rs` for the crate.
to avoid forgetting, i've added a new test that ensures the directory is in sync with the file.
## benchmarks
before this change, recompiling all tests took 32-50 seconds and running a single test took 3.5 seconds:
```
; hyperfine 'touch lib/src/lib.rs && cargo t --test test_working_copy'
Time (mean ± σ): 3.543 s ± 0.168 s [User: 2.597 s, System: 1.262 s]
Range (min … max): 3.400 s … 3.847 s 10 runs
```
after this change, recompiling all tests take 4 seconds:
```
; hyperfine 'touch lib/src/lib.rs ; cargo t --test runner --no-run'
Time (mean ± σ): 4.055 s ± 0.123 s [User: 3.591 s, System: 1.593 s]
Range (min … max): 3.804 s … 4.159 s 10 runs
```
and running a single test takes about the same:
```
; hyperfine 'touch lib/src/lib.rs && cargo t --test runner -- test_working_copy'
Time (mean ± σ): 4.129 s ± 0.120 s [User: 3.636 s, System: 1.593 s]
Range (min … max): 3.933 s … 4.346 s 10 runs
```
about 1.4 seconds of that is the time for the runner, of which .4 is the time for the linker. so
there may be room for further improving the times.
This uses the [`clap-markdown`] library. It's not very flexible, but seems to
work. Its implementation is not difficult. If needed, we could later
reimplement the part that iterates over all subcommands and have a different
output (e.g., the boring version of each help text inside its own code block,
or a different file per subcommand, or some fancy template in handlebars or
another rust-supported templating language). I don't want to do it right now,
though.
The library does turn out to have some annoying bugs, e.g.
https://github.com/ConnorGray/clap-markdown/pull/18, but I think that's not a
deal-braker. The fix seems to be 3 lines; if the fix doesn't get merged soon, we
could vendor the library maybe?
[`clap-markdown`]: https://docs.rs/clap-markdown/latest/clap_markdown/
The count() function in this trait is used by "jj branch" to determine
(and then report) how many commits a certain branch is ahead/behind
another branch. This is currently implemented by walking all commits
in the revset, counting how many were encountered. But this could be
improved: if the number is large, it is probably sufficient to report
"at least N" (instead of walking all the way), and this does not scale
well to jj backends that may not have all commits present locally (which
may prefer to return an estimate, rather than access the network).
Therefore, add a function that is explicitly documented to be O(1)
and that can return a range of values if the backend so chooses.
Also remove count(), as it is not immediately obvious that it is an
expensive call, and callers that are willing to pay the cost can obtain
the exact same functionality through iter().count() anyway. (In this
commit, all users of count() are migrated to iter().count() to preserve
all existing functionality; they will be migrated to count_estimate() in
a subsequent commit.)
"branch" needed to be updated due to this change. Although jj
is currently only available in English, I have attempted to keep
user-visible text from being assembled piece by piece, so that if we
later decide to translate jj into other languages, things will be easier
for translators.
During the transition to using more async code, I keep running into
https://github.com/rust-lang/futures-rs/issues/2090. Right now, I want
to convert `MergedTree::diff()` into a `Stream`. I don't want to
update all call sites at once, so instead I'm adding a
`MergedTree::diff_stream()` method, which just wraps
`MergedTree::diff()` in a `Stream. However, since the iterator is
synchronous, it needs to block on the async `Backend::read_tree()`
calls. If we then also block on the `Stream` in the CLI, we run into
the panic.
Since gix::Repository::config_snapshot() borrows the repo instance, it has to
be allocated in caller's stack. That's why GitBackend::git_config() is removed.
We need to let async-ness propagate up from the backend because
`block_on()` doesn't like to be called recursively. The conflict
materialization code is a good place to make async because it doesn't
depends on anything that isn't already async-ready.
The commit backend at Google is cloud-based (and so are the other
backends); it reads and writes commits from/to a server, which stores
them in a database. That makes latency much higher than for disk-based
backends. To reduce the latency, we have a local daemon process that
caches and prefetches objects. There are still many cases where
latency is high, such as when diffing two uncached commits. We can
improve that by changing some of our (jj's) algorithms to read many
objects concurrently from the backend. In the case of tree-diffing, we
can fetch one level (depth) of the tree at a time. There are several
ways of doing that:
* Make the backend methods `async`
* Use many threads for reading from the backend
* Add backend methods for batch reading
I don't think we typically need CPU parallelism, so it's wasteful to
have hundreds of threads running in order to fetch hundreds of objects
in parallel (especially when using a synchronous backend like the Git
backend). Batching would work well for the tree-diffing case, but it's
not as composable as `async`. For example, if we wanted to fetch some
commits at the same time as we were doing a diff, it's hard to see how
to do that with batching. Using async seems like our best bet.
I didn't make the backend interface's write functions async because
writes are already async with the daemon we have at Google. That
daemon will hash the object and immediately return, and then send the
object to the server in the background. I think any cloud-based
solution will need a similar daemon process. However, we may need to
reconsider this if/when jj gets used on a server with a custom backend
that writes directly to a database (i.e. no async daemon in between).
I've tried to measure the performance impact. That's the largest
difference I've been able to measure was on `jj diff
--ignore-working-copy -s --from v5.0 --to v6.0` in the Linux repo,
which increases from 749 ms to 773 ms (3.3%). In most cases I've
tested, there's no measurable difference. I've tried diffing from the
root commit, as well as `jj --ignore-working-copy log --no-graph -r
'::v3.0 & author(torvalds)' -T 'commit_id ++ "\n"'` (to test a
commit-heavy load).
https://github.com/cargo-bins/cargo-binstall
Note that `jj` will only become installable once the next release
is published to crates.io. For this reason, I am planning to
wait until then before documenting the fact that `jj` can be
installed this way.
At that point, `cargo binstall jj-cli` should be sufficient.
Before then, it's possible to test that this will work by doing
```
cargo binstall jj-cli --force --strategies crate-meta-data --log-level debug --dry-run --manifest-path cli/Cargo.toml
```
Without --dry-run, this should install the 0.9 release if run
on `cli/Cargo.toml` form this commit.
