History in 2.9 is corrupted by a recent merge of master into 2.9

[SebKuzminsky]

A recent merge (see #2415) brought all of master's history into the 2.9 branch. The merge was reverted, but the history remains.

I have locked master and 2.* while we figure out how to deal with this...

The last time this happened, in January of this year, we had this discussion: https://sourceforge.net/p/emc/mailman/emc-developers/thread/tpc52q%24oro%241%40ciao.gmane.io/#msg37758545

I propose the following two-part fix, one to undo the immediate breakage and one to prevent it happening again:

1. Roll back 2.9 to before the merge of master & force-push, roll back master to before the merge of the bogus 2.9 & force-push, reapply correct changes where they belong, merge fixed 2.9 into fixed master.
2. Enable github "branch protections" on master and 2.*, so that all changes have to go through the PR process.

I'd appreciate feedback from the core devs (@jepler, @andypugh, @c-morley, @phillc54, @petterreinholdtsen, @snowgoer540, @dngarrett, etc, sorry if i missed anyone) and anyone else with an opinion on this issue .

[hansu]

@jepler and I advocated then for enabling github's "branch protection" feature on our important long-lived branches (2.8, 2.9, and master). This would mean that all changes to these branches would go through the PR process, so at least two people would have to make a mistake before history got corrupted, instead of just one like now.

The problem are not the individual commits on those branches, more the merges. The problem here was a conflict that could not be solved via the Github GUI. The Github conflict solving strategy is to merge the target branch (here master) into the feature branch (here 2.9) which is what we absolutely not want.
The result would be that the merge can only be done by a very small group of persons like you and Jeff, right?
Ideal would be to allow developers to push single commits but not to do merges. But I don't think that is possible.

[jepler]

I support force-pushing 2.9 back to the state from before this error if there are no additional commits yet.

Thanks @hansu for additional information; I was not aware of this misfeature of how github tries to handle this situation. Is there a best practice advice for how to do this?

I would also suggest considering whether 2.9 can be left in a protected state, so that changes to it have to go through the pull request process. This is different than my suggestion the other time we recently had such a problem, as it would be applied to release branches only and not main master. This decision would probably be up to the Release Manager, as they are intended to be able to exercise any necessary control over the changes that go in to release branches.

[hansu]

More information here: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/addressing-merge-conflicts/resolving-a-merge-conflict-on-github

[snowgoer540]

I am in favor of doing whatever needs done to fix that which got messed up.

I am not in favor of option 2. As @hansu said, I see this as a merging problem brought on by the fact that there are two open branches we are trying to keep straight, and not a problem with the normal commits.

I am new to the release process in LinuxCNC, but I do wonder when the line gets drawn in the sand and we stop maintaining two branches to this level? What is the criteria for "release ready"? I don't think the project will ever be perfect.

[phillc54]

I am sorry that I caused this issue, it seemed to me that resolving the conflict via the Github GUI would be simple and it did appear so at the time. Knowing what I know now I will stick to merging in my local repo.

I am also in favour of doing whatever is required to fix my mess but it is beyond my skill set.

Is it possible to prevent mergeing from the Github GUI?

[c-morley]

If we can block github merges that sounds like a good idea. I don't support restricting pushing directly to branches. A release schedule and manager would obviously be good. If reverting made things more complicated I'm sorry about that. Could you describe the dos or/and don'ts to fix if this ever happens again? Thank you. Chris

…

________________________________ From: Greg Carl ***@***.***> Sent: April 4, 2023 9:55 PM To: LinuxCNC/linuxcnc ***@***.***> Cc: c-morley ***@***.***>; Mention ***@***.***> Subject: Re: [LinuxCNC/linuxcnc] History in 2.9 is corrupted by a recent merge of master into 2.9 (Issue #2420) I am in favor of doing whatever needs done to fix that which got messed up. I am not in favor of #2<#2>. As @hansu<https://github.com/hansu> said, I see this as a merging problem brought on by the fact that there are two open branches we are trying to keep straight, and not a problem with the normal commits. I am new to the release process in LinuxCNC, but I do wonder when the line gets drawn in the sand and we stop maintaining two branches to this level? What is the criteria for "release ready"? I don't think the project will ever be perfect. — Reply to this email directly, view it on GitHub<#2420 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ABYGWNL55X7Y3IAZTZXTT5DW7SKGBANCNFSM6AAAAAAWTCKLRQ>. You are receiving this because you were mentioned.Message ID: ***@***.***>

[hansu]

Sorry also for having that initiated. I thought a PR would be a good way to show up the conflicts, but I didn't think about that this implies more or less to be also solved via Github. Maybe an issue would have been less misleading.

[petterreinholdtsen]

[Sebastian Kuzminsky]

I propose the following two-part fix, one to undo the immediate breakage and one to prevent it happening again: 1. Roll back 2.9 to before the merge of master & force-push, roll back master to before the merge of the bogus 2.9 & force-push, reapply correct changes where they belong, merge fixed 2.9 into fixed master.

Without coordination with the Weblate git repository, this will cause git merge conflicts on weblate that is hard to recover from. I spent several days and had to ask for help from the Weblate administrator the last time I did it, and I fear we lost some translations in the process. Because of this, I suggest any changes in weblate is merged into master first, Weblate updated and locked, and the PO files in master be stored away during the rollback and re-inserted in master after the rollback. The git repo on Weblate can then be reset to the new rolled back master and unlocked. This will of course also "invalidate" every git clones for all developer and others around the globe, causing confusion and requests for help. I suspect a recipe should be published on the wiki on how to recover, to allow us to point everyone to the same solution description.

2. Enable github "branch protections" on master and 2.*, so that all changes have to go through the PR process.

It seem like a good idea, but it depend on people with time and the will to review changes in a timely manner. Do we have it? Perhaps only do so for the stable release branches to reduse the work load, but I fear it will just reduce the number of people with time and will to review to one, the release manager? It could cause work on the stable branches to a standstill.

…

-- Happy hacking Petter Reinholdtsen

[andypugh]

I can definitely see an argument for protecting 2.9 at this point, given that it is meant to be on a release trajectory.

[jepler]

I have locked all the "components" on linuxcnc weblate. Depending when the roll-back of 2.9 is performed, I will help with preserving weblate changes if I'm available and someone feels my expertise with github commandline stuff is helpful.

[SebKuzminsky]

I think this is mostly resolved now.

I reset 2.9 back to just before the merge of master, then re-applied the one commit after the bad merge (@snowgoer540's "qtvcp: another attempt to prevent segfaults on exit"), then force-pushed to github.

I updated our branch protection for 2.* branches to require pull requests with at least 1 approval.

I reset master back to just before the merge of the bogus 2.9, then merged the fixed 2.9 there (the conflict resolution was straight-forward and obvious in the terminal). I re-applied the commits that looked like they made sense, which was the weblate translations and @petterreinholdtsen's small fix to the Czech ("cs") translations. I skipped the revert of the merge, and the three larger commits that all tried to repair the .po/.pot files. I then force-pushed the resulting master branch.

All branches are now unlocked.

@petterreinholdtsen, I verified that the debs still build on master, can you see if the .po/.pot files need any updates? I'm embarrassed to admit i still don't really feel confident in my ability to work with the translation infrastructure...

@jepler, please do whatever needs to be done to update Weblate on the new branches.

Devs! Please fetch from github and rebase or cherry-pick any local changes on top of the fixed branches. Do not merge your old stuff into the new branches! Carefully inspect your branches before pushing. If you feel at all uncertain what's going on, please use the PR process to get a review instead of just pushing (this is required on 2.* and optional on master). Come ask on IRC if you want any help with any of the git stuff, I'll try to pay extra close attention there over the next few days.

[andypugh]

Thanks

[andypugh]

Outside github I always:

git push --dry-run {target}

Then git responds with a pair of hashes 12345...45566
I then copy them and paste them in to

git log -p 12345...45566

Then scroll through the result, looking for surprises.

Is that enough to prevent this?

[SebKuzminsky]

Yeah, that sounds like a really good check!

[c-morley]

I think the lock on 2.9 is going to be annoying. Now if I have a bug fix for 2.9 and master, I either have to wait for someone to approve the 2.9 so I can merge to masyer or put separate commits in 2.9 and master which possibly will create merge problems later.
I really don't think we have enough active devs for this to work well. I also don't like being treated like a kid.

[SebKuzminsky]

or put separate commits in 2.9 and master which possibly will create merge problems later

Don't do this! Do the first thing you said: put the fix in 2.9, then merge 2.9 to master.

I added the branch protection based on this nearly-endorsement from the 2.9 release manager, @andypugh:

I can definitely see an argument for protecting 2.9 at this point, given that it is meant to be on a release trajectory.

I respectfully ask that we all try to spend some time reviewing pull requests, and that we all try to make easy-to-review pull requests for each other (small simple commits, good commit messages).

If it's all too painful, we can talk about moving back to the "push anything you want" model we have now.

[c-morley]

I thought Andy gave up being release manager months ago as he could make no headway?
Andy said 'see an argument' meaning it needs discussion.

I'm sure it will work fine for a little while as it's new.

The problem was Phill tried something new and out of the ordinary for him - using github to merge - I bet he will never make that mistake again! I don't know what happened with Dewey but I'm sure it's a one off too.
neither of these had anything to do with the code - it was the merge.
github cheat sheet/best practice docs for developers would be more helpful then restricting access.

Restricting pushes will eventually lead to only a few outcomes:
pull requests sit too long - discouraging bug fixes in released branches - though master will starting getting busy again.
one person does the bulk of the reviews - annoying for them
people make pacts -approve mine, I'll approve yours (maybe with cursory looks) -defeats the purpose of locking it.

[snowgoer540]

I am thankful there are people in this project who understand github and have the ability to fix issues when they arise. But I agree with Chris, this seems to be an over reaction to a simple github related mistake, and it has nothing to do with the content or the quality of the individual commits. While it may not be apparent, much of at least the qtvcp part of the project is peer-reviewed between Chris, Phill, and myself. Having the approval process, and to potentially have to defend/explain the changes to yet another person is discouraging.

Speaking only for myself, pushing to 2 branches was annoying enough as it is, I am likely to just move on from 2.9. There seems to be no leader and no plan, but now a peer review process. So I ask again, what is the plan for 2.9? What are we doing?

[petterreinholdtsen]

[Sebastian Kuzminsky]

@petterreinholdtsen, I verified that the debs still build on master, can you see if the .po/.pot files need any updates? I'm embarrassed to admit i still don't really feel confident in my ability to work with the translation infrastructure... @jepler, please do whatever needs to be done to update Weblate on the new branches.

On weblate I see this on <URL: https://hosted.weblate.org/projects/linuxcnc/#repository >: 29 outgoing commits in the Weblate repository 42 missing commits in the Weblate repository In addition I see this: On branch master Your branch and 'origin/master' have diverged, and have 14 and 21 different commits each, respectively. (use "git pull" to merge the remote branch into yours) nothing to commit, working tree clean It seem to me my suggested procedure of merging everything from Weblate before starting and keeping the PO files was not followed? It is the only scenario I can come up with that would lead to 29 commits to translations on Weblate being considered missing in master on Weblate. @jepler, I guess we will have to coordinate the cleanup on IRC. I am only rarely available the next few days.

…

-- Happy hacking Petter Reinholdtsen

[snowgoer540]

Where did we land on this? Is 2.9 still set to require peer checking or?

Do we have a plan for the future? Or do we solicit feedback and then just do whatever we want anyways?

[SebKuzminsky]

The current setting is that everyone can just push to master like always, and 2.* require a PR with at least 1 approval. It's easy to change if that's too much friction...

[c-morley]

With respectful understanding of your intention to safeguard released branches, I say this was too far without a majority consensus or immediate emergency need.

2.9 is not released, as far as I can tell it's in stabilization mode with no actual target date.

We have never used a release person as a commit by commit arbiter, only as a overall guide you can ask if there is question of reasonableness.

Thank you for your help with this last situation and the good discussion it created.

[snowgoer540]

It's easy to change if that's too much friction...

Yes, please.

[andypugh]

There does seem to be a lot of increased friction, my PR for bugfixes and improvements in PktUART has been sat waiting for approval since Friday.

[phillc54]

I could approve it but it is not a part of the code that I am comfortable with.

[SebKuzminsky]

Ok, I removed the branch protection. Merge away.

[petterreinholdtsen]

[Petter Reinholdtsen 2023-04-06]

@jepler, I guess we will have to coordinate the cleanup on IRC. I am only rarely available the next few days.

This never happened, so yesterday I decied to have a look by myself. I verified the latest PO files commited on weblate were already in master and used Maintenance->Reset on the repository admin page to throw away the existing branch on Weblate to clear the conflict. The project is now open for translations again. I hope and believe no translations were lost.

…

-- Happy hacking Petter Reinholdtsen

[andypugh]

I hope and believe no translations were lost.

Thanks