RFE: Add rebase command

[sils]

Copied from #870

---

So the rebase command would obviously suit people using a rebase workflow with git, for example ours described at http://coala.readthedocs.org/en/latest/Getting_Involved/Review/ . So the rebase is done to avoid a merge commit as a merge commit internally with git has two originating states and one target state i.e. two diffs that would need to be reviewed which is hard and a flaw in most review processes because this is the place where actual logical bugs can slip in. So the ideal solution for this problem (IMO) is doing

# Wildly assuming pseudo-environment variables you probably don't have in that form
git checkout $(TARGET)
git rebase $(BASE)
git checkout -b $(TARGET)_v1  # Or v2 and so on
git push -u origin $(TARGET)_v1

and automatically creating a PR out of this one closing the old one and adding a comment to the old one referencing that this PR is replaced by the new one. Bonus points for a comment in the new commit listing the new commits with their short SHA checksums and oneline shortlog messages which were added to the $(BASE) branch.

So that is one usecase for people who care a lot about full reviewability and an unbreakable master because a commit has first to be proven by CI and to the reviewer and then the master only fastforwards which is basically only resetting the master "pointer".

[alex-palevsky]

@yegor256 dispatch this issue please

[yegor256]

@sils1297 I don't understand what you're suggesting. Can you please explain again, in simple words. What happens when I say @rultor rebase?

[sils]

@yegor256 sure.

@rultor rebase will rebase the branch I use for the pull request on the target branch and push it to a _v1 branch (or increasing version number). I.e. out of one pull request with non-fast-forwardable commits it creates a new pull request with fast-forwardable commits.

Example

11fe34 - 237d64
        \- ae8d72

• On master we have a commit 11fe34.
• A developer builds his work onto this commit and opens a PR, 253, on branch wip/dev/anything, ae8d72.
• In the meantime new commits get on master so it gets fastforwarded to 237d64.
• @rultor merge fails with ff=only set
• @rultor rebase is now able to rebase ae8d72 onto 11fe34, creates 198342 and pushes that on wip/dev/anything_v2, closing PR 253, opening a new PR 255 from wip/dev/anything_v2. It adds a comment on 253 referencing to the new 255 replacement PR. It adds a comment to 255 listing all commits and their messages that were added to master and rebased upon, i.e. in the example this would be 237d64 which was added in the meantime. This allows quick investigation if changes may interfere when doing proper commit messages.

11fe34 - 237d64 .
                 \- 198342

That clearer?

[yegor256]

@sils1297 it's clear now, but I think it's too much for Rultor. I think, these operations can and should be done by the developer, the author of the pull request.

[sils]

@yegor256 IMO it would be nice since this is a very routine operation in our workflow. When having this nonautomated people usually begin to use push -f on the same branch which deletes the old review comments out of lazyness.

Given that this is more often used than releasing in a typical workflow it spares the devs a lot work. After all you don't seem to consider releasing too much for rultor even though it's also a rather complex process. (We don't use that yet so I don't know how rultor does it but its at least adding a tag and github release, executing a custom script and optionaly creating a release branch that can be used for backporting fixes or so.) Pulling, creating new branch, rebase, pushing is a pretty simple thing to me.

If you still disagree feel free to close this matter.

[yegor256]

@sils1297 maybe I don't understand this idea enough. I never do anything like this, maybe that's why. My biggest concern is the creation of new pull request. Why can't we just rebase an existing branch and push its new version to remote?

[sils]

@yegor256 so actually you can and that's what I meant is what often happens. However if you do that you need to force push. If you force push, github will not show the old commits in the PR and only show the new ones which means your previous review comments are lost.

In order to keep the review comments one can open a new on a _v2 branch.

[yegor256]

@sils1297 how about rebase before merge? We can add an option to merge pipeline and rultor will do the rebase before merging the branch. Thus, your PR will stay intact, but the merge with --ff-only will pass. What do you think?

[sils]

@yegor256 technically this is one solution - however the main reason we use --ff-only is not to have a linear history (which is a nice side effect) but to have a complete review chain:

Merge commits are evil. If you look at them technically they have two originating commits and join them into one. Effectively in order to review them one would have to review two diffs at the same time. So if you use merge commits you have a hole in your review. Thats why the ideal workflow on a PR that needs rebase would be:

