Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve upstream squashed commit detection #5818

Open
ionTea opened this issue Dec 12, 2024 · 8 comments
Open

Improve upstream squashed commit detection #5818

ionTea opened this issue Dec 12, 2024 · 8 comments
Assignees
Labels
bug Something isn't working rust Pull requests that update Rust code

Comments

@ionTea
Copy link

ionTea commented Dec 12, 2024

Version

0.14.3

Operating System

macOS

Distribution Method

dmg (Apple Silicon)

Describe the issue

I really like the addition of stacked PRs, but there seems to be some issue when the base of a stack is merged into master and the rest of the branches on the stack needs to rebase.

I took some screen shots to show what happens.

Here I got a stack with two PRs, and the base has been merged into master
Screenshot 2024-12-12 at 09 30 19

I want to update my stack so the

  1. Merged branch is removed, since it's now part of the base
  2. The remaining PR in the stack is updated so it points to merging into master

So I update my workspace - no issues indicated so far:
Screenshot 2024-12-12 at 09 30 54

But now all of a sudden my remaining PR in the stack has a new commit that wasn't there before, and it somehow conflicts with the version I have on remote?

Screenshot 2024-12-12 at 09 31 26

I don't want to deal with any conflicts, since I know the remote version of this branch was fine, so I reset to remote.

Screenshot 2024-12-12 at 09 39 33

But now my branch has the commit I had from my previous PR, which was integrated?
Screenshot 2024-12-12 at 09 41 02

So I guess I just don't know what to do to handle this? What I did now was that I manually went through each of the commits that were in the old branch and just uncommitted them, so my branch only included the commits you see in my first image. That's what I would expect.

Am I using this feature wrong? How am I supposed to manage stacks when the base is merged?

How to reproduce

See the issue description

Expected behavior

I would expect that the PRs look the same and contain the same commits after a part of the stack is merged

Relevant log output

No response

@ionTea ionTea added the bug Something isn't working label Dec 12, 2024
@Byron
Copy link
Collaborator

Byron commented Dec 12, 2024

Thanks a lot for reporting, and the sequence of pictures makes it very straightforward to follow what's happening.

I tried to follow the steps, created one base PR and another dependent one.

When merging the base PR, I got a couple of errors - I think this is due to me accidentally clicking the button twice or thrice even. CC @mtsgrd in case there is a way to generally solve 'async-button' problem.

Screenshot 2024-12-12 at 12 44 36

Then I update the workspace…

Screenshot 2024-12-12 at 12 48 49

…and it went through, leaving only the commit in the secondary branch, like one would expect.

Screenshot 2024-12-12 at 12 49 26

So my feeling is that in the case described in this issue the conflict prevented this from working correctly, and follow-up operations accumulated more error.

I would hope that the changes in #5722 and related PRs can help with preventing such conflicts that shouldn't be, but it's only a guess for now.

Also CC @Caleb-T-Owens as rebasing is probably involved.

@ionTea
Copy link
Author

ionTea commented Dec 12, 2024

Good I'm glad it helped, I have a full recording of the events too if you need it 👍

@Caleb-T-Owens
Copy link
Contributor

Hi @ionTea,

Could you let me know what merge stratergy you use on GitHub, IE "Merge", "Squash and Rebase", or "Rebase"?

Thanks

@Byron Byron added the feedback requested Feedback was requested to help resolve the issue label Dec 14, 2024
@ionTea
Copy link
Author

ionTea commented Dec 18, 2024

Sorry for the slow response, we use "Squash and Merge" 🙂

@Byron Byron removed the feedback requested Feedback was requested to help resolve the issue label Dec 18, 2024
@Caleb-T-Owens
Copy link
Contributor

No worries!

I believe the "Cannot change pr base branch..." message has now been resolved.

As for the other issues, I believe this is in a large part due to the complexity of trying to detect whether a set of commits are part of one larger squashed commit, especially in any O(reasonable) complexity. I do believe @krlvi and @mtsgrd have talked about this problem a bit more and might have a plan. (Perhaps @Byron has some thoughts too...)

I would recommend using either the Merge strategy or as a second option Rebase and Merge. Especially if you are crafting individual commits rather than fire-hosing updates, both options also make the history easier for others to explore. With these two strategies GitButler is able to track which commits & branches have been integrated upstream more accurately.

I'm going to rename this issue to track this squashed commit detection problem.

@Caleb-T-Owens Caleb-T-Owens added the rust Pull requests that update Rust code label Dec 18, 2024
@Caleb-T-Owens Caleb-T-Owens changed the title Stacked PR issues when base is merged Improve upstream squashed commit detection Dec 18, 2024
@Byron
Copy link
Collaborator

Byron commented Dec 18, 2024

(Perhaps @Byron has some thoughts too...)

I took another look and in theory, is_integrated() already does exactly that - it assumes something is present in the target branch if it merges cleanly, but I might be misunderstanding it as well.

let mut merge_output = self
.gix_repo
.merge_trees(
merge_base_tree_id,
git2_to_gix_object_id(commit.tree_id()),
self.upstream_tree_id,
Default::default(),
merge_options,
)
.context("failed to merge trees")?;
if merge_output.has_unresolved_conflicts(conflict_kind) {
return Ok(false);
}

However, squashed or not, ideally that target commit is not too advanced so it didn't diverge too much. When looking at the following blob-merge as example:

mkdir partial-match-2
(cd partial-match-2
  cat <<EOF > base.blob
0
1
2
3
4
5
EOF

  cat <<EOF > ours.blob
0
X1
X2
X3
X4
5
EOF

  cat <<EOF > theirs.blob
0
1
X2
X3
4
5
EOF
)

Then the diff will be conflicting as the states differ, even though the changes are overlapping.

0
<<<<<<< partial-match-2/ours.blob
X1
X2
X3
X4
=======
1
X2
X3
4
>>>>>>> partial-match-2/theirs.blob
5

Probably there is a way to devise an algorithm to check if only the changes are included on the other side and call it good if that's the case. The merge algorithm already normalizes the regions to compare and fills in the common ancestor (lines 1 and 4 here), and if it wouldn't do that it could conclude that both changes are the same, if it would compare only the common portion.
The devil will definitely hide in plain sight here 😅.

@Caleb-T-Owens
Copy link
Contributor

I've come up with an algorithm that should be much more accurate. I'll have a go at implementing it tomorrow. https://gitbutler.notion.site/correctly-detecting-squashed-integration

@Caleb-T-Owens Caleb-T-Owens self-assigned this Dec 18, 2024
@Byron
Copy link
Collaborator

Byron commented Dec 19, 2024

I took a look at the linked document, but immediately admit that I couldn't follow. This doesn't mean anything though, just that whatever I share here isn't based on something you might already have written down or clarified.

There is just one question that came to my mind while thinking about this: If there are three commits, A, A-reverted and B, and I want to know if A exists in the squash of A, A-reverted and B, then there would be not chance to find it. However, the final state of B would be equal to the squashed commit naturally.
And even in this simplistic example it's implied that the actual squash commit, i.e. the closest commit in the target of merges, was already found so a comparison to a tip further in the future can be avoided in the first place.

I am glad you seem to be having a solution though, hoping that eventually I also arrive there 😅.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working rust Pull requests that update Rust code
Projects
None yet
Development

No branches or pull requests

3 participants