Skip to content

Commit

Permalink
finally figure out how symbolic ref updates should work (#450)
Browse files Browse the repository at this point in the history
It's still to be clarified what git would do, and if git does that at
all. For now this is done by hand unfortunately, but at least the rules
presented here seem correct.
  • Loading branch information
Byron committed Oct 31, 2022
1 parent 61cd430 commit 376829a
Show file tree
Hide file tree
Showing 3 changed files with 48 additions and 15 deletions.
40 changes: 33 additions & 7 deletions git-repository/src/remote/connection/fetch/update_refs/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -196,9 +196,34 @@ mod update {
}

#[test]
fn local_symbolic_refs_are_never_written() {
// TODO: symbolic -> other symbolic is OK, and so is symbolic -> peeled and peeled -> symbolic but only if forced
#[ignore]
fn local_symbolic_refs_are_never_written_unless_forced() {
let repo = repo("two-origins");
for source in ["refs/heads/main", "refs/heads/symbolic", "HEAD"] {
let (mappings, specs) = mapping_from_spec(&format!("{source}:refs/heads/symbolic"), &repo);
let out = fetch::refs::update(&repo, prefixed("action"), &mappings, &specs, fetch::DryRun::Yes).unwrap();

assert_eq!(out.edits.len(), 0);
assert_eq!(
out.updates,
vec![fetch::refs::Update {
mode: fetch::refs::update::Mode::RejectedSymbolic,
edit_index: None
}],
"remote ref '{source}' assertion"
);
}
}

#[test]
#[ignore]
fn local_refs_are_never_written_with_symbolic_ones_unless_forced() {
let repo = repo("two-origins");
let (mappings, specs) = mapping_from_spec("refs/heads/main:refs/heads/symbolic", &repo);
let (mappings, specs) = mapping_from_spec(
&format!("refs/heads/symbolic:refs/heads/not-currently-checked-out"),
&repo,
);
let out = fetch::refs::update(&repo, prefixed("action"), &mappings, &specs, fetch::DryRun::Yes).unwrap();

assert_eq!(out.edits.len(), 0);
Expand Down Expand Up @@ -245,7 +270,7 @@ mod update {

#[test]
#[ignore]
fn remote_symbolic_refs_can_be_written_locally_if_new() {
fn remote_symbolic_refs_can_be_written_locally_and_point_to_tracking_branch() {
let repo = repo("two-origins");
let (mappings, specs) = mapping_from_spec("HEAD:refs/remotes/origin/HEAD", &repo);
let out = fetch::refs::update(&repo, prefixed("action"), &mappings, &specs, fetch::DryRun::Yes).unwrap();
Expand All @@ -262,16 +287,17 @@ mod update {
match &edit.change {
Change::Update { log, new, .. } => {
assert_eq!(log.message, "action: storing head");
assert!(
new.try_id().is_some(),
"remote is peeled, so local will be peeled as well"
assert_eq!(
new.try_name().expect("symbolic name").as_bstr(),
"refs/remotes/origin/main",
"remote is symbolic, so local will be symbolic as well, but is rewritten to tracking branch"
);
}
_ => unreachable!("only updates"),
}
assert_eq!(
edit.name.as_bstr(),
"refs/heads/HEAD",
"refs/remotes/origin/HEAD",
"it's not possible to refer to the local HEAD with refspecs"
);
}
Expand Down
1 change: 1 addition & 0 deletions git-repository/tests/fixtures/make_fetch_repos.sh
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ git clone --shared base clone-as-base-with-changes
git add new-file
git commit -m "add new-file"
git tag -m "new-file introduction" v1.0
git symbolic-ref refs/heads/symbolic refs/tags/v1.0
)

git clone --shared base two-origins
Expand Down
22 changes: 14 additions & 8 deletions git-repository/tests/remote/fetch.rs
Original file line number Diff line number Diff line change
Expand Up @@ -83,8 +83,8 @@ mod blocking_io {
),
(
Some(git::protocol::transport::Protocol::V1),
3,
"c75114f60ab2c9389916f3de1082bbaa47491e3b",
4,
"d07c527cf14e524a8494ce6d5d08e28079f5c6ea", // TODO: if these are the same, remove them
),
] {
let (mut repo, _tmp) = repo_rw("two-origins");
Expand Down Expand Up @@ -128,15 +128,15 @@ mod blocking_io {
} => {
assert_eq!(write_pack_bundle.pack_version, git::odb::pack::data::Version::V2);
assert_eq!(write_pack_bundle.object_hash, repo.object_hash());
assert_eq!(write_pack_bundle.index.num_objects, expected_objects, "this value is 4 when git does it with 'consecutive' negotiation style, but could be 33 if completely naive.");
assert_eq!(write_pack_bundle.index.num_objects, expected_objects, "{dry_run}: this value is 4 when git does it with 'consecutive' negotiation style, but could be 33 if completely naive.");
assert_eq!(
write_pack_bundle.index.index_version,
git::odb::pack::index::Version::V2
);
assert_eq!(write_pack_bundle.index.index_hash, hex_to_id(expected_hash));
assert!(write_pack_bundle.data_path.map_or(false, |f| f.is_file()));
assert!(write_pack_bundle.index_path.map_or(false, |f| f.is_file()));
assert_eq!(update_refs.edits.len(), 1);
assert_eq!(update_refs.edits.len(), if dry_run { 1 } else { 2 }); // TODO: why is this?
assert!(
!write_pack_bundle.keep_path.map_or(false, |f| f.is_file()),
".keep files are deleted if there is one edit"
Expand All @@ -150,10 +150,16 @@ mod blocking_io {

assert_eq!(
refs.updates,
vec![fetch::refs::Update {
mode: fetch::refs::update::Mode::New,
edit_index: Some(0),
}]
vec![
fetch::refs::Update {
mode: fetch::refs::update::Mode::New,
edit_index: Some(0),
},
fetch::refs::Update {
mode: fetch::refs::update::Mode::New,
edit_index: Some(1),
}
]
);
for (_update, mapping, _spec, edit) in
refs.iter_mapping_updates(&outcome.ref_map.mappings, remote.refspecs(Fetch))
Expand Down

0 comments on commit 376829a

Please sign in to comment.