fix!: handle symbolic ref updates far more gracefully and with more l…

…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
* existing refs would not become 'unborn', i.e. point to a reference that doesn't exist and won't be created due to ref-specs

This makes it possible to update unborn remote refs to local refs if these are new or unborn themselves.
This also 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.

gix/src/reference/log.rs

@@ -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.

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

@@ -19,6 +19,10 @@ mod error {
         OpenWorktreeRepo(#[from] crate::open::Error),
         #[error("Could not find local commit for fast-forward ancestor check")]
         FindCommit(#[from] crate::object::find::existing::Error),
+        #[error("Could not peel symbolic local reference to its ID")]
+        PeelToId(#[from] crate::reference::peel::Error),
+        #[error("Failed to follow a symbolic reference to assure worktree isn't affected")]
+        FollowSymref(#[from] gix_ref::file::find::existing::Error),
     }
 }
 
@@ -35,11 +39,14 @@ pub struct Outcome {
     pub updates: Vec<super::Update>,
 }
 
-/// Describe the way a ref was updated
+/// Describe the way a ref was updated, with particular focus on how the (peeled) target commit was affected.
+///
+/// Note that for all the variants that signal a change or `NoChangeNeeded` it's additionally possible to change the target type
+/// from symbolic to direct, or the other way around.
 #[derive(Debug, Clone, PartialEq, Eq)]
 pub enum Mode {
     /// No change was attempted as the remote ref didn't change compared to the current ref, or because no remote ref was specified
-    /// in the ref-spec.
+    /// in the ref-spec. Note that the expected value is still asserted to uncover potential race conditions with other processes.
     NoChangeNeeded,
     /// The old ref's commit was an ancestor of the new one, allowing for a fast-forward without a merge.
     FastForward,
@@ -62,14 +69,19 @@ pub enum Mode {
     RejectedTagUpdate,
     /// The reference update would not have been a fast-forward, and force is not specified in the ref-spec.
     RejectedNonFastForward,
-    /// The update of a local symbolic reference was rejected.
-    RejectedSymbolic,
+    /// The remote has an unborn symbolic reference where we have one that is set. This means the remote
+    /// has reset itself to a newly initialized state or a state that is highly unusual.
+    /// It may also mean that the remote knows the target name, but it's not available locally and not included in the ref-mappings
+    /// to be created, so we would effectively change a valid local ref into one that seems unborn, which is rejected.
+    /// Note that this mode may have an associated ref-edit that is a no-op, or current-state assertion, for logistical reasons only
+    /// and having no edit would be preferred.
+    RejectedToReplaceWithUnborn,
     /// The update was rejected because the branch is checked out in the given worktree_dir.
     ///
     /// Note that the check applies to any known worktree, whether it's present on disk or not.
     RejectedCurrentlyCheckedOut {
-        /// The path to the worktree directory where the branch is checked out.
-        worktree_dir: PathBuf,
+        /// The path(s) to the worktree directory where the branch is checked out.
+        worktree_dirs: Vec<PathBuf>,
     },
 }
 
@@ -84,19 +96,32 @@ impl std::fmt::Display for Mode {
             Mode::RejectedSourceObjectNotFound { id } => return write!(f, "rejected ({id} not found)"),
             Mode::RejectedTagUpdate => "rejected (would overwrite existing tag)",
             Mode::RejectedNonFastForward => "rejected (non-fast-forward)",
-            Mode::RejectedSymbolic => "rejected (refusing to write symbolic refs)",
-            Mode::RejectedCurrentlyCheckedOut { worktree_dir } => {
+            Mode::RejectedToReplaceWithUnborn => "rejected (refusing to overwrite existing with unborn ref)",
+            Mode::RejectedCurrentlyCheckedOut { worktree_dirs } => {
                 return write!(
                     f,
                     "rejected (cannot write into checked-out branch at \"{}\")",
-                    worktree_dir.display()
+                    worktree_dirs
+                        .iter()
+                        .filter_map(|d| d.to_str())
+                        .collect::<Vec<_>>()
+                        .join(", ")
                 )
             }
         }
         .fmt(f)
     }
 }
 
+/// Indicates that a ref changes its type.
+#[derive(Debug, Clone, Copy, PartialEq, Eq, Ord, PartialOrd, Hash)]
+pub enum TypeChange {
+    /// A local direct reference is changed into a symbolic one.
+    DirectToSymbolic,
+    /// A local symbolic reference is changed into a direct one.
+    SymbolicToDirect,
+}
+
 impl Outcome {
     /// Produce an iterator over all information used to produce the this outcome, ref-update by ref-update, using the `mappings`
     /// used when producing the ref update.

gix/src/remote/fetch.rs

@@ -154,6 +154,18 @@ impl Source {
         }
     }
 
+    /// Return the target that this symbolic ref is pointing to, or `None` if it is no symbolic ref.
+    pub fn as_target(&self) -> Option<&crate::bstr::BStr> {
+        match self {
+            Source::ObjectId(_) => None,
+            Source::Ref(r) => match r {
+                gix_protocol::handshake::Ref::Peeled { .. } | gix_protocol::handshake::Ref::Direct { .. } => None,
+                gix_protocol::handshake::Ref::Symbolic { target, .. }
+                | gix_protocol::handshake::Ref::Unborn { target, .. } => Some(target.as_ref()),
+            },
+        }
+    }
+
     /// Returns the peeled id of this instance, that is the object that can't be de-referenced anymore.
     pub fn peeled_id(&self) -> Option<&gix_hash::oid> {
         match self {

gix/tests/clone/mod.rs

@@ -348,8 +348,22 @@ mod blocking_io {
                             .expect("computable")
                             .1
                             .starts_with_str(remote_name));
-                        let mut logs = r.log_iter();
-                        assert_reflog(logs.all());
+                        match r.target() {
+                            TargetRef::Peeled(_) => {
+                                let mut logs = r.log_iter();
+                                assert_reflog(logs.all());
+                            }
+                            TargetRef::Symbolic(_) => {
+                                // TODO: it *should* be possible to set the reflog here based on the referent if deref = true
+                                //       when setting up the edits. But it doesn't seem to work. Also, some tests are
+                                //       missing for `leaf_referent_previous_oid`.
+                                assert!(
+                                    !r.log_exists(),
+                                    "symbolic refs don't have object ids, so they can't get \
+                                      into the reflog as these need previous and new oid"
+                                )
+                            }
+                        }
                     }
                 }
                 let mut out_of_graph_tags = Vec::new();

gix/tests/fixtures/make_remote_repos.sh

@@ -350,3 +350,22 @@ function optimize_repo() {
     optimize_repo
   )
 )
+
+git init unborn
+(cd unborn
+  git symbolic-ref refs/heads/existing-unborn-symbolic refs/heads/main
+  git symbolic-ref refs/heads/existing-unborn-symbolic-other refs/heads/other
+)
+
+git init one-commit-with-symref
+(cd 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 checkout main
+)
+
+git clone one-commit-with-symref one-commit-with-symref-missing-branch
+(cd one-commit-with-symref-missing-branch
+  git branch valid-locally
+)

gix/tests/remote/fetch.rs

@@ -559,10 +559,23 @@ mod blocking_and_async_io {
                         );
                         let edit = &update_refs.edits[1];
                         assert_eq!(edit.name.as_bstr(), "refs/remotes/changes-on-top-of-origin/symbolic");
-                        assert!(
-                            edit.change.new_value().expect("no deletion").try_id().is_some(),
-                            "on the remote this is a symbolic ref, we just write its destination object id though"
-                        );
+                        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"
+                                );
+                            }
+                            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"
+                                );
+                            }
+                            gix::protocol::transport::Protocol::V0 => {
+                                unreachable!("we don't test this here as there is no need")
+                            }
+                        }
 
                         assert!(
                             !write_pack_bundle.keep_path.map_or(false, |f| f.is_file()),
@@ -589,10 +602,12 @@ mod blocking_and_async_io {
                     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::New,
+                            type_change: None,
                             edit_index: Some(1),
                         }
                     ]
