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 #9044

Closed
wants to merge 2 commits into from

Conversation

jalafel
Copy link
Contributor

@jalafel jalafel commented Oct 12, 2016

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

Docs.

Description of change

Fixes issue: #8893.

Updates documentation on why the green GitHub merge button is not used. Also mentioned in the above issue was a question on why Reviewed-By is important to add on PRs. The documentation has also been updated to answer this.

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
@nodejs-github-bot nodejs-github-bot added the doc Issues and PRs related to the documentations. label Oct 12, 2016
* If you do, please force-push removing the merge.
* Reasons for not using the web interface button:
* The old merge method will write an ugly commit message.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the fact that it added a merge commit was more of a problem?

Copy link
Contributor

@MylesBorins MylesBorins Oct 12, 2016

Choose a reason for hiding this comment

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

I'd avoid using the word ugly here. As @evanlucas was suggesting you might want to try something like

  • The old merge method would add an unnecessary merge commit

@MylesBorins
Copy link
Contributor

@jessicaquynh thanks for submitting this! All the commit meta data looks good. With the small nit above taken care of this will be ready to land.

@jalafel
Copy link
Contributor Author

jalafel commented Oct 12, 2016

@evanlucas @thealphanerd sorry about that! The file has been changed and updated.

Copy link
Contributor

@MylesBorins MylesBorins left a comment

Choose a reason for hiding this comment

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

I added one last nit. If you update based on that and squash the changes into the original commit it will LGTM

* If you do, please force-push removing the merge.
* Reasons for not using the web interface button:
* The old merge method would add an unnecessary merge commit.
Copy link
Contributor

Choose a reason for hiding this comment

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

one more nit: it may make sense to not refer to these as old / new

more features are bound to come, and after a certain amount of time latest won't exactly make sense.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That makes sense! Just to clarify, do you recommend removing the qualifiers completely? So that "The old merge method ... " would simply become "The merge method ... " etc.? Thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

that's exactly what I was thinking

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wonderful! Changes have been updated, and commits squashed.

@MylesBorins
Copy link
Contributor

LGTM

@gibfahn
Copy link
Member

gibfahn commented Oct 12, 2016

@jessicaquynh I think you still have the ugly line in the latest version:

The merge method will write an ugly commit message. -> The merge method will add an unnecessary merge commit

@jalafel
Copy link
Contributor Author

jalafel commented Oct 12, 2016

@gibfahn Sorry about that error! (It is admittedly my first time using git squash) That should be fixed now. Thank you for pointing that out.

@MylesBorins
Copy link
Contributor

looks great!

@jessicaquynh I usually use an interactive rebase for squashing and the like, let me know if you want to do a screen share some time and I can walk you through my workflow

@jalafel
Copy link
Contributor Author

jalafel commented Oct 12, 2016

@thealphanerd that would be really great! I'd really appreciate that. I will message you and work out more details there :)

@gibfahn
Copy link
Member

gibfahn commented Oct 12, 2016

Yeah interactive rebase is amazing, I'd have no idea what I was doing without it.

* The rebase & merge method adds metadata to the commit title.
* The rebase method changes the author.
* The squash & merge method has been known to add metadata to the commit title.

Copy link
Member

Choose a reason for hiding this comment

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

Silly question: is this empty line wanted? It wasn't there before.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi! I added the new line upon noticing that the majority of lists had two empty lines at the end before moving onto a new topic. Let me know if this should be removed!

Copy link
Member

Choose a reason for hiding this comment

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

You are right! Keep it and thanks.

Copy link
Member

@lpinca lpinca left a comment

Choose a reason for hiding this comment

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

LGTM

* The merge method will add an unnecessary merge commit.
* The rebase & merge method adds metadata to the commit title.
* The rebase method changes the author.
* The squash & merge method has been known to add metadata to the commit title.
Copy link
Contributor

Choose a reason for hiding this comment

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

Another point would be, if more than one author has contributed to the PR, only the latest author will be considered during the squashing.

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
@Qard
Copy link
Member

Qard commented Oct 14, 2016

LGTM

@MylesBorins
Copy link
Contributor

landed in ec7f3a1...98934d2

congrats on the first contribution @jessicaquynh 🎉

MylesBorins pushed a commit that referenced this pull request 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]>
MylesBorins pushed a commit that referenced this pull request 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 pull request 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 pull request 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 pull request 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 pull request 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 pull request 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 pull request 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]>
This was referenced Nov 22, 2016
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.

8 participants