6 ms·

GitHub Fork and Pull Request Workflow - Quick Reference Guide

83 points by susam 9y ago

foo101 9y ago
There are two different methods to merge pull requests in the article. When do people prefer one over the other? Are there any good reasons why someone should try to rebase and merge (fast-forward) a pull request to avoid a merge commit? It involves more steps than the method which creates a merge commit. What benefits warrant these additional steps?
- deleted 9y ago
  [deleted]
- mpetrovich 9y ago
  I personally prefer merging via squash since it keeps a linear commit history without polluting the history with tons of intermediate work-in-progress commits. A linear history is helpful when debugging since it makes git bisect a lot easier. Having individual PR commits in the history makes git bisect useless since it’s unfortunately common for individual PR commits to leave the system in a broken state.
  - u801e 9y ago
    > I personally prefer merging via squash since it keeps a linear commit history without polluting the history with tons of intermediate work-in-progress commits. How do you deal with cases where your changes require more than a single commit? Some changes are easier to review when they're separated into several logical commits rather than having one giant diff from a single commit.
    - faitswulff 9y ago
      You can rebase locally into commits that make sense and then merge normally on GitHub.
    - mpetrovich 9y ago
      The squash is only performed when merging the pull request (via GitHub’s merge button on the pull request). The actual work is done and reviewed as individual commits. The nice part of this workflow is that the changes are all squashed into a single commit in the trunk, but there’s always a link to the pull request where you can see the individual commits pre-squash.
      - mnsc 9y ago
        Does the individual commits pre-squash still exist in the repo or are they only saved in the PR, ie. duplicated and saved somewhere in Github's infrastructure?
        jmuhlich 9y ago
        Yes, the original individual commits are kept alive and never garbage collected from GitHub's copy of the repository. You can get back to them on the website via the "Commits" tab of the PR. Local clones won't contain those branches though unless you explicitly fetch them.
    - gonewest 9y ago
      I deal with that by keeping the separate logical commits, like you said, and I squash only the commits that don't add any historical meaning (like "fix a typo" or "forgot to update the changelog"). I put the issue number in the comments so you can view the history and see a group of commits are all related to a single issue.
- CJefferson 9y ago
  Some people like a linear history. Personally I don't, because it produces commits which have "never existed", and have certainly never been tested to check they even compile.
- rahkiin 9y ago
  I have found that rebasing often creates more issues than it is worth it. Only when there are only a couple of commits I might try but you are still rewriting history: something that is destructive and needs force-pushing which I almost always disallow. Merging does create a merge commit which is sometimes annoying (when you are merging a single-line commit, for example). It does however preserve full history.
  - u801e 9y ago
    > It does however preserve full history. Frequently, I see a commit history like: Implemented feature Forgot semi-colon Fixed syntax error Addressing review comments Removed unnecessary method History like that, other than the initial commit, is not very useful. It makes more sense to rebase to clean up the commit history such that it becomes a logical series of commits, which can then be viewed via git log or git blame.
  - foo101 9y ago
    Why would rebase require force-pushing? You can always rebase the pull request on the most current master. There would be no conflicts then and no need to force-push. If the push fails because the master has changed during the few minutes you spent in rebasing, just rebase again on the most current master and push again. No force-push is required.
- u801e 9y ago
  > Are there any good reasons why someone should try to rebase and merge (fast-forward) a pull request to avoid a merge commit? I've always thought of it as a way to effectively create a branch, do your work, and fast-foward merge it back in to the main branch without having any other work included in it. To put it another way, let's say you create a fork, do your work, and in the interim, several other contributors do the same thing and merge their work back into the main repo. Now, you could merge the mainline back into your fork and have a merge commit in your private branch that shows the changes that the other contributors made since you created your branch, or you could merge your branch back into the main branch at the end and have it show that it branched off earlier than the current head of the main branch. In the first case, you now have a commit that shows changes that you didn't actually make. In the second case, it shows that you didn't base your work on the latest version of the main branch. In contrast, if you rebase and only merge your work into the main branch via a fast-forward merge, your branch will only show commits that you made and it will effectively show that it was based on the latest version of the main branch before it was merged back in.
