PRs awaiting merge

I have two small PRs which have graduated to “awaiting merge” status. Every few days I update the branches to make sure everything remains ready to go, but it seems eventually a conflict will arise. Is there some official way to request merges to take place? I don’t have check-in privilege (and don’t really want it). Is it common for PRs to get stuck at this final state? Of all the places for them to stall, this seems like the lowest possible hanging fruit.

What are the PR numbers (links)?

Typically the thing to do is to ask the core dev who reviewed it to merge it for you. Possibly they forgot about the PR (I know I forget about PRs all the time), or possibly they assumed that you (with some core dev status left still :slight_smile: would merge it yourself. Just ping the issue to start, that usually puts it back on their radar.

Thanks. Hugo supplied the PR references. I will ping the conversations to get someone to merge them.

I dug into GitHub search a bit and came up with this query for open PRs which are awaiting merge:

is:open is:pr label:"awaiting merge" 

That produced 81 hits, 8 of which had the DO-NOT-MERGE label. That seems like a non-trivial amount of work going to waste.

It also seems a large amount of work to go over each of them and see if the review is truly done. Sometimes a core dev approves but leaves a note that someone else needs to resolve – but the bot that adds the “awaiting merge” label doesn’t know that.

If you find any that looks clearly forgotten, by all means ping them to remind the reviewer(s).

Thanks. I wasn’t aware that awaiting merge was a bot activity. I’ll spend a bit of time working through them. All those without label:DO-NOT-MERGE do have is:approved, FWIW.

And you can use a -label: to ignore a label in a search, e.g. -label:DO-NOT-MERGE so you don’t have to filter those out manually.

This actually makes it mildly more difficult to merge the PR, because merging main retriggers CI, and I can’t merge it until the CI is done. It’s good to perform this sort of update for very old PRs, though.

Feel free to ping me on simple docs PRs that you think are ready, but I might not always be able to respond quickly.

3 Likes

What should I do when PR looks fine to me but another look from other core dev (expert) is needed before merge?

  • Approve the PR, remove “awaiting merge” label, and add “awaiting core review” label.
  • Do not approve the PR, but leave “LGTM, but needs review from another core dev” comment.

As @Jelle also mentioned, and my Git Safety Commission report also concluded, its not really necessary to do this unless you have some specific need to pick up a certain change, concern that combined with other changes it may cause or some other concrete reason to do so. Otherwise, this just creates noise on the PR and the chance of mistakes when the local and remote branch gets out of sync. GitHub will tell you when there’s a merge conflict, and on merge your PR will be integrated and tested with the latest main.

To me that’s actually quite a surprisingly low number, considering the number of open PRs—I thought it would be much higher. And yeah, as others say, at least in my experience (mostly with docs PRs, but also some others) it is quite common that multiple people review and approve before a PR is merged.

Didn’t @Mariatta say to never touch those labels or things could break with the bot?

Sadly the GitHub UI is fairly insistent in recommending that you do so. Can we tweak that feature?

Also, where’s the report from the Git Safety Commission? I didn’t even know we had such a commission.

1 Like

Yeah, exactly; that’s the one reason I hesitate to enable it on many of the repos I am a maintainer of (along with lack of control over whether rebase or merge are shown, which depending on repo policy and workflow one or the other could be particularly undesirable). Typically, I find it most useful when it is being requested or triggered by a core reviewer/maintainer of the project in question, who can’t otherwise do so directly but typically is most familiar when it is appropriate to use. However, there’s no way currently to limit it to that. @ezio-melotti , thoughts?

Sorry for the confusion, I meant the Git Investigative Team :slightly_smiling_face: It’s here, if you’re like me and enjoy NTSB-style accident reports as leisure reading…

We can turn off the “update branch” button, but then there’s no way to actually do the update before merging either if you think there’s a reason to.

1 Like

For cpython I find the “update branch” button useful to retrigger bots (instead of closing and reopening the PR). It also comes in handy when there are updates to the GitHub workflows, since IIRC GitHub uses the ones in the branch.

Updating branches manually is much more involved than clicking the button, especially if you don’t have a local copy of the branch, so I would lean towards leaving it there.

FWIW I generally enable it in the repos I manage, but mostly for reasons that don’t quite apply to cpython

3 Likes