[DEV] The horrifying "rebase needed"

For people working on the C++ code.
User avatar
Wuzzy
Member
 
Posts: 3783
Joined: Mon Sep 24, 2012 15:01
GitHub: Wuzzy2
IRC: Wuzzy
In-game: Wuzzy

[DEV] The horrifying "rebase needed"

by Wuzzy » Wed Oct 23, 2019 23:52

Am I the only one who is always horrified by the horrifying “rebase needed” label on GitHub? I find this very demoralizing, every time. I am horrified because I am forced to type in “git rebase” because it's the Danger Zone(TM) and I risk of destroying my Git repo. And this is part of the expected workflow. It has become so bad, I am now even used to simply copying the entire local Git repo as backup simply to have a stable state in case I manage to screw up my local repo once again …

Or did I just completely misunderstand the whole point of this? It would be nice if there would be something about this in the Dev Wiki.

How many finished PRs are there floating around on GitHub for which the only reason not to merge is a “rebase needed”? I gonna bet it's a lot.

Why does the submitter has to do the rebase in the first place? Can't this be fixed by those with merge rights?
I often saw the PR submitter “earned” the label not due to own wrongdoing, but simply because the review took so long, that after the review, the PR is now so old, that there are now tons of commits between the PR's HEAD and the official MT repo's HEAD … It's very frustrating when this happens and it makes me wonder why I should even bother trying to submit PRs in the first place …

What I also find strange is that Minetest is the only project I am aware of that has this “rebase needed”.
My creations. I gladly accept bitcoins: 17fsUywHxeMHKG41UFfu34F1rAxZcrVoqH
 

sofar
Developer
 
Posts: 2106
Joined: Fri Jan 16, 2015 07:31
GitHub: sofar
IRC: sofar
In-game: sofar

Re: [DEV] The horrifying "rebase needed"

by sofar » Thu Oct 24, 2019 04:23

Wuzzy wrote:Why does the submitter has to do the rebase in the first place? Can't this be fixed by those with merge rights?


No, nobody knows the change better than the submitter. Asking reviewers to also rebase changes is a recipe for disaster - you'll get really nasty bugs.

Rebases are nasty because people don't know how to do it well. It's not difficult to learn, but it requires experience and practice. I think I covered it in the big git discussion thread, but I'm not sure.


Wuzzy wrote:What I also find strange is that Minetest is the only project I am aware of that has this “rebase needed”.


Every github project can trigger this condition. It's everywhere, trust me - I've hit it for many projects.
 

User avatar
Linuxdirk
Member
 
Posts: 2338
Joined: Wed Sep 17, 2014 11:21
Location: Germany
In-game: Linuxdirk

Re: [DEV] The horrifying "rebase needed"

by Linuxdirk » Thu Oct 24, 2019 07:14

Wuzzy wrote:What I also find strange is that Minetest is the only project I am aware of that has this “rebase needed”.

Because pull requests are open for AGES and thus most of the code has changed before any of the devs even looks at. And by the time, well, a rebase is needed.
 

Xudo
Member
 
Posts: 161
Joined: Wed Nov 09, 2016 16:43
GitHub: akryukov92
In-game: Xudo

Re: [DEV] The horrifying "rebase needed"

by Xudo » Thu Oct 24, 2019 10:26

It is common problem in projects with multiple developers.
When amount of new PRs greater than 1, then second PR might become obsolete.
There are two ways to make second PR up-to-date: rebase or "merge master in develop".
Merging master to develop produces a lot of meaningless merge-commits. So rebasing produce cleaner commit history.

I agree with sofar thesis: nobody knows change better than submitter. That's why he is the only man who can update his PR correctly.

Though, the more PRs are opened, the more rebase is needed. It can lead to combinatory explosion of needed rebases.
Repository owner can reduce amount of rebase by establishing merging queue.
This way each submitter could rebase his work only once, before merging to master.
But this approach requires discipline, commitment and obligations.
 

User avatar
Wuzzy
Member
 
Posts: 3783
Joined: Mon Sep 24, 2012 15:01
GitHub: Wuzzy2
IRC: Wuzzy
In-game: Wuzzy

Re: [DEV] The horrifying "rebase needed"

by Wuzzy » Thu Oct 24, 2019 10:43

I'm not really satisfied with this response.

The part I do understand is that the submitter indeed understands best what needs to be done. I agree here.

BUT:

The problem is that every rebase is a risk of destroying your repo. One mistake and all your work is destroyed. Just saying “just be more careful” is not good enough. We all can have a bad day.

