[#10] Add rebase on top of default branch

[sancho20021]

Description

Rebase current changes in remote update branch on top
of remote default branch if possible. Otherwise report an error.

I used the version before this commit, but inserted rebase abort in case of rebase commit fails.

Fixes #10

Some notes:

When I tried to reproduce merge conflicts, the rebase.commit operation failed with this error:

unstaged changes exist in workdir; class=Rebase (29); code=Unmerged (-10)

But the state of the rebase looked like a state when a user needs to resolve conflicts and continue rebasing. So I don't understand why it's marked as unstaged changes.

Way to resolve conflicts

When update-bot fails to rebase, the user (human) can do the rebase by himself, resolve all the conflicts and (force)push. Update-bot will be able to continue working in the existing branch.

[notgne2]

I'm not entirely sure I understand how this addresses the discussion in #10, if I am understanding correctly this is almost the same logic as what we removed, and the issue with that was that if there were conflicts it would stop working, but with rebase.abort() added in the case of rebase failure. What state does this actually leave PRs in if there are conflicts, and how is it different than from before?

But the state of the rebase looked like a state when a user needs to resolve conflicts and continue rebasing. So I don't understand why it's marked as unstaged changes.

iirc you do this by fixing the conflicts, then git add and git rebase --continue, but before doing so the branch is left with unstaged changes (of conflict markers), so doesn't this make sense?

[sancho20021]

this is almost the same logic as what we removed, but with rebase.abort() added in the case of rebase failure.

Yes

What state does this actually leave PRs in if there are conflicts, and how is it different than from before?

Situation:

1. Conflicts in PR
2. Someone fixes these conflicts (not bot)
3. Bot runs one more time

The difference is in result after "Bot runs one more time"

How worked before:

INFO  update_daemon > Updating ssh://[email protected]/sancho20021/update-daemon-test
 ERROR update_daemon > ssh://[email protected]/sancho20021/update-daemon-test: Error during update branch setup: Error committing rebase patch: unstaged changes exist in workdir; class=Rebase (29); code=Unmerged (-10)
 ERROR update_daemon > Errors occured, please see above logs

How works now:

smth like "Successfully updated PR"

[balsoft]

What state does this leave the local repository with if the rebase fails? Does it cleanly check out some sensible branch?

[sancho20021]

What state does this leave the local repository with if the rebase fails? Does it cleanly check out some sensible branch?

@balsoft

Oh, no, it stays in something strange like detached HEAD or so. I try to fix this and my idea is to:

1. abort rebase in case of any failure
2. checkout to the default branch

I'm trying to do that but I can't find the list of operations with Repository object that I have to perform. I tried:

• repo.set_head("refs/heads/{default_branch}")
• and optionally repo.checkout_head()

but it either didn't move to default branch or left uncommited changes.

Perhaps if I understand the logic here, it will help. Could you please explain what is happening here maybe in git operations?

[sancho20021]

I worked out a way to checkout to the default branch. Now some bugs need to be fixed

[sancho20021]

Ok the bug that I was facing was found accidentally and it won't appear in normal usage. But I will describe it here just in case:

If the last commit in the main branch and in the automatic-update branch are identical (including commit messages), then during the rebase this error occurrs:

 ERROR update_daemon > ssh://[email protected]/sancho20021/update-daemon-test: Error during update branch setup: Error committing rebase patch: this patch has already been applied; class=Rebase (29); code=Applied (-18)
 ERROR update_daemon > Errors occured, please see above logs

[sancho20021]

So currently the bot checks out to the default branch in case of any rebase failure.

[cab404]

@sancho20021 do you think I should merge it?

[sancho20021]

@sancho20021 do you think I should merge it?

Not sure

@cab404
Good:

• It does work on all scenarios that I tested (I tested just a few).

Bad:

• I think I forgot or don't fully understand what each line of code does exactly. I relied on the previous implementation.

Maybe I or @balsoft should remember and comment the part of the code about the rebase. What do you think?

[PhilTaken]

Bad:

I think I forgot or don't fully understand what each line of code does exactly. I relied on the previous implementation.

Maybe I or @balsoft should remember and comment the part of the code about the rebase. What do you think?

I think the code reads relatively well

for a rebase it creates some sort of rebase object and then for each commit in the rebase it

1. checks if the commit is valid
2. commits the rebase commit onto the update branch

finally, it finishes the rebase

if any of those fail it hard resets all changes

If that is the case then I don't see a need for further documentation.
Changes look good to me!

[rvem]

Changes look ok for me as well

[lierdakil]

I have some questions:

1. Does this actually fix TODO: Attempt to rebase human commits instead of failing immediately #10? TODO: Attempt to rebase human commits instead of failing immediately #10 speaks of rebasing human commits in the update branch on top of the automated update (I actually think it would be easier to do the other way around, but that's beside the point). This PR doesn't do anything about human commits in the update branch. It does rebase the update branch on top of the default branch, but that's largely orthogonal, the update logic will still overwrite any human commits in the update branch, now that the check is gone. Not to say rebasing on top of the default branch isn't useful.
2. What is safe about safe_abort? It just bails when the abort failed, that doesn't sound safe?
3. Would it make sense to do an in-memory rebase? IIUC, it doesn't mutate the repo during rebase (or at least it doesn't mutate HEAD or workdir). It might require a bit more work with finalizing the rebase, but at least we won't get the cache into a potentially inconsistent state if something goes wrong?

[lierdakil]

Also, if we ignore human commits in the update branch for a moment, I don't think we actually need to do the rebase proper? The update logic goes like this currently:

1. get flake.lock from the default branch
2. get flake.lock from the update branch
3. update flake.lock from the update branch
4. if there are differences in flake.lock between (2) and (3):
   1. soft-reset to default branch
   2. commit all changes
5. if there are differences in flake.lock between (1) and (3):
   1. push
   2. update the PR

(the actual logic is a little more convoluted, but this is the gist of it).

So there's a hole in this logic: the update branch is only updated to the default branch (via soft-reset) in (4), but not in (5). The tricky bit is that we don't want to unnecessarily force-push, hence we'll need to check whether there are new commits in the default branch. I think we just need to check graph_ahead_behind to decide.

[rvem]

hence we'll need to check whether there are new commits in the default branch. I think we just need to check graph_ahead_behind to decide.

That indeed sounds like a much easier and less error-prone approach. Apparently, the human commits can be cherry-picked on top of the soft-reset branch

[lierdakil]

cherry-picked on top of the soft-reset branch

The issue isn't as much in how to put human commits into the update branch, although there are many modes of failure for this operation, too, but rather in how to find them properly. And then there's the question of what to do if there are human commits but no bot ones (e.g. because flake.lock was updated in the default branch manually). Not to imply these challenges are insurmountable, but this is bound to make already somewhat convoluted logic more convoluted and introduce more edge cases. I wouldn't worry as much if we had good test coverage here, but we don't.