Fixing a bad pull request

Question

I branched a friend's master repo, made some commits, then made a pull request.

There were a number of problems with my request:

One, the commit style was incorrect.

Two, some of the code was bad.

Three, apparently I need to do a rebase the pull request instead of a merge.

I have fixed all of the code, but I'm scared to commit it to my branch; do I need to force push it or something? Further, how do I fix the other issues?

The PR in question: https://github.com/jleclanche/fireplace/pull/344

daniel.bak@STPC039 ~/documents/github/fireplace (master)
$ git rebase jleclanche/master
First, rewinding head to replay your work on top of it...
Applying: Corrupted Healbot + test
Applying: Implemented Bilefine Tidehunter & Zealous Initiat
Applying: Fixing Zealous Initiate name
Applying: Corrupted Seer
Applying: Implement Shadow Strike

daniel.bak@STPC039 ~/documents/github/fireplace (master)
$ git rebase -i jleclance/master
fatal: Needed a single revision
invalid upstream jleclance/master

daniel.bak@STPC039 ~/documents/github/fireplace (master)
$ git rebase -i jleclance/master
fatal: Needed a single revision
invalid upstream jleclance/master

daniel.bak@STPC039 ~/documents/github/fireplace (master)

more:

daniel.bak@STPC039 ~/documents/github/fireplace (master)
$ git log jleclanche/master
commit 644c375b065c10edd02b8ba2450842404971919a
Author: Jerome Leclanche <[email protected]>
Date:   Fri Apr 29 14:26:39 2016 +0300

    Implement Silithid Swarmer, with tests

commit 1f33e4888cdbdec159e932ab4259b9e4f015adc8
Author: Jerome Leclanche <[email protected]>
Date:   Fri Apr 29 13:20:57 2016 +0300

    Add a NUM_ATTACKS_THIS_TURN attribute selector

commit 13b58c2ee4249c4195416d257213cf1c4e0a276c
daniel.bak@STPC039 ~/documents/github/fireplace (master)
:

    Implement buff-based enrage from 12051

commit 72b95d2bff4c1af5b2be0331ce6c8b2266298d52
Author: Jerome Leclanche <[email protected]>
Date:   Tue Apr 26 06:53:49 2016 +0300

    Add tests for Dalaran Aspirant

Answer 1

Unless the project has some extra requirements, and a lot of them do, if you need to fix a coding mistake in your PR all you should need to do is commit and push your changes to your existing branch. They should appear as updates in the pull request.

Here's an example of a pull request that needed some fixups. You can see my inline commentary on the initial pull request (flattened into the pull request) and the original submitter update commits.

Your case is different. They have two issues with your commit and they both are Git style nit-picks.

The first is they don't want to see any merges in your PR. Your history looks something like this:

1 - 2 - 3 - 4 - 5 - 6 [jleclanche/master]
         \           \
          A - B - C - D - E - F [feature/wog_cards]

Having merges in the middle of branches can make it difficult to understand the history in large projects with a lot of contributions. They want you to redo the entire branch as if it was all done on top of the latest commit.

1 - 2 - 3 - 4 - 5 - 6 [jleclanche/master]
                     \
                      A - B - C - D - E - F [feature/wog_cards]

Fortunately, Git can do this for you. First, make sure the jleclanche remote is up to date with a fetch. Then checkout feature/wog_cards (or whatever your branch is called) and run git rebase jleclanche/master.

• git fetch jleclanche
• git checkout feature/wog_cards
• git rebase jleclanche/master

This will repatch each change in your branch on top of jleclanche/master. There may be conflicts, resolve them as normal and follow the instructions git gives you (ie. use git rebase --continue).

Once that's done, you can also use rebase to fix your commit messages to follow their style guide. This time it's an interactive rebase. This lets you edit your commits. From your branch run git rebase -i jleclanche/master. It will bring up an editor with instructions. You want to use the "reword" option on each commit. Git will present you with each commit message in turn and you should edit them to follow their style guide.

Here's some more information about rewriting history.

Finally you need to push these changes. Because you've changed history your changes cannot go cleanly on top of your existing commits. So you have to force push. In this case it's fine, the only repository you can mess up is your own. git push --force origin. Then your changes should show up in the PR.

As you can see, this involves some fairly advanced knowledge of Git. IMO custodial fixes to contributions by first time contributors should be taken care of by the project.