Someone rebases it. Someone opens a new PR with a link to the old one. And here's the clue, all commits need to be re-reviewed by another human because the position in the history matters in a commit! (A merge commit can break things, also in a way that is undetectable by CI.) And that's where my proposal kicks in: rultor can automate this new PR creating and on top of that provide the information how the history changed below those commit so they can be rereviewed in context. (I.e. it lists the new commits on master that were rebased upon.)

This closes this merge-hole in the review chain. And if you only rebase and then ff the master then you still have unreviewed context which is similar to this merge-hole.

I am personally of the opinion that this ff/rebase workflow is, qualitatively speaking (!), the best one because it is the only one where commits are reviewed right in the context and thus the only one preventing the master branch from breaking logically, i.e. in ways that CI/test suites can't detect.

[sils]

We can add an option to merge pipeline and rultor will do the rebase before merging the branch.

You already added that btw. IIRC.

[sils]

I.e. it lists the new commits on master that were rebased upon

Doing that manually is painful so nobody does it despite it really raises the ability to review the contextual changes done through the rebase. Doing that automatically would be limited to a few lines of code I presume (-> comparatively easy) and be really helpful.

[sils]

@yegor256 does this explanation help? This kind of command would really improve our workflow and help us raising code quality through being better able to review rebases.

[original-brownbear]

@sils1297 I have reviewed this ticket and maybe this will help :)

Merge commits are evil. If you look at them technically they have two originating commits and join them into one. Effectively in order to review them one would have to review two diffs at the same time.

Not really :) Think about it, all you really care about is what happens to your master branch. What changes would need to be applied to the feature branch to be merged in ... who cares ? :)
This is also what Github shows you. Easy example:

• Have a repo with many files and commits constantly coming in
• Ask someone to edit one file that is rarely changed, say the readme.
• That someone does so but is very very slow ... hundreds of commits happen in master meanwhile, he doesn't rebase
• Finally the PR comes :)

Now you could claim that you need to review two diffs:

• One very small one from the last master revision to the merge commit
  • This will only show the readme changes and this is what Github would display
• One huge diff, showing the difference between the merge result and the "old" feature branch missing hundreds of commits

=> really, linear history is significantly overrated here in my opinion. You can just follow the chain of commits in master when reviewing and pretty much get the same thing.

A merge commit can break things, also in a way that is undetectable by CI.

Not for Rultor, it merges first then runs the build :)

All commits need to be re-reviewed by another human because the position in the history matters in a commit!

Unless you have conflicts not really :) Since any automated git rebase/merge or similar will always only work in the no-conflicts scenario, it really doesn't matter all that much.
All that a PR that is fully rebased (assuming you only review diffs originating from the master branch ) gives you relative to a not-rebased PR is the ability to check individual commits.
I'm hard pressed to imagine a scenario where you would want to do that :)

So :)
Since Rultor runs your build against a rebased branch and there is no difference in the display of the PR overall result, wouldn't you agree that this feature would provide effectively no value to you ?

Hope this was a little helpful, but regarding this ticket I think being some kind of rebase/PR creator is well outside the Rultor project's scope :)

[original-brownbear]

@alex-palevsky this is invalid.

[sils]

I currently have no time to do a discussion like this. We can keep it like this: rultor is not able to do the full CI integration which might e.g. include builds running on windows for technical reasons. If it would be able to do that it'd be able to replace the CI right?

This is not affecting the assessement and resolution of the bug though.

[original-brownbear]

@sils1297 sorry for taking so long to reply here.

Agreed, Rultor cannot replace CI for Windows, though this offers a suggestion on how to combine CI + Rultor in this case.

But yes that's a different topic. Still I I think creating PRs is outside of the scope of the intentionally simple design of Rultor sorry. The above link though contains an approach though that would allow you to leverage Rultors rebasing capabilities with CI services.
Unless I missed something here, I suggest closing this one since outside of the advice above I don't think I can offer any features here, sorry :)

[original-brownbear]

@alex-palevsky invalid issue.

[original-brownbear]

@alex-palevsky this is invalid.

[original-brownbear]

@sils1297 sorry to bother you here, but could you please close this ? :)

[original-brownbear]

@alex-palevsky could you please close this, this has been going on forever now :) ?

[sils]

didnt see it

[original-brownbear]

@sils1297 thanks! :)