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`.
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`.
`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.
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 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.
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.
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.
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>
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>
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.
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.
Lorenz Leutgeb
Erik Kundt
Matthias Beyer
Fintan Halpenny
Sebastian Martinez
