5 ms·

This is a good article that covers how to communicate in a pull request. I think there are two other essentials in making good pull requests. 1. Good commit me

This is a good article that covers how to communicate in a pull request. I think there are two other essentials in making good pull requests. 1. Good commit messages. Follow this guide: https://chris.beams.io/posts/git-commit/ https://chris.beams.io/posts/git-commit/ 2. Following the pull request workflow correctly. Follow this guide: https://github.com/susam/gitpr https://github.com/susam/gitpr Writing good and consistent commit messages make the commit log easy to read and search. Pull request workflow is equally important. It is kind of a rules of engagement that co-developers follow to keep working on their stuff concurrently. It keeps unnecessary merges to minimum and keeps the commit history clean. By following these two things, you are not only going to be nice to your co-developers but you are going to be nice to your managers and release departments too who could look at your commit log and figure out what bugs were fixed and what features were released.

chenhan 7y ago
I will elaborate why I think the two points in my comment above are important. Git Rebase, Git bisect and other operations display the commit summary line when they get stuck with merge conflicts or find an issue, so I find good commit summary lines very helpful during those operations. Without good commit messages, resolving issues during those operations can get confusing. This is one of the reasons why writing good commit messages are important. I see many developers working on their PR branch and constantly merging new commits from master into the PR branch as the master keeps moving ahead. It creates a big mess of merges in both directions-- (A) from master to PR branch during development and (B) later again from PR branch to master when the PR gets merged. The first set of merges from master to PR branch are totally unnecessary. It adds nothing meaningful to the commit history. They are just extra commits to scroll through while looking at git log. Every time the master branch moves ahead, just rebase your PR branch on master. The commit history remains clean and minimal. A good PR workflow teaches you that. Having said that, merge commits from PR branch to master are totally fine. They do add something meaningful. They show the point at which a PR was merged into the main project.
- dimes 7y ago
  If you share a development branch with someone, then you should prefer merging over rebasing because rebasing changes rewrites the commit history, causing the locally checked out branches among collaborators to disagree. You never know when someone will need to take your branch to develop on, so generally it’s a good idea to merge over rebase. When master is merged into your PR branch, the fork point is forwarded to the last master commit, so you shouldn’t have any trouble merging back into master.
  - pm90 7y ago
    I think the OPs concern was more with the commit history in the PR being polluted with merge commits. I do agree that merge commits can obfuscate history somewhat. However, I agree with dimes that its better to not rewrite history just to keep the commit history clean... the cleanliness is not worth the price for inability to collaborate effectively. Also, any professional code review tool will not let merge commits affect your review. Highly recommend reviewable.io for this.
  - polyphonicist 7y ago
    > If you share a development branch with someone, then you should prefer merging over rebasing Your parent comment is suggesting rebase only for pulling latest changes in master into your pull request. For merging someone's pull request to the team's master, sure use a merge commit. But if you are working on a pull request and while you are working on it, the team's master gets updated and now you want to base your work on the recent master, by all means, use git rebase. That is what it is meant for, to rebase your work on another work. It's in the name itself. Right tool for the right job.
    - dimes 7y ago
      Using a rebase to update from master will cause conflicts for anyone else working on the PR branch. To understand why, imagine a developer branches from master at commit A, and then creates two commits on the branch B and C. In the meantime, master gets updated with commits D and E. Rebasing in this situation, results in a branch that looks like A, D, E, F, G, where F and G contain the same content as B and C, but are now tracked under different hashes. If a collaborator has this branch, git will report the local branch has two different commits (B and C) and that the remote branch has two different commits (F and G). Doing a typical “git pull” in this situation will lead to merge conflicts in everything touched by commits B and C. You can work around this specific problem by using git pull —rebase. However, git works best when the remote history is immutable. If you have a specific commit in your PR that you want someone to look at, you send them a link to it using the commit hash. Once you rebase, that commit hash will no longer point to anything. Rebasing is useful for completely changing the branch your PR branch is cut from. But once you rebase from one base branch to another, you should then continue to merge from the new base branch.

chenhan 7y ago

I will elaborate why I think the two points in my comment above are important. Git Rebase, Git bisect and other operations display the commit summary line when they get stuck with merge conflicts or find an issue, so I find good commit summary lines very helpful during those operations. Without good commit messages, resolving issues during those operations can get confusing. This is one of the reasons why writing good commit messages are important. I see many developers working on their PR branch and constantly merging new commits from master into the PR branch as the master keeps moving ahead. It creates a big mess of merges in both directions-- (A) from master to PR branch during development and (B) later again from PR branch to master when the PR gets merged. The first set of merges from master to PR branch are totally unnecessary. It adds nothing meaningful to the commit history. They are just extra commits to scroll through while looking at git log. Every time the master branch moves ahead, just rebase your PR branch on master. The commit history remains clean and minimal. A good PR workflow teaches you that. Having said that, merge commits from PR branch to master are totally fine. They do add something meaningful. They show the point at which a PR was merged into the main project.

dimes 7y ago
If you share a development branch with someone, then you should prefer merging over rebasing because rebasing changes rewrites the commit history, causing the locally checked out branches among collaborators to disagree. You never know when someone will need to take your branch to develop on, so generally it’s a good idea to merge over rebase. When master is merged into your PR branch, the fork point is forwarded to the last master commit, so you shouldn’t have any trouble merging back into master.
- pm90 7y ago
  I think the OPs concern was more with the commit history in the PR being polluted with merge commits. I do agree that merge commits can obfuscate history somewhat. However, I agree with dimes that its better to not rewrite history just to keep the commit history clean... the cleanliness is not worth the price for inability to collaborate effectively. Also, any professional code review tool will not let merge commits affect your review. Highly recommend reviewable.io for this.
- polyphonicist 7y ago
  > If you share a development branch with someone, then you should prefer merging over rebasing Your parent comment is suggesting rebase only for pulling latest changes in master into your pull request. For merging someone's pull request to the team's master, sure use a merge commit. But if you are working on a pull request and while you are working on it, the team's master gets updated and now you want to base your work on the recent master, by all means, use git rebase. That is what it is meant for, to rebase your work on another work. It's in the name itself. Right tool for the right job.
  - dimes 7y ago
    Using a rebase to update from master will cause conflicts for anyone else working on the PR branch. To understand why, imagine a developer branches from master at commit A, and then creates two commits on the branch B and C. In the meantime, master gets updated with commits D and E. Rebasing in this situation, results in a branch that looks like A, D, E, F, G, where F and G contain the same content as B and C, but are now tracked under different hashes. If a collaborator has this branch, git will report the local branch has two different commits (B and C) and that the remote branch has two different commits (F and G). Doing a typical “git pull” in this situation will lead to merge conflicts in everything touched by commits B and C. You can work around this specific problem by using git pull —rebase. However, git works best when the remote history is immutable. If you have a specific commit in your PR that you want someone to look at, you send them a link to it using the commit hash. Once you rebase, that commit hash will no longer point to anything. Rebasing is useful for completely changing the branch your PR branch is cut from. But once you rebase from one base branch to another, you should then continue to merge from the new base branch.