Ensure hunks are sorted correctly when committing (#9198)

Author: Byron

crates/but-workspace/src/commit_engine/hunks.rs

@@ -30,7 +30,12 @@ pub fn apply_hunks(
     for selected_hunk in hunks {
         let old_skips = (selected_hunk.old_start as usize)
             .checked_sub(old_cursor)
-            .context("hunks must be in order from top to bottom of the file")?;
+            .with_context(|| {
+                format!(
+                    "`old_skips = start({start}) - cursor({old_cursor})` mut be >= 0, hunk = {selected_hunk:?}",
+                    start = selected_hunk.old_start
+                )
+            })?;
         let catchup_base_lines = old_iter.by_ref().take(old_skips);
         for old_line in catchup_base_lines {
             result_image.extend_from_slice(old_line);

crates/but-workspace/src/commit_engine/tree/mod.rs

@@ -1,4 +1,4 @@
-use crate::commit_engine::{MoveSourceCommit, RejectionReason, apply_hunks};
+use crate::commit_engine::{HunkRange, MoveSourceCommit, RejectionReason, apply_hunks};
 use crate::{DiffSpec, HunkHeader};
 use bstr::{BStr, ByteSlice};
 use but_core::{RepositoryExt, UnifiedDiff};
@@ -7,6 +7,7 @@ use gix::merge::tree::TreatAsUnresolved;
 use gix::object::tree::EntryKind;
 use gix::prelude::ObjectIdExt;
 use std::borrow::Cow;
+use std::collections::BTreeMap;
 use std::io::Read;
 use std::path::Path;
 
@@ -348,76 +349,108 @@ pub(crate) fn worktree_file_to_git_in_buf(
 /// hunks from `hunks_to_keep` that couldn't be associated with `worktree_hunks_no_context` because they weren't included.
 ///
 /// `worktree_hunks` is the hunks with a given amount of context, usually 3, and it's used to quickly select original hunks
-/// without selection.
-/// `hunks_to_keep` indicate that they are a selection by marking the other side with `0,0`, i.e. `-1,2 +0,0` selects old `1,2`,
-/// and `-0,0 +2,3` selects new `2,3`.
+/// without sub-selection, which is needed when no sub-selections are specified for all hunks. Those with sub-selections and without
+/// can be mixed freely though.
+///
+/// `hunks_to_keep` indicate that they are a selection of either old or new by marking the other side with `0,0`, i.e. `-1,2 +0,0` selects *old* `1,2`,
+/// and `-0,0 +2,3` selects *new* `2,3`. Our job here is to rebuild the original hunk selections from that, as if the user had
+/// selected them in the UI, and we want to recreate the hunks that would match their selection.
 ///
 /// The idea here is that `worktree_hunks_no_context` is the smallest-possible hunks that still contain the designated
 /// selections in the old or new image respectively. This is necessary to maintain the right order in the face of context lines.
 /// Note that the order of changes is still affected by what which selection comes first, i.e. old and new, or vice versa, if these
 /// selections are in the same hunk.
+#[allow(clippy::indexing_slicing)]
 fn to_additive_hunks(
     hunks_to_keep: impl IntoIterator<Item = HunkHeader>,
     worktree_hunks: &[HunkHeader],
     worktree_hunks_no_context: &[HunkHeader],
 ) -> (Vec<HunkHeader>, Vec<HunkHeader>) {
-    let mut hunks_to_commit = Vec::new();
     let mut rejected = Vec::new();
-    let mut previous = HunkHeader {
-        old_start: 1,
-        old_lines: 0,
-        new_start: 1,
-        new_lines: 0,
-    };
-    let mut last_wh = None;
+
+    let mut map = BTreeMap::<HunkHeader, Vec<HunkHeader>>::new();
     for selected_hunk in hunks_to_keep {
         let sh = selected_hunk;
         if sh.new_range().is_null() {
             if let Some(wh) = worktree_hunks_no_context
                 .iter()
                 .find(|wh| wh.old_range().contains(sh.old_range()))
             {
-                if last_wh != Some(*wh) {
-                    last_wh = Some(*wh);
-                    previous.new_start = wh.new_start;
+                let hunks = map.entry(*wh).or_default();
+                if let Some(existing_wh_pos) = hunks.iter_mut().position(|wh| wh.old_lines == 0) {
+                    let wh = &mut hunks[existing_wh_pos];
+                    wh.old_start = sh.old_start;
+                    wh.old_lines = sh.old_lines;
+
+                    let iter = existing_wh_pos..hunks.len();
+                    for (prev, next) in iter.clone().zip(iter.skip(1)) {
+                        hunks[next].old_start = hunks[prev].old_range().end();
+                    }
+                } else {
+                    hunks.push(HunkHeader {
+                        old_start: sh.old_start,
+                        old_lines: sh.old_lines,
+                        new_start: hunks
+                            .last()
+                            .and_then(|wh| {
+                                wh.new_range()
+                                    .contains(HunkRange {
+                                        start: wh.new_start,
+                                        lines: 0,
+                                    })
+                                    .then_some(wh.new_range().end())
+                            })
+                            .unwrap_or(wh.new_start),
+                        new_lines: 0,
+                    });
                 }
-                hunks_to_commit.push(HunkHeader {
-                    old_start: sh.old_start,
-                    old_lines: sh.old_lines,
-                    new_start: previous.new_start,
-                    new_lines: 0,
-                });
-                previous.old_start = sh.old_range().end();
                 continue;
             }
         } else if sh.old_range().is_null() {
             if let Some(wh) = worktree_hunks_no_context
                 .iter()
                 .find(|wh| wh.new_range().contains(sh.new_range()))
             {
-                if last_wh != Some(*wh) {
-                    last_wh = Some(*wh);
-                    previous.old_start = wh.old_start;
+                let hunks = map.entry(*wh).or_default();
+
+                if let Some(existing_wh_pos) = hunks.iter_mut().position(|wh| wh.new_lines == 0) {
+                    let wh = &mut hunks[existing_wh_pos];
+                    wh.new_start = sh.new_start;
+                    wh.new_lines = sh.new_lines;
+
+                    let iter = existing_wh_pos..hunks.len();
+                    for (prev, next) in iter.clone().zip(iter.skip(1)) {
+                        hunks[next].new_start = hunks[prev].new_range().end();
+                    }
+                } else {
+                    hunks.push(HunkHeader {
+                        old_start: hunks
+                            .last()
+                            .and_then(|wh| {
+                                wh.old_range()
+                                    .contains(HunkRange {
+                                        start: wh.old_start,
+                                        lines: 0,
+                                    })
+                                    .then_some(wh.old_range().end())
+                            })
+                            .unwrap_or(wh.old_start),
+                        old_lines: 0,
+                        new_start: sh.new_start,
+                        new_lines: sh.new_lines,
+                    });
                 }
-                hunks_to_commit.push(HunkHeader {
-                    old_start: previous.old_start,
-                    old_lines: 0,
-                    new_start: sh.new_start,
-                    new_lines: sh.new_lines,
-                });
-                previous.new_start = sh.new_range().end();
                 continue;
             }
         } else if worktree_hunks.contains(&sh) {
-            previous.old_start = sh.old_range().end();
-            previous.new_start = sh.new_range().end();
-            last_wh = Some(sh);
-            hunks_to_commit.push(sh);
+            let hunks = map.entry(sh).or_default();
+            hunks.push(sh);
             continue;
         }
         rejected.push(sh);
     }
-    (hunks_to_commit, rejected)
+
+    (map.into_values().flatten().collect(), rejected)
 }
 
 #[cfg(test)]

crates/but-workspace/src/commit_engine/tree/tests.rs

@@ -1,5 +1,5 @@
 mod to_additive_hunks {
-    use super::super::to_additive_hunks;
+    use crate::commit_engine::tree::to_additive_hunks;
     use crate::utils::hunk_header;
 
     #[test]
@@ -78,8 +78,7 @@ mod to_additive_hunks {
         ), @r#"
         (
             [
-                HunkHeader("-1,0", "+1,1"),
-                HunkHeader("-5,2", "+2,0"),
+                HunkHeader("-5,2", "+1,1"),
                 HunkHeader("-7,0", "+10,1"),
             ],
             [],
@@ -96,15 +95,53 @@ mod to_additive_hunks {
         ), @r#"
         (
             [
-                HunkHeader("-1,1", "+1,0"),
-                HunkHeader("-2,0", "+5,2"),
+                HunkHeader("-1,1", "+5,2"),
                 HunkHeader("-10,1", "+7,0"),
             ],
             [],
         )
         "#);
     }
 
+    #[test]
+    fn only_selections_workspace_example() {
+        let wth = vec![hunk_header("-1,10", "+1,10")];
+        let actual = to_additive_hunks(
+            [
+                // commit NOT '2,3' of the old
+                hunk_header("-2,2", "+0,0"),
+                // commit NOT '6,7' of the old
+                hunk_header("-6,2", "+0,0"),
+                // commit NOT '9' of the old
+                hunk_header("-9,1", "+0,0"),
+                // commit NOT '10' of the old
+                hunk_header("-10,1", "+0,0"),
+                // commit '11' of the new
+                hunk_header("-0,0", "+1,1"),
+                // commit '15,16' of the new
+                hunk_header("-0,0", "+5,2"),
+                // commit '19,20' of the new
+                hunk_header("-0,0", "+9,2"),
+            ],
+            &wth,
+            &wth,
+        );
+        insta::assert_debug_snapshot!(actual, @r#"
+        (
+            [
+                HunkHeader("-2,2", "+1,0"),
+                HunkHeader("-6,2", "+1,0"),
+                HunkHeader("-9,1", "+1,0"),
+                HunkHeader("-10,1", "+1,0"),
+                HunkHeader("-11,0", "+1,1"),
+                HunkHeader("-11,0", "+5,2"),
+                HunkHeader("-11,0", "+9,2"),
+            ],
+            [],
+        )
+        "#);
+    }
+
     #[test]
     fn selections_and_full_hunks() {
         let wth = vec![
@@ -119,16 +156,14 @@ mod to_additive_hunks {
                 // partial match to same hunk
                 hunk_header("-15,2", "+0,0"),
                 hunk_header("-0,0", "+22,3"),
-                // Last hunk isn't used
             ],
             &wth,
             &wth,
         ), @r#"
         (
             [
                 HunkHeader("-1,10", "+1,10"),
-                HunkHeader("-15,2", "+20,0"),
-                HunkHeader("-17,0", "+22,3"),
+                HunkHeader("-15,2", "+22,3"),
             ],
             [],
         )
@@ -147,7 +182,6 @@ mod to_additive_hunks {
                 // full match
                 hunk_header("-1,10", "+1,10"),
                 hunk_header("-15,5", "+20,5"),
-                // Last hunk isn't used
             ],
             &wth,
             &wth,
@@ -221,8 +255,7 @@ mod to_additive_hunks {
         ), @r#"
         (
             [
-                HunkHeader("-96,1", "+96,0"),
-                HunkHeader("-97,0", "+96,2"),
+                HunkHeader("-96,1", "+96,2"),
             ],
             [],
         )
@@ -240,4 +273,96 @@ mod to_additive_hunks {
         )
         "#);
     }
+
+    #[test]
+    fn real_world_issue() {
+        let wth = vec![hunk_header("-1,214", "+1,55")];
+        let wth0 = vec![
+            hunk_header("-4,13", "+4,0"),
+            hunk_header("-18,19", "+5,1"),
+            hunk_header("-38,79", "+7,3"),
+            hunk_header("-118,64", "+11,0"),
+            hunk_header("-183,1", "+12,1"),
+            hunk_header("-185,15", "+14,2"),
+            hunk_header("-201,5", "+17,5"),
+            hunk_header("-207,1", "+23,26"),
+            hunk_header("-209,3", "+50,3"),
+        ];
+
+        let actual = to_additive_hunks(
+            [
+                hunk_header("-0,0", "+23,26"),
+                hunk_header("-0,0", "+50,3"),
+                hunk_header("-207,1", "+0,0"),
+                hunk_header("-209,3", "+0,0"),
+            ],
+            &wth,
+            &wth0,
+        );
+        insta::assert_debug_snapshot!(actual, @r#"
+        (
+            [
+                HunkHeader("-207,1", "+23,26"),
+                HunkHeader("-209,3", "+50,3"),
+            ],
+            [],
+        )
+        "#);
+
+        let actual = to_additive_hunks(
+            [
+                hunk_header("-0,0", "+23,1"),
+                hunk_header("-0,0", "+25,1"),
+                hunk_header("-0,0", "+27,2"),
+                hunk_header("-0,0", "+30,2"),
+                hunk_header("-0,0", "+50,3"),
+                hunk_header("-207,1", "+0,0"),
+                hunk_header("-209,1", "+0,0"),
+                hunk_header("-211,1", "+0,0"),
+            ],
+            &wth,
+            &wth0,
+        );
+        insta::assert_debug_snapshot!(actual, @r#"
+        (
+            [
+                HunkHeader("-207,1", "+23,1"),
+                HunkHeader("-208,0", "+25,1"),
+                HunkHeader("-208,0", "+27,2"),
+                HunkHeader("-208,0", "+30,2"),
+                HunkHeader("-209,1", "+50,3"),
+                HunkHeader("-211,1", "+53,0"),
+            ],
+            [],
+        )
+        "#);
+
+        let actual = to_additive_hunks(
+            [
+                hunk_header("-207,1", "+0,0"),
+                hunk_header("-209,1", "+0,0"),
+                hunk_header("-211,1", "+0,0"),
+                hunk_header("-0,0", "+23,1"),
+                hunk_header("-0,0", "+25,1"),
+                hunk_header("-0,0", "+27,2"),
+                hunk_header("-0,0", "+30,2"),
+                hunk_header("-0,0", "+50,3"),
+            ],
+            &wth,
+            &wth0,
+        );
+        insta::assert_debug_snapshot!(actual, @r#"
+        (
+            [
+                HunkHeader("-207,1", "+23,1"),
+                HunkHeader("-208,0", "+25,1"),
+                HunkHeader("-208,0", "+27,2"),
+                HunkHeader("-208,0", "+30,2"),
+                HunkHeader("-209,1", "+50,3"),
+                HunkHeader("-211,1", "+53,0"),
+            ],
+            [],
+        )
+        "#);
+    }
 }

crates/but-workspace/src/lib.rs

@@ -122,7 +122,7 @@ impl From<TreeChange> for DiffSpec {
 }
 
 /// The header of a hunk that represents a change to a file.
-#[derive(Clone, Copy, PartialEq, Serialize, Deserialize)]
+#[derive(Clone, Copy, PartialEq, Eq, Ord, PartialOrd, Serialize, Deserialize)]
 #[serde(rename_all = "camelCase")]
 pub struct HunkHeader {
     /// The 1-based line number at which the previous version of the file started.