git amend/force-push

[teamon]

Open discussion:

Scenario:

• There are 3 commits in feature branch A -> B -> C
• Commit B is wrong (bad code, committed comments, etc)

Requirement:

• Fix it!

Possible solutions:

• Add one more commit D that fixes the issues (simple one)
• Rewrite history changing commit B in place (requires more caution and git skills)

Which one should we use?

ping @jandudulski, @sheerun, @teamon, @szajbus, @Ostrzy, @porada, @jcieslar

[porada]

We’re git-skilled. If it’s in a feature branch, amend the commit and notify the rest of contributors.

[sheerun]

I disagree with @porada Add commit, and when merging feature to master, rebase, removing bogus commit.

[jandudulski]

Add another commit D.

Don't cheat the history that you f**ed up something before.

[Ostrzy]

for changing history

[teamon]

One rebasing to master should only do the necessary changes, not changing the whole branch of commits. The person responsible for branch should make it as straightforward as possible.

I am actually more into changing commits and force pushing feature branch. Git history shouldn't be a place for public blame, the less commits with "fix: xxx" the more readable commits tree. (If you did small errors in five commits I'd rather have five force pushed amended commits than five more (which makes it double!) with half of them as fixes)

[porada]

[sheerun]

You're wrong. There is nothing wrong with changing whole branch of commits in one rebase and removing commits, which is equivalent to one forced push (and if you don't want to rewrite every commit in branch, you can rebase to merge base, or onto branch commit). On the other hand repeatedly force pushing code is extremely wrong.

[sheerun]

It's worth mentioning that force pushing commit immediately after it is committed (for example to fix commit message, or remove some debugging code), is OK in our case, as there's little change someone pulls during that 2 minutes. On the other hand, there's huge chance someone pulled and will work on this branch if you force-pushed fixed commit from even 1 hour ago.

[jandudulski]

Agree

[teamon]

What is wrong is the responsibility. As a repo maintainer I do not want to fix others work, feature branch should be ready to merge with no changes required.

[sheerun]

If branch requires rebase before merge, tell maintainer of this branch to rebase it appropriately. You don't need to do it.

[teamon]

So then he has to force push it anyway, so what's the difference?

[sheerun]

Rebase implies force pushing. Rebasing without force pushing (leaving old branch commits on server) is bad smell. We're talking if to rebase right before merge or repeatedly.

[sheerun]

And after such force push to server, branch (label!) is deleted anyway (right after someone merges is immediately to master). Rebased commits are still in repository if someone needs them.

[sheerun]

By "leaving old branch commits on server" I mean leaving pre-rebased branch (label pointing to head of branch commits before rebase) on server for longer period of time.

[teamon]

So your proposal is:

• After receiving rejected review add new commit that fixes previous mistakes
• When feature is done, rebase and force push to make the history clean (not squash into one big commit, just combine broken commits with their fix commits)
• Feel free to force push anything within 60 seconds since previous push :smiling_imp:

Right?

[sheerun]

Yes. If you don't want to rebase all the commits from branch up to current master (I'm not sure why), you can do git rebase -i $(git merge-base HEAD master), but it's IMO not necessary (rebase doesn't change the author's timestamps, only the committer ones). Moreover that way there should be presumably no merge conflicts (in merging commits). The only downside is everyone needs to learn how to rebase..

[sheerun]

That said, feature is ready for merge if:

1. There won't be any conflicts when doing merge to master.
2. There are no "fix" commits in feature branch.

The solution is:

1. Type rebase -i master on feature branch
2. Marking fix commits as fixup, and moving them just above commit they fix.
3. Marking commits with bad messages with reword
4. Resolving merge conflicts, adding files to index, and typing git rebase --continue

[sheerun]

By the way, 2. can be done automatically if fix commits are marked with sha of commit they fix.

[porada]

But it can be tricky if you need 2 or more fix-up commits.

[sheerun]

There is no problem with that. All we need is commit sha they fix and their timestamps. There is problem if fix-up commit fixes more than one commit, but I don't think it's big issue.

[teamon]

git rebase -i + mark with e - this is only sane way to make fixes, fix commit is the worst thing ever, with fix commit it is impossible to rebase at all (zylions od conflicts even if the total diff is okey).