Skip to content

Commit

Permalink
feat(push): revert overzealous modified status
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
jpgrayson committed Sep 9, 2024
1 parent 1fd0d5d commit 343925d
Show file tree
Hide file tree
Showing 2 changed files with 11 additions and 8 deletions.
11 changes: 5 additions & 6 deletions src/stack/transaction/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1090,6 +1090,7 @@ impl<'repo> StackTransaction<'repo> {
conflicts: false,
})?;
self.current_tree_id = tree_id;
push_status = PushStatus::Modified;
tree_id
}
Ok(false) => {
Expand Down Expand Up @@ -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
Expand Down
8 changes: 6 additions & 2 deletions t/t1204-push-updated.sh
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down

0 comments on commit 343925d

Please sign in to comment.