cli: Fix ahead/behind and diff behavior

In certain cases, eg. after a rebase, or when patches are stacked, the
"ahead/behind" and/or the diff stats can be wrong.

This fixes it.
This commit is contained in:
Alexis Sellier 2023-06-12 17:16:04 +02:00
parent ec064891cd
commit 48f44f4af5
No known key found for this signature in database
10 changed files with 211 additions and 29 deletions

1
.gitignore vendored
View File

@ -1 +1,2 @@
/target
/radicle-cli/target

View File

@ -0,0 +1,118 @@
In this example, we explore how the `ahead/behind` indicator works, and what is
shown as diffs in the case of divergent branches.
First we add the `CONTRIBUTORS` file to `master`, which contains one entry:
```
$ git checkout -q master
$ git add CONTRIBUTORS
$ git commit -a -q -m "Add contributors"
$ git push rad master
$ cat CONTRIBUTORS
Alice Jones
```
Then we create a feature branch which adds another entry:
```
$ git checkout -q -b feature/1
$ sed -i '$a Alan K' CONTRIBUTORS
$ git commit -a -q -m "Add Alan"
```
We go back to master, and add a different second entry, essentially forking
the history:
```
$ git checkout -q master
$ sed -i '$a Jason Bourne' CONTRIBUTORS
$ git commit -a -q -m "Add Jason"
$ git push rad master
$ git log --graph --decorate --abbrev-commit --pretty=oneline --all
* 5c88a79 (feature/1) Add Alan
| * e101a99 (HEAD -> master, rad/master) Add Jason
|/ [..]
* f64fb2c Add contributors
* f2de534 Second commit
* 08c788d Initial commit
```
Then we create a patch from `feature/1`:
``` (stderr)
$ git push rad feature/1:refs/patches
✓ Patch 866f59c001cd4d78a151f444b34265566c83c264 opened
To rad://z42hL2jL4XNk6K8oHQaSWfMgCL7ji/z6MknSLrJoTcukLrE435hVNQT4JUhbvWLX4kUzqkEStBU8Vi
* [new reference] feature/1 -> refs/patches
```
When listing, we see that it has one addition:
```
$ rad patch list
╭─────────────────────────────────────────────────────────────────────────────╮
│ ● ID Title Author Head + - Updated │
├─────────────────────────────────────────────────────────────────────────────┤
│ ● 866f59c Add Alan z6MknSL…StBU8Vi (you) 5c88a79 +1 -0 [ ... ] │
╰─────────────────────────────────────────────────────────────────────────────╯
```
When showing the patch, we see that it is `ahead 1, behind 1`, since master has
diverged by one commit:
```
$ rad patch show -p 866f59c
╭────────────────────────────────────────────────────────────────────╮
│ Title Add Alan │
│ Patch 866f59c001cd4d78a151f444b34265566c83c264 │
│ Author did:key:z6MknSLrJoTcukLrE435hVNQT4JUhbvWLX4kUzqkEStBU8Vi │
│ Head 5c88a79d75f5c2b4cc51ee6f163d2db91ee198d7 │
│ Branches feature/1 │
│ Commits ahead 1, behind 1 │
│ Status open │
├────────────────────────────────────────────────────────────────────┤
│ 5c88a79 Add Alan │
├────────────────────────────────────────────────────────────────────┤
│ ● opened by (you) (z6MknSL…StBU8Vi) [ ... ] │
╰────────────────────────────────────────────────────────────────────╯
commit 5c88a79d75f5c2b4cc51ee6f163d2db91ee198d7
Author: radicle <radicle@localhost>
Date: Thu Dec 15 17:28:04 2022 +0000
Add Alan
diff --git a/CONTRIBUTORS b/CONTRIBUTORS
index 3f60d25..6829c43 100644
--- a/CONTRIBUTORS
+++ b/CONTRIBUTORS
@@ -1 +1,2 @@
Alice Jones
+Alan K
```
Then, we stack another change onto `feature/1`, adding another contributor:
``` (stderr)
$ git checkout -q -b feature/2 feature/1
$ sed -i '$a Mel Farna' CONTRIBUTORS
$ git commit -a -q -m "Add Mel"
$ git push -o patch.message="Add Mel" rad HEAD:refs/patches
✓ Patch 57cb9b2758518e547de324456ac967fda456c6c1 opened
To rad://z42hL2jL4XNk6K8oHQaSWfMgCL7ji/z6MknSLrJoTcukLrE435hVNQT4JUhbvWLX4kUzqkEStBU8Vi
* [new reference] HEAD -> refs/patches
```
When we look at the patch, we see that it has both commits, because this new
patch uses the same base as the previous patch:
```
$ rad patch show 57cb9b2758518e547de324456ac967fda456c6c1
╭────────────────────────────────────────────────────────────────────╮
│ Title Add Mel │
│ Patch 57cb9b2758518e547de324456ac967fda456c6c1 │
│ Author did:key:z6MknSLrJoTcukLrE435hVNQT4JUhbvWLX4kUzqkEStBU8Vi │
│ Head 7f63fcbcf23fc39eea784c091ad3d20d7e4bd005 │
│ Branches feature/2 │
│ Commits ahead 2, behind 1 │
│ Status open │
├────────────────────────────────────────────────────────────────────┤
│ 7f63fcb Add Mel │
│ 5c88a79 Add Alan │
├────────────────────────────────────────────────────────────────────┤
│ ● opened by (you) (z6MknSL…StBU8Vi) 5 months ago │
╰────────────────────────────────────────────────────────────────────╯
```

