Represent git statuses more faithfully (zed-industries#23082)

First, parse the output of `git status --porcelain=v1` into a representation that can handle the full "grammar" and doesn't lose information. Second, as part of pushing this throughout the codebase, expand the use of the existing `GitSummary` type to all the places where status propagation is in play (i.e., anywhere we're dealing with a mix of files and directories), and get rid of the previous `GitSummary -> GitFileStatus` conversion. - [x] Synchronize new representation over collab - [x] Update zed.proto - [x] Update DB models - [x] Update `GitSummary` and summarization for the new `FileStatus` - [x] Fix all tests - [x] worktree - [x] collab - [x] Clean up `FILE_*` constants - [x] New collab tests to exercise syncing of complex statuses - [x] Run it locally and make sure it looks good Release Notes: - N/A --------- Co-authored-by: Mikayla <[email protected]> Co-authored-by: Conrad <[email protected]>
CharlesChen0823 · Jan 16, 2025 · a41d72e · a41d72e
1 parent 224f3d4
commit a41d72e
Show file tree

Hide file tree

Showing 24 changed files with 1,019 additions and 556 deletions.
diff --git a/crates/collab/migrations.sqlite/20221109000000_test_schema.sql b/crates/collab/migrations.sqlite/20221109000000_test_schema.sql
@@ -112,6 +112,9 @@ CREATE TABLE "worktree_repository_statuses" (
     "work_directory_id" INT8 NOT NULL,
     "repo_path" VARCHAR NOT NULL,
     "status" INT8 NOT NULL,
+    "status_kind" INT4 NOT NULL,
+    "first_status" INT4 NULL,
+    "second_status" INT4 NULL,
     "scan_id" INT8 NOT NULL,
     "is_deleted" BOOL NOT NULL,
     PRIMARY KEY(project_id, worktree_id, work_directory_id, repo_path),

diff --git a/crates/collab/migrations/20250113230049_expand_git_status_information.sql b/crates/collab/migrations/20250113230049_expand_git_status_information.sql
@@ -0,0 +1,13 @@
+ALTER TABLE worktree_repository_statuses
+ADD COLUMN status_kind INTEGER,
+ADD COLUMN first_status INTEGER,
+ADD COLUMN second_status INTEGER;
+
+UPDATE worktree_repository_statuses
+SET
+    status_kind = 0;
+
+ALTER TABLE worktree_repository_statuses
+ALTER COLUMN status_kind
+SET
+    NOT NULL;
diff --git a/crates/collab/src/db.rs b/crates/collab/src/db.rs
@@ -35,6 +35,7 @@ use std::{
 };
 use time::PrimitiveDateTime;
 use tokio::sync::{Mutex, OwnedMutexGuard};
+use worktree_repository_statuses::StatusKind;
 use worktree_settings_file::LocalSettingsKind;
 
 #[cfg(test)]
@@ -805,3 +806,92 @@ impl LocalSettingsKind {
         }
     }
 }
+
+fn db_status_to_proto(
+    entry: worktree_repository_statuses::Model,
+) -> anyhow::Result<proto::StatusEntry> {
+    use proto::git_file_status::{Tracked, Unmerged, Variant};
+
+    let (simple_status, variant) =
+        match (entry.status_kind, entry.first_status, entry.second_status) {
+            (StatusKind::Untracked, None, None) => (
+                proto::GitStatus::Added as i32,
+                Variant::Untracked(Default::default()),
+            ),
+            (StatusKind::Ignored, None, None) => (
+                proto::GitStatus::Added as i32,
+                Variant::Ignored(Default::default()),
+            ),
+            (StatusKind::Unmerged, Some(first_head), Some(second_head)) => (
+                proto::GitStatus::Conflict as i32,
+                Variant::Unmerged(Unmerged {
+                    first_head,
+                    second_head,
+                }),
+            ),
+            (StatusKind::Tracked, Some(index_status), Some(worktree_status)) => {
+                let simple_status = if worktree_status != proto::GitStatus::Unmodified as i32 {
+                    worktree_status
+                } else if index_status != proto::GitStatus::Unmodified as i32 {
+                    index_status
+                } else {
+                    proto::GitStatus::Unmodified as i32
+                };
+                (
+                    simple_status,
+                    Variant::Tracked(Tracked {
+                        index_status,
+                        worktree_status,
+                    }),
+                )
+            }
+            _ => {
+                return Err(anyhow!(
+                    "Unexpected combination of status fields: {entry:?}"
+                ))
+            }
+        };
+    Ok(proto::StatusEntry {
+        repo_path: entry.repo_path,
+        simple_status,
+        status: Some(proto::GitFileStatus {
+            variant: Some(variant),
+        }),
+    })
+}
+
+fn proto_status_to_db(
+    status_entry: proto::StatusEntry,
+) -> (String, StatusKind, Option<i32>, Option<i32>) {
+    use proto::git_file_status::{Tracked, Unmerged, Variant};
+
+    let (status_kind, first_status, second_status) = status_entry
+        .status
+        .clone()
+        .and_then(|status| status.variant)
+        .map_or(
+            (StatusKind::Untracked, None, None),
+            |variant| match variant {
+                Variant::Untracked(_) => (StatusKind::Untracked, None, None),
+                Variant::Ignored(_) => (StatusKind::Ignored, None, None),
+                Variant::Unmerged(Unmerged {
+                    first_head,
+                    second_head,
+                }) => (StatusKind::Unmerged, Some(first_head), Some(second_head)),
+                Variant::Tracked(Tracked {
+                    index_status,
+                    worktree_status,
+                }) => (
+                    StatusKind::Tracked,
+                    Some(index_status),
+                    Some(worktree_status),
+                ),
+            },
+        );
+    (
+        status_entry.repo_path,
+        status_kind,
+        first_status,
+        second_status,
+    )
+}
diff --git a/crates/collab/src/db/queries/projects.rs b/crates/collab/src/db/queries/projects.rs
@@ -360,6 +360,8 @@ impl Database {
                         update.updated_repositories.iter().flat_map(
                             |repository: &proto::RepositoryEntry| {
                                 repository.updated_statuses.iter().map(|status_entry| {
+                                    let (repo_path, status_kind, first_status, second_status) =
+                                        proto_status_to_db(status_entry.clone());
                                     worktree_repository_statuses::ActiveModel {
                                         project_id: ActiveValue::set(project_id),
                                         worktree_id: ActiveValue::set(worktree_id),
@@ -368,8 +370,11 @@ impl Database {
                                         ),
                                         scan_id: ActiveValue::set(update.scan_id as i64),
                                         is_deleted: ActiveValue::set(false),
-                                        repo_path: ActiveValue::set(status_entry.repo_path.clone()),
-                                        status: ActiveValue::set(status_entry.status as i64),
+                                        repo_path: ActiveValue::set(repo_path),
+                                        status: ActiveValue::set(0),
+                                        status_kind: ActiveValue::set(status_kind),
+                                        first_status: ActiveValue::set(first_status),
+                                        second_status: ActiveValue::set(second_status),
                                     }
                                 })
                             },
@@ -384,7 +389,9 @@ impl Database {
                         ])
                         .update_columns([
                             worktree_repository_statuses::Column::ScanId,
-                            worktree_repository_statuses::Column::Status,
+                            worktree_repository_statuses::Column::StatusKind,
+                            worktree_repository_statuses::Column::FirstStatus,
+                            worktree_repository_statuses::Column::SecondStatus,
                         ])
                         .to_owned(),
                     )
