Merge pull request #10088 from Byron/fix

Fixup to_additive_hunks()

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,9 +349,12 @@ 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.
@@ -417,7 +421,144 @@ fn to_additive_hunks(
         }
         rejected.push(sh);
     }
-    (hunks_to_commit, rejected)
+
+    if !hunks_to_commit
+        .iter()
+        .zip(hunks_to_commit.iter().skip(1))
+        .all(|(prev, next)| prev < next)
+    {
+        tracing::info!("Using alternative hunk algorithm for {hunks_to_commit:?}");
+        to_additive_hunks_fallback(
+            hunks_to_commit.into_iter().map(
+                |HunkHeader {
+                     old_start,
+                     old_lines,
+                     new_start,
+                     new_lines,
+                 }| {
+                    if old_lines == 0 {
+                        HunkHeader {
+                            old_start: 0,
+                            old_lines: 0,
+                            new_start,
+                            new_lines,
+                        }
+                    } else {
+                        HunkHeader {
+                            old_start,
+                            old_lines,
+                            new_start: 0,
+                            new_lines: 0,
+                        }
+                    }
+                },
+            ),
+            worktree_hunks,
+            worktree_hunks_no_context,
+        )
+    } else {
+        (hunks_to_commit, rejected)
+    }
+}
+
+/// This algorithm is better when the basic one fails, but both have their merit.
+/// Right now this is a brute-force one or the other approach, but we could also apply them selectively.
+/// Note that what we really want to simulate is what the UI shows. But since the UI also doesn't really know hunks,
+/// we have to fiddle it together here, at least we know the hunks themselves.
+///
+/// Note that this algorithm is kind of the opposite of what people would expect if it's run where `to_additive_hunks()` works.
+/// But here we are… just making this work.
+#[allow(clippy::indexing_slicing)]
+fn to_additive_hunks_fallback(
+    hunks_to_keep: impl IntoIterator<Item = HunkHeader>,
+    worktree_hunks: &[HunkHeader],
+    worktree_hunks_no_context: &[HunkHeader],
+) -> (Vec<HunkHeader>, Vec<HunkHeader>) {
+    let mut rejected = Vec::new();
+
+    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()))
+            {
+                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,
+                    });
+                }
+                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()))
+            {
+                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,
+                    });
+                }
+                continue;
+            }
+        } else if worktree_hunks.contains(&sh) {
+            let hunks = map.entry(sh).or_default();
+            hunks.push(sh);
+            continue;
+        }
+        rejected.push(sh);
+    }
+
+    (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]
@@ -240,4 +240,135 @@ 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"),
+            ],
+            [],
+        )
+        "#);
+    }
+
+    #[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"),
+            ],
+            [],
+        )
+        "#);
+    }
 }

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.