Skip to content

Commit

Permalink
fix!: handle symbolic ref updates far more gracefully and with more l…
Browse files Browse the repository at this point in the history 
…ogical consistency.

Previously, refspecs couldn't be used to update sybolic references locally, particularly because the logic
to do so correctly isn't trivial and `git` itself also seems to cover only the most common cases.

However, the logic now changed so that remote updates will only be rejected if

* fast-forward rules are violated
* the local ref is currently checked out

This makes it possible to update unborn remote refs to local refs if these are new or unborn themselves.
This allows to create mirrors more easily and allows us to handle `HEAD` without special casing it.
Bare repositories have an easier time to update local symbolic refs.
  • Loading branch information
@Byron
Byron committed Jul 31, 2023
1 parent d43364d commit 1dc045d
Show file tree
Hide file tree
Showing 8 changed files with 465 additions and 123 deletions.
5 changes: 5 additions & 0 deletions gix/src/reference/log.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,11 @@ impl<'repo> Reference<'repo> {
pub fn log_iter(&self) -> gix_ref::file::log::iter::Platform<'_, '_> {
self.inner.log_iter(&self.repo.refs)
}

/// Return true if a reflog is present for this reference.
pub fn log_exists(&self) -> bool {
self.inner.log_exists(&self.repo.refs)
}
}

/// Generate a message typical for git commit logs based on the given `operation`, commit `message` and `num_parents` of the commit.
Expand Down
262 changes: 182 additions & 80 deletions gix/src/remote/connection/fetch/update_refs/mod.rs
View file

Large diffs are not rendered by default.

194 changes: 175 additions & 19 deletions gix/src/remote/connection/fetch/update_refs/tests.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,8 +41,10 @@ mod update {
let repo = gix::open_opts(dir.path().join(name), restricted()).unwrap();
(repo, dir)
}
use gix_ref::{transaction::Change, TargetRef};
use gix_ref::transaction::{LogChange, PreviousValue, RefEdit, RefLog};
use gix_ref::{transaction::Change, Target, TargetRef};

