fix: local refs created during fetching will now always be valid.

Previously it could create symbolic refs that were effectively unborn, i.e.
point to a reference which doesn't exist.

Now these will always point to the peeled object instead.

Author: Byron

gix/src/remote/connection/fetch/update_refs/mod.rs

@@ -229,12 +229,12 @@ pub(crate) fn update(
                             Mode::New,
                             reflog_msg,
                             name,
-                            PreviousValue::ExistingMustMatch(new_value_by_remote(remote, mappings)?),
+                            PreviousValue::ExistingMustMatch(new_value_by_remote(repo, remote, mappings)?),
                         )
                     }
                 };
 
-                let new = new_value_by_remote(remote, mappings)?;
+                let new = new_value_by_remote(repo, remote, mappings)?;
                 let type_change = match (&previous_value, &new) {
                     (
                         PreviousValue::ExistingMustMatch(Target::Peeled(_))
@@ -387,7 +387,11 @@ fn update_needs_adjustment_as_edits_symbolic_target_is_missing(
     }
 }
 
-fn new_value_by_remote(remote: &Source, mappings: &[fetch::Mapping]) -> Result<Target, update::Error> {
+fn new_value_by_remote(
+    repo: &Repository,
+    remote: &Source,
+    mappings: &[fetch::Mapping],
+) -> Result<Target, update::Error> {
     let remote_id = remote.as_id();
     Ok(
         if let Source::Ref(
@@ -411,11 +415,22 @@ fn new_value_by_remote(remote: &Source, mappings: &[fetch::Mapping]) -> Result<T
                     Target::Symbolic(local_branch)
                 }
                 None => {
-                    // If we can't map it, it's usually a an unborn branch causing this.
-                    // If not, it means we might create an unborn branch and earlier we made sure that we don't turn
-                    // a perfectly good local branch unto an unborn branch.
-                    // However, we can freely change unborn local branches.
-                    Target::Symbolic(target.try_into()?)
+                    // If we can't map it, it's usually a an unborn branch causing this, or a the target isn't covered
+                    // by any refspec so we don't officially pull it in.
+                    match remote_id {
+                        Some(desired_id) => {
+                            if repo.try_find_reference(target)?.is_some() {
+                                // We are allowed to change a direct reference to a symbolic one, which may point to other objects
+                                // than the remote. The idea is that we are fine as long as the resulting refs are valid.
+                                Target::Symbolic(target.try_into()?)
+                            } else {
+                                // born branches that we don't have in our refspecs we create peeled. That way they can be used.
+                                Target::Peeled(desired_id.to_owned())
+                            }
+                        }
+                        // Unborn branches we create as such, with the location they point to on the remote which helps mirroring.
+                        None => Target::Symbolic(target.try_into()?),
+                    }
                 }
             }
         } else {

gix/src/remote/connection/fetch/update_refs/tests.rs

@@ -351,7 +351,7 @@ mod update {
     }
 
     #[test]
-    fn remote_symbolic_refs_with_locally_unavailable_target_result_in_inborn_branches() -> Result {
+    fn remote_symbolic_refs_with_locally_unavailable_target_result_in_valid_peeled_branches() -> Result {
         let remote_repo = named_repo("one-commit-with-symref");
         let local_repo = named_repo("unborn");
         let (mappings, specs) = mapping_from_spec("refs/heads/symbolic:refs/heads/new", &remote_repo);
@@ -376,6 +376,7 @@ mod update {
             }]
         );
         assert_eq!(out.edits.len(), 1);
+        let target = Target::Peeled(hex_to_id("66f16e4e8baf5c77bb6d0484495bebea80e916ce"));
         assert_eq!(
             out.edits[0],
             RefEdit {
@@ -385,10 +386,8 @@ mod update {
                         force_create_reflog: false,
                         message: "action: storing head".into(),
                     },
-                    expected: PreviousValue::ExistingMustMatch(Target::Symbolic(
-                        "refs/heads/branch".try_into().expect("valid"),
-                    )),
-                    new: Target::Symbolic("refs/heads/branch".try_into().expect("valid")),
+                    expected: PreviousValue::ExistingMustMatch(target.clone()),
+                    new: target,
                 },
                 name: "refs/heads/new".try_into().expect("valid"),
                 deref: false,
@@ -403,7 +402,7 @@ mod update {
     fn remote_symbolic_refs_with_locally_unavailable_target_dont_overwrite_valid_local_branches() -> Result {
         let remote_repo = named_repo("one-commit-with-symref");
         let local_repo = named_repo("one-commit-with-symref-missing-branch");
-        let (mappings, specs) = mapping_from_spec("refs/heads/symbolic:refs/heads/valid-locally", &remote_repo);
+        let (mappings, specs) = mapping_from_spec("refs/heads/unborn:refs/heads/valid-locally", &remote_repo);
         assert_eq!(mappings.len(), 1);
 
         let out = fetch::refs::update(
@@ -421,29 +420,10 @@ mod update {
             vec![fetch::refs::Update {
                 mode: fetch::refs::update::Mode::RejectedToReplaceWithUnborn,
                 type_change: None,
-                edit_index: Some(0)
+                edit_index: None
             }]
         );
-        assert_eq!(out.edits.len(), 1);
-        assert_eq!(
-            out.edits[0],
-            RefEdit {
-                change: Change::Update {
-                    log: LogChange {
-                        mode: RefLog::AndReference,
-                        force_create_reflog: false,
-                        message: "no-op".into(),
-                    },
-                    expected: PreviousValue::MustExistAndMatch(Target::Peeled(hex_to_id(
-                        "66f16e4e8baf5c77bb6d0484495bebea80e916ce"
-                    ))),
-                    new: Target::Peeled(hex_to_id("66f16e4e8baf5c77bb6d0484495bebea80e916ce")),
-                },
-                name: "refs/heads/valid-locally".try_into().expect("valid"),
-                deref: false,
-            },
-            "edits are created for logistical reasons (we don't currently update edit-indices) so have to create a no-op"
-        );
+        assert_eq!(out.edits.len(), 0);
         Ok(())
     }
 

gix/tests/fixtures/make_remote_repos.sh

@@ -363,6 +363,7 @@ git init one-commit-with-symref
   touch content && git add content && git commit -m "init"
   git checkout -b branch
   git symbolic-ref refs/heads/symbolic refs/heads/branch
+  git symbolic-ref refs/heads/unborn refs/heads/non-existing
   git checkout main
 )
 

gix/tests/remote/fetch.rs

@@ -563,14 +563,14 @@ mod blocking_and_async_io {
                         match version.unwrap_or_default() {
                             gix::protocol::transport::Protocol::V2 => {
                                 assert!(
-                                    edit.change.new_value().expect("no deletion").try_name().is_some(),
-                                    "{version:?} on the remote this is a symbolic ref, and we maintain it as symref information is kept"
+                                    edit.change.new_value().expect("no deletion").try_name().is_none(),
+                                    "{version:?} on the remote this is a symbolic ref to a tag, but we don't pull tags, hence we point to the tag object itself (instead of the refname)"
                                 );
                             }
                             gix::protocol::transport::Protocol::V1 => {
                                 assert!(
                                     edit.change.new_value().expect("no deletion").try_id().is_some(),
-                                    "on the remote this is a symbolic ref, but in V1 symrefs are not visible"
+                                    "on the remote this is a symbolic ref, but in V1 symrefs are never visible"
                                 );
                             }
                             gix::protocol::transport::Protocol::V0 => {