It kind of flies in the face of the idea of a version control system. Normally, you are allowed to screw up in Git. Just revert. This is a feature, not a bug. But you are basically forcing submitters to rewrite history regularily. Which is always Danger Zone. And if you screw up, there's no way to revert …

Rewriting history regularily is bad because we are basically throwing away one of THE key benefits of version control: Being allowed to screw up and revert your changes.

I understand the “rebase needed” is needed when there's a really big time gap. But the problem I have is that this condition happens way too often in Minetest. It's nasty to force your submitters to risk all their work almost every time they submit a PR … I wonder if I am the only one who is frustrated by this or if there are other people as well.

Isn't there really any better (i.e. safer) way to do it?

Why not a merge? Is it really that big of a deal when there's merge commits? Especially if it's only a small merge. I don't understand the obsession with a “clean” history. Being able to go back in history is more important than a “clean” history. I'm not saying we should stop caring about commit messages but I'm saying that it's a little strange to be so obsessed with a “clean” history when the price you have to pay is to regularily risk your work. I can understand “rebase needed” for BIG changes with a BIG time gap (because it would blow up the history), but I really don't understand why it's needed for every conflict, even if there's only 1 commit in between.

What I also find a bit frustrating is that the way how “rebase needed” in Minetest is a bit like a cat-and-mouse game. Why should I trust the core devs my PR is actually merged after the rebase, or if it is just left to rot for another month? If other commits managed to sneak in before mine, and cause conflicts, then the first rebase was completely pointless and it starts over!

I'm not saying I will never ever do the rebases ever again, but I think something is rotten about the whole PR submitting system.
My creations. I gladly accept bitcoins: 17fsUywHxeMHKG41UFfu34F1rAxZcrVoqH
 

User avatar
rubenwardy
Moderator
 
Posts: 5961
Joined: Tue Jun 12, 2012 18:11
Location: United Kingdom
GitHub: rubenwardy
IRC: rubenwardy
In-game: rubenwardy

Re: [DEV] The horrifying "rebase needed"

by rubenwardy » Thu Oct 24, 2019 10:59

Wuzzy wrote: And if you screw up, there's no way to revert …


This is incorrect, you can revert rebases using the reflog
 

User avatar
Hugues Ross
Member
 
Posts: 49
Joined: Mon May 06, 2019 22:52
GitHub: Df458

Re: [DEV] The horrifying "rebase needed"

by Hugues Ross » Thu Oct 24, 2019 11:25

I honestly don't see much risk in rebasing, if you have an open PR then you have an online copy of the repo and you'll need a local copy to perform the rebase. If you screw up, the worst case is having to re-download the original repo and start over.

Even if it was risky (which it isn't at all) rebasing is not difficult, especially not if you do it regularly to keep your PR up-to-date with upstream changes.
 

Xudo
Member
 
Posts: 161
Joined: Wed Nov 09, 2016 16:43
GitHub: akryukov92
In-game: Xudo

Re: [DEV] The horrifying "rebase needed"

by Xudo » Thu Oct 24, 2019 12:12

If you need local backup, you can make backup branch and rebase develop branch. Backup branch will keep unrebased commits.
Your remote branch will also keep unrebased work.
There are enough means to protect yourself from mistakes.

Though I agree that rewriting history regularly is bad workflow. Keeping branch up-to-date constantly is very tiresome.
Ideally you need to Rebase only once, just before merging.
It is very possible to do so, but requires more communication between repository owner and submitter.
Something like:
Maintainer: Hey, I want to merge your work next, could you please rebase in two days?
Submitter: Sure
Submitter rebases
Maintainer merges PR

Or
Maintainer: Hey, I want to merge your work next, could you please rebase in two days?
Submitter: sorry no, I am busy atm.
Maintsineer: ok
Maintsineer asks someone else

Or
Maintainer: Hey, I want to merge your work next, could you please rebase in two days?
Submitter didn't respond
2days after
Maintsineer asks someone else


Here are example of exercise with 5 developers commuting in single repository. After some point they constantly get conflicts in code and had to merge master to develop:
https://github.com/AKryukov92/dev007oct ... its/master
As you can see, repository history quickly become cluttered by merge commits.
 

User avatar
Krock
Developer
 
Posts: 4469
Joined: Thu Oct 03, 2013 07:48
Location: Switzerland
GitHub: SmallJoker

Re: [DEV] The horrifying "rebase needed"

by Krock » Thu Oct 24, 2019 17:07

If you're afraid of rebase, it's also possible to merge master into your branch (I believe). This will also show up conflicts and lets you resolve them. If the PR is not too big it is going to be squashed and rebase-merged into master - it does not matter how you resolved the conflicts as long you only modify your own commits.
PS: The longer you wait to rebase, the harder it will be. Some PRs even needed adoptions because the author was no longer keen to keep it up to date, or to fix bugs.

Both - you and me are sometimes upset about PR delays, but only the following will help:
1) Code in an understandable manner
2) Find someone who understands the code area, or someone who checks the PR in functionality and for possible bugs, no matter whether core dev or not
3) Proof that your PR really works, and highlight edge cases you checked just for case
4) Put a friendly reminder in #minetest-dev and wait

