doc: note that PR-URL should be first metadata

[Trott]

Checklist

• documentation is changed or added
• the commit message follows commit guidelines

Affected core subsystem(s)

doc

Description of change

Convention has coalesced around putting the PR-URL as the first metadata
item in pull requests, so much so that collaborators are now flagging
other collaborators' commit messages when they don't do that. So let's
make it official and document that the PR-URL should be first.

Refs: nodejs/build#325
Refs: #6385

[MylesBorins]

LGTM

[benjamingr]

LGTM, although this is a nit more than anything else.

[jbergstroem]

@Trott gave me push permission to his repo to do some minor tweaks. PTAL.

I'd also like to rework the technical howto so we can get people force pushing the commit to their branch before merging it to master, but I realise it's a bigger rewrite than a few lines added here. I'll abstain unless someone thinks otherwise.

[benjamingr]

Honestly the current process is simple and it works, unless we can get CI to a point where it's stable enough to merge from GitHub with the new merge-rebase option - I don't understand why we'd want to spend time taking care of it.

Has it actually been a problem in the past?

[jbergstroem]

@benjamingr force pushing to the remote branch is already done by the majority of collaborators since it gives the added benefit of auto-closing the PR and Fixes references (I think fixes gets closed regardless). Additionally, if someone is unsure about landing a commit; reviewing the "final" commit before its merged – for instance while onboarding – would likely improve quality.

[benjamingr]

@jbergstroem well, collaborators already review the final commit by git diff and git log (collaborator_guide shows this already IIRC).

I think fixes also works already after the merge.

The only added benefit I'm aware of is that it looks like a merge in GitHub rather than closing the issue and leaving a "refs" like comment.

I don't feel strongly about it - I'm just wondering if it's necessary given the current process works.

[jbergstroem]

@benjamingr the final commit sure; final commit message not so much.

[benjamingr]

From the current guide:

Check number of commits and commit messages

$ git log origin/master...master

[jbergstroem]

@benjamingr yes but thats still for the collaborator and not the reviewers that wants to help, no?

[bnoordhuis]

Single data point but I've always put the tags in alphabetical order. I just :sort the block in vi.

[Fishrock123]

Convention has coalesced around putting the PR-URL as the first metadata
item in pull requests, so much so that collaborators are now flagging
other collaborators' commit messages when they don't do that. So let's
make it official and document that the PR-URL should be first.

Uh, I always do this order:

• Refs
• Fixes
• PR-URL
• Review sign-off

This doesn't actually matter, and it more or less doesn't matter to tooling either...

[cjihrig]

I'm in the same boat as @Fishrock123.

[jasnell]

Agree with @Fishrock123 and @cjihrig ... not seeing this as critical.

[Trott]

If there's no expected order, that's fine by me. I just wanted it documented if it was something new collaborators were going to get nits on, like one did yesterday. I'm going to go ahead and close this. If anyone (@jbergstroem? @evanlucas?) wants to make the counterargument, by all means, please do, feel free to re-open, whatever works.