use crate::remote::fetch::refs::update::TypeChange;
use crate::{
bstr::BString,
remote::{
Expand Down Expand Up @@ -112,7 +114,7 @@ mod update {
(
"+refs/remotes/origin/g:refs/heads/main",
fetch::refs::update::Mode::RejectedCurrentlyCheckedOut {
worktree_dir: repo.work_dir().expect("present").to_owned(),
worktree_dirs: vec![repo.work_dir().expect("present").to_owned()],
},
None,
"checked out branches cannot be written, as it requires a merge of sorts which isn't done here",
Expand Down Expand Up @@ -148,6 +150,7 @@ mod update {
assert_eq!(
out.updates,
vec![fetch::refs::Update {
type_change: None,
mode: expected_mode.clone(),
edit_index: reflog_message.map(|_| 0),
}],
Expand Down Expand Up @@ -211,8 +214,9 @@ mod update {
out.updates,
vec![fetch::refs::Update {
mode: fetch::refs::update::Mode::RejectedCurrentlyCheckedOut {
worktree_dir: root.join(path_from_root),
worktree_dirs: vec![root.join(path_from_root)],
},
type_change: None,
edit_index: None,
}],
"{spec}: checked-out checks are done before checking if a change would actually be required (here it isn't)"
Expand All @@ -223,9 +227,148 @@ mod update {
}

#[test]
fn local_symbolic_refs_are_never_written() {
fn unborn_remote_branches_can_be_created_locally_if_they_are_new() -> Result {
let repo = repo("unborn");
let (mappings, specs) = mapping_from_spec("HEAD:refs/remotes/origin/HEAD", &repo);
assert_eq!(mappings.len(), 1);
let out = fetch::refs::update(
&repo,
prefixed("action"),
&mappings,
&specs,
&[],
fetch::Tags::None,
fetch::DryRun::Yes,
fetch::WritePackedRefs::Never,
)?;
assert_eq!(
out.updates,
vec![fetch::refs::Update {
mode: fetch::refs::update::Mode::New,
type_change: None,
edit_index: Some(0)
}]
);
assert_eq!(out.edits.len(), 1, "we are OK with creating unborn refs");
Ok(())
}

#[test]
fn unborn_remote_branches_can_update_local_unborn_branches() -> Result {
let repo = repo("unborn");
let (mappings, specs) = mapping_from_spec("HEAD:refs/heads/existing-unborn-symbolic", &repo);
assert_eq!(mappings.len(), 1);
let out = fetch::refs::update(
&repo,
prefixed("action"),
&mappings,
&specs,
&[],
fetch::Tags::None,
fetch::DryRun::Yes,
fetch::WritePackedRefs::Never,
)?;
assert_eq!(
out.updates,
vec![fetch::refs::Update {
mode: fetch::refs::update::Mode::NoChangeNeeded,
type_change: None,
edit_index: Some(0)
}]
);
assert_eq!(out.edits.len(), 1, "we are OK with updating unborn refs");

let (mappings, specs) = mapping_from_spec("HEAD:refs/heads/existing-unborn-symbolic-other", &repo);
assert_eq!(mappings.len(), 1);
let out = fetch::refs::update(
&repo,
prefixed("action"),
&mappings,
&specs,
&[],
fetch::Tags::None,
fetch::DryRun::Yes,
fetch::WritePackedRefs::Never,
)?;
assert_eq!(
out.updates,
vec![fetch::refs::Update {
mode: fetch::refs::update::Mode::Forced,
type_change: None,
edit_index: Some(0)
}]
);
assert_eq!(
out.edits.len(),
1,
"we are OK with creating unborn refs even without actually forcing it"
);
Ok(())
}

#[test]
fn local_symbolic_refs_can_be_overwritten() {
let repo = repo("two-origins");
for source in ["refs/heads/main", "refs/heads/symbolic", "HEAD"] {
// refs/heads/symbolic points to `refs/heads/main`
for (source, expected_update, expected_edit) in [
(
// symbolic to direct
"refs/heads/main",
fetch::refs::Update {
mode: fetch::refs::update::Mode::NoChangeNeeded,
type_change: Some(TypeChange::SymbolicToDirect),
edit_index: Some(0),
},
Some(RefEdit {
change: Change::Update {
log: LogChange {
mode: RefLog::AndReference,
force_create_reflog: false,
message: "action: no update will be performed".into(),
},
expected: PreviousValue::MustExistAndMatch(Target::Symbolic(
"refs/heads/main".try_into().expect("valid"),
)),
new: Target::Peeled(hex_to_id("f99771fe6a1b535783af3163eba95a927aae21d5")),
},
name: "refs/heads/symbolic".try_into().expect("valid"),
deref: false,
}),
),
(
// symbolic to symbolic (same)
"refs/heads/symbolic",
fetch::refs::Update {
mode: fetch::refs::update::Mode::NoChangeNeeded,
type_change: None,
edit_index: Some(0),
},
Some(RefEdit {
change: Change::Update {
log: LogChange {
mode: RefLog::AndReference,
force_create_reflog: false,
message: "action: no update will be performed".into(),
},
expected: PreviousValue::MustExistAndMatch(Target::Symbolic(
"refs/heads/main".try_into().expect("valid"),
)),
new: Target::Symbolic("refs/heads/main".try_into().expect("valid")),
},
name: "refs/heads/symbolic".try_into().expect("valid"),
deref: false,
}),
),
(
"HEAD",
fetch::refs::Update {
mode: fetch::refs::update::Mode::NoChangeNeeded,
type_change: None,
edit_index: None,
},
None,
),
] {
let (mappings, specs) = mapping_from_spec(&format!("{source}:refs/heads/symbolic"), &repo);
let out = fetch::refs::update(
&repo,
Expand All @@ -239,15 +382,17 @@ mod update {
)
.unwrap();

assert_eq!(out.edits.len(), 0);
assert_eq!(
out.updates,
vec![fetch::refs::Update {
mode: fetch::refs::update::Mode::RejectedSymbolic,
edit_index: None
}],
"we don't overwrite these as the checked-out check needs to consider much more than it currently does, we are playing it safe"
);
if source == "HEAD" {
dbg!(&out.edits, &out.updates);
// todo!("handle HEAD: proper expectation");
} else {
dbg!(&out.edits, &out.updates);
assert_eq!(out.edits.len(), usize::from(expected_edit.is_some()));
assert_eq!(out.updates, vec![expected_update], "we can overwrite symbolic refs");
if let Some(expected) = expected_edit {
assert_eq!(out.edits, vec![expected]);
}
}
}
}

Expand Down Expand Up @@ -275,17 +420,19 @@ mod update {
)
.unwrap();

assert_eq!(out.edits.len(), 1);
assert_eq!(out.edits.len(), 2, "symbolic refs are handled just like any other ref");
assert_eq!(
out.updates,
vec![
fetch::refs::Update {
mode: fetch::refs::update::Mode::New,
type_change: None,
edit_index: Some(0)
},
fetch::refs::Update {
mode: fetch::refs::update::Mode::RejectedSymbolic,
edit_index: None
mode: fetch::refs::update::Mode::NoChangeNeeded,
type_change: Some(TypeChange::SymbolicToDirect),
edit_index: Some(1)
}
],
);
Expand All @@ -303,7 +450,7 @@ mod update {
}

#[test]
fn local_direct_refs_are_never_written_with_symbolic_ones_but_see_only_the_destination() {
fn local_direct_refs_are_written_with_symbolic_ones() {
let repo = repo("two-origins");
let (mappings, specs) = mapping_from_spec("refs/heads/symbolic:refs/heads/not-currently-checked-out", &repo);
let out = fetch::refs::update(
Expand All @@ -323,6 +470,7 @@ mod update {
out.updates,
vec![fetch::refs::Update {
mode: fetch::refs::update::Mode::NoChangeNeeded,
type_change: Some(fetch::refs::update::TypeChange::DirectToSymbolic),
edit_index: Some(0)
}],
);
Expand All @@ -349,6 +497,7 @@ mod update {
out.updates,
vec![fetch::refs::Update {
mode: fetch::refs::update::Mode::New,
type_change: None,
edit_index: Some(0),
}],
);
Expand Down Expand Up @@ -399,10 +548,12 @@ mod update {
vec![
fetch::refs::Update {
mode: fetch::refs::update::Mode::New,
type_change: None,
edit_index: Some(0),
},
fetch::refs::Update {
mode: fetch::refs::update::Mode::NoChangeNeeded,
type_change: None,
edit_index: Some(1),
}
],
Expand Down Expand Up @@ -446,6 +597,7 @@ mod update {
out.updates,
vec![fetch::refs::Update {
mode: fetch::refs::update::Mode::FastForward,
type_change: None,
edit_index: Some(0),
}],
"The caller has to be aware and note that dry-runs can't know about fast-forwards as they don't have remote objects"
Expand Down Expand Up @@ -480,6 +632,7 @@ mod update {
out.updates,
vec![fetch::refs::Update {
mode: fetch::refs::update::Mode::RejectedNonFastForward,
type_change: None,
edit_index: None,
}]
);
Expand All @@ -502,6 +655,7 @@ mod update {
out.updates,
vec![fetch::refs::Update {
mode: fetch::refs::update::Mode::FastForward,
type_change: None,
edit_index: Some(0),
}]
);
Expand Down Expand Up @@ -535,6 +689,7 @@ mod update {
out.updates,
vec![fetch::refs::Update {
mode: fetch::refs::update::Mode::FastForward,
type_change: None,
edit_index: Some(0),
}]
);
Expand Down Expand Up @@ -597,9 +752,10 @@ mod update {

fn remote_ref_to_item(r: &gix_protocol::handshake::Ref) -> gix_refspec::match_group::Item<'_> {
let (full_ref_name, target, object) = r.unpack();
static NULL: gix_hash::ObjectId = gix_hash::Kind::Sha1.null();
gix_refspec::match_group::Item {
full_ref_name,
target: target.expect("no unborn HEAD"),
target: target.unwrap_or(NULL.as_ref()),
object,
}
}
Expand Down
Loading

0 comments on commit 1dc045d

Please sign in to comment.