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

wip changes against more than one parent #1753

Merged
merged 2 commits into from
Jan 10, 2025

Conversation

Byron
Copy link
Member

@Byron Byron commented Jan 10, 2025

The PR #1751 as it should have been.

cruessler and others added 2 commits January 10, 2025 08:11
Before this commit, blame would have been passed to the first parent in
cases where there was more than one.

This commit changes that by only removing a particular suspect from
further consideration once it has been compared to all of its parents.
For hunks where blame cannot be passed to any parent, we can safely
assume that they were introduced by a particular suspect, so we remove
those hunks from `hunks_to_blame` and create a `BlameEntry` out of them.

We can illustrate the change using the following small example history:

```text
---1----2
    \    \
     3----4---
```

Let’s now say that we’re blaming a file that has the following content:

```text
line 1 # introduced by (1), then changed by (3)
line 2 # introduced by (1)
line 3 # introduced by (1), then changed by (2)
```

The resulting blame should look like this:

```text
(3) line 1
(1) line 2
(2) line 3
```

The previous version of the algorithm would have passed blame to just
(2) or (3), depending on which one came first in the list of parents.
- try to not force `clone_from()` into move semantics unless necessary
@cruessler
Copy link
Contributor

Thanks a lot for the refactor! Again, I’ve learned one or two things from it!

@EliahKagan
Copy link
Member

Something seems to be wrong with the checks here: there are none. I don't know why. Maybe it is a bug in GitHub triggered by some combination of force pushes? In any case, it looks like the intent here is to merge this once tests pass, but auto-merge is unable to proceed because it is waiting on checks that are not running, nor queued, nor even enumerated.

@Byron Byron disabled auto-merge January 10, 2025 09:13
@Byron Byron enabled auto-merge January 10, 2025 09:13
@Byron Byron disabled auto-merge January 10, 2025 09:13
@Byron Byron merged commit a22f13b into main Jan 10, 2025
@Byron Byron deleted the wip-changes-against-more-than-one-parent branch January 10, 2025 09:15
@Byron
Copy link
Member Author

Byron commented Jan 10, 2025

That is very strange indeed!

Instead of trying to fix it by maybe redoing the push after a trivial change, I thought it was easiest to just force-merge with admin overrides.
I'd hope that main stays green but if not, I hope the follow-up PR to fix it will work again.

@EliahKagan
Copy link
Member

On main since the merge, it looks like there's a readily fixable clippy error, but otherwise everything is okay.

@Byron
Copy link
Member Author

Byron commented Jan 10, 2025

So close :D, here is the fix gitbutlerapp/gitbutler#5910.

@cruessler
Copy link
Contributor

The link goes to gitbutler, but main is green again. :-)

@EliahKagan
Copy link
Member

I think #1754 had the fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants