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: explain why we don't use the GitHub merge button #8893

Closed
Trott opened this issue Oct 2, 2016 · 11 comments
Closed

doc: explain why we don't use the GitHub merge button #8893

Trott opened this issue Oct 2, 2016 · 11 comments
Labels
doc Issues and PRs related to the documentations. good first issue Issues that are suitable for first-time contributors.

Comments

@Trott
Copy link
Member

Trott commented Oct 2, 2016

I get asked why we can't use the GitHub merge button at onboardings from time to time and I never know what issues with it are real and what I merely imagine.

onboarding.md currently says:

  • Please never use GitHub's green "Merge Pull Request" button.
    • If you do, please force-push removing the merge.

And that's it. One or two sentences explaining the deal-breaker incompatibilities with our workflow would be great. @cjihrig? Someone else?

@Trott Trott added the doc Issues and PRs related to the documentations. label Oct 2, 2016
@MylesBorins
Copy link
Contributor

the latest rebase feature changes the author

the old rebase and merge feature adds meta data to the commit title

the original merge method adds a merge commit

On Sun, Oct 2, 2016, 1:53 AM Rich Trott [email protected] wrote:

I get asked this at onboardings from time to time and I never know what
issues with it are real and what's just in my mind.

onboarding.md currently says:

  • Please never use GitHub's green "Merge Pull Request" button.
    • If you do, please force-push removing the merge.

And that's it. One or two sentences explaining the deal-breaker
incompatibilities with our workflow would be great. @cjihrig
https://github.com/cjihrig? Someone else?


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#8893, or mute the thread
https://github.com/notifications/unsubscribe-auth/AAecVzDskV5JaHh9uSbBEZ2RUDqgIIDyks5qv0a7gaJpZM4KL8Sz
.

@not-an-aardvark
Copy link
Contributor

the old rebase and merge feature adds meta data to the commit title

Squash/merge does that by default, but isn't it possible to modify the commit message while merging?

@MylesBorins
Copy link
Contributor

@not-an-aardvark I've personally experienced it adding the meta data without an obvious way to avoid it. At the very least it is error prone enough that I do not believe the pain is worth the benefit

@evanlucas
Copy link
Contributor

the latest rebase feature changes the author

yep, I dealt with that last week while cutting v6.7.0.

I would say that @thealphanerd's comment (#8893 (comment)) describes it pretty well.

@Fishrock123
Copy link
Contributor

Yeah, what @thealphanerd said.

@Fishrock123
Copy link
Contributor

(All of which make viewing commits / tooling / attribution / etc more difficult.)

@fhinkel
Copy link
Member

fhinkel commented Oct 3, 2016

Have we documented anywhere why we add Reviewed-By? I'm not sure what it's good for.

@bnoordhuis
Copy link
Member

It's like git blame for reviewers.

@addaleax
Copy link
Member

addaleax commented Oct 4, 2016

Have we documented anywhere why we add Reviewed-By? I'm not sure what it's good for.

It also gives a list of people to @mention / contact when something goes wrong, and doesn’t require the assumption that Github lasts forever for that ;)

@fhinkel fhinkel added the good first issue Issues that are suitable for first-time contributors. label Oct 7, 2016
@sonalbhadani496
Copy link

We generally don't use Github merge button when we don't want the changes in our branch to be merged with the upstream branch ; for this we can also close the pull request without merging.

jalafel added a commit to jalafel/node that referenced this issue Oct 12, 2016
Adds documentation and explicit reasons on why
the GitHub web interface button is not used. This was
explained in the referenced issue by @thealphanerd.

Fixes: nodejs#8893
@jalafel
Copy link
Contributor

jalafel commented Oct 12, 2016

Hello everybody, I took a shot at this and have opened up a pull request. Hopefully some of you can review it and see if it closes this issue.

Thank you :)

jalafel added a commit to jalafel/node that referenced this issue Oct 13, 2016
Adds verbose reasons to the documentation on why the
Reviewed-By metadata on a pull request is important.
This was loosely mentioned as an issue in the referenced
issue below, and answered by @addaleax.

Ref: nodejs#8893

change wording on documentation update

Changes the initial commit to the recommended, and more
accurate wording.

Removed time qualifiers on documentation for git merge

removes the ugly wording

add a new reason why autosquashing is prohibited
MylesBorins pushed a commit that referenced this issue Oct 14, 2016
Adds verbose reasons to the documentation on why the
Reviewed-By metadata on a pull request is important.
This was loosely mentioned as an issue in the referenced
issue below, and answered by @addaleax.

Ref: #8893
PR-URL: #9044
Reviewed-By: Myles Borins <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
jasnell pushed a commit that referenced this issue Oct 14, 2016
Adds documentation and explicit reasons on why
the GitHub web interface button is not used. This was
explained in the referenced issue by @thealphanerd.

Fixes: #8893
PR-URL: #9044
Reviewed-By: Myles Borins <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
jasnell pushed a commit that referenced this issue Oct 14, 2016
Adds verbose reasons to the documentation on why the
Reviewed-By metadata on a pull request is important.
This was loosely mentioned as an issue in the referenced
issue below, and answered by @addaleax.

Ref: #8893
PR-URL: #9044
Reviewed-By: Myles Borins <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
MylesBorins pushed a commit that referenced this issue Nov 11, 2016
Adds documentation and explicit reasons on why
the GitHub web interface button is not used. This was
explained in the referenced issue by @thealphanerd.

Fixes: #8893
PR-URL: #9044
Reviewed-By: Myles Borins <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
MylesBorins pushed a commit that referenced this issue Nov 11, 2016
Adds verbose reasons to the documentation on why the
Reviewed-By metadata on a pull request is important.
This was loosely mentioned as an issue in the referenced
issue below, and answered by @addaleax.

Ref: #8893
PR-URL: #9044
Reviewed-By: Myles Borins <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
MylesBorins pushed a commit that referenced this issue Nov 11, 2016
Adds documentation and explicit reasons on why
the GitHub web interface button is not used. This was
explained in the referenced issue by @thealphanerd.

Fixes: #8893
PR-URL: #9044
Reviewed-By: Myles Borins <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
MylesBorins pushed a commit that referenced this issue Nov 11, 2016
Adds verbose reasons to the documentation on why the
Reviewed-By metadata on a pull request is important.
This was loosely mentioned as an issue in the referenced
issue below, and answered by @addaleax.

Ref: #8893
PR-URL: #9044
Reviewed-By: Myles Borins <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
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. good first issue Issues that are suitable for first-time contributors.
Projects
None yet
Development

No branches or pull requests

10 participants