I would really like to get more developers into the team, but barely one of the possible candidates show up in IRC.
Look, I programmed a bug for you. >> Mod Search Engine << - Mods by Krock - DuckDuckGo mod search bang: !mtmod <keyword here>
 

User avatar
Mantar
Member
 
Posts: 84
Joined: Thu Oct 05, 2017 18:46

Re: [DEV] The horrifying "rebase needed"

by Mantar » Thu Oct 24, 2019 18:04

If you don't want to rebase, you can close it and make a new PR. Git's cherrypick function may be helpful for pulling the commits from your broken PR branch to a new, clean one.
 

sofar
Developer
 
Posts: 2106
Joined: Fri Jan 16, 2015 07:31
GitHub: sofar
IRC: sofar
In-game: sofar

Re: [DEV] The horrifying "rebase needed"

by sofar » Thu Oct 24, 2019 18:10

Wuzzy wrote:The problem is that every rebase is a risk of destroying your repo. One mistake and all your work is destroyed. Just saying “just be more careful” is not good enough. We all can have a bad day.


This is an education problem: as in, you need to learn how to not wipe your code and use the tools correctly.

$ git pull --rebase origin/master

.. fix patch 1, git add $conflictfiles, git rebase --continue

.. fix patch 2, git add $conflictfiles, git rebase --continue

<rebase 3/5> $

afugedaboutitigiveup!!!

$ git rebase --abort

Boom, you're back where you started, nothing was lost.
 

sofar
Developer
 
Posts: 2106
Joined: Fri Jan 16, 2015 07:31
GitHub: sofar
IRC: sofar
In-game: sofar

Re: [DEV] The horrifying "rebase needed"

by sofar » Thu Oct 24, 2019 18:11

Mantar wrote:If you don't want to rebase, you can close it and make a new PR. Git's cherrypick function may be helpful for pulling the commits from your broken PR branch to a new, clean one.


From my experience, this is never less work than properly rebasing.
 

User avatar
v-rob
Member
 
Posts: 713
Joined: Thu Mar 24, 2016 03:19
Location: Right behind you.
GitHub: v-rob

Re: [DEV] The horrifying "rebase needed"

by v-rob » Thu Oct 24, 2019 23:12

I've only messed up a rebase once before commiting and that's because I used the wrong command and it wasn't really a rebase. Otherwise, I've always either aborted the rebase or used the reflog.

So no, I've never found rebases dangerous, just kinda annoying and time-taking.
 

User avatar
paramat
Developer
 
Posts: 3555
Joined: Sun Oct 28, 2012 00:05
Location: UK
GitHub: paramat
IRC: paramat

Re: [DEV] The horrifying "rebase needed"

by paramat » Fri Oct 25, 2019 00:57

Rebasing is horrifying for everyone, i need to concentrate very hard to do it right.

> How many finished PRs are there floating around on GitHub for which the only reason not to merge is a “rebase needed”?

Not many, there are usually other reasons why it is not mergeable.
In the situation you describe the core devs usually add comments to try to get it rebased and merged.
If a rebase is the only obstacle to merge then that is the responsibility of the author, not the core devs.

See sofar's first post above, i agree with that.
Of course, pull requests are open for ages in many projects, there are many projects moving slower than MT.
It happens often, simply as a result of the situation, and it is frustrating, core devs suffer this too because we are also contributors.

> Why should I trust the core devs my PR is actually merged after the rebase, or if it is just left to rot for another month?

You cannot expect a merge after a rebase if the PR has not been reviewed, tested and approved for a merge, so you are not being asked to trust in this.
However, if a rebase is all that is delaying a merge it will be merged, the core devs are desperate to get PRs merged.

> I think something is rotten about the whole PR submitting system.