View File

@ -1,6 +1,6 @@
use anyhow::{anyhow, Context};
use radicle::cob::patch::{Clock, MergeTarget, Patch, PatchId, Patches};
use radicle::cob::patch::{Clock, Patch, PatchId, Patches};
use radicle::git;
use radicle::git::raw::Oid;
use radicle::prelude::*;
@ -52,22 +52,6 @@ pub fn get_merge_target(
Ok((get_branch(qualified_ref), (*target_oid).into()))
}
/// Return the [`Oid`] of the merge target.
pub fn patch_merge_target_oid(target: MergeTarget, repository: &Repository) -> anyhow::Result<Oid> {
match target {
MergeTarget::Delegates => {
if let Ok((_, target)) = repository.head() {
Ok(*target)
} else {
anyhow::bail!(
"failed to determine default branch head for project {}",
repository.id,
);
}
}
}
}
/// Get the diff stats between two commits.
pub fn diff_stats(
repo: &git::raw::Repository,

View File

@ -98,7 +98,8 @@ pub fn row(
) -> anyhow::Result<[term::Line; 9]> {
let state = patch.state();
let (_, revision) = patch.latest();
let stats = common::diff_stats(repository.raw(), revision.base(), &revision.head())?;
let (from, to) = patch.range(repository)?;
let stats = common::diff_stats(repository.raw(), &from, &to)?;
let author = patch.author().id;
Ok([

View File

@ -10,17 +10,15 @@ use radicle_term::{
use crate::terminal as term;
use super::common::*;
use super::*;
fn show_patch_diff(patch: &patch::Patch, storage: &Repository) -> anyhow::Result<()> {
let target_head = patch_merge_target_oid(patch.target(), storage)?;
let base_oid = storage.raw().merge_base(target_head, **patch.head())?;
let diff = format!("{}..{}", base_oid, patch.head());
fn show_patch_diff(patch: &patch::Patch, stored: &Repository) -> anyhow::Result<()> {
let (from, to) = patch.range(stored)?;
let range = format!("{}..{}", from, to);
process::Command::new("git")
.current_dir(storage.path())
.args(["log", "--patch", &diff])
.current_dir(stored.path())
.args(["log", "--patch", &range])
.stdout(process::Stdio::inherit())
.stderr(process::Stdio::inherit())
.spawn()?
@ -30,7 +28,8 @@ fn show_patch_diff(patch: &patch::Patch, storage: &Repository) -> anyhow::Result
}
fn patch_commits(patch: &patch::Patch, stored: &Repository) -> anyhow::Result<Vec<term::Line>> {
let range = format!("{}..{}", patch.base(), patch.head());
let (from, to) = patch.range(stored)?;
let range = format!("{}..{}", from, to);
let mut revwalk = stored.revwalk(*patch.head())?;
let mut lines = Vec::new();
@ -67,8 +66,11 @@ pub fn run(
let (_, revision) = patch.latest();
let state = patch.state();
let branches = common::branches(&revision.head(), workdir)?;
let target_head = common::patch_merge_target_oid(patch.target(), stored)?;
let ahead_behind = common::ahead_behind(stored.raw(), revision.head().into(), target_head)?;
let ahead_behind = common::ahead_behind(
stored.raw(),
*revision.head(),
*patch.target().head(stored)?,
)?;
let mut attrs = Table::<2, term::Line>::new(TableOptions {
spacing: 2,

View File

@ -1,6 +1,6 @@
use std::env;
use std::path::Path;
use std::str::FromStr;
use std::{env, fs};
use std::{thread, time};
use radicle::git;
@ -276,6 +276,28 @@ fn rad_patch() {
test("examples/rad-patch.md", working.path(), Some(home), []).unwrap();
}
#[test]
fn rad_patch_ahead_behind() {
let mut environment = Environment::new();
let profile = environment.profile("alice");
let working = tempfile::tempdir().unwrap();
let home = &profile.home;
// Setup a test repository.
fixtures::repository(working.path());
fs::write(working.path().join("CONTRIBUTORS"), "Alice Jones\n").unwrap();
test("examples/rad-init.md", working.path(), Some(home), []).unwrap();
test(
"examples/rad-patch-ahead-behind.md",
working.path(),
Some(home),
[],
)
.unwrap();
}
#[test]
fn rad_patch_draft() {
let mut environment = Environment::new();

View File

@ -24,6 +24,7 @@ use crate::cob::thread::Thread;
use crate::cob::{store, ActorId, EntryId, ObjectId, TypeName};
use crate::crypto::{PublicKey, Signer};
use crate::git;
use crate::identity;
use crate::identity::doc::DocError;
use crate::identity::PayloadError;
use crate::prelude::*;
@ -155,6 +156,18 @@ pub enum MergeTarget {
Delegates,
}
impl MergeTarget {
/// Get the head of the target branch.
pub fn head<R: ReadRepository>(&self, repo: &R) -> Result<git::Oid, identity::IdentityError> {
match self {
MergeTarget::Delegates => {
let (_, target) = repo.head()?;
Ok(target)
}
}
}
}
#[derive(Debug, Clone, PartialEq, Eq)]
pub struct Patch {
/// Title of the patch.
@ -290,6 +303,28 @@ impl Patch {
&self.latest().1.base
}
/// Get the merge base of this patch.
pub fn merge_base<R: ReadRepository>(&self, repo: &R) -> Result<git::Oid, git::ext::Error> {
let target = self.target().head(repo).map_err(|_| {
git::ext::Error::NotFound(git::ext::NotFound::NoSuchBranch(String::from("HEAD")))
})?;
let base = repo.merge_base(&target, self.head())?;
Ok(base)
}
/// Get the commit range of this patch.
pub fn range<R: ReadRepository>(
&self,
repo: &R,
) -> Result<(git::Oid, git::Oid), git::ext::Error> {
if self.is_merged() {
Ok((*self.base(), *self.head()))
} else {
Ok((self.merge_base(repo)?, *self.head()))
}
}
/// Index of latest revision in the revisions list.
pub fn version(&self) -> RevisionIx {
self.revisions
@ -320,6 +355,11 @@ impl Patch {
self.latest().1.timestamp()
}
/// Check if the patch is merged.
pub fn is_merged(&self) -> bool {
matches!(self.state(), State::Merged { .. })
}
/// Check if the patch is open.
pub fn is_open(&self) -> bool {
matches!(self.state(), State::Open { .. })

View File

@ -437,6 +437,9 @@ pub trait ReadRepository {
/// Get the repository's identity document at a specific commit.
fn identity_doc_at(&self, head: Oid) -> Result<identity::Doc<Unverified>, DocError>;
/// Get the merge base of two commits.
fn merge_base(&self, left: &Oid, right: &Oid) -> Result<Oid, git::ext::Error>;
}
/// Allows read-write access to a repository.

View File

@ -595,6 +595,13 @@ impl ReadRepository for Repository {
}
Ok(longest.into())
}
fn merge_base(&self, left: &Oid, right: &Oid) -> Result<Oid, ext::Error> {
self.backend
.merge_base(**left, **right)
.map(Oid::from)
.map_err(ext::Error::from)
}
}
impl WriteRepository for Repository {

View File

@ -233,6 +233,10 @@ impl ReadRepository for MockRepository {
fn canonical_identity_head(&self) -> Result<Oid, IdentityError> {
Ok(Oid::from_str("cccccccccccccccccccccccccccccccccccccccc").unwrap())
}
fn merge_base(&self, _left: &Oid, _right: &Oid) -> Result<Oid, git::ext::Error> {
todo!()
}
}
impl WriteRepository for MockRepository {