In an effort to reduce the change of version numbers, dependents of
`radicle-node` should not need to know about the modules:
- `control`
- `wire`
- `worker`
These are internal to the `radicle-node` running, and none of its dependents
accessed them.
This resulted in removing two unused type aliases.
To improve crate version churn, mark the error types as `non_exhaustive`.
This removes the need to change the version number of the crate when adding a
new variant.
Note that these errors are never matched on, so no other code is changed in
dependent crates.
Some tests actually do not use qualified patterns. The removed lines
*should* not compile because the `qualified_pattern!` proc macro
*should* validate the string literals at compile time, but it contains
a bug and actually does not assert well-formedness of qualified
refspec patterns. Rectify.
On Windows, all attempts to clone repositories failed with
Fetch failed for rad:… from z6Mk…: Access is denied. (os error 5)
The reason is that, other than Unix-like systems, it forbids that
directories that are in use are moved.
To improve the situation, take back control over what is moved and
removed exactly by implementing `cleanup` instead of relying
on `impl Drop for tempfile::TempDir`.
A new type `TempRepository` is introduced to capture a repository that can be
used temporarily, and either cleaned up on failure, or moved on success.
This `TempRepository` can be constructed using `Storage::temporary_repository` –
renamed from `Storage::lock_repository` since it is only creating a temporary
resource and not locking it.
In the future (once Rust 1.89 is a little less cutting edge and more
widely available), we may opt for actual locking via
`std::fs::File::lock`.
Allow the fetch interface to accept anything that implements
`AsRef<Repository>`. This allows flexibility in the types that the `Handle` can
accept.
This change is motivated by wanting to introduce a type that is a temporary
repository that wraps a `Repository`.
The interface of the `Handle` has two methods: `Handle::repository` and
`Handle::repository_mut`. However, the calling logic would always access the
`Handle::repo` field.
This is cleaned up by making this field private, and using the
`Handle::repository` method instead. There were no calls to `repository_mut`,
and so the method was removed.
It is impossible to preserve the head of a revision and only change
the base of same revision via push.
Strenghthen the precondition for skipping updates to also consider the
base commit.
Without quorum for the default branch, the remote helper would report
an error, suggesting that the patch was not updated, while,
inconsistently, actually updating the ref in storage without creating
a new revision.
$ git push …
To rad://z…5/z…z
…
! [remote rejected] 612… -> patches/a77… (…)
error: failed to push some refs to 'rad://z…5/z…z'
…
$ git ls-remote rad://z…5/z…z
612… refs/heads/patches/a77…
This is very confusing.
Fix this by using a temporary reference name that is different from the
reference name that the patch would use, implementing cleanup via
`Drop`.
While at it, also move the logic into `patch_base` and share it
between `patch_open` and `patch_update`. Computation of the canonical
head via `stored.canonical_head` is moved to a branch that is only
taken if the user did not specify a base explicitly. This allows to
update patches even when there is no quorum for the default branch.
This function (and the helpers `ancestry` and `find_and_peel`) are
eagerly peeling to commits, leading to updates of tags that target the
same commit to be missed.
For example, there could be tag two tag objects with *different* OIDs
A and B, from *different* authors, using *different* tag names, signed
with *different* secret keys, and both pointing to *the same* commit C.
The implementation would consider A and B to be the same, just becuase
A and B both peel to C, skipping the update.
This is counterintuitive, and when combined with canonical references
can be quite confusing.
Change this to only reason about an ancestry if the two objects in
question really both are commits directly. Otherwise, treat cases where
no structure can be used as ancestry similarly to non-fast-forward
updates.
A rewrite of the argument parsing portions of the `rad issue` subcommand
using `clap` instead of `lexopt`.
From a user's perspective, the look-and-feel of `rad issue` does not
change much. Although, the most prominent change is to rely on
`clap`'a default error template which looks a bit different from what we
use usually (`error: ...` vs. `✗ Error: ...`).
Leaving the above restriction aside, subcommand mis-usage is
reported more verbosely.
Also configure `clap` to use colored output for errors and help pages.
In help output, headers are colored with `Magenta` to match the overall
CLI styling.
Specify a type that captures the different actions that can be take
when using the `comment` command: `Comment`, `Reply`, and `Edit`.
Move the functions that run the corresponding actions into a `comment`
module.
To preserve the "default command" behaviour being `rad issue list`,
e.g., `rad issue --solved` behaving like `rad issue list --solved`,
introduce a new type `EmptyArgs`to also parse arguments without a
subcommand. In case no subcommand was parsed, construct `Command::List`
with from `EmptyArgs`.
Co-authored-by: Matthias Beyer <mail@beyermatthias.de>
Co-authored-by: Lorenz Leutgeb <lorenz.leutgeb@radicle.xyz>
Co-authored-by: Erik Kundt <erik@zirkular.io>
Co-authored-by: Fintan Halpenny <fintan.halpenny@gmail.com>
With this change, the location of the secret SSH key can be configured
through `${RAD_HOME}/config.json` so that the node key does not have to
be placed under `${RAD_HOME}/keys` anymore.
Further, there is now an option to override `config.json` directly when
executing `radicle-node` via the command line argument
`--secret`.
The primary motivation is more flexible deployments, for example
leveraging external secret management solutions, like
<https://systemd.io/CREDENTIALS/>.
The secret key is fingerprinted by taking the fingerprint of the public
key corresponding to the secret key. This allows to detect when the
secret key changes.
The implementation of `Keystore` requires a particular layout and naming
of keys. This is a rather strong limitation. Lift it and allow
specifying an arbitrary path for the secret and public key.
As it is possible to derive the public key from the secret key, make
the public key in `Keystore` optional. Also allow constructing
`MemorySigner` can now be initialized without a public key, by
deriving it.
When using Jujutsu and a non-colocated Git repository, the detection
using `git2` directly fails (just as `git` in a shell would fail)
because there is no `.git` directory found by traversing up the
filesystem hierarchy.
Add an invocation of `jj git root` as a fallback.
When run with a secret and public key that do not cryptographically
match, `fn radicle_cli::terminal::io::signer` would prompt the user
for a password to unlock the secret key, and then carry on with a
mismatching key pair.
Fix this by verifying that the keys match in `MemorySigner::load`
and only swallow errors initializing the signer in cases where
prompting for a password makes sense. It does not make sense in the
case of mismatched keys.
Register a panic handler that logs a backtrace via the `backtrace`
crate, instead of setting `RUST_BACKTRACE`.
Co-authored-by: Lorenz Leutgeb <lorenz.leutgeb@radicle.xy>
Previously it was possible to create an issue with an empty description
(i.e. an empty "root" comment), and also to change it later to a
non-empty string, however it was not possible to clear it again.
Optionally depend on crate `structured-logger`. Allow enabling it via
`--log-logger structured`. For future compatibility, provide
`--log-format` which currently only supports JSON.
Co-authored-by: Lorenz Leutgeb <lorenz.leutgeb@radicle.xyz>
Logging initialization was infallible previously, i.e., a logger would
always have to be chosen, falling back to our own logger
implementation in case of failure with setting up journald submission.
This lead to complexity like attempting to log errors from parsing
arguments, as there is a circular dependency.
Change this to make logging initialization fallible. If it fails, exit
non-zero instead of falling back to another logger. This makes the
initialization code slightly simpler.
At the same time, make choosing the logger possible via the command
line and deprecate `--log`.
Note that we still default to journald if detected.
The dependency `systemd-journal-logger` does not build on macOS, and
there are no tests for other Unixes. Also, systemd is most common on
Linux.
To avoid compilation errors on non-Linux platforms, only depend on the
crate when building on Linux.
Co-authored-by: Lorenz Leutgeb <lorenz.leutgeb@radicle.xyz>
This removes the `rad issue` test examples from being used in the
`fn rad_patch` test example execution. Removing it here is fine, since
it is being executed on its own anyways.
The output of this command is confusing. It is not clear which of the
hashes printed is that of the revision and which one is the respective
head commit.
Further, the hash is omitted on the initial revision, adding confusion.
Further, the terminology of "revised" vs. "updated" is confusing.
This rewrites `timeline.rs` to drop the distinction between various
revisions (in wording, the small icon is still different) and cleans up
the output.
Attention is paid to alignment of the output. Now all verbs of updates
("accepted", "rejected", "reviewed", "merged") are aligned, and authors
("… by …") are also aligned.
The rewrite itself is not for better code quality or readability.
Note that all the code in this file only accessed via one `fn` by one
callsite in the implementation of `rad patch`.
Translation of the string passed as the value for push-option base
happens delayed.
Change this by parsing as soon as the value is written.
This also decreases the dependency fingerprint on `radicle-cli`.
The `--setup-signing` flag is a no-op when combined with `--existing`
and errors on bare repositories.
Make it effective in combination with `--existing` and also rewrite it
to support bare repositories, gracefully falling back to just avoid
writing to `.gitsigners`.
The contract for Git remote handlers says that we can expect
`GIT_DIR` to be set, and this also simplifies logic regarding
bare vs. non-bare repositories.
Group `["option", "progress", ..]`, and `["option", ..]` cases together during
match.
Make the key/value matching cleaner using `ok_or_else` and then matching on the
key value.
This crate is not a library, i.e., it is not intended to be depended
upon by other crates. Rather, it implements the `git-remote-rad`
binary.
Reflect this by moving `lib.rs` to `main.rs`, merging it with
`git-remote-rad.rs`.
This introduces a `PaintTarget`, that defines the stream an operation
uses to draw to (`stdout`, `stderr`, `sink`). It will be used in
subsequent commits to configure the new spinner.
The context of the invocation of `git push` "internally" by the
remote helper is the same as by the user invoking `git push` most
of the time. That is, these pushes run within the same repository,
thus the configuration for pre-push hooks applies to both. This
leads to every push being verified *twice*.
If the user intends to configure pre-push hooks, then they would
do that on the remote with URL `rad://…`, and so the hook would
be executed before `git-remote-rad` even gets called.
We thus specify `--no-verify` to not verify again.
`radicle::run` interprets the `Output` returned by the `git`
process. However, depending on the application we have different
requirements for interpreting the output, e.g., how to handle errors.
Make `radicle::run` not interpret `Output`, but move this
logic to the respective binary crates (`radicle-remote-helper` and
`radicle-cli`).
The `verbosity` option is ignored, which makes it more difficult to
obtain debugging information if the "internal" push, i.e., the
`git push` invocation by `git-remote-rad` fails.
Provide two types for verbosity, and wire them up.
Rewrite module declarations that use the `#[path="…"]` annotation
to the more idomatic `pub mod …` + `pub use …` forms.
For the "self" module, that is not possible because `self` is an
identifier in Rust, and also `r#self` is not admissable as a raw
identifier, so stick to `rad_self`.
Files in submodules were patched as appropriate.
To get to a point of separating the users' identity from the node, then the `rad
self` command should not display information related to the node so prominently.
The relation between `rad` and `radicle-node` is really similar to that between
`rad` and SSH Agent. They communicate via socket. So, when this connection is
successful, it is printed, but not more.
There may be some re-learning for users here, but it is worth the improvement.
Note that the information being removed here is available via `rad node status`
(see previous commit).
If we ever want to disentangle the users' identity from the node, then
the `rad node` commands must learn to print the information related to
the node, so here we go.
Ideally, we would like to use a name that is compliant with RFC 2606,
such as "radicle.example.com", but this would change *lots* of hashes,
which is not worth the hassle.
Instead, make changing this in the future as easy as changing one
constant.
This makes the default Radicle Explorer URL configurable at compile-time
via the environment variable `RADICLE_EXPLORER`.
In order to stay backwards compatible, we still default to
"app.radicle.xyz"
As a library crate, it is bad to return such generic errors.
To get there, errors from `inquire` are downcasted.
This allowed to clean up the public interface of `radicle-term`, so that
it does also not leak the `inquire::InquireError` in its public API.
This is dead code that imports `anyhow`. On the way to removing the
dependency on `anyhow` from `radicle-term`, it just seems to be a good
idea to remove the cruft.
`anyhow` is only used to capture a handful of errors. It also turns out that
usage of `lexopt` was wrong, since it features `parse_with`, which neatly
integrates with `FromStr`. The types, luckily, implement this trait already –
making for cleaner parsing.
For now, the execution errors are all "transparent", which shouldn't be
much of a regression, if at all.
This way the Iterator::size_hint() function can be used by the
Extend::extend() implementation, which might safe reallocations for
the underlying buffer.
Signed-off-by: Matthias Beyer <mail@beyermatthias.de>
In many tests we assert that encoding and decoding gives the same
object. Let's call that a "roundrip", and also provide a proc macro to
generate such tests.
It turns out that failing to read configuration resulted in no output,
because the logger would only be set after configuration had been read.
Fix this, by setting the logger early, with a log level that is then
corrected as soon as configuration is available.
While at it, also clean this up to allow logging errors about failure to
set up the logger.
Adds a test to ensure that canonical reference updates are received on the node
events stream.
Added a helper to `NodeHandle` for perfoming a commit to a reference and signing
the nodes references, to help with this use case.
After a fetch is performed, `set_head` is called, which essentially updates the
default branch based on the canonical reference rule for that branch.
Later, `set_canonical_refs` is called, which originally could be the empty set
of rules. This would mean that the event for the default branch would never be
emitted.
The approach taken here is to always use the default branch rule if there are no
rules set in the identity document. Unfortunately, this ends up in computing the
default branch twice, due to `set_head`.
Another approach is to use the result of `set_head` to update the set of
`UpdatedCanonicalRefs`, to prevent the need for defaulting to the default branch
rule. However, this does not prevent the double counting as soon as the rules
are added to the identity document, since the default branch rule will then be
synthesised into the rule set.
The logging version was verbose, and not required. It also could end up double
processing canonical reference updates since multiple remotes may update the
same reference.
Instead, use `filter_map` to convert all the reference names, stripping
namespaces, and collect them into a `BTreeSet` to ensure uniqueness.
Whenever the node fetches new updates, it checks if canonical references can be
updated. The node has learned how to return these results and emit them as node
events. This is a breaking change since it adds a new variant the `Event` type,
which is not forwards-compatible.
Since the commit is now always pushed to the Radicle storage, there is no need
to check for fast-forward. If heads diverge, it will be reported by the quorum
error.
This change was inspired by the a story as old as time:
𝕃𝕖𝕥 𝕦𝕤 𝕞𝕚𝕩 𝕠𝕦𝕣 𝕓𝕦𝕤𝕚𝕟𝕖𝕤𝕤 𝕝𝕠𝕘𝕚𝕔 𝕨𝕚𝕥𝕙 𝕠𝕦𝕣 𝕀𝕆!
It was motivated by the fact that the canonical quorum logic was spread across
two modules and also two different repositories (in the API sense). The
`radicle-remote-helper` contained special logic for computing the quorum, and
also relied on the logic with `radicle` itself. It would also mix using the
Radicle storage repository and the working copy repository – resulting in issues
where objects could exist in one and not the other.
The change begins by separating away the IO away from any of the business logic
in the `git::canonical` module. To follow along, there are two important submodules:
1. `voting` captures the different type of voting processes for commits and tags
a. Tags are simple, where one `Oid` means one vote
b. Commits are slightly more complicated. They begin with one `Oid` means one
vote, but then it is expected that merge bases are calculated for pairs of
commits. These merge bases are used to increase a vote for an `Oid` if it
is the merge base of another commit.
2. `quorum` builds on top of `voting` and uses the voting processes as well as
the `threshold` to find the quorum for the tag or commit reference.
a. For tags, the first past the threshold wins, but if multiple pass then it
is an error.
b. For commits, the merge base process should be used to increase the votes,
until the caller is ready to find the quorum. At this point, the commits
that pass the `threshold` are then compared to find the commit that is the
child-most commit of all other candidates. If they diverge, then an error
is returned.
There is also a `convergence` module that captures the logic that is required
for the `radicle-remote-helper`. It essentially checks if a candidate object
matches the expected objects, tags or commits, and performs the necessary
convergence logic – checking that the candidate commit is converging with
at least of the other `Did`s.
These two quorum processes, and the convergence process, essentially act as
state machines and can be driven by the use of a Git repository for finding the
merge bases. This is where the `effects` module comes in. The `effects` capture
the necessary traits that are required to drive the state machines of the quorum
processes. It is expected that a Git repository implements these, and in fact, a
`git2::Repository` implementation is provided. The traits are useful, since it
means that a `git2::Repository` can be swapped out and the logic would stay the
same.
This all culminates into the new and improved `Canonical` and
`CanonicalWithConvergence`. Both of which have methods `find_quorum` for
performing the quorum process using a *single* provided repository.
This resulted in the semantic change of requiring that the
`radicle-remote-helper` pushes the candidate commit, irregardless of whether it
will be a fast-forward. For now, this is something that will be accepted while
the UX can be improved in the future by providing detailed warnings of the
divergence and ways to fix it. The benefit is that the tooling will never stop
someone from diverging if that is in fact what they want to do.
Our implementation for the control socket is based on AF_UNIX, and as
the note (which is removed) suggests, we should actually check that the
received socket really is of that domain.
This check was not implemented because it is not exposed via `std`, and
a bit cumbersome to do via `libc`.
I did not realize that `socket2` which neatly sits between `std` and
`libc` in terms of its abstractions and is cross-platform allows us to
do this, and we even already depend on it!
So, add the suggested check.
While at it, refactor the function to return early in cases. Now the
progression from a file descriptor to a socket to a listener is nicely
captured in the types and not obstructed too much by indentation.
Also log at the error level.
I was greeted by `rad patch redact` with
called `Result::unwrap()` on an `Err` value:
Error { code: None, message: Some("failed to convert") }
stack backtrace:
[…]
3: core::result::Result<T,E>::unwrap
at …/rust-1.88.0/lib/rustlib/src/rust/library/core/src/result.rs:1137:23
4: sqlite::cursor::Row::read
at …/index.crates.io-1949cf8c6b5b557f/sqlite-0.32.0/src/cursor.rs:136:9
5: radicle::cob::patch::cache::query::find_by_revision
at ./crates/radicle/src/cob/patch/cache.rs:624:65
6: <… as radicle::cob::patch::cache::Patches>::find_by_revision
at ./crates/radicle/src/cob/patch/cache.rs:553:9
7: radicle_cli::commands::rad_patch::redact::run
at ./crates/radicle-cli/src/commands/patch/redact.rs:23:9
8: radicle_cli::commands::rad_patch::run
at ./crates/radicle-cli/src/commands/patch.rs:1026:13
[…]
It turns out that `sqlite::cursor::Row::read` is the panicky version of
`sqlite::cursor::Row::try_read` (which returns a `Result`).
While it is somewhat rare that SQLite reads fail, it is not unheard of.
In `radicle-cli` it might not be critical, but also `radicle-protocol`
and `radicle-fetch` are affected, and they could potentially panic a
`radicle-node` process.
Use `try_read` instead, and propagate down the error handling.
Remove `fn deserialize` and instead provide a trait `fn decode_exact`
for testing.
Refactor errors, so that we can now cleanly distinguish between
`Error::UnexpectedEnd` which means that decoding might work given
more data, and `Error::Invalid`, which means that no amount of
additional data will decode.
While at it, cleanup the variants of `Error::Invalid`, and provide `ControlType`.
What made the issue fixed in the parent commit harder to spot
for me was the multitude of concepts and terms involved in
encoding: What's *serializing* vs. *encoding*? Why is there
this odd `Frame::to_bytes`?
So I decided to clean up a bit. I removed `fn serialize` so that the
logic stays close to the data, and instead of it provide
`fn encode_vec` in `trait Encode` so that we don't add up with
one-offs like `Frame::to_bytes` again.
Also we make use of `encode_vec` in the tests.
In 3c5668edd2 I mistakenly moved the check
the length of an encoded `wire::Message` from its `impl wire::Encode` to
the `fn serialize`, so that it would not apply only to messages (as
originally intended), but to *any* argument of `serialize`.
This is not a problem for gossip messages, but catastrophic for Git
streams, which tend to send a lot of data.
The fix is simple: Move the check back into `impl Encode for Message`.
As `std::os::unix` is obviously not available on Windows, resort to the
`winpipe` crate, which implements a very similar API for named pipes.
Apart from simple changes to imports, we need to jump through a few
hoops because the `std::io::{Read, Write}` impls are on `&mut` for
`winpipe`, thus yielding different mutability requirements on Unix vs.
Windows.
We resort to cloning, but as this fallible, swallow the added complexity
of a platform-dependent implementation to not risk panics on Unix.
The `winpipe` crate which is our current best bet for support on Windows
requires that named pipes are at the magical location `\\.pipe\`.
Provide a default on Windows that matches this, and use our freshly
assigned IANA service name for it.
This is a breaking change, as we remove a `pub const`, it should be
reasonably easy to fix by dependents, and probably it was a mistake to
make the default `pub const` as dependents should call `Home::socket`
anyway.
The signals crate does not build on non-Unix-like platforms, such as
Windows. It uses constants for signals that are not exported from
`libc` on Windows, and transitive dependencies such as `sem_safe` fail
to compile.
Resolve this, by guarding most of the crate, certainly all real
features, by `cfg(unix)`.
To require less uses of the `cfg` macro, move all affected parts into a
new module, and re-export it.
The crate now builds on Windows (although it does not do anything
interesting on that platform) and its Unix interface remains unchanged.
Changes are integrated into `radicle-node`, but only affect Windows.
Microsoft Windows has Universal Naming Convention (UNC) paths, see
<https://learn.microsoft.com/en-us/dotnet/standard/io/file-path-formats#unc-paths>
The Rust standard library normalizes paths to this form when calling
`std::fs::canonicalize`, see
<https://doc.rust-lang.org/std/fs/fn.canonicalize.html>
However, some programs, do not parse these paths correctly, including
our ally `git`:
$ git clone \\?\C:\Users\lorenz\tmp
Cloning into 'tmp'...
ssh: Could not resolve hostname \\\\?\\c: No such host is known.
fatal: Could not read from remote repository.
Please make sure you have the correct access rights
and the repository exists.
The `dunce` crate solves the problem of having to deal with
normalization and avoiding weird UNC paths for us. Note that on
non-Windows platforms, it *behaves exactly like*
`std::fs::canonicalize`!
The `fn io_other` disguises all sorts of errors as I/O errors, making
errors in the transport component of `radicle-fetch` harder to
understand.
Instead, expose a new error type that allows to discriminate.
The stream tests did not take into account that the tips of previous COB entries
need to be inserted so that a Git history can be formed.
This was surfaced by the added regression test.
If the tests are run without a global Git config, then the tests will error with
no name or email set for the commits.
Instead, use the test fixture setup to ensure there is a name and email for
running the tests.
This patch adds the `cob::common::Title` struct, this allows instead of
using `std::str::String` or generics and traits for it, to define more
precisely what a title should be.
It trims the provided string and makes sure it contains no carriage
return or new line characters.
Where a `std::str::String` makes more sense so it's easier to mutate it,
we keep the code as is.
Co-authored-by: Fintan Halpenny <fintan.halpenny@gmail.com>
Add type aliases to specialise the `Stream` type for each of `Patch`, `Issue`,
and `Identity`.
An `init` constructor is also added for each to make it easier to construct each
stream.
Introduce a new `stream` module to `radicle::cob`. The purpose of this module is
to provide an API for iterating over a COB's operations, given a range of
commits.
The `CobStream` trait provides the generic API for implementing and using, while
`Stream` provides a concrete implementation using the `git2` crate.
The approach is to use a `git2::Revwalk` for walking over each commit in the
range, then loading the `Op` for that commit – skipping any commits that are
unrelated to that COB type, in the case of associated COBs.
The `Stream` is tested using some properties of the API.
The next steps are to introduce specialised types for `Patch` and `Issue` to
ensure that it works for those.
Adds an `Op::load` helper method for loading an `Entry`, given its `Oid` and the
`store` it is stored in.
This can be useful for downstream consumers to inspect operations on COBs given
a single `Oid`, without having to load the entire object and/or graph.
It can also be useful when looking at implementing COB stream primitives.
Add a helper method `Op::manifest_of`, to allow the caller to inspect the
`Manifest` of the entry. This can be used to avoid attempting to call `Op:load`
with a manifest that would not match the expected `Action`s of the `Op`.
This patch clearifies (in the help message of the command) how the
"clear" subcommand behaves when passing ids.
Signed-off-by: Matthias Beyer <mail@beyermatthias.de>
This patch optimizes how the issues are collected.
Before, the issues where pushed to a new instance of `Vec`, which was
not preallocated to the size of the iterator.
This could be slow when a large number of issues is returned by the
iterator.
With the `Iterator::collect` function, the `FromIterator` implementation
of `Vec` is used, which makes use of `Iterator::size_hint` for
optimizing how much space is preallocated when collecting into the
`Vec`.
Signed-off-by: Matthias Beyer <mail@beyermatthias.de>
This patch adds an implementation of the Iterator::size_hint() trait
function so that the compiler can optimize allocations on the collecting
side of the iterator better.
From the trait function docs:
> size_hint() is primarily intended to be used for optimizations such
> as reserving space for the elements of the iterator, but must not be
> trusted to e.g., omit bounds checks in unsafe code. An incorrect
> implementation of size_hint() should not lead to memory safety
> violations.
Source: https://doc.rust-lang.org/stable/std/iter/trait.Iterator.html#method.size_hint
And:
> Vec may use any or none of the following strategies, depending on the
> supplied iterator:
>
> * preallocate based on Iterator::size_hint() and panic if the number
> of items is outside the provided lower/upper bounds
Source: https://doc.rust-lang.org/stable/std/iter/trait.FromIterator.html
Signed-off-by: Matthias Beyer <mail@beyermatthias.de>
Symbols signaling status of a process and similar are duplicated in
three different crates. Also two different symbols to signal success are
used:
1. ✓ ( Check Mark, U+2713)
2. ✔ (Heavy Check Mark, U+2714)
Work against that by referring to `radicle-term` from the other crates.
When initializing the `radicle-node` process, see
`radicle-node/src/runtime.rs`, an attempt is made to read from a node
announcement file.
This file is never written, therefore all of these reads fail.
Even though this caching mechanism might make sense or might have made
sense at some point, it is dead code now, and therefore removed.
Also, the fact that the file name was defined in the `radicle` crate is
ugly.
Ensures that preferred seeds that are not synced are made part of the `to_sync`
set, and the Announcer is constructed without an error.
As well as this, ensure that the `synced` and `to_sync` sets do not overlap by
constructing the `to_sync` and removing any `synced` elements from it.
Making a note of that `Announcer::can_continue` might never be able to hit the
`Break` case. It could be defensive programming, so I'm going to leave it in for
the time being.
Adds a test case to show that the Announcer is allowed to mark an "unknown" node
as synced.
There may be a case where gossip reached a node that the current node was not
aware of. This should be allowed and should count towards the final replication
factor.
The `Announcer` logic was subtly different to the fetching logic, where the
preferred seeds need to be announced to *and* the replication factor had to be
reached.
This patch relaxes that constraint, and allows it to hit either targets before
exiting.
This also surfaced an issue with the with the `Progress` calculation, where
calculating the `unsynced` used the `synced` value to subtract. However, this
was not required because `to_sync` is mutated, so the value is simply
`to_sync.len()`. A test case was added to ensure this is correct by checking
that invariant that the total between synced and unsynced nodes always matches
the total of synced and unsynced progress.
The upgrade of the `radicle-crypto` crate was missed when upgrading
semver versions.
Unfortunately, since it is at the base of the dependency tree, all
crates that depend on it require an additional upgrade.
If one attempted to compile the test target for `radicle-node` by itself, it
would result in a compiler error – due to missing trait implementations that are
provided by `radicle-protocol/test`.
It would compile fine when `--all` is used due to cargo unifying feature sets,
so it went unnoticed until now.
This fixes the issue by adding `radicle-protocol` to the `dev-dependencies` and
enabling the `test` feature flag.
This is morally a revert of 70fb0d3fef
while additionally getting to compile
`crates/radicle-term/src/editor.rs` on Windows.
To get the changes that were made on top of the revert, check
git diff 70fb0d3fef~1 <this commit> -- crates/radicle-term/src/editor.rs
There were reports of the inquire editor being unrealiable.
Only the latest log was kept as `node.log.old`. Now, log files are
numbered (`node.log.1`, `node.log.2` and so on). Also `node.log` is now
a hard link to the current log file. `rad node stop` will delete
`node.log` (note that `node.log.x` stays intact) and `rad node start`
will delete `node.log` before it creates `node.log.y` in case the node
crashed.
Also, when running in foreground mode, now a log file is created. It
just contains the hint that the node was started in foreground mode,
just to avoid confusion.
The implementation is split into a sans I/O part as `struct LogRotator`,
while the I/O counterpart is captured by `struct LogRotatorFileSystem`.
The logic for computing canonical references conflated the semantics of
annotated and lighweight tags, yielding confusing/wrong results. The
main culprit was the call to `peel_to_commit` in
`impl ReadRepository for Repository`, peeling tag objects away.
To resolve this, we separate out quroum computation per object type: one
implementation for commits, and one for tags.
Also move move canonical error types and methods into their own
module to have a cleaner file structure for the main logic.
Capture the `BTreeMap<Oid, u8>` type into a `Votes` struct, with its own API.
We were getting the objects from the repository twice – once in `Canonical::new`
and once again in `ensure_commit_or_tag` – so the latter was removed.
Use an `enum CanonicalObject` to specify which object types the canonical
process supports.
During `converges`, separate commits and tags, and ensure that we are only
looking at objects of one type. Note, that Fintan think this is actually the
*wrong* place to do it because we already filter the objects – so we should
keep track of that information higher up the callstack.
Co-authored-by: Fintan Halpenny <fintan.halpenny@gmail.com>
When we fail to get the info for the binary names of `radicle-node` and
`git-remote-rad`, we still mention the binary name which is usually a part of
their output.
After upgrading from Rust 1.85 to 1.88 in 586eefc, clippy started
warning about large error types. Box action to make
`radicle::cob::patch::Error` lighter.
Provide a default message that informs the user of the `(e)`, `(enter)`, and
`(esc)` functionality.
Unfortunately, it is not possible to provide any message that is better
customised to each case, due to `inquire` requiring a `'a` lifetime on all its
inputs – this rules out providing any kind of `String` input.
The first paramter of `Editor::new` can already be used to provide more
contextual information. The second parameter is left configurable, but for now
`Editor::HELP` is used everywhere.
To avoid that users on Windows have to explcitly set `HOME`, also accept
`USERPROFILE` which is considered equivalent.
To avoid repeating to join `.radicle`, split the logic up into two
phases. Also take care to provide a good error message, as this error is
fatal and user-facing.
The trait `radicle_ssh::agent::client::ClientStream` trait was
simplified, because `winpipe::WinStream` is so similar to
`std::os::unix::net::UnixStream` that we can just use the same
implementation for both platforms.
However, obtaining the process identifier of the SSH agent process is
not common on Windows, so we instead track the path that we used to
connect to the agent. This is also helpful on Unix. In some cases, the
path will even contain the process identifier. The path is printed by
`rad self`.
Errors are cleaned up, and error messages are improved.
Introducing the new enum `TerminalFile` to model which standard stream
stream the terminal uses allows us to remove the dependency on `libc`
which does not build on Windows.
`radicle-term` includes quite a lot of rather "low-level" logic to spawn
the preferred editor of the user. In particular, this logic is
platform-dependent and only works on Unix-like platforms.
Also, `radicle-term` already depends on `inquire` which features an
editor prompt. It is a cross-platform solution to spawn the editor.
This commit changes the implementation of `Editor` to be a wrapper of
`inquire::Editor`, keeping the interface mostly intact.
Downsides:
- We cannot edit a file "in place" this way, instead we have to read
it, as `inquire` only supports editing temporary files. This can also
be viewed as a benefit, as we lessen the risk of corruption of these
files.
- `inquire::Editor` contains borrows, so it is quite cumbersome to
extend it with a custom mechanism for looking up the preferred
editor. The lookup logic was kept, but needs to be invoked by the
caller. In the future we might consider just using the logic provided
by `inquire` or requesting a change to `inquire`.
- The prompt is a bit strange. It does not show the current state of
the output, see <https://github.com/mikaelmello/inquire/issues/280>
and <https://github.com/mikaelmello/inquire/pull/271>.
The `radicle-signals` crate only supports Unix, so using it prevents
`radicle-term` to build on Windows.
Of course, this means that non-Unix platforms will not support any
signal handling at all, but at least we can build.
Directory builder extensions are platform specific and only available on
Unix. Move their use into a conditionally compiled block, so that we can
build on Windows.
When using `rad init` the default branch for the project can be predetermined by
inspecting the working copy repository.
To improve the experience, it will first check `init.defaultBranch` in the Git
configuration file.
If it does not find this, it falls back to choosing the default head of the
repository at the time. This may be HEAD, and in this case it will error and
provide helpful information.
The repository may also not have any default head at the time, and this also
results in an error and hint.
Fintan Halpenny
Lorenz Leutgeb
Christopher Fredén
Defelo
Sebastian Martinez
Yorgos Saslis
Erik Kundt
Matthias Beyer
Sekhat Temporus
Tobias Hunger
