tests/inline_anchor_test.rs
Ref: Size: 18.1 KiB History
//! Where an inline review comment lands, and what it means.
//!
//! Three defects, one subject: an inline comment is an *anchor* into a
//! revision, and until now nothing checked that the anchor resolved
//! (fe555587), nothing in `patch diff` told a reviewer what to anchor to
//! (9a0a143b), and every comment that did land read as a demand (0655b32f).
mod common;
use common::TestRepo;
/// A patch on `branch` whose head adds `path` with `content`.
/// Returns the abbreviated patch id.
fn patch_with_file(repo: &TestRepo, branch: &str, path: &str, content: &str) -> String {
repo.git(&["checkout", "-b", branch]);
repo.commit_file(path, content, &format!("add {}", path));
let out = repo.run_ok(&["patch", "create", "-t", branch, "-B", branch]);
repo.git(&["checkout", "main"]);
out.trim()
.strip_prefix("Created patch ")
.unwrap_or_else(|| panic!("unexpected patch create output: {}", out))
.to_string()
}
/// Every collab ref and the object it points at, as a single sortable blob.
/// A read must leave this identical.
fn collab_refs(repo: &TestRepo) -> String {
let mut lines: Vec<String> = repo
.git(&["for-each-ref", "--format=%(refname) %(objectname)", "refs/"])
.lines()
.map(|l| l.to_string())
.collect();
lines.sort();
lines.join("\n")
}
// ===========================================================================
// fe555587: the anchor must resolve in the revision it is anchored to
// ===========================================================================
#[test]
fn a_file_absent_from_the_revision_is_rejected_by_name() {
let repo = TestRepo::new("Alice", "alice@example.com");
let id = patch_with_file(&repo, "feat", "src/lib.rs", "fn a() {}\nfn b() {}\n");
let err = repo.run_err(&[
"patch",
"comment",
&id,
"-b",
"probe",
"-f",
"does/not/exist.rs",
"-l",
"1",
]);
assert!(
err.contains("does/not/exist.rs"),
"the error must name the path it could not find: {}",
err
);
assert!(
err.contains("revision 1"),
"the error must name the revision it checked against: {}",
err
);
assert!(
err.contains("--line-numbers"),
"the error must name the fix — the command that prints pasteable anchors: {}",
err
);
}
#[test]
fn a_line_past_the_end_of_the_file_is_rejected_with_the_range() {
let repo = TestRepo::new("Alice", "alice@example.com");
let id = patch_with_file(&repo, "feat", "src/lib.rs", "fn a() {}\nfn b() {}\n");
let err = repo.run_err(&[
"patch",
"comment",
&id,
"-b",
"probe",
"-f",
"src/lib.rs",
"-l",
"9000",
]);
assert!(
err.contains("9000"),
"the error must name the line that was rejected: {}",
err
);
assert!(
err.contains("1-2"),
"the error must name the range that would be accepted: {}",
err
);
}
#[test]
fn line_zero_is_rejected() {
let repo = TestRepo::new("Alice", "alice@example.com");
let id = patch_with_file(&repo, "feat", "src/lib.rs", "fn a() {}\n");
let err = repo.run_err(&[
"patch",
"comment",
&id,
"-b",
"probe",
"-f",
"src/lib.rs",
"-l",
"0",
]);
assert!(
err.contains("1-1"),
"line 0 is out of range like any other: {}",
err
);
}
#[test]
fn a_directory_is_not_a_comment_anchor() {
let repo = TestRepo::new("Alice", "alice@example.com");
let id = patch_with_file(&repo, "feat", "src/lib.rs", "fn a() {}\n");
let err = repo.run_err(&[
"patch", "comment", &id, "-b", "probe", "-f", "src", "-l", "1",
]);
assert!(
err.contains("directory"),
"a tree entry that is not a blob has no lines to anchor to: {}",
err
);
}
/// The deliberate decision: a file the revision contains but the patch never
/// touched is a legitimate anchor. "You changed the caller here and not the
/// parallel one over there" is a real review move, and the anchor still
/// resolves against real content, so it cannot drift.
#[test]
fn a_file_the_patch_did_not_touch_is_a_legitimate_anchor() {
let repo = TestRepo::new("Alice", "alice@example.com");
repo.commit_file("untouched.rs", "fn already_here() {}\n", "pre-existing");
let id = patch_with_file(&repo, "feat", "src/lib.rs", "fn a() {}\n");
let out = repo.run_ok(&[
"patch",
"comment",
&id,
"-b",
"this one needs the same change",
"-f",
"untouched.rs",
"-l",
"1",
]);
assert!(out.contains("untouched.rs:1"), "unexpected output: {}", out);
}
/// Comments are revision-anchored so they do not drift. Validation has to
/// follow: a line that exists in revision 1 stays a valid anchor for revision 1
/// however far the working tree has moved on.
#[test]
fn validation_is_against_the_anchored_revision_not_the_working_tree() {
let repo = TestRepo::new("Alice", "alice@example.com");
let id = patch_with_file(&repo, "feat", "gone.rs", "one\ntwo\nthree\n");
// Revision 2 deletes the file entirely.
repo.git(&["checkout", "feat"]);
repo.git(&["rm", "-q", "gone.rs"]);
repo.git(&["commit", "-q", "-m", "drop it"]);
repo.git(&["checkout", "main"]);
repo.run_ok(&["patch", "revise", &id, "-b", "r2", "-B", "feat"]);
// r1 still has it: the anchor resolves.
let out = repo.run_ok(&[
"patch",
"comment",
&id,
"-b",
"on the old text",
"-f",
"gone.rs",
"-l",
"3",
"--revision",
"1",
]);
assert!(out.contains("gone.rs:3"), "unexpected output: {}", out);
// r2 does not.
let err = repo.run_err(&[
"patch",
"comment",
&id,
"-b",
"on nothing",
"-f",
"gone.rs",
"-l",
"3",
"--revision",
"2",
]);
assert!(err.contains("revision 2"), "unexpected error: {}", err);
}
#[test]
fn the_confirmation_echoes_the_anchor_it_recorded() {
let repo = TestRepo::new("Alice", "alice@example.com");
let id = patch_with_file(&repo, "feat", "src/lib.rs", "fn a() {}\nfn b() {}\n");
let out = repo.run_ok(&[
"patch",
"comment",
&id,
"-b",
"nit",
"-f",
"src/lib.rs",
"-l",
"2",
]);
assert!(
out.contains("src/lib.rs:2") && out.contains("r1"),
"a bare 'Comment added.' hides a bad anchor; echo it: {}",
out
);
}
#[test]
fn a_rejected_comment_records_no_event() {
let repo = TestRepo::new("Alice", "alice@example.com");
let id = patch_with_file(&repo, "feat", "src/lib.rs", "fn a() {}\n");
let before = collab_refs(&repo);
repo.run_err(&[
"patch", "comment", &id, "-b", "probe", "-f", "nope.rs", "-l", "1",
]);
assert_eq!(before, collab_refs(&repo), "a refusal must not move a ref");
}
// ===========================================================================
// 0655b32f: a comment that is a suggestion, not a demand
// ===========================================================================
#[test]
fn a_non_blocking_comment_is_marked_in_patch_show() {
let repo = TestRepo::new("Alice", "alice@example.com");
let id = patch_with_file(&repo, "feat", "src/lib.rs", "fn a() {}\nfn b() {}\n");
repo.run_ok(&[
"patch",
"comment",
&id,
"-b",
"could use a doc comment, land it anyway",
"-f",
"src/lib.rs",
"-l",
"1",
"--non-blocking",
]);
repo.run_ok(&[
"patch",
"comment",
&id,
"-b",
"this is a bug",
"-f",
"src/lib.rs",
"-l",
"2",
]);
let out = repo.run_ok(&["patch", "show", &id]);
let suggestion = out
.lines()
.find(|l| l.contains("src/lib.rs:1"))
.unwrap_or_else(|| panic!("no line for the suggestion: {}", out));
let demand = out
.lines()
.find(|l| l.contains("src/lib.rs:2"))
.unwrap_or_else(|| panic!("no line for the demand: {}", out));
assert!(
suggestion.contains("non-blocking"),
"the suggestion must say so: {}",
suggestion
);
assert!(
!demand.contains("non-blocking"),
"an unmarked comment is blocking, and must not be labelled: {}",
demand
);
}
#[test]
fn non_blocking_is_a_json_field_that_defaults_to_false() {
let repo = TestRepo::new("Alice", "alice@example.com");
let id = patch_with_file(&repo, "feat", "src/lib.rs", "fn a() {}\nfn b() {}\n");
repo.run_ok(&[
"patch",
"comment",
&id,
"-b",
"suggestion",
"-f",
"src/lib.rs",
"-l",
"1",
"--non-blocking",
]);
repo.run_ok(&[
"patch",
"comment",
&id,
"-b",
"demand",
"-f",
"src/lib.rs",
"-l",
"2",
]);
let out = repo.run_ok(&["patch", "show", &id, "--json"]);
let json: serde_json::Value = serde_json::from_str(&out).unwrap();
let inline = json["inline_comments"].as_array().unwrap();
assert_eq!(inline.len(), 2);
assert_eq!(
inline[0]["non_blocking"], true,
"scripted callers read JSON only: {}",
out
);
assert_eq!(
inline[1]["non_blocking"], false,
"the field must be present and false, not absent: {}",
out
);
}
#[test]
fn non_blocking_needs_an_anchor() {
let repo = TestRepo::new("Alice", "alice@example.com");
let id = patch_with_file(&repo, "feat", "src/lib.rs", "fn a() {}\n");
let err = repo.run_err(&["patch", "comment", &id, "-b", "nit", "--non-blocking"]);
assert!(
err.contains("--file") && err.contains("--at"),
"the error must name the fix: {}",
err
);
}
/// A verdict is a vote and a comment is a comment. Marking every inline comment
/// non-blocking says nothing about the vote the reviewer cast: `request-changes`
/// stays `request-changes` until its author changes it.
#[test]
fn request_changes_stays_blocking_when_every_comment_is_non_blocking() {
let repo = TestRepo::new("Alice", "alice@example.com");
let id = patch_with_file(&repo, "feat", "src/lib.rs", "fn a() {}\n");
repo.run_ok(&[
"patch",
"review",
&id,
"-v",
"request-changes",
"-b",
"see comments",
]);
repo.run_ok(&[
"patch",
"comment",
&id,
"-b",
"only a suggestion",
"-f",
"src/lib.rs",
"-l",
"1",
"--non-blocking",
]);
let out = repo.run_ok(&["patch", "show", &id, "--json"]);
let json: serde_json::Value = serde_json::from_str(&out).unwrap();
assert_eq!(
json["reviews"][0]["verdict"], "request-changes",
"comment metadata must not rewrite a vote: {}",
out
);
}
// ===========================================================================
// 9a0a143b: a diff a reviewer can comment from
// ===========================================================================
/// The reported `c/src/cli.rs c/src/cli.rs` is what libgit2 emits under
/// `diff.mnemonicPrefix`, which the reporter had set globally. So the prefixes
/// were not merely wrong, they varied by whose config rendered the diff. This
/// pins them against the config that produced the bug.
#[test]
fn diff_headers_use_a_and_b_prefixes() {
let repo = TestRepo::new("Alice", "alice@example.com");
repo.git(&["config", "diff.mnemonicPrefix", "true"]);
let id = patch_with_file(&repo, "feat", "src/lib.rs", "fn a() {}\n");
// Modify a tracked file rather than add one, so both sides have a path.
repo.git(&["checkout", "feat"]);
repo.commit_file("src/lib.rs", "fn a() {}\nfn b() {}\n", "and another");
repo.git(&["checkout", "main"]);
repo.run_ok(&["patch", "revise", &id, "-b", "r2", "-B", "feat"]);
let out = repo.run_ok(&["patch", "diff", &id, "--between", "1", "2"]);
assert!(
out.contains("diff --git a/src/lib.rs b/src/lib.rs"),
"old and new must be distinguishable in the header: {}",
out
);
assert!(
out.contains("--- a/src/lib.rs"),
"old side must be a/: {}",
out
);
assert!(
out.contains("+++ b/src/lib.rs"),
"new side must be b/: {}",
out
);
}
/// The whole point: what the reviewer copies out of the diff goes straight into
/// `patch comment` with no editing.
#[test]
fn a_line_numbered_diff_yields_an_anchor_that_pastes_verbatim() {
let repo = TestRepo::new("Alice", "alice@example.com");
let id = patch_with_file(&repo, "feat", "src/lib.rs", "fn a() {}\nfn b() {}\n");
let out = repo.run_ok(&["patch", "diff", &id, "--line-numbers"]);
let anchor = out
.lines()
.find_map(|l| {
let token = l.split_whitespace().next()?;
token.starts_with("src/lib.rs:").then(|| token.to_string())
})
.unwrap_or_else(|| panic!("no pasteable anchor in the gutter:\n{}", out));
assert_eq!(anchor, "src/lib.rs:1");
let confirm = repo.run_ok(&["patch", "comment", &id, "-b", "here", "--at", &anchor]);
assert!(confirm.contains("src/lib.rs:1"), "unexpected: {}", confirm);
}
#[test]
fn a_removed_line_gets_no_new_side_anchor() {
let repo = TestRepo::new("Alice", "alice@example.com");
repo.commit_file("f.txt", "keep\ndrop\n", "base");
let id = patch_with_file(&repo, "feat", "other.txt", "x\n");
repo.git(&["checkout", "feat"]);
repo.commit_file("f.txt", "keep\n", "remove a line");
repo.git(&["checkout", "main"]);
repo.run_ok(&["patch", "revise", &id, "-b", "r2", "-B", "feat"]);
let out = repo.run_ok(&["patch", "diff", &id, "--line-numbers"]);
let removed = out
.lines()
.find(|l| l.trim_end().ends_with("-drop"))
.unwrap_or_else(|| panic!("no removed line in:\n{}", out));
assert!(
!removed.contains("f.txt:"),
"a deleted line is not in the new side and cannot be anchored to: {}",
removed
);
}
#[test]
fn diff_stat_gives_a_per_file_breakdown() {
let repo = TestRepo::new("Alice", "alice@example.com");
repo.git(&["checkout", "-b", "feat"]);
repo.commit_file("one.txt", "a\nb\n", "one");
repo.commit_file("two.txt", "c\n", "two");
let out = repo.run_ok(&["patch", "create", "-t", "two files", "-B", "feat"]);
repo.git(&["checkout", "main"]);
let id = out.trim().strip_prefix("Created patch ").unwrap();
let stat = repo.run_ok(&["patch", "diff", id, "--stat"]);
assert!(stat.contains("one.txt"), "unexpected stat: {}", stat);
assert!(stat.contains("two.txt"), "unexpected stat: {}", stat);
assert!(
!stat.contains("+a"),
"--stat is a summary, not the diff body: {}",
stat
);
}
#[test]
fn diff_can_be_scoped_to_one_path() {
let repo = TestRepo::new("Alice", "alice@example.com");
repo.git(&["checkout", "-b", "feat"]);
repo.commit_file("one.txt", "a\n", "one");
repo.commit_file("two.txt", "c\n", "two");
let out = repo.run_ok(&["patch", "create", "-t", "two files", "-B", "feat"]);
repo.git(&["checkout", "main"]);
let id = out.trim().strip_prefix("Created patch ").unwrap();
let scoped = repo.run_ok(&["patch", "diff", id, "--path", "one.txt"]);
assert!(scoped.contains("one.txt"), "unexpected: {}", scoped);
assert!(
!scoped.contains("two.txt"),
"--path must exclude everything else: {}",
scoped
);
}
/// "No diff available (commits may be identical)" is true of a patch with no
/// changes and false of a `--path` nobody in the patch touched. One message for
/// two situations is the same defect as accepting an unresolvable anchor.
#[test]
fn a_path_filter_that_matches_nothing_says_so() {
let repo = TestRepo::new("Alice", "alice@example.com");
let id = patch_with_file(&repo, "feat", "one.txt", "a\n");
let err = repo.run_err(&["patch", "diff", &id, "--path", "nosuch.txt"]);
assert!(
err.contains("nosuch.txt"),
"the error must name the path that matched nothing: {}",
err
);
assert!(
err.contains("--stat"),
"the error must name the fix — how to find the paths the patch has: {}",
err
);
}
#[test]
fn stat_and_line_numbers_are_refused_together() {
let repo = TestRepo::new("Alice", "alice@example.com");
let id = patch_with_file(&repo, "feat", "one.txt", "a\n");
let err = repo.run_err(&["patch", "diff", &id, "--stat", "--line-numbers"]);
assert!(
err.contains("--stat") && err.contains("--line-numbers"),
"the error must name both flags and which to drop: {}",
err
);
}
#[test]
fn rendering_a_diff_moves_no_ref() {
let repo = TestRepo::new("Alice", "alice@example.com");
let id = patch_with_file(&repo, "feat", "src/lib.rs", "fn a() {}\n");
let before = collab_refs(&repo);
repo.run_ok(&["patch", "diff", &id]);
repo.run_ok(&["patch", "diff", &id, "--line-numbers"]);
repo.run_ok(&["patch", "diff", &id, "--stat"]);
repo.run_ok(&["patch", "diff", &id, "--path", "src/lib.rs"]);
assert_eq!(
before,
collab_refs(&repo),
"rendering a diff is a read and must write nothing"
);
}
#[test]
fn at_and_file_line_are_two_ways_to_say_one_thing() {
let repo = TestRepo::new("Alice", "alice@example.com");
let id = patch_with_file(&repo, "feat", "src/lib.rs", "fn a() {}\n");
let err = repo.run_err(&[
"patch",
"comment",
&id,
"-b",
"x",
"--at",
"src/lib.rs:1",
"-f",
"src/lib.rs",
"-l",
"1",
]);
assert!(
err.contains("--at"),
"naming the anchor twice is a mistake worth reporting: {}",
err
);
}
#[test]
fn a_malformed_at_names_the_form_it_wanted() {
let repo = TestRepo::new("Alice", "alice@example.com");
let id = patch_with_file(&repo, "feat", "src/lib.rs", "fn a() {}\n");
let err = repo.run_err(&["patch", "comment", &id, "-b", "x", "--at", "src/lib.rs"]);
assert!(
err.contains("<path>:<line>"),
"the error must show the form: {}",
err
);
}