Sync Version 1.4.0 rc to main

[chrispsommers]

Sync version-1.4.0-rc to main prior to making more changes to tie up loose ends. Splitting closed #496 into separate "sync" and "update changelog" PRs.

[smolkaj]

Instead of merging/squashing, I wonder if we should we be rebasing instead?
I'm a bit worried about potentially losing history otherwise?

[chrispsommers]

Thanks for bringing this up. In hindsight, I think we should omit the squashing and preserve the same commits as main.
What benefit would rebasing have?

[chrispsommers]

Also, arggghh: Look slike I inherited 4 missing signoffs:
https://github.com/p4lang/p4runtime/pull/498/checks?check_run_id=29638974713

[smolkaj]

Rebasing does exactly that -- preserve all the commits from main without change.

It also puts your extra commits on top and gets rid of the merge commit, so less noise in the history.

And of course, that means you don't inherit things like missing sign offs :)

But it does mean you may have to deal with some merge conflicts when rebasing.

[chrispsommers]

@smolkaj I rebased and force-pushed, check out the commits and see if it satisfies. Thanks for the idea. I also did commit --amend -s to fix one missing signoff of my own, but there are four remaining, see checks. What do you think the best way to handle this is? There are caveats about rewriting history which I'm unsure about.

[chrispsommers]

@smolkaj @jafingerhut Any suggestions how to resolve the unsigned commits? Conventional wisdom is to rebase -s but also warns about force-pushing public commits. Shall I just go ahead and back-sign those commits anyway?

[antoninbas]

@chrispsommers you shouldn't force push to a branch that's not in your own repo (fork) usually. The unsigned commits predate the addition of the DCO check to this repo AFAIK. These 4 commits were also not authored by you, so you should not be signing them anyway. This is also mentioned in the DCO help page:

You should only do this if:

• You are the only author of the commits in this branch
• You are absolutely certain nobody else is doing any work based upon this branch

The best thing to do is to ignore the DCO check here. The check should only apply to new contributions, it is not relevant to previous contributions that you are cherry-picking / merging into another branch.

[chrispsommers]

@antoninbas Thanks for jumping in! I am working from a forked branch in my repo, so any forcing would be done on that branch.

Assuming I follow your advice and ignore the DCO check, should I be able to merge into version-1.4.0-rc w/o problems?

[jafingerhut]

I am not certain, but the DCO settings on this repo might be still "DCO optional", meaning that people with the right permissions can merge PRs that fail the DCO checks anyway if they wish. Some p4lang repos (maybe only p4c right now?) are "DCO mandatory", and do not permit that.

If this repo is still in "DCO optional", I'd say go for forcing the merge regardless of DCO failures for now, but plan ahead for a few months from now that you might want to try getting all PRs passing the DCO check, in case you want to do branching/merging stuff that would be blocked by failing DCO checks.

[fruffy]

As maintainer you should also see a little escape hatch :)

[smolkaj]

Sorry for leading you astray here, Chris. I'm not used to this "fork of a fork of main" workflow, I guess rebase + force push is not recommended in that case. If we're okay with making an exception this time around, you should be able to do

git push -f origin version-1.4.0-rc

but of course that bypasses the review process.

(I double checked that force push to the main branch is disabled, so at least there should be no danger of accidentally corrupting main.)

What is the typical review process in this "fork of a fork of main" model? Do people review once when things get merged into the feature branch, and then again when the feature branch gets merged into main?
(This is probably a stupid question, but since we don't use this workflow at Google I don't have experience with it.)

[chrispsommers]

As maintainer you should also see a little escape hatch :)
@fruffy Nice, thanks for pointing this out!

[chrispsommers]

Closed, superseded by #499