From 343925d0f8f0a06eb9658266778b9e54ce2754c8 Mon Sep 17 00:00:00 2001
From: Peter Grayson <pete@jpgrayson.net>
Date: Sun, 1 Sep 2024 17:24:06 -0400
Subject: [PATCH] feat(push): revert overzealous modified status

When pushing a patch, we would ideally like to show the "(modified)"
status when the patch's diff changes. In StGit 2.4.11, the push behavior
was changed such that "(modified)" would be displayed whenever the
patch's underlying commit changed. But this meant that "(modified)"
would be shown in many circumstances where the patch itself didn't
change or didn't have a material change.

Revert to the previous behavior where "(modified)" would only be shown
if the initial patch application failed such that `git merge-recursive`
had to be used to apply the patch.

With this change, pushes will effectively never be shown as
"(modified)". This remains a regression in StGit 2.x relative to 1.x.
The root of this regression is that StGit 2.x, when pushing patches,
uses `git apply --3way` whereas `git apply` withouth the `--3way` option
was used in StGit 1.x. As a consequence, StGit 2.x patch application
effectively never has to fallback to using `git merge-recursive` because
`--3way` is correct/effective. But that also means that StGit's previous
mechanism for determining whether a push modifies a patch no longer
works.
---
 src/stack/transaction/mod.rs | 11 +++++------
 t/t1204-push-updated.sh      |  8 ++++++--
 2 files changed, 11 insertions(+), 8 deletions(-)

diff --git a/src/stack/transaction/mod.rs b/src/stack/transaction/mod.rs
index 6900b821..f16e872e 100644
--- a/src/stack/transaction/mod.rs
+++ b/src/stack/transaction/mod.rs
@@ -1090,6 +1090,7 @@ impl<'repo> StackTransaction<'repo> {
                             conflicts: false,
                         })?;
                         self.current_tree_id = tree_id;
+                        push_status = PushStatus::Modified;
                         tree_id
                     }
                     Ok(false) => {
@@ -1130,12 +1131,10 @@ impl<'repo> StackTransaction<'repo> {
                 // execute() performs the checkout. Setting the transaction head
                 // here ensures that the real stack top will be checked-out.
                 self.updated_head = Some(commit.clone());
-            } else if push_status != PushStatus::AlreadyMerged {
-                if new_tree_id == new_parent_ref.tree() {
-                    push_status = PushStatus::Empty;
-                } else {
-                    push_status = PushStatus::Modified;
-                }
+            } else if push_status != PushStatus::AlreadyMerged
+                && new_tree_id == new_parent_ref.tree()
+            {
+                push_status = PushStatus::Empty;
             }
 
             self.updated_patches
diff --git a/t/t1204-push-updated.sh b/t/t1204-push-updated.sh
index 8adf43ae..08e6e578 100755
--- a/t/t1204-push-updated.sh
+++ b/t/t1204-push-updated.sh
@@ -49,9 +49,13 @@ test_expect_success 'Move hunk to p0' '
     stg refresh
 '
 
-test_expect_success 'Push indicates p1 was modified' '
+# NOTE: since StGit started using --3way when pushing patches, it is no longer
+# trivial to determine whether a patch was "modified" due to the push. In this
+# test, the status would ideally be "(modified)", but in practice StGit cannot
+# determine that cheaply.
+test_expect_success 'Push indicates p1 was not modified' '
     cat >expected <<-\EOF &&
-	> p1 (modified)
+	> p1
 	EOF
     stg push p1 >out &&
     test_cmp expected out