No, it is just how it is, and it is the same in many projects, because you are dealing with the unavoidable basic processes of development.
I get the feeling you are trying to blame MT specifically, but you are just raging against reality.

Contributing is hard work, and i can assure you it is even more difficult to be a core dev.
 

User avatar
TumeniNodes
Member
 
Posts: 2835
Joined: Fri Feb 26, 2016 19:49
Location: in the dark recesses of the mind
GitHub: TumeniNodes
IRC: tumeninodes
In-game: TumeniNodes

Re: [DEV] The horrifying "rebase needed"

by TumeniNodes » Fri Oct 25, 2019 04:17

I suffer from rebasephobia but, I am able to do it, I just need to make sure I'm focusing when performing a rebase, else I tend to type too fast and use wrong characters or something and then notice it just as I hit ENTER. =:o

I don't mind rebasing a PR once or twice... any more than that though and I start to get annoyed and motivation starts to drop.
I don't find this unreasonable as my PRs tend to be literally 5 minute reviews/tests at most. But I also realize for a core dev's end... 6, 5 min reviews/tests add up, especially with 3 or 4, 30 min reviews/tests besides.
And then there are some PRs which are even more involved and need to be reviewed even more thoroughly.
It's easy and common for some to beat down core devs, when not being the one doing what they do... aside from life outside of MT/MTG
Performing their own work, plus reviewing and testing other people's work, plus sometimes needing to make decisions which are not popular but are critical and in-line with the overall scheme of dev style for the project...
Even those who feel they could do better or have better ideas related to how dev is carried out.. if brought into a core dev position would realize quickly, that 70% of what they think prior will go right out the window once in that position... that is when the reality will sink in.

Not in this project but, I have seen developers on other projects actually receive threats for decisions or changes they have made. It's not always all fun and actually quite often becomes not so fun but you keep at it because a feeling of obligation sets in.
In particular, I recall the transition from KDE 3.5 to 4.0 being quite an ugly scene in the Linux community.
Ich mag keine grünen Eier und Schinken, ich mag sie nicht Sam I Am
 

User avatar
Linuxdirk
Member
 
Posts: 2338
Joined: Wed Sep 17, 2014 11:21
Location: Germany
In-game: Linuxdirk

Re: [DEV] The horrifying "rebase needed"

by Linuxdirk » Fri Oct 25, 2019 06:34

paramat wrote:You cannot expect a merge after a rebase if the PR has not been reviewed, tested and approved for a merge, so you are not being asked to trust in this.

So in theory rebasing a PR over several months without a dev even having a look at it is possible. Wow, great motivational situation.

So better not open any PRs at all?
 

ShadMOrdre
Member
 
Posts: 569
Joined: Mon Dec 29, 2014 08:07
Location: USA
GitHub: ShadMOrdre
In-game: shadmordre

Re: [DEV] The horrifying "rebase needed"

by ShadMOrdre » Fri Oct 25, 2019 18:33

We can all rant. If you want your PR merged, rebase it. If you don't want to rebase, or you simply want to complain, well, that will get you very close to nowhere, and fast!

+ Spoiler
MY MODS: lib_ecology lib_materials lib_clouds lib_node_shapes ---- Inspired By: Open Source Virtual World Simulator Opensimulator.
 

User avatar
rubenwardy
Moderator
 
Posts: 5961
Joined: Tue Jun 12, 2012 18:11
Location: United Kingdom
GitHub: rubenwardy
IRC: rubenwardy
In-game: rubenwardy

Re: [DEV] The horrifying "rebase needed"

by rubenwardy » Fri Oct 25, 2019 19:28

It is problematic to ask for rebases never review. There's some implication that asking for a rebase means that the core dev is actually interest

Also see this: viewtopic.php?f=7&t=19877
 

User avatar
Hybrid Dog
Member
 
Posts: 2726
Joined: Thu Nov 01, 2012 12:46

Re: [DEV] The horrifying "rebase needed"

by Hybrid Dog » Fri Nov 08, 2019 09:39

Rebases are required when conflicts happen, and by default conflicts are detected even when trivial changes happen at the same place, e.g. a comment is changed in the master branch.
There exists software which can resolve conflicts based on the code semantic. It could reduce the number of manual rebases.
I think an example is semanticmerge, which is free for open source projects. I haven't tested it.
https://www.semanticmerge.com/features

‮‪‮
‮‪‮
‮‪‮
‮‪‮
‮‪‮
‮‪‮
‮‪‮
‮‪‮
‮‪
 


Return to Partly official engine development



Who is online

Users browsing this forum: No registered users and 1 guest