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: add optional step to onboarding doc #8774

Closed
wants to merge 2 commits into from
Closed

Conversation

Trott
Copy link
Member

@Trott Trott commented Sep 25, 2016

Checklist
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

doc

Description of change

Include information on how to force push to the contributor's own branch
so that pull requests show as merged in GitHub interface.

Include information on how to force push to the contributor's own branch
so that pull requests show as merged in GitHub interface.
@Trott Trott added the doc Issues and PRs related to the documentations. label Sep 25, 2016
@Trott
Copy link
Member Author

Trott commented Sep 25, 2016

Copy link
Contributor

@eljefedelrodeodeljefe eljefedelrodeodeljefe left a comment

Choose a reason for hiding this comment

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

LGTM. Haven't tested it yet myself, but I trust this is working and hence super useful.

Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

LGTM!

@@ -184,6 +184,7 @@ Landing a PR
* This will automatically close the PR when the commit lands in master.
* `Refs: <full-url>`
* Full URL of material that might provide additional useful information or context to someone trying to understand the change set or the thinking behind it.
* Optional: Force push the amended commit to the branch you used to open the pull request. If your branch is called `bugfix`, then the command would be `git push --force-with-lease origin master:bugfix`. When the pull request is closed, this will cause the pull request to show the purple merged status rather than the red closed status that is usually used for pull requests that weren't merged. You will likely only do this when landing your own contributions.
Copy link
Member

Choose a reason for hiding this comment

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

I think we can say You should only do this …, at least as long as there are no well-established social rules around the “Allow edits by maintainers” feature?

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree.

Copy link
Member Author

Choose a reason for hiding this comment

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

I went a bit further and dropped the You should as well: Only do this when...

Copy link
Contributor

Choose a reason for hiding this comment

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

In the long run establishing some kind of rule for every merge would be good though, let alone for "merged PR" statistics, if don't do this already properly.

Copy link
Member

@targos targos left a comment

Choose a reason for hiding this comment

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

LGTM

@targos
Copy link
Member

targos commented Sep 25, 2016

I take this opportunity to share my process for merging my own PRs because it's kind of the opposite of the guide 😆 :

git checkout bugfix
git rebase -i upstream/master
git push --force-with-lease
git push upstream bugfix:master

I can do almost the same for other's work, using git checkout https://github.com/nodejs/node/pull/XXXX and dropping the first push, but that's because I use hub.

@Fishrock123
Copy link
Contributor

Fishrock123 commented Sep 25, 2016

What's --force-with-lease?

I do git push origin +head:<pr-branch>...

@gibfahn
Copy link
Member

gibfahn commented Sep 26, 2016

@targos FYI if you don't have hub you can still fetch PR branches, I have this in my .bashrc (use with gfp origin 7654, and then gpp origin while on the branch):

# Set up local branch from Github PR, e.g. gfp origin 7654
gfp() { git fetch $1 pull/$2/head:pr-$2; } # $1: remote name, $2: PR number 
gpp() { # Update branch created with gfp, $1: remote name
  pr=`git symbolic-ref --short HEAD | cut -d "-" -f 2` # Assumes pr-7654 style branch
  git pull $1 pull/$pr/head 
}

@Fishrock123 --force-with-lease will do the force push as long as your locally fetched copy of the remote branch matches the remote repo's real remote branch (e.g. upstream/master matches github.com:nodejs/node#master). Really useful in case someone updates master between your last fetch and when you force push.

@jbergstroem
Copy link
Member

I do something similar to @targos (perhaps add some git fetch in there for completeness). I'd really like to see this flow as the suggested default instead of the patch workflow. Anyway, LGTM -- having source PR branch at the same commit as the one being merged is a great improvement and should tell the github ui to auto-close.

Copy link
Member

@jbergstroem jbergstroem left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@imyller imyller left a comment

Choose a reason for hiding this comment

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

LGTM

@jasnell
Copy link
Member

jasnell commented Sep 27, 2016

LGTM

Trott added a commit to Trott/io.js that referenced this pull request Sep 29, 2016
Include information on how to force push to the contributor's own branch
so that pull requests show as merged in GitHub interface.

PR-URL: nodejs#8774
Reviewed-By: Robert Jefe Lindstaedt <robert.lindstaedt@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Johan Bergstrom <bugs@bergstroem.nu>
Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@Trott
Copy link
Member Author

Trott commented Sep 29, 2016

Landed in 676e624

@Trott Trott closed this Sep 29, 2016
jasnell pushed a commit that referenced this pull request Sep 29, 2016
Include information on how to force push to the contributor's own branch
so that pull requests show as merged in GitHub interface.

PR-URL: #8774
Reviewed-By: Robert Jefe Lindstaedt <robert.lindstaedt@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Johan Bergstrom <bugs@bergstroem.nu>
Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Fishrock123 pushed a commit that referenced this pull request Oct 11, 2016
Include information on how to force push to the contributor's own branch
so that pull requests show as merged in GitHub interface.

PR-URL: #8774
Reviewed-By: Robert Jefe Lindstaedt <robert.lindstaedt@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Johan Bergstrom <bugs@bergstroem.nu>
Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@Trott Trott deleted the force-push branch January 13, 2022 22:44
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.

10 participants