@@ -759,10 +766,7 @@ impl Database {
                     let mut updated_statuses = Vec::new();
                     while let Some(status_entry) = repository_statuses.next().await {
                         let status_entry: worktree_repository_statuses::Model = status_entry?;
-                        updated_statuses.push(proto::StatusEntry {
-                            repo_path: status_entry.repo_path,
-                            status: status_entry.status as i32,
-                        });
+                        updated_statuses.push(db_status_to_proto(status_entry)?);
                     }
 
                     worktree.repository_entries.insert(

diff --git a/crates/collab/src/db/queries/rooms.rs b/crates/collab/src/db/queries/rooms.rs
@@ -732,10 +732,7 @@ impl Database {
                             if db_status.is_deleted {
                                 removed_statuses.push(db_status.repo_path);
                             } else {
-                                updated_statuses.push(proto::StatusEntry {
-                                    repo_path: db_status.repo_path,
-                                    status: db_status.status as i32,
-                                });
+                                updated_statuses.push(db_status_to_proto(db_status)?);
                             }
                         }
 

diff --git a/crates/collab/src/db/tables/worktree_repository_statuses.rs b/crates/collab/src/db/tables/worktree_repository_statuses.rs
@@ -12,11 +12,26 @@ pub struct Model {
     pub work_directory_id: i64,
     #[sea_orm(primary_key)]
     pub repo_path: String,
+    /// Old single-code status field, no longer used but kept here to mirror the DB schema.
     pub status: i64,
+    pub status_kind: StatusKind,
+    /// For unmerged entries, this is the `first_head` status. For tracked entries, this is the `index_status`.
+    pub first_status: Option<i32>,
+    /// For unmerged entries, this is the `second_head` status. For tracked entries, this is the `worktree_status`.
+    pub second_status: Option<i32>,
     pub scan_id: i64,
     pub is_deleted: bool,
 }
 
+#[derive(Clone, Copy, Debug, PartialEq, Eq, EnumIter, DeriveActiveEnum)]
+#[sea_orm(rs_type = "i32", db_type = "Integer")]
+pub enum StatusKind {
+    Untracked = 0,
+    Ignored = 1,
+    Unmerged = 2,
+    Tracked = 3,
+}
+
 #[derive(Copy, Clone, Debug, EnumIter, DeriveRelation)]
 pub enum Relation {}
 

diff --git a/crates/collab/src/tests/integration_tests.rs b/crates/collab/src/tests/integration_tests.rs
@@ -13,7 +13,8 @@ use client::{User, RECEIVE_TIMEOUT};
 use collections::{HashMap, HashSet};
 use fs::{FakeFs, Fs as _, RemoveOptions};
 use futures::{channel::mpsc, StreamExt as _};
-use git::repository::GitFileStatus;
+
+use git::status::{FileStatus, StatusCode, TrackedStatus, UnmergedStatus, UnmergedStatusCode};
 use gpui::{
     px, size, AppContext, BackgroundExecutor, Model, Modifiers, MouseButton, MouseDownEvent,
     TestAppContext, UpdateGlobal,
@@ -2889,11 +2890,20 @@ async fn test_git_status_sync(
     const A_TXT: &str = "a.txt";
     const B_TXT: &str = "b.txt";
 
+    const A_STATUS_START: FileStatus = FileStatus::Tracked(TrackedStatus {
+        index_status: StatusCode::Added,
+        worktree_status: StatusCode::Modified,
+    });
+    const B_STATUS_START: FileStatus = FileStatus::Unmerged(UnmergedStatus {
+        first_head: UnmergedStatusCode::Updated,
+        second_head: UnmergedStatusCode::Deleted,
+    });
+
     client_a.fs().set_status_for_repo_via_git_operation(
         Path::new("/dir/.git"),
         &[
-            (Path::new(A_TXT), GitFileStatus::Added),
-            (Path::new(B_TXT), GitFileStatus::Added),
+            (Path::new(A_TXT), A_STATUS_START),
+            (Path::new(B_TXT), B_STATUS_START),
         ],
     );
 
@@ -2913,7 +2923,7 @@ async fn test_git_status_sync(
     #[track_caller]
     fn assert_status(
         file: &impl AsRef<Path>,
-        status: Option<GitFileStatus>,
+        status: Option<FileStatus>,
         project: &Project,
         cx: &AppContext,
     ) {
@@ -2926,20 +2936,29 @@ async fn test_git_status_sync(
     }
 
     project_local.read_with(cx_a, |project, cx| {
-        assert_status(&Path::new(A_TXT), Some(GitFileStatus::Added), project, cx);
-        assert_status(&Path::new(B_TXT), Some(GitFileStatus::Added), project, cx);
+        assert_status(&Path::new(A_TXT), Some(A_STATUS_START), project, cx);
+        assert_status(&Path::new(B_TXT), Some(B_STATUS_START), project, cx);
     });
 
     project_remote.read_with(cx_b, |project, cx| {
-        assert_status(&Path::new(A_TXT), Some(GitFileStatus::Added), project, cx);
-        assert_status(&Path::new(B_TXT), Some(GitFileStatus::Added), project, cx);
+        assert_status(&Path::new(A_TXT), Some(A_STATUS_START), project, cx);
+        assert_status(&Path::new(B_TXT), Some(B_STATUS_START), project, cx);
+    });
+
+    const A_STATUS_END: FileStatus = FileStatus::Tracked(TrackedStatus {
+        index_status: StatusCode::Added,
+        worktree_status: StatusCode::Unmodified,
+    });
+    const B_STATUS_END: FileStatus = FileStatus::Tracked(TrackedStatus {
+        index_status: StatusCode::Deleted,
+        worktree_status: StatusCode::Unmodified,
     });
 
     client_a.fs().set_status_for_repo_via_working_copy_change(
         Path::new("/dir/.git"),
         &[
-            (Path::new(A_TXT), GitFileStatus::Modified),
-            (Path::new(B_TXT), GitFileStatus::Modified),
+            (Path::new(A_TXT), A_STATUS_END),
+            (Path::new(B_TXT), B_STATUS_END),
         ],
     );
 
@@ -2949,52 +2968,22 @@ async fn test_git_status_sync(
     // Smoke test status reading
 
     project_local.read_with(cx_a, |project, cx| {
-        assert_status(
-            &Path::new(A_TXT),
-            Some(GitFileStatus::Modified),
-            project,
-            cx,
-        );
-        assert_status(
-            &Path::new(B_TXT),
-            Some(GitFileStatus::Modified),
-            project,
-            cx,
-        );
+        assert_status(&Path::new(A_TXT), Some(A_STATUS_END), project, cx);
+        assert_status(&Path::new(B_TXT), Some(B_STATUS_END), project, cx);
     });
 
     project_remote.read_with(cx_b, |project, cx| {
-        assert_status(
-            &Path::new(A_TXT),
-            Some(GitFileStatus::Modified),
-            project,
-            cx,
-        );
-        assert_status(
-            &Path::new(B_TXT),
-            Some(GitFileStatus::Modified),
-            project,
-            cx,
-        );
+        assert_status(&Path::new(A_TXT), Some(A_STATUS_END), project, cx);
+        assert_status(&Path::new(B_TXT), Some(B_STATUS_END), project, cx);
     });
 
     // And synchronization while joining
     let project_remote_c = client_c.join_remote_project(project_id, cx_c).await;
     executor.run_until_parked();
 
     project_remote_c.read_with(cx_c, |project, cx| {
-        assert_status(
-            &Path::new(A_TXT),
-            Some(GitFileStatus::Modified),
-            project,
-            cx,
-        );
-        assert_status(
-            &Path::new(B_TXT),
-            Some(GitFileStatus::Modified),
-            project,
-            cx,
-        );
+        assert_status(&Path::new(A_TXT), Some(A_STATUS_END), project, cx);
+        assert_status(&Path::new(B_TXT), Some(B_STATUS_END), project, cx);
     });
 }