9 ms·

This is a bad idea masquerading as a good idea. Before making a pull request (or doing any sort of merge), you should rebase against upstream master (or whateve

This is a bad idea masquerading as a good idea. Before making a pull request (or doing any sort of merge), you should rebase against upstream master (or whatever you're going to push to). However, keeping distinct atomic commits that change one and only one small thing, when possible, is much preferable if bisect or blame is used. If you have broken or poorly written commits, use fixup, reword, squash, etc. in rebase -i. Using fast-forward (and possibly only allowing fast-forward) is a good idea. Squashing entire pull requests that may change multiple things into a single commit is a very bad idea.

michaelmior 10y ago
I used to feel the same way (and still do to some degree). However I think the issue is more nuanced. I agree that rebasing beforehand is a good idea. But I can see the value in keeping commits on the master branch corresponding to specific features or bug fixes (which presumably map to PRs). I think the argument can be made that if you don't feel comfortable performing a squashed merge of a PR, then that PR contains too much work and should be split up. However, I don't think there's an easy rule to decide in either case.
- cballard 10y ago
  Small PRs are an issue because PRs are dependent on other users and can't be dependent on a prior PR. Let's say we're adding an interface/typeclass/protocol and a concrete implementation. I'd say these should be two separate commits, as they're adding two different things. An interface doesn't require a provided implementation to work. But, if we were to create those as two separate pull requests, it would be more work for the project maintainers, and the initiator wouldn't be able to create the PR for the concrete implementation until the interface PR was merged - the concrete PR can't be added as a dependent PR of the interface one, or something to that effect. Since you can "compare" almost anything on Github, small commits aren't really an issue, just view a larger-scope comparison to get an idea of the whole PR. Another way to put this might be that commits are for individual code changes that build up to a pull request, which is a conceptual change?
  - simplify 10y ago
    How does not squashing your commits help the protocol/implementation scenario you described?
  - knicholes 10y ago
    You can merge the interface PR into the concrete implementation's PR. You don't have to work off of just one remote branch.
  - lomnakkus 10y ago
    > and can't be dependent on a prior PR. This pinpoints the major problem exactly. Without dependencies between PRs there's really no sane way (with this feature enabled) to submit a series of commits while expecting those commits to remain separate. Oh, and I object to the general sentiment in the responses to your post that seem to value drive-by/inexperienced contributors over the "experts". Yes, we definitely should make things as easy/simple as possible for new contributors, but NOT at the expense of adding a gotcha for expert contributors. The experts are what keep a project going over many years instead of just releasing version upon version of trivial spelling fixes. (And, btw, the default "merge" option for GitHub PRs also sucks. It should be possible to simply disallow non-FF merges and to force all merges to be FF. EDIT: Interestingly, this seems to be about the only workflow explicitly forbidden by the new rules... unless, of course, you're willing to merge everything manually using your local copy of the repo and pushing from that to GH.)
    - yxhuvud 10y ago
      Yeah, it is also the obvious thing that is missing in the review stage of a pull request - viewing all the content and all the diffs separately but on one page, in a serial way that corresponds to the actual order they will show up when you do git log, all on the same page.
    - JoshTriplett 10y ago
      This is something that Gerrit supports natively: you can have a Gerrit CL that depends on another CL. It's unfortunate that Github doesn't support any equivalent.
3JPLW 10y ago
Sure, it's a terrible feature to always use. And it's likely to be of little use to contributors who know how to use Git well. But in large open-source projects, often new contributors make a small change that needs a few minor corrections. Eliminating that final back-and-forth ("squash please") is a huge win for maintainers.
- Bahamut 10y ago
  Not only maintainers - anyone tracing back through the history to find out what broke their use case.
haberman 10y ago
There's no guarantee that every individual commit of a feature branch is meaningful, or even builds. It also makes the history of the master branch a lot harder to read when it has tons of commits representing the minutiae of the feature's development.
- jakub_g 10y ago
  It really depends on each individual's workflow. I tend to use lots of "in progress" commits (each time things are "green"), and as I go, I regularly squash the commits, so in the final pull request I typically have several commits (and if I wasn't squashing it would have been a dozen). If I do feature and a refactor, they are always separate commits, it's easier to review these and bisect if something turns out to go wrong. Some people might do similar things but they might not assure each commit is green, and they never squash anything (so you end up with non-meaningful commits). As @3JPLW said, I see when it can be useful for opensource maintainers to have the option to squash someone's commits, when the change is small, but there are many commits (due to a review ping-pong etc)
- echion 10y ago
  There's no guarantee, but there are many benefits to striving for this ("git bisect run", CI test results).
- yxhuvud 10y ago
  If it isn't meaningful, then that is something the review stage should catch.
natrius 10y ago
I used to strongly believe what you do until my company started using Phabricator, which forces the squash workflow on you. It makes your history more useful, not less. The pull request is the appropriate unit of change for software. Make small commits as you develop, then squash them down into a single meaningful change to the behavior of your software.
- dsmithatx 10y ago
  As a git novice I wonder, doesn't a proper workflow do the same thing? When I submit a feature branch it might have a lot of ugly commits. However, once I merge it to an integration branch there is one nice commit explaining what I did. When coworkers create Pull requests I don't go through all of their commits and changes along the way. I just look at the diff so, I don't see the need for them to squash it first.
  - hnrodey 10y ago
    Once merged, the history of your feature branch becomes part of the history of the integration branch. Sounds like you're using GitHub (Enterprise) or something similar where the pull request view shows you all of the changes in a "squashed" fashion.
    - dsmithatx 10y ago
      Yes Bitbucket so, maybe that explains it.
- glhaynes 10y ago
  It seems like it'd be nice to have two levels of granularity exposed in views of a source control system's history, basically corresponding to pull requests and commits. So you could drill-down to individual commits as needed, but would normally be able to work at the PR level.
  - prodigal_erik 10y ago
    Does "git log --merges" get us there?
falsedan 10y ago
I use this workflow: 1. branch off master 2. work, commit, push, test (on CI server) 3. decide it's time to ship 4. rebase -i, push, test (again) 5. git checkout master && git merge --no-ff feature_branch (make the merge commit message a summary of the feature) master ends up being a list of feature branch commits, bookended by the merge commit which introduced the feature. Getting the squash commit diff is as easy as 'git diff feature_branch_merge^..feature_branch_merge'.
BinaryIdiot 10y ago
> Before making a pull request (or doing any sort of merge), you should rebase against upstream master (or whatever you're going to push to) See, and maybe this is because I'm just dumb or something, but I have never gotten rebasing to work for me. Ever. Every single time I do it I read at east 3 articles about it so I don't screw something up, I attempt to do it and ultimately I lose a bunch of work. I just don't get it. I can write web, mobile and desktop apps and I like to think I'm pretty good at it. But I'm one of those people who constantly have commits of merges in their code because for whatever reason I just can't get my head around making rebasing work correctly. Am I the only one? Sorry for the derail but it's bothering me that I've never gotten this to work correctly and I feel otherwise normally smart. ¯\_(ツ)_/¯
- tunesmith 10y ago
  I think the advice to rebase runs up against the business pattern of pushing your branch as soon as you create it (git-flow and a lot of jira/stash integrations work like this). Also some teams want to see evidence of your commits as you make them, which means pushing as you commit. If you have a branch and it's already pushed, rebasing just feels kind of funny and can sometimes cause a lot of problems if anyone else has checked it out. If you have a branch and it's local only, then merging from mainline into your branch and selecting rebase instead of merge is relatively painless.
- joshuahutt 10y ago
  $ git checkout master $ git pull $ git checkout branch-name $ git rebase master If there are merge conflicts, open the affected file(s) and resolve them. Then: $ git add filename.ext $ git rebase --continue Finally: $ git push origin branch-name If you've already pushed the branch, use -f. Make sure to always specify the branch name when using that flag!
  - mcguire 10y ago
    For those cases where you have created a fork of a project and are preparing a pull request, would that be something like: $ git checkout master $ git fetch upstream # https://help.github.com/articles/syncing-a-fork/ $ # git merge upstream/master # <- leaves merge commits in your fork $ git checkout branch-name $ git rebase upstream/master # Use rebase instead of merge? $ git push -f origin branch-name
golergka 10y ago
Why would you want to rewrite your whole work history and change the actual state of the repository at each of your commits? Why don't just merge?
JoshTriplett 10y ago
If someone prepares a pull request with a well-structured series of commits, making a logical series of changes, where the project builds and passes tests after each commit, then those commits shouldn't get squashed. However, I frequently see people adding more commits on top of a pull request to fix typos, or do incremental development, where only the final result builds and passes, but not the intermediate stages, and where the changes are scattered among the commits with no logical grouping. In that case, I'd rather see them squashed and merged than merged in their existing form, and having a button to do that makes it more likely to happen.
- andersonvom 10y ago
  Plain squashing commits, while still a valid option in very few cases, will likely lead to gigantic commits that are hard to reason about. I've seen projects where maintainers clean up poor commits before merging them: rebase/squash/reword only what's appropriate.
- nhaehnle 10y ago
  The trick is to not squash everything into one giant commit, but to use rebase -i liberally to squash/fixup those typo fix commits where they belong.
  - JoshTriplett 10y ago
    That's what the author of the pull request should do. But this provides a potentially useful alternative when that doesn't happen.
- Tyr42 10y ago
  It's also the case that you lose the code review if you force push to a PR's branch after adding in a typo fix and squashing locally, right? That's a pretty good reason not to squash till the review is done.
  - JoshTriplett 10y ago
    > It's also the case that you lose the code review if you force push to a PR's branch after adding in a typo fix and squashing locally, right? Not as far as I can tell; I've force-pushed pull request branches many times, and the code reviews seem to stick around. (Perhaps they wouldn't if the code changed more drastically, like files disappearing; I haven't tried that.)
phasmantistes 10y ago
Squashing entire pull requests that change multiple things into a single commit is a bad idea, yes. But uploading and asking for review on such wide-reaching pull requests is a bad idea in the first place. Using fast-forward without squash is also a bad idea in many cases: the string of commits may contain multiple points that don't actually build or pass tests, even if the final commit in the chain fixes all that. There's no point in landing those broken commits, and doing so will confuse bisection tools. Fast-forward with squash, and enforcing reasonably sized code reviews as a matter of culture, is the best of all worlds in my opinion.
jgraham 10y ago
It's a bad idea because it's a bad implementation. If it allowed you to select what to squash, defaulting to the behaviour of git rebase -i --autosquash master then it would be a clearly good feature.
draw_down 10y ago
Rebasing seems to clutter the Github PR's commit history and diff with all the commits to master that were made between the time the branch was cut and the time the rebase happens. But it doesn't do that if you merge in master. I never understood this.
zb 10y ago
I think that GitHub's pull-request-based model is fundamentally broken. Gerrit's model, where every commit is quasi-independent (and hence must pass tests) and you can easily edit without force-pushing anything or losing review history, is superior (though not perfect) in almost all cases. (Exception: merging a long-running feature branch where all the commits in the branch have already been reviewed.) This is GitHub's attempt to solve the problem without really changing anything. It won't really change anything. Since pull requests routinely contain a mixture of both changes that should be squashed (fixups) and changes that should not be squashed (independent changes), this just means that you get to pick your poison.
serge2k 10y ago
> Squashing entire pull requests that may change multiple things into a single commit is a very bad idea. If changes are too large/complex/disjoint to fix in a single commit then why have them in one PR?
jibsen 10y ago
I wonder why they did not add `--ff-only` as an option, like GitLab has.

michaelmior 10y ago

I used to feel the same way (and still do to some degree). However I think the issue is more nuanced. I agree that rebasing beforehand is a good idea. But I can see the value in keeping commits on the master branch corresponding to specific features or bug fixes (which presumably map to PRs). I think the argument can be made that if you don't feel comfortable performing a squashed merge of a PR, then that PR contains too much work and should be split up. However, I don't think there's an easy rule to decide in either case.

cballard 10y ago
Small PRs are an issue because PRs are dependent on other users and can't be dependent on a prior PR. Let's say we're adding an interface/typeclass/protocol and a concrete implementation. I'd say these should be two separate commits, as they're adding two different things. An interface doesn't require a provided implementation to work. But, if we were to create those as two separate pull requests, it would be more work for the project maintainers, and the initiator wouldn't be able to create the PR for the concrete implementation until the interface PR was merged - the concrete PR can't be added as a dependent PR of the interface one, or something to that effect. Since you can "compare" almost anything on Github, small commits aren't really an issue, just view a larger-scope comparison to get an idea of the whole PR. Another way to put this might be that commits are for individual code changes that build up to a pull request, which is a conceptual change?
- simplify 10y ago
  How does not squashing your commits help the protocol/implementation scenario you described?
- knicholes 10y ago
  You can merge the interface PR into the concrete implementation's PR. You don't have to work off of just one remote branch.
- lomnakkus 10y ago
  > and can't be dependent on a prior PR. This pinpoints the major problem exactly. Without dependencies between PRs there's really no sane way (with this feature enabled) to submit a series of commits while expecting those commits to remain separate. Oh, and I object to the general sentiment in the responses to your post that seem to value drive-by/inexperienced contributors over the "experts". Yes, we definitely should make things as easy/simple as possible for new contributors, but NOT at the expense of adding a gotcha for expert contributors. The experts are what keep a project going over many years instead of just releasing version upon version of trivial spelling fixes. (And, btw, the default "merge" option for GitHub PRs also sucks. It should be possible to simply disallow non-FF merges and to force all merges to be FF. EDIT: Interestingly, this seems to be about the only workflow explicitly forbidden by the new rules... unless, of course, you're willing to merge everything manually using your local copy of the repo and pushing from that to GH.)
  - yxhuvud 10y ago
    Yeah, it is also the obvious thing that is missing in the review stage of a pull request - viewing all the content and all the diffs separately but on one page, in a serial way that corresponds to the actual order they will show up when you do git log, all on the same page.
  - JoshTriplett 10y ago
    This is something that Gerrit supports natively: you can have a Gerrit CL that depends on another CL. It's unfortunate that Github doesn't support any equivalent.

3JPLW 10y ago

Sure, it's a terrible feature to always use. And it's likely to be of little use to contributors who know how to use Git well. But in large open-source projects, often new contributors make a small change that needs a few minor corrections. Eliminating that final back-and-forth ("squash please") is a huge win for maintainers.

Bahamut 10y ago
Not only maintainers - anyone tracing back through the history to find out what broke their use case.

haberman 10y ago

There's no guarantee that every individual commit of a feature branch is meaningful, or even builds. It also makes the history of the master branch a lot harder to read when it has tons of commits representing the minutiae of the feature's development.

jakub_g 10y ago
It really depends on each individual's workflow. I tend to use lots of "in progress" commits (each time things are "green"), and as I go, I regularly squash the commits, so in the final pull request I typically have several commits (and if I wasn't squashing it would have been a dozen). If I do feature and a refactor, they are always separate commits, it's easier to review these and bisect if something turns out to go wrong. Some people might do similar things but they might not assure each commit is green, and they never squash anything (so you end up with non-meaningful commits). As @3JPLW said, I see when it can be useful for opensource maintainers to have the option to squash someone's commits, when the change is small, but there are many commits (due to a review ping-pong etc)
echion 10y ago
There's no guarantee, but there are many benefits to striving for this ("git bisect run", CI test results).
yxhuvud 10y ago
If it isn't meaningful, then that is something the review stage should catch.

natrius 10y ago

I used to strongly believe what you do until my company started using Phabricator, which forces the squash workflow on you. It makes your history more useful, not less. The pull request is the appropriate unit of change for software. Make small commits as you develop, then squash them down into a single meaningful change to the behavior of your software.

dsmithatx 10y ago
As a git novice I wonder, doesn't a proper workflow do the same thing? When I submit a feature branch it might have a lot of ugly commits. However, once I merge it to an integration branch there is one nice commit explaining what I did. When coworkers create Pull requests I don't go through all of their commits and changes along the way. I just look at the diff so, I don't see the need for them to squash it first.
- hnrodey 10y ago
  Once merged, the history of your feature branch becomes part of the history of the integration branch. Sounds like you're using GitHub (Enterprise) or something similar where the pull request view shows you all of the changes in a "squashed" fashion.
  - dsmithatx 10y ago
    Yes Bitbucket so, maybe that explains it.
glhaynes 10y ago
It seems like it'd be nice to have two levels of granularity exposed in views of a source control system's history, basically corresponding to pull requests and commits. So you could drill-down to individual commits as needed, but would normally be able to work at the PR level.
- prodigal_erik 10y ago
  Does "git log --merges" get us there?

falsedan 10y ago

I use this workflow: 1. branch off master 2. work, commit, push, test (on CI server) 3. decide it's time to ship 4. rebase -i, push, test (again) 5. git checkout master && git merge --no-ff feature_branch (make the merge commit message a summary of the feature) master ends up being a list of feature branch commits, bookended by the merge commit which introduced the feature. Getting the squash commit diff is as easy as 'git diff feature_branch_merge^..feature_branch_merge'.

BinaryIdiot 10y ago

> Before making a pull request (or doing any sort of merge), you should rebase against upstream master (or whatever you're going to push to) See, and maybe this is because I'm just dumb or something, but I have never gotten rebasing to work for me. Ever. Every single time I do it I read at east 3 articles about it so I don't screw something up, I attempt to do it and ultimately I lose a bunch of work. I just don't get it. I can write web, mobile and desktop apps and I like to think I'm pretty good at it. But I'm one of those people who constantly have commits of merges in their code because for whatever reason I just can't get my head around making rebasing work correctly. Am I the only one? Sorry for the derail but it's bothering me that I've never gotten this to work correctly and I feel otherwise normally smart. ¯\_(ツ)_/¯

tunesmith 10y ago
I think the advice to rebase runs up against the business pattern of pushing your branch as soon as you create it (git-flow and a lot of jira/stash integrations work like this). Also some teams want to see evidence of your commits as you make them, which means pushing as you commit. If you have a branch and it's already pushed, rebasing just feels kind of funny and can sometimes cause a lot of problems if anyone else has checked it out. If you have a branch and it's local only, then merging from mainline into your branch and selecting rebase instead of merge is relatively painless.
joshuahutt 10y ago
$ git checkout master $ git pull $ git checkout branch-name $ git rebase master If there are merge conflicts, open the affected file(s) and resolve them. Then: $ git add filename.ext $ git rebase --continue Finally: $ git push origin branch-name If you've already pushed the branch, use -f. Make sure to always specify the branch name when using that flag!
- mcguire 10y ago
  For those cases where you have created a fork of a project and are preparing a pull request, would that be something like: $ git checkout master $ git fetch upstream # https://help.github.com/articles/syncing-a-fork/ $ # git merge upstream/master # <- leaves merge commits in your fork $ git checkout branch-name $ git rebase upstream/master # Use rebase instead of merge? $ git push -f origin branch-name

golergka 10y ago

Why would you want to rewrite your whole work history and change the actual state of the repository at each of your commits? Why don't just merge?

JoshTriplett 10y ago

If someone prepares a pull request with a well-structured series of commits, making a logical series of changes, where the project builds and passes tests after each commit, then those commits shouldn't get squashed. However, I frequently see people adding more commits on top of a pull request to fix typos, or do incremental development, where only the final result builds and passes, but not the intermediate stages, and where the changes are scattered among the commits with no logical grouping. In that case, I'd rather see them squashed and merged than merged in their existing form, and having a button to do that makes it more likely to happen.

andersonvom 10y ago
Plain squashing commits, while still a valid option in very few cases, will likely lead to gigantic commits that are hard to reason about. I've seen projects where maintainers clean up poor commits before merging them: rebase/squash/reword only what's appropriate.
nhaehnle 10y ago
The trick is to not squash everything into one giant commit, but to use rebase -i liberally to squash/fixup those typo fix commits where they belong.
- JoshTriplett 10y ago
  That's what the author of the pull request should do. But this provides a potentially useful alternative when that doesn't happen.
Tyr42 10y ago
It's also the case that you lose the code review if you force push to a PR's branch after adding in a typo fix and squashing locally, right? That's a pretty good reason not to squash till the review is done.
- JoshTriplett 10y ago
  > It's also the case that you lose the code review if you force push to a PR's branch after adding in a typo fix and squashing locally, right? Not as far as I can tell; I've force-pushed pull request branches many times, and the code reviews seem to stick around. (Perhaps they wouldn't if the code changed more drastically, like files disappearing; I haven't tried that.)

phasmantistes 10y ago

Squashing entire pull requests that change multiple things into a single commit is a bad idea, yes. But uploading and asking for review on such wide-reaching pull requests is a bad idea in the first place. Using fast-forward without squash is also a bad idea in many cases: the string of commits may contain multiple points that don't actually build or pass tests, even if the final commit in the chain fixes all that. There's no point in landing those broken commits, and doing so will confuse bisection tools. Fast-forward with squash, and enforcing reasonably sized code reviews as a matter of culture, is the best of all worlds in my opinion.

jgraham 10y ago

It's a bad idea because it's a bad implementation. If it allowed you to select what to squash, defaulting to the behaviour of git rebase -i --autosquash master then it would be a clearly good feature.

draw_down 10y ago

Rebasing seems to clutter the Github PR's commit history and diff with all the commits to master that were made between the time the branch was cut and the time the rebase happens. But it doesn't do that if you merge in master. I never understood this.

zb 10y ago

I think that GitHub's pull-request-based model is fundamentally broken. Gerrit's model, where every commit is quasi-independent (and hence must pass tests) and you can easily edit without force-pushing anything or losing review history, is superior (though not perfect) in almost all cases. (Exception: merging a long-running feature branch where all the commits in the branch have already been reviewed.) This is GitHub's attempt to solve the problem without really changing anything. It won't really change anything. Since pull requests routinely contain a mixture of both changes that should be squashed (fixups) and changes that should not be squashed (independent changes), this just means that you get to pick your poison.

serge2k 10y ago

> Squashing entire pull requests that may change multiple things into a single commit is a very bad idea. If changes are too large/complex/disjoint to fix in a single commit then why have them in one PR?

jibsen 10y ago

I wonder why they did not add `--ff-only` as an option, like GitLab has.