1

I'm in the following scenario:

  1. I commit some patches to a dev branch
  2. I push the branch on stash or github or bitbucket
  3. I open a merge request from dev to master
  4. I push some new patches that fix issues reported by reviewers

It turns out some of the patches can be squashed with the previous ones, in order to increase clarity and consistency.

Is it OK to do so (and then force push to the remote dev branch)? Is there some situations where this could break something in the merge-request-experience?

EDIT

Thanks for your answers.

I'm in fact the owner of a project's repository, not an actual user creating a merge requests. I completely understand git's merging and rebasing mechanisms so no need to explain git's internals.

The question is more: as a reviewer, would you ask a submitter to squash some of its patches when it makes sense? In your experience, does it disable or break some opportunities that offer pull requests.

Antoine
  • 1,782
  • 1
  • 14
  • 32

3 Answers3

0

Once commits have been pushed, rewriting history (which is really creating new history) is going to cause problems. After you've pushed your changes, your repository looks like this.

A - B - C - D - E - F [origin/master] [master]
             \
              1 - 2 - 3 - 4 - 5 - 6 - 7 - 8 [dev] [origin/dev]

Let's say you decide 6 can be squashed into 3 and 7 can be squashed into 4. You do a rebase and wind up with this...

A - B - C - D - E - F [origin/master] [master]
             \
              1 - 2 - 3 - 4 - 5 - 6 - 7 - 8 [origin/dev]
                   \
                    3b - 4b - 5b - 8b [dev]

Even though 5 and 8 were not directly changed, they must be rewritten because their parents changed and they wind up with new IDs. Your dev branch has diverged with the one in the remote. If you push you have to force it and blow away 3, 4, 5, 6, 7 and 8. This causes problems for everyone else.

Anyone who has pulled your dev branch will now get an error when they try to push or pull. They have to pull --force to update. If they've done work on top of your branch it gets complicated.

Anything which refers to commits 3 - 8 (reviews, comments, log entries, etc...) will now be pointing to the wrong place.

However, it should merge fine.

Some pull request systems deal with this better than others. Last I looked, Github will present your rebased commits as new commits and it can get a bit confusing. Because of this, I would suggest you ask the project if they want you to clean up your branch or just leave it as is.

Schwern
  • 153,029
  • 25
  • 195
  • 336
0

  1. Reviewers generally like to look at small differences between commits which you make based on their feedback. If you squash commits, then they might get confused as they have to review many changes now.

  2. On the other hand if you do not squash commits, there would be times where you might need to re-base your branch on to of some other branch. And since you do not have squashed commit, you could potentially see merge conflicts for every commit which is painful.

Kiran
  • 56,921
  • 15
  • 176
  • 161
  • If you squash commits, you will still get merge conflicts, but they will be worse because Git won't know that the squashed patch is part of both branches! – Dietrich Epp Feb 03 '15 at 01:42
  • @DietrichEpp: I was not suggesting that by squashing one would not see merge conflicts,. Example: if you have 10 commits in your PR which modify a file `A`,then when you rebase there is a possibility of all those 10 commits generating merge conflicts the same file 'A', but if you had squashed, you would have to resolve the conflict only once. Also the OP is talking about squashing commits in his PR's branch, right?...if yes, this should not affect other users using the `dev` branch?...not sure if am missing something.. – Kiran Feb 03 '15 at 03:59
  • I think something has gone gravely wrong if you are rebasing a published pull request. – Dietrich Epp Feb 03 '15 at 05:34
  • @DietrichEpp: hmm, can you be more specific?...PR sign-off process could take days before it gets merged in and meanwhile the other branch could have had commits from others, which could have modified the same files you are working in your PR...so ideally you want to rebase on other's changes and possibly make changes in your PR and update it... – Kiran Feb 03 '15 at 05:54
  • When you make a pull request, it's a published branch. Don't rebase published branches. Pull requests should be merged, not rebased. Rebasing a pull request destroys the history from the pull request, and will cause merge conflicts if anyone else is using code which includes the pull request. – Dietrich Epp Feb 03 '15 at 06:32
  • @DietrichEpp: hmm, a PR branch even though published is still a feature specific branch which is going to be short lived only for the duration of the PR and would get merged later onto a branch like `dev` for example and deleted...so wondering why one cannot rebase on a PR branch...I would not expect users to work on my PR's feature specific branch...right? – Kiran Feb 03 '15 at 06:43
  • The reviewers will use it. Sometimes a pull request may get merged into a fork. Rebasing the pull request invalidates testing done on the branch. I think a lot of people have gotten careless with rebasing and gotten bitten by it in the end, I would rather be able to see the true, accurate history of the pull request instead of destroying it and creating a fake history. – Dietrich Epp Feb 03 '15 at 06:56
  • Or, to put it another way: a rebase is really just a special kind of merge that erases the original commit from history. In general, it's preferable to use a plain old regular merge, which preserves the existing history. See a 2009 post from Linus on dri-devel: http://www.mail-archive.com/dri-devel@lists.sourceforge.net/msg39091.html – Dietrich Epp Feb 03 '15 at 07:06
  • Sorry but I don't find your reasoning very convincing. – Kiran Feb 03 '15 at 07:37
0

No, absolutely not1.

As soon as you have pushed changes for other people to review, you have published your history. And you rarely want to edit published history. You should only squash and rebase your own personal commits that you haven't shared with anyone.

I can understand the desire to show a "clean, perfect" history. It would be nice to push a pristine history out to the world with no errors in any of your commits. But it's too late! You made some mistakes, and some other developers found them. So you want to hide your mistakes by rewriting history. In this world, your history can be clean, or it can be accurate, but it cannot be both at the same time.

Yes, it is very tempting to rewrite history because your are embarrassed about it.

If you don't squash,

  • The reviewers can see in the history exactly how you've addressed their concerns.

  • The history can be used later as a reference, if someone raises similar concerns with other pieces of code. History is a valuable tool for learning about code!

  • The reviewers don't have to review your whole pull request from scratch, because they've already reviewed an earlier version.

  • You're trustworthy! You're not hiding things.

  • If someone has pulled to review your changes, they don't get problems next time they pull.

If you do squash,

  • You get to feel like you're the kind of perfect developer who doesn't ever make a mistake. What an ego rush!

No squashing published history!

1: In some cases, if you made some accidental changes to formatting / whitespace, or accidentally committed build products, or something that should never have been in the history in the first place, go ahead and rewrite history (so you don't break git blame). So don't take the word "absolutely" at face value.

Dietrich Epp
  • 205,541
  • 37
  • 345
  • 415