git: Remove JobStatus from PendingOp in favour of in-flight pruning (#42955)

Jakub Konka created

The idea is that we only store running (`!self.finished`) or finished
(`self.finished`) pending ops, while everything else (skipped, errored)
jobs are pruned out immediately. We don't really need them in the grand
scheme of things anyway.

Release Notes:

- N/A

Change summary

crates/git_ui/src/git_panel.rs             |  2 
crates/project/src/git_store.rs            | 25 ++++++++----
crates/project/src/git_store/pending_op.rs | 28 +-------------
crates/project/src/project_tests.rs        | 46 +++++++++--------------
4 files changed, 39 insertions(+), 62 deletions(-)

Detailed changes

crates/git_ui/src/git_panel.rs 🔗

@@ -2662,7 +2662,7 @@ impl GitPanel {
                 && pending
                     .ops
                     .iter()
-                    .any(|op| op.git_status == pending_op::GitStatus::Reverted && op.finished())
+                    .any(|op| op.git_status == pending_op::GitStatus::Reverted && op.finished)
             {
                 continue;
             }

crates/project/src/git_store.rs 🔗

@@ -5404,18 +5404,27 @@ impl Repository {
         let ids = self.new_pending_ops_for_paths(paths, git_status);
 
         cx.spawn(async move |this, cx| {
-            let (job_status, result) = match f(this.clone(), cx).await {
-                Ok(()) => (pending_op::JobStatus::Finished, Ok(())),
-                Err(err) if err.is::<Canceled>() => (pending_op::JobStatus::Skipped, Ok(())),
-                Err(err) => (pending_op::JobStatus::Error, Err(err)),
+            let (finished, result) = match f(this.clone(), cx).await {
+                Ok(()) => (true, Ok(())),
+                Err(err) if err.is::<Canceled>() => (false, Ok(())),
+                Err(err) => (false, Err(err)),
             };
 
             this.update(cx, |this, _| {
                 let mut edits = Vec::with_capacity(ids.len());
                 for (id, entry) in ids {
                     if let Some(mut ops) = this.snapshot.pending_ops_for_path(&entry) {
-                        if let Some(op) = ops.op_by_id_mut(id) {
-                            op.job_status = job_status;
+                        if finished {
+                            if let Some(op) = ops.op_by_id_mut(id) {
+                                op.finished = true;
+                            }
+                        } else {
+                            let idx = ops
+                                .ops
+                                .iter()
+                                .position(|op| op.id == id)
+                                .expect("pending operation must exist");
+                            ops.ops.remove(idx);
                         }
                         edits.push(sum_tree::Edit::Insert(ops));
                     }
@@ -5443,7 +5452,7 @@ impl Repository {
             ops.ops.push(PendingOp {
                 id,
                 git_status,
-                job_status: pending_op::JobStatus::Running,
+                finished: false,
             });
             edits.push(sum_tree::Edit::Insert(ops));
             ids.push((id, path));
@@ -5721,7 +5730,7 @@ async fn compute_snapshot(
     let pending_ops_by_path = SumTree::from_iter(
         prev_snapshot.pending_ops_by_path.iter().filter_map(|ops| {
             let inner_ops: Vec<PendingOp> =
-                ops.ops.iter().filter(|op| op.running()).cloned().collect();
+                ops.ops.iter().filter(|op| !op.finished).cloned().collect();
             if inner_ops.is_empty() {
                 None
             } else {

crates/project/src/git_store/pending_op.rs 🔗

@@ -11,14 +11,6 @@ pub enum GitStatus {
     Unchanged,
 }
 
-#[derive(Clone, Copy, Debug, PartialEq, Eq)]
-pub enum JobStatus {
-    Running,
-    Finished,
-    Skipped,
-    Error,
-}
-
 #[derive(Clone, Debug, PartialEq, Eq)]
 pub struct PendingOps {
     pub repo_path: RepoPath,
@@ -29,7 +21,7 @@ pub struct PendingOps {
 pub struct PendingOp {
     pub id: PendingOpId,
     pub git_status: GitStatus,
-    pub job_status: JobStatus,
+    pub finished: bool,
 }
 
 #[derive(Clone, Debug)]
@@ -114,7 +106,7 @@ impl PendingOps {
     /// File is staged if the last job is finished and has status Staged.
     pub fn staged(&self) -> bool {
         if let Some(last) = self.ops.last() {
-            if last.git_status == GitStatus::Staged && last.job_status == JobStatus::Finished {
+            if last.git_status == GitStatus::Staged && last.finished {
                 return true;
             }
         }
@@ -124,24 +116,10 @@ impl PendingOps {
     /// File is staged if the last job is not finished and has status Staged.
     pub fn staging(&self) -> bool {
         if let Some(last) = self.ops.last() {
-            if last.git_status == GitStatus::Staged && last.job_status != JobStatus::Finished {
+            if last.git_status == GitStatus::Staged && !last.finished {
                 return true;
             }
         }
         false
     }
 }
-
-impl PendingOp {
-    pub fn running(&self) -> bool {
-        self.job_status == JobStatus::Running
-    }
-
-    pub fn finished(&self) -> bool {
-        matches!(self.job_status, JobStatus::Finished | JobStatus::Skipped)
-    }
-
-    pub fn error(&self) -> bool {
-        self.job_status == JobStatus::Error
-    }
-}

crates/project/src/project_tests.rs 🔗

@@ -8538,11 +8538,8 @@ fn merge_pending_ops_snapshots(
                     .find_map(|(op, idx)| if op.id == s_op.id { Some(idx) } else { None })
                 {
                     let t_op = &mut t_ops.ops[op_idx];
-                    match (s_op.job_status, t_op.job_status) {
-                        (pending_op::JobStatus::Running, _) => {}
-                        (s_st, pending_op::JobStatus::Running) => t_op.job_status = s_st,
-                        (s_st, t_st) if s_st == t_st => {}
-                        _ => unreachable!(),
+                    if s_op.finished {
+                        t_op.finished = true;
                     }
                 } else {
                     t_ops.ops.push(s_op);
@@ -8634,7 +8631,7 @@ async fn test_repository_pending_ops_staging(
                 Some(&pending_op::PendingOp {
                     id: id.into(),
                     git_status,
-                    job_status: pending_op::JobStatus::Running
+                    finished: false,
                 })
             );
             task
@@ -8649,7 +8646,7 @@ async fn test_repository_pending_ops_staging(
                 Some(&pending_op::PendingOp {
                     id: id.into(),
                     git_status,
-                    job_status: pending_op::JobStatus::Finished
+                    finished: true,
                 })
             );
         });
@@ -8675,27 +8672,27 @@ async fn test_repository_pending_ops_staging(
             pending_op::PendingOp {
                 id: 1u16.into(),
                 git_status: pending_op::GitStatus::Staged,
-                job_status: pending_op::JobStatus::Finished
+                finished: true,
             },
             pending_op::PendingOp {
                 id: 2u16.into(),
                 git_status: pending_op::GitStatus::Unstaged,
-                job_status: pending_op::JobStatus::Finished
+                finished: true,
             },
             pending_op::PendingOp {
                 id: 3u16.into(),
                 git_status: pending_op::GitStatus::Staged,
-                job_status: pending_op::JobStatus::Finished
+                finished: true,
             },
             pending_op::PendingOp {
                 id: 4u16.into(),
                 git_status: pending_op::GitStatus::Unstaged,
-                job_status: pending_op::JobStatus::Finished
+                finished: true,
             },
             pending_op::PendingOp {
                 id: 5u16.into(),
                 git_status: pending_op::GitStatus::Staged,
-                job_status: pending_op::JobStatus::Finished
+                finished: true,
             }
         ],
     );
@@ -8792,18 +8789,11 @@ async fn test_repository_pending_ops_long_running_staging(
             .get(&worktree::PathKey(repo_path("a.txt").as_ref().clone()), ())
             .unwrap()
             .ops,
-        vec![
-            pending_op::PendingOp {
-                id: 1u16.into(),
-                git_status: pending_op::GitStatus::Staged,
-                job_status: pending_op::JobStatus::Skipped
-            },
-            pending_op::PendingOp {
-                id: 2u16.into(),
-                git_status: pending_op::GitStatus::Staged,
-                job_status: pending_op::JobStatus::Finished
-            }
-        ],
+        vec![pending_op::PendingOp {
+            id: 2u16.into(),
+            git_status: pending_op::GitStatus::Staged,
+            finished: true,
+        }],
     );
 
     repo.update(cx, |repo, _cx| {
@@ -8904,12 +8894,12 @@ async fn test_repository_pending_ops_stage_all(
             pending_op::PendingOp {
                 id: 1u16.into(),
                 git_status: pending_op::GitStatus::Staged,
-                job_status: pending_op::JobStatus::Finished
+                finished: true,
             },
             pending_op::PendingOp {
                 id: 2u16.into(),
                 git_status: pending_op::GitStatus::Unstaged,
-                job_status: pending_op::JobStatus::Finished
+                finished: true,
             },
         ],
     );
@@ -8923,12 +8913,12 @@ async fn test_repository_pending_ops_stage_all(
             pending_op::PendingOp {
                 id: 1u16.into(),
                 git_status: pending_op::GitStatus::Staged,
-                job_status: pending_op::JobStatus::Finished
+                finished: true,
             },
             pending_op::PendingOp {
                 id: 2u16.into(),
                 git_status: pending_op::GitStatus::Unstaged,
-                job_status: pending_op::JobStatus::Finished
+                finished: true,
             },
         ],
     );