- tedmiston 9y ago
  IMO rebasing is always worth it. Commit history is read more than it's written. For community projects, squashed focused PRs that are rebased are simplest for everyone else besides the author.
asmdev 9y ago
After we merge a pull request with rebase + fast forward merge, there is no merge commit created. This leads to a linear commit history. Does anyone know anyway to look at the linear commit history alone (not GitHub) and tell where a pull request was merged and who merged it?
- zoul 9y ago
  I think that’s not possible, by definition. There is no merge commit, ergo no information about the merge. You may or may not consider that a feature.
- mikepurvis 9y ago
  GitLab offers this as a merge option, and I really dislike it for this exact reason. Worse still than not knowing which merge is associated with the commit is that you can't even tell the difference between reviewed commits and those which were simply pushed directly to master. IMO it's far preferable to have an explicit "merge" commit referencing the PR in question, whether that that is a --no-ff merge or a single linear squashed commit.
- gonewest 9y ago
  Git tracks the committer and the author separately: you should see the “who merged” preserved as the committer and the “who wrote it” as the author. If each commit notes the issue number in its comment (“Add indentation and trailing white space check to CI [#123]”) then you can see easily when each merge occurred by issue number. Additionally in GitHub there are conveniences built from these little conventions. The “#123” is displayed as a hyperlink to the issue, and merging a commit that has “Fix #123” automatically closes the issue. In cases where a feature is big enough to justify multiple commits for clarity, you can mention “#123” in all but the last commit, and “fix #123” in the final commit. Doing so partitions the linear history. In fact, if someone else notices and fixes a regression caused by the merge, they can push additional commits later on but mention “#123” in their commit as well for documentary purposes. Seeing downstream commits “out of sequence” but with some earlier issue number is a clue this was probably a regression.
  - 9y ago
zie 9y ago
I just wish they would also support patches. Forcing us to fork and merge back just for GitHub is annoying. Especially for quick drive-by type patches and smaller doc changes and just minor fixes. I git clone'd the project, built it, noticed something small, fix it locally, and then decide OK I'll just send this up real quick. oh crap. NOPE I have to do a bunch of GitHub crap with forking and merging to get it back to the master tree. Annoying. Forking and merging is perfectly fine for regular contributions, but for smaller one-time type fixes it's just annoying.
- zoul 9y ago
  FWIW, for smaller drive-by fixes you may press the Edit button directly on a file in some else’s repo.
  - bastawhiz 9y ago
    And in truth, the "github crap" is clicking "Fork" and running "git remote add". Pushing and filing a PR are the same process.
    - lozenge 9y ago
      Except your fork now prominently appears even after the PR is merged. However, I don't think Github has a way to see all your PRs.
      - shlomi-noach 9y ago
        Here's how to see all your PRs: https://github.com/search?q=is:pr+author:YOURNAME&type=Issues&ref=searchresults https://github.com/search?q=is:pr+author:YOURNAME&type=Issue... Here's how to see all your open PRs: https://github.com/search?q=is:pr+is:open+author:YOURNAME&type=Issues&ref=searchresults https://github.com/search?q=is:pr+is:open+author:YOURNAME&ty... Replace YOURNAME with your GitHub handle, or pick your colleagues names.
        9y ago
whatastorymark1 9y ago
Semi-related question: Assuming I drafted up a software license with generally permissive terms (freedom to use, modify, sell, etc.), but included some perverse clause that would forbid forking (such as only allowing the software to be distributed from the canonical repository on my profile), would I have legal grounds to request repository removal from GitHub if someone pressed the repo's fork button?
- carreau 9y ago
  Yes, when you created a GH account, you read and accepted the TOS, which enforce your project must be forkable.
  - whatastorymark1 9y ago
    Interesting, thank you! Relevant excerpt: > If you set your pages and repositories to be viewed publicly, you grant each User of GitHub a nonexclusive, worldwide license to use, display, and perform Your Content through the GitHub Service and to reproduce Your Content solely on GitHub as permitted through GitHub's functionality (for example, through forking). For those interested, GitHub's entire TOS is written in a very approachable way. https://help.github.com/articles/github-terms-of-service/ https://help.github.com/articles/github-terms-of-service/

83 points by susam 9y ago

There are two different methods to merge pull requests in the article. When do people prefer one over the other? Are there any good reasons why someone should try to rebase and merge (fast-forward) a pull request to avoid a merge commit? It involves more steps than the method which creates a merge commit. What benefits warrant these additional steps?

deleted 9y ago
[deleted]
mpetrovich 9y ago
I personally prefer merging via squash since it keeps a linear commit history without polluting the history with tons of intermediate work-in-progress commits. A linear history is helpful when debugging since it makes git bisect a lot easier. Having individual PR commits in the history makes git bisect useless since it’s unfortunately common for individual PR commits to leave the system in a broken state.
- u801e 9y ago
  > I personally prefer merging via squash since it keeps a linear commit history without polluting the history with tons of intermediate work-in-progress commits. How do you deal with cases where your changes require more than a single commit? Some changes are easier to review when they're separated into several logical commits rather than having one giant diff from a single commit.
  - faitswulff 9y ago
    You can rebase locally into commits that make sense and then merge normally on GitHub.
  - mpetrovich 9y ago
    The squash is only performed when merging the pull request (via GitHub’s merge button on the pull request). The actual work is done and reviewed as individual commits. The nice part of this workflow is that the changes are all squashed into a single commit in the trunk, but there’s always a link to the pull request where you can see the individual commits pre-squash.
    - mnsc 9y ago
      Does the individual commits pre-squash still exist in the repo or are they only saved in the PR, ie. duplicated and saved somewhere in Github's infrastructure?
      - jmuhlich 9y ago
        Yes, the original individual commits are kept alive and never garbage collected from GitHub's copy of the repository. You can get back to them on the website via the "Commits" tab of the PR. Local clones won't contain those branches though unless you explicitly fetch them.
  - gonewest 9y ago
    I deal with that by keeping the separate logical commits, like you said, and I squash only the commits that don't add any historical meaning (like "fix a typo" or "forgot to update the changelog"). I put the issue number in the comments so you can view the history and see a group of commits are all related to a single issue.
CJefferson 9y ago
Some people like a linear history. Personally I don't, because it produces commits which have "never existed", and have certainly never been tested to check they even compile.
rahkiin 9y ago
I have found that rebasing often creates more issues than it is worth it. Only when there are only a couple of commits I might try but you are still rewriting history: something that is destructive and needs force-pushing which I almost always disallow. Merging does create a merge commit which is sometimes annoying (when you are merging a single-line commit, for example). It does however preserve full history.
- u801e 9y ago
  > It does however preserve full history. Frequently, I see a commit history like: Implemented feature Forgot semi-colon Fixed syntax error Addressing review comments Removed unnecessary method History like that, other than the initial commit, is not very useful. It makes more sense to rebase to clean up the commit history such that it becomes a logical series of commits, which can then be viewed via git log or git blame.
- foo101 9y ago
  Why would rebase require force-pushing? You can always rebase the pull request on the most current master. There would be no conflicts then and no need to force-push. If the push fails because the master has changed during the few minutes you spent in rebasing, just rebase again on the most current master and push again. No force-push is required.
u801e 9y ago
> Are there any good reasons why someone should try to rebase and merge (fast-forward) a pull request to avoid a merge commit? I've always thought of it as a way to effectively create a branch, do your work, and fast-foward merge it back in to the main branch without having any other work included in it. To put it another way, let's say you create a fork, do your work, and in the interim, several other contributors do the same thing and merge their work back into the main repo. Now, you could merge the mainline back into your fork and have a merge commit in your private branch that shows the changes that the other contributors made since you created your branch, or you could merge your branch back into the main branch at the end and have it show that it branched off earlier than the current head of the main branch. In the first case, you now have a commit that shows changes that you didn't actually make. In the second case, it shows that you didn't base your work on the latest version of the main branch. In contrast, if you rebase and only merge your work into the main branch via a fast-forward merge, your branch will only show commits that you made and it will effectively show that it was based on the latest version of the main branch before it was merged back in.
tedmiston 9y ago
IMO rebasing is always worth it. Commit history is read more than it's written. For community projects, squashed focused PRs that are rebased are simplest for everyone else besides the author.

asmdev 9y ago

After we merge a pull request with rebase + fast forward merge, there is no merge commit created. This leads to a linear commit history. Does anyone know anyway to look at the linear commit history alone (not GitHub) and tell where a pull request was merged and who merged it?

zoul 9y ago
I think that’s not possible, by definition. There is no merge commit, ergo no information about the merge. You may or may not consider that a feature.
mikepurvis 9y ago
GitLab offers this as a merge option, and I really dislike it for this exact reason. Worse still than not knowing which merge is associated with the commit is that you can't even tell the difference between reviewed commits and those which were simply pushed directly to master. IMO it's far preferable to have an explicit "merge" commit referencing the PR in question, whether that that is a --no-ff merge or a single linear squashed commit.
gonewest 9y ago
Git tracks the committer and the author separately: you should see the “who merged” preserved as the committer and the “who wrote it” as the author. If each commit notes the issue number in its comment (“Add indentation and trailing white space check to CI [#123]”) then you can see easily when each merge occurred by issue number. Additionally in GitHub there are conveniences built from these little conventions. The “#123” is displayed as a hyperlink to the issue, and merging a commit that has “Fix #123” automatically closes the issue. In cases where a feature is big enough to justify multiple commits for clarity, you can mention “#123” in all but the last commit, and “fix #123” in the final commit. Doing so partitions the linear history. In fact, if someone else notices and fixes a regression caused by the merge, they can push additional commits later on but mention “#123” in their commit as well for documentary purposes. Seeing downstream commits “out of sequence” but with some earlier issue number is a clue this was probably a regression.
- 9y ago

zie 9y ago

I just wish they would also support patches. Forcing us to fork and merge back just for GitHub is annoying. Especially for quick drive-by type patches and smaller doc changes and just minor fixes. I git clone'd the project, built it, noticed something small, fix it locally, and then decide OK I'll just send this up real quick. oh crap. NOPE I have to do a bunch of GitHub crap with forking and merging to get it back to the master tree. Annoying. Forking and merging is perfectly fine for regular contributions, but for smaller one-time type fixes it's just annoying.

zoul 9y ago
FWIW, for smaller drive-by fixes you may press the Edit button directly on a file in some else’s repo.
- bastawhiz 9y ago
  And in truth, the "github crap" is clicking "Fork" and running "git remote add". Pushing and filing a PR are the same process.
  - lozenge 9y ago
    Except your fork now prominently appears even after the PR is merged. However, I don't think Github has a way to see all your PRs.
    - shlomi-noach 9y ago
      Here's how to see all your PRs: https://github.com/search?q=is:pr+author:YOURNAME&type=Issues&ref=searchresults https://github.com/search?q=is:pr+author:YOURNAME&type=Issue... Here's how to see all your open PRs: https://github.com/search?q=is:pr+is:open+author:YOURNAME&type=Issues&ref=searchresults https://github.com/search?q=is:pr+is:open+author:YOURNAME&ty... Replace YOURNAME with your GitHub handle, or pick your colleagues names.
      - 9y ago

whatastorymark1 9y ago

Semi-related question: Assuming I drafted up a software license with generally permissive terms (freedom to use, modify, sell, etc.), but included some perverse clause that would forbid forking (such as only allowing the software to be distributed from the canonical repository on my profile), would I have legal grounds to request repository removal from GitHub if someone pressed the repo's fork button?

carreau 9y ago
Yes, when you created a GH account, you read and accepted the TOS, which enforce your project must be forkable.
- whatastorymark1 9y ago
  Interesting, thank you! Relevant excerpt: > If you set your pages and repositories to be viewed publicly, you grant each User of GitHub a nonexclusive, worldwide license to use, display, and perform Your Content through the GitHub Service and to reproduce Your Content solely on GitHub as permitted through GitHub's functionality (for example, through forking). For those interested, GitHub's entire TOS is written in a very approachable way. https://help.github.com/articles/github-terms-of-service/ https://help.github.com/articles/github-terms-of-service/