@@ -604,21 +619,32 @@ mod blocking_and_async_io {
                 ) {
                     let edit = edit.expect("refedit present even if it's a no-op");
                     if dry_run {
-                        assert_eq!(
-                            edit.change.new_value().expect("no deletions").id(),
-                            mapping.remote.as_id().expect("no unborn")
-                        );
+                        match edit.change.new_value().expect("no deletions") {
+                            gix_ref::TargetRef::Peeled(id) => {
+                                assert_eq!(id, mapping.remote.as_id().expect("no unborn"))
+                            }
+                            gix_ref::TargetRef::Symbolic(target) => {
+                                assert_eq!(target.as_bstr(), mapping.remote.as_target().expect("no direct ref"))
+                            }
+                        }
                         assert!(
                             repo.try_find_reference(edit.name.as_ref())?.is_none(),
                             "no ref created in dry-run mode"
                         );
                     } else {
                         let r = repo.find_reference(edit.name.as_ref()).unwrap();
-                        assert_eq!(
-                            r.id(),
-                            *mapping.remote.as_id().expect("no unborn"),
-                            "local reference should point to remote id"
-                        );
+                        match r.target() {
+                            gix_ref::TargetRef::Peeled(id) => {
+                                assert_eq!(
+                                    id,
+                                    mapping.remote.as_id().expect("no unborn"),
+                                    "local reference should point to remote id"
+                                );
+                            }
+                            gix_ref::TargetRef::Symbolic(target) => {
+                                assert_eq!(target.as_bstr(), mapping.remote.as_target().expect("no direct ref"))
+                            }
+                        }
                     }
                 }
             }