Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

doc: include V8 commit URL in V8 backport guide #16054

Closed
wants to merge 1 commit into from

Conversation

gibfahn
Copy link
Member

@gibfahn gibfahn commented Oct 6, 2017

Use Commit: for the V8 commit, and PR-URL: for the Node PR URL.

Bikeshedding over Commit: welcome.

Refs: #16053 (comment)

Checklist
Affected core subsystem(s)

doc, v8

cc/ @nodejs/v8

@nodejs-github-bot nodejs-github-bot added the doc Issues and PRs related to the documentations. label Oct 6, 2017
@@ -196,7 +196,8 @@ Original commit message:
Review-Url: https://codereview.chromium.org/2159683002
Cr-Commit-Position: refs/heads/master@{#37833}

PR-URL: <pr link>
Commit: https://github.com/v8/v8/commit/a51f429772d1e796744244128c9feeab4c26a854
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be Refs:?

brb getting more paint

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SGTM. Surprised you didn't ask for Ref: though...

@targos
Copy link
Member

targos commented Oct 6, 2017

I'm not sure we need that. You can already look at the changes at the Reviewed-on: URL that is always present in the original commit message.

Use `Commit:` for the V8 commit, and `PR-URL:` for the Node PR URL.

Refs: nodejs#16053 (comment)
@gibfahn
Copy link
Member Author

gibfahn commented Oct 6, 2017

I'm not sure we need that. You can already look at the changes at the Reviewed-on: URL that is always present in the original commit message.

True, but that requires understanding of the Chromium pull request UI, which I find pretty opaque compared to the Github equivalent.

@targos
Copy link
Member

targos commented Oct 6, 2017

Fair enough. I'm not against this change if it can help people.

Note to self: update https://github.com/targos/update-v8 when this has landed.

@joyeecheung
Copy link
Member

Landed in 9f1e6e7, thanks!

joyeecheung pushed a commit that referenced this pull request Oct 13, 2017
Use `Commit:` for the V8 commit, and `PR-URL:` for the Node PR URL.

Refs: #16053 (comment)
PR-URL: #16054
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
@gibfahn gibfahn deleted the v8-guide branch October 13, 2017 13:27
addaleax pushed a commit to ayojs/ayo that referenced this pull request Oct 15, 2017
Use `Commit:` for the V8 commit, and `PR-URL:` for the Node PR URL.

Refs: nodejs/node#16053 (comment)
PR-URL: nodejs/node#16054
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this pull request Oct 18, 2017
Use `Commit:` for the V8 commit, and `PR-URL:` for the Node PR URL.

Refs: #16053 (comment)
PR-URL: #16054
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos added a commit to targos/update-v8 that referenced this pull request Oct 18, 2017
MylesBorins pushed a commit that referenced this pull request Nov 16, 2017
Use `Commit:` for the V8 commit, and `PR-URL:` for the Node PR URL.

Refs: #16053 (comment)
PR-URL: #16054
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Nov 21, 2017
MylesBorins pushed a commit that referenced this pull request Nov 21, 2017
Use `Commit:` for the V8 commit, and `PR-URL:` for the Node PR URL.

Refs: #16053 (comment)
PR-URL: #16054
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Nov 28, 2017
Use `Commit:` for the V8 commit, and `PR-URL:` for the Node PR URL.

Refs: #16053 (comment)
PR-URL: #16054
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants