394

I've submitted a change to an Open Source project on Github, and received code review comments from one of the core team members.

I would like to update the code taking into account the review comments, and re-submit it. What is the best workflow for doing this? From my limited knowledge of git/github, I could do any of the following:

  1. Update the code as a new commit, and add both the initial and updated commit to my pull request.

  2. Somehow (??) rollback the old commit from my repository, and create a single new commit containing everything, then raise a pull request for that?

  3. git commit has an amend feature, but I've heard that you shouldn't use it after you've pushed the commit outside of your local repository? In this case I have made the change on my local PC and pushed to my github branch of the project. Would this be OK to use 'amend'?

  4. Something else?

It seems like option 2/3 would be nice, as the open source project would only have one commit in their history which would implement everything, but I'm not sure how to do this.

Note: I don't know if this affects the answer or not, but I didn't make the changes in a separate branch, I just did a commit on top of master

dimo414
  • 47,227
  • 18
  • 148
  • 244
Orion Edwards
  • 121,657
  • 64
  • 239
  • 328

2 Answers2

273

To update a pull request

To update a pull request (point #1), the only thing you need to do is checkout the same branch the pull request is from and push to it again:

cd /my/fork
git checkout master
...
git commit -va -m "Correcting for PR comments"
git push

Optional - Cleaning commit history

You may be asked to squash your commits together so that the repository history is clean, or yourself want to remove intermediary commits which distract from "the message" in your pull request (point #2). For example if your commit history looks like this:

$ git remote add parent git@github.com:other-user/project.git
$ git fetch parent
$ git log --oneline parent/master..master
e4e32b8 add test case as per PR comments
eccaa56 code standard fixes as per PR comments
fb30112 correct typos and fatal error
58ae094 fixing problem

It's a good idea to squash things together so they appear as a single commit:

$ git rebase -i parent/master 

This will prompt you to choose how to rewrite the history of your pull request, the following will be in your editor:

pick 58ae094 fixing actual problem
pick fb30112 correct typos
pick eccaa56 code standard fixes
pick e4e32b8 add test case as per PR comments

For any commit you want to be part of the previous commit - change pick to squash:

pick 58ae094 fixing actual problem
squash fb30112 correct typos
squash eccaa56 code standard fixes
squash e4e32b8 add test case as per PR comments

And close your editor. Git will then rewrite the history and prompt you to provide a commit message for the one combined commit. Amend accordingly and your commit history will now be concise:

$ git log --oneline parent/master..master
9de3202 fixing actual problem

Push that to your fork:

$ git push -f
Counting objects: 19, done.
Delta compression using up to 4 threads.
Compressing objects: 100% (5/5), done.
Writing objects: 100% (11/11), 978 bytes, done.
Total 11 (delta 9), reused 7 (delta 6)
To git@github.com:me/my-fork.git
   f1238d0..9de3202  HEAD -> master

and your pull request will contain a single commit, incorporating all changes previously split into several commits.

Changing history on public repos is a bad thing

Rewriting history and using git push -f on a branch that, potentially, someone else has already cloned is a bad thing - it causes the repository's history and that of the checkout to diverge.

However, amending the history of your fork to correct the change you are proposing to be integrated into a repository - is a good thing. As such have no reservations squashing "noise" out of your pull requests.

A note on branches

In the above I show the pull request as having come from the master branch of your fork, there's nothing wrong with that necessarily but it does create certain limitations such as, if this is your standard technique, only being able to have one PR open per repository. It's a better idea though to create a branch for each individual change you wish to propose:

$ git branch feature/new-widgets
$ git checkout feature/new-widgets
...
Hack hack hack
...
$ git push
# Now create PR from feature/new-widgets
Maxim
  • 7,207
  • 1
  • 30
  • 29
AD7six
  • 63,116
  • 12
  • 91
  • 123
  • 32
    +1 for mentioning how to clean up the commits rather than pushing additional fixup commits. – mgalgs Sep 14 '13 at 03:50
  • 4
    I ran into some problems picking/squashing and [this answer](http://stackoverflow.com/a/2568581) helped me out. Also noticed that Github removed the previous conversation after I did `git push -f`. There wasn't a lot of comments, but that's something I wasn't expecting. – Hitesh Dec 04 '13 at 19:31
  • 6
    Just to be clear, when rebasing to have a clean history, you are indeed changing your public commits, you are just assuming that no-one cares because it is a fork. – brita_ Feb 07 '15 at 11:02
  • 2
    Follow-up: best practice when master changes during your PR? – Kevin Suttle Jun 04 '15 at 16:09
  • @KevinSuttle either `git fetch; git rebase origin/master; git push -f` or `git merge origin/master; git push`. I prefer to rebase, but opinions vary. – AD7six Jun 04 '15 at 16:36
  • You're ok with using `git push -f (--force)`? – Kevin Suttle Jun 05 '15 at 02:41
  • @mikew Weird. Thanks for the heads-up. SO won't let me edit the comment, so I've deleted it. Here's [my blog post on this subject](http://blog.adamspiers.org/2015/03/24/why-and-how-to-correctly-amend-github-pull-requests/). – Adam Spiers Oct 23 '15 at 01:37
  • excellent answer, much better than the other 3-4 explanations I found. Although you mention the `-f` flag, and it's the right thing to do here, I think it'd be worthwhile adding a bit more detail. Force pushing can potentially be a pretty disastrous thing. – craq Jun 27 '18 at 05:43
  • 4
    Consider that rewriting history on pull requests that were reviewed (or that in general have commenting about/referring code) could lead to confusion, since the history won't match anymore what comments were referring to. There is no easy solution: somebody will close the PR and refer it in a new one (to not rewrite history); my idea'd be to just backup the latest commit SHA that is being reset/rewritten and refer it in a comment on the PR, after the forced-push has been performed. **IF** `prune` doesn't remove that detached commit then its history will still be matching PR's comments. – Kamafeather Jul 24 '18 at 09:25
  • 1
    another idea to prevent the pruning could be to `git tag PR-NUMBER-draft-1` on the last commit, just *before* `git push --force`, so that Github is told to *not* prune the previous PR history, and **theoretically** let it still available and reachable by the references in the PR comments. – Kamafeather Apr 28 '20 at 08:27
  • @Kamafeather please write an answer if you disagree with the advice given in this answer :) – AD7six Apr 28 '20 at 14:43
244

Just add a new commit to the branch used in the pull request and push the branch to GitHub. The pull request will automatically be updated with the additional commit.

#2 and #3 are unnecessary. If people want to see only where your branch was merged in (and not the additional commits), they can use git log --first-parent to only view the merge commit in the log.

Amber
  • 507,862
  • 82
  • 626
  • 550
  • 7
    `master` is a branch too, so technically it does not matter :) – poke Oct 30 '11 at 22:16
  • 13
    @OrionEdwards - as poke mentioned, master is a branch, thus, updating it will cause any pull requests based on it to be updated as well. (This is a good reason to use a separate branch for anything you plan to submit a pull request for.) – Amber Oct 30 '11 at 22:34
  • 2
    @Amber - Ahh that explains the behaviour of the last 2 pull requests I made (they got rolled into a single request and I couldn't figure out why), because they were both on master – Orion Edwards Oct 31 '11 at 03:25
  • 24
    Since the code is still in *review* it's usually better to fix the commit(s) rather than introducing additional fixup commits that just clutter the history... – mgalgs Sep 14 '13 at 03:49
  • 4
    @mgalgs That's a matter of preference. – Amber Sep 17 '13 at 15:00
  • 2
    imho doing an extra commit is helpful to see the changes between previous and current patch, fixing commit is simpler, I agree with Amber it is a matter of preference. Unrelated comment: Gerrit review allows you to see changes between patches, so you can always amend the commit and push when fixing a patch with suggestions from reviewers and still be able to see the diff between 2 patch versions. – Alan Evangelista Sep 13 '14 at 00:15
  • 5
    I don't like this answer for reasons explained in [the blog post I just wrote](http://blog.adamspiers.org/2015/03/24/why-and-how-to-correctly-amend-github-pull-requests/); I believe [the other answer](http://stackoverflow.com/a/15055649/179332) is much better. – Adam Spiers Mar 24 '15 at 14:12
  • For anyone else who was looking for how to do this on BitBucket, it's exactly the same process. Just push to the pull request branch, and the pull request gets updated. – Michael Hoffmann Feb 23 '18 at 06:50
  • I have such a policy usually to have only working commits in a branch, so if something doesn't work, just adding commit with a fix is not something i'd like to have, would be nice to squash somehow a fix with that commit which is definitely wouldn't be a last one in the row. how do you solve that? – holms Feb 28 '18 at 14:53