Fix #922 and uncover additional bugs

CHANGELOG.md

@@ -105,6 +105,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 * It is no longer allowed to create branches at the root commit.
 
+* In colocated repos, a bug causing conflicts when undoing branch moves (#922)
+  has been fixed. There remain some known similar, but less severe, bugs when
+  `jj undo` interacts with `jj git push`, for example, in colocated repos.
+
 ## [0.7.0] - 2023-02-16
 
 ### Breaking changes

lib/gen-protos/src/main.rs

@@ -16,7 +16,12 @@ use std::io::Result;
 use std::path::Path;
 
 fn main() -> Result<()> {
-    let input = ["op_store.proto", "store.proto", "working_copy.proto"];
+    let input = [
+        "git_ref_view.proto",
+        "op_store.proto",
+        "store.proto",
+        "working_copy.proto",
+    ];
 
     let root = Path::new(env!("CARGO_MANIFEST_DIR")).parent().unwrap();
     let protos_dir = root.join("src").join("protos");

lib/src/git_ref_view.rs

@@ -0,0 +1,119 @@
+// Copyright 2023 The Jujutsu Authors
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// https://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+use std::collections::BTreeMap;
+use std::fmt::Debug;
+use std::fs;
+use std::path::PathBuf;
+
+use prost::Message;
+use thiserror::Error;
+
+use crate::backend::{CommitId, ObjectId};
+
+// TODO: consider making this more expressive. Currently, it's based on
+// OpStoreError
+#[derive(Debug, Error)]
+pub enum GitRefViewError {
+    #[error("{0}")]
+    Other(String),
+}
+
+impl From<std::io::Error> for GitRefViewError {
+    fn from(err: std::io::Error) -> Self {
+        GitRefViewError::Other(err.to_string())
+    }
+}
+
+impl From<prost::DecodeError> for GitRefViewError {
+    fn from(err: prost::DecodeError) -> Self {
+        GitRefViewError::Other(err.to_string())
+    }
+}
+
+pub type RefName = String;
+
+#[derive(Clone, Debug, Default, PartialEq, Eq)]
+pub struct GitRefView {
+    pub refs: BTreeMap<RefName, CommitId>,
+}
+
+type GitRefViewProto = crate::protos::git_ref_view::GitRefView;
+type GitRefProto = crate::protos::git_ref_view::GitRef;
+
+impl GitRefView {
+    pub fn read_view_from_file(view_path: PathBuf) -> Result<Self, GitRefViewError> {
+        let buf = fs::read(view_path)?;
+
+        let proto = GitRefViewProto::decode(&*buf)?;
+        Ok(view_from_proto(proto))
+    }
+
+    pub fn write_view_to_file(&self, path: PathBuf) -> Result<(), GitRefViewError> {
+        let proto = view_to_proto(self);
+        Ok(std::fs::write(path, proto.encode_to_vec())?)
+    }
+}
+
+fn view_to_proto(ref_view: &GitRefView) -> GitRefViewProto {
+    let mut proto = GitRefViewProto::default();
+    for (ref_name, commit_id) in &ref_view.refs {
+        proto.refs.push(GitRefProto {
+            name: ref_name.clone(),
+            commit_id: commit_id.to_bytes(),
+        });
+    }
+
+    proto
+}
+
+fn view_from_proto(proto: GitRefViewProto) -> GitRefView {
+    let mut view = GitRefView::default();
+    for GitRefProto {
+        name,
+        commit_id: id_bytes,
+    } in proto.refs
+    {
+        view.refs.insert(name, CommitId::new(id_bytes));
+    }
+
+    view
+}
+
+#[cfg(test)]
+mod tests {
+
+    use maplit::btreemap;
+
+    use super::*;
+    use crate::backend::{CommitId, ObjectId};
+
+    fn create_view() -> GitRefView {
+        GitRefView {
+            refs: btreemap! {
+                "ref1".to_string() => CommitId::from_hex("aaa111"),
+                "ref2".to_string() => CommitId::from_hex("aaa222"),
+            },
+        }
+    }
+
+    #[test]
+    fn test_read_write_view() {
+        let temp_file = testutils::new_temp_dir().into_path().join("git_ref_view");
+        let view = create_view();
+        view.write_view_to_file(temp_file.clone()).unwrap();
+        let read_view = GitRefView::read_view_from_file(temp_file).unwrap();
+        assert_eq!(read_view, view);
+    }
+}

lib/src/lib.rs

@@ -16,6 +16,7 @@
 
 #[macro_use]
 mod content_hash;
+mod git_ref_view;
 
 pub mod backend;
 pub mod commit;

lib/src/protos/git_ref_view.proto

@@ -0,0 +1,34 @@
+// Copyright 2023 The Jujutsu Authors
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// https://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+syntax = "proto3";
+
+package git_ref_view;
+
+message GitRef {
+  string name = 1;
+  bytes commit_id = 2;
+}
+
+// This is the view of the last seen state of refs in the backing Git
+// repository. Unlike the refs in jj's "View", these are not affected by `jj
+// undo`. They also do not support conflicted states. Note that this implies
+// that this data does not support concurrent changes and should be modified
+// under lock.
+message GitRefView {
+    // TODO: We may eventually also track the HEAD (or HEADs if we support
+    // multiple worktrees) here as well. For now, we do not allow it to be
+    // conflicted.
+    repeated GitRef refs = 1;
+}

lib/src/protos/git_ref_view.rs

@@ -0,0 +1,22 @@
+#[allow(clippy::derive_partial_eq_without_eq)]
+#[derive(Clone, PartialEq, ::prost::Message)]
+pub struct GitRef {
+    #[prost(string, tag = "1")]
+    pub name: ::prost::alloc::string::String,
+    #[prost(bytes = "vec", tag = "2")]
+    pub commit_id: ::prost::alloc::vec::Vec<u8>,
+}
+/// This is the view of the last seen state of refs in the backing Git
+/// repository. Unlike the refs in jj's "View", these are not affected by `jj
+/// undo`. They also do not support conflicted states. Note that this implies
+/// that this data does not support concurrent changes and should be modified
+/// under lock.
+#[allow(clippy::derive_partial_eq_without_eq)]
+#[derive(Clone, PartialEq, ::prost::Message)]
+pub struct GitRefView {
+    /// TODO: We may eventually also track the HEAD (or HEADs if we support
+    /// multiple worktrees) here as well. For now, we do not allow it to be
+    /// conflicted.
+    #[prost(message, repeated, tag = "1")]
+    pub refs: ::prost::alloc::vec::Vec<GitRef>,
+}

lib/src/protos/mod.rs

@@ -1,3 +1,6 @@
+pub mod git_ref_view {
+    include!("git_ref_view.rs");
+}
 pub mod op_store {
     include!("op_store.rs");
 }

lib/tests/test_git.rs

@@ -799,6 +799,74 @@ fn test_export_import_sequence() {
     );
 }
 
+// This test is just like `test_export_import_sequence`, but deletes the "last seen git refs" file between import/export operations. Unless there are `undo` or `restore` operations, jj should be able to recover these refs from its view of git refs.
+//
+// The purpose is to make sure that people who upgrade from a version of jj that
+// didn't generate the "last seen git refs" file to one that does, the new
+// version can use the old version's data.
+//
+// TODO: This test was added before jj 0.8.0, remove it sometime after jj 0.10.0
+// release.
+#[test]
+fn test_export_import_sequence_without_git_repo_view_file() {
+    // Import a branch pointing to A, modify it in jj to point to B, export it,
+    // modify it in git to point to C, then import it again. There should be no
+    // conflict.
+    let test_data = GitRepoData::create();
+    let git_settings = GitSettings::default();
+    let git_repo = test_data.git_repo;
+    let mut tx = test_data
+        .repo
+        .start_transaction(&test_data.settings, "test");
+    let mut_repo = tx.mut_repo();
+    let commit_a = write_random_commit(mut_repo, &test_data.settings);
+    let commit_b = write_random_commit(mut_repo, &test_data.settings);
+    let commit_c = write_random_commit(mut_repo, &test_data.settings);
+    // TODO: Create the git_last_seen_refs file in `jj git init`, uncomment the
+    // following line.
+    // std::fs::remove_file(mut_repo.base_repo().repo_path()
+    //    .join("git_last_seen_refs")).unwrap();
+
+    // Import the branch pointing to A
+    git_repo
+        .reference("refs/heads/main", git_id(&commit_a), true, "test")
+        .unwrap();
+    git::import_refs(mut_repo, &git_repo, &git_settings).unwrap();
+    std::fs::remove_file(mut_repo.base_repo().repo_path().join("git_last_seen_refs")).unwrap();
+    assert_eq!(
+        mut_repo.get_git_ref("refs/heads/main"),
+        Some(RefTarget::Normal(commit_a.id().clone()))
+    );
+
+    // Modify the branch in jj to point to B
+    mut_repo.set_local_branch("main".to_string(), RefTarget::Normal(commit_b.id().clone()));
+
+    // Export the branch to git
+    assert_eq!(git::export_refs(mut_repo, &git_repo), Ok(vec![]));
+    std::fs::remove_file(mut_repo.base_repo().repo_path().join("git_last_seen_refs")).unwrap();
+    assert_eq!(
+        mut_repo.get_git_ref("refs/heads/main"),
+        Some(RefTarget::Normal(commit_b.id().clone()))
+    );
+
+    // Modify the branch in git to point to C
+    git_repo
+        .reference("refs/heads/main", git_id(&commit_c), true, "test")
+        .unwrap();
+
+    // Import from git
+    git::import_refs(mut_repo, &git_repo, &git_settings).unwrap();
+    std::fs::remove_file(mut_repo.base_repo().repo_path().join("git_last_seen_refs")).unwrap();
+    assert_eq!(
+        mut_repo.get_git_ref("refs/heads/main"),
+        Some(RefTarget::Normal(commit_c.id().clone()))
+    );
+    assert_eq!(
+        mut_repo.view().get_local_branch("main"),
+        Some(RefTarget::Normal(commit_c.id().clone()))
+    );
+}
+
 #[test]
 fn test_import_export_no_auto_local_branch() {
     // Import a remote tracking branch and export it. We should not create a git

src/commands/git.rs

@@ -489,6 +489,9 @@ fn do_git_clone(
         GitFetchError::InvalidGlob => {
             unreachable!("we didn't provide any globs")
         }
+        GitFetchError::StateReadWriteError(s) => {
+            CommandError::InternalError(format!("io error: {s}"))
+        }
     })?;
     fetch_tx.finish(ui)?;
     Ok((workspace_command, maybe_default_branch))