
Your project's RCS history affects ease of contribution (or: don't squash PRs) - JoshTriplett
http://mjg59.dreamwidth.org/42759.html
======
ereyes01
Your commit history should be readable like your code and your tests. It's
part of the final product you deliver to the rest of your team. It's easy to
forget or ignore your commit history, which is probably what leads people to
encouraging squashing all commits in a PR. Ugly commit history is a code
smell.

I agree with the OP that it's a bad blanket policy. We are fortunate that git
has powerful abilities to edit commit history (git rebase -i is my favorite).
Rather than just requiring everything to be squashed as a matter of policy,
maintainers should instead request that commit history be cleaned up if it's
not presentable, IMO.

~~~
newjersey
> Your commit history should be readable like your code and your tests. It's
> part of the final product you deliver to the rest of your team. It's easy to
> forget or ignore your commit history, which is probably what leads people to
> encouraging squashing all commits in a PR. Ugly commit history is a code
> smell.

> I agree with the OP that it's a bad blanket policy. We are fortunate that
> git has powerful abilities to edit commit history (git rebase -i is my
> favorite). Rather than just requiring everything to be squashed as a matter
> of policy, maintainers should instead request that commit history be cleaned
> up if it's not presentable, IMO.

For what it is worth, I think history is sacred. Never rewrite history (except
in extraordinary circumstances and that too very rarely). Of course, do
whatever you want on your local machine before pushing to remote but once you
have pushed, stop worrying about it.

~~~
ereyes01
> Of course, do whatever you want on your local machine before pushing to
> remote but once you have pushed, stop worrying about it.

This is what I'm talking about. After you push and share with others, I think
you are absolutely right. Before it leaves your computer via git push to a
shared location, I advocate that you should tidy things up. Thanks for
clarifying...

------
ikawe
On the other hand, a couple years down the road, a generic pull-request-level
comment (added feature foo) can be more useful than a bunch of noisy commits
like

    
    
        - WIP: feature foo
        - fixed typo
        - whoops! 
        - working
        - reformatted
    

As is often the case with new contributors to open source projects.

In theory --no-ff merges give you the best of both worlds, but in practice, I
traverse git history with 'git blame' or 'git log -p <filename>' neither of
which do a good job of surfacing "this commit was merged as part of
feature/foo".

Of course, all the info is there... But I haven't yet found a good UI for it.

~~~
fennecfoxen
In theory, this is why we rebase our history and make an elegant series of
coherent patches. At least if "we" are the Linux dev team or something.

~~~
ikawe
Yup, I agree that would be ideal. I think squash commits are a pragmatic way
to deal with messy incoherent commits.

Plus it's easier to revert a squashed commit than a merge. (which is the index
number of the parent I want?!)

------
marssaxman
Some people clearly pay a great deal more attention to commit history than I
ever do. The only difference this makes to me is that adding more hoops I have
to jump through before you'll accept my patch makes it less likely that I will
bother to submit one in the first place.

~~~
jonaf
Regardless of your opinion, if you have any interest in code quality (hint:
you should), then you should respect the contributing guidelines for the
project you are contributing to.

Your argument / opinion as stated could have "commit history" replaced with
"unit tests" and in many circles (for some reason), people are still making
that argument. For a project to be healthy, a critical eye must be used for
every change: from the code, to the tests, to the commit message. If you don't
care about any one of these, you are saying you don't care about the quality
of the software. I would deny your PR on principle. My project needs more
quality than it needs more contributors.

Note that I did not suggest squashing is good or bad. My opinion on that is
not related to the fact that you should _care_ about it and treat it as just
as important as your contribution, and you should respect the project enough
to abide their guidelines for contribution (unless your contribution is
revising those guidelines).

~~~
marssaxman
Of course. I'm not talking about projects where I have already made a
commitment to participate, though: I'm talking about the process before that,
where there's some open-source project I am using or otherwise interested in,
and I consider getting involved and contributing. The higher the initial
barrier to entry, the more likely it is that I'll just shrug my shoulders and
say "fuck it". A complex process for contributing changes, with lots of
unusual fiddly steps about exactly how commits should be formatted, makes it
more likely that I won't ever make that first commit - especially because the
first commit tends to be something trivial where it's as much about dipping my
toe in and seeing if I want to do something as about the actual change I've
made.

So, yes, I certainly do respect the project enough to abide by their
guidelines for contribution: but if those guidelines demand too much work of
me, that respect will take the form of quietly walking away and spending my
time on something else, and the maintainers of the project will likely never
know I had any interest in helping to begin with. Guessing that there might be
other people like me in the world, then, I suggest that having a complex and
specific process for open-source contribution is likely to reduce the pool of
devs willing to get involved, and it's worth considering whether the amount of
work saved by requiring devs to jump through these hoops outweighs the
obstacle that creates for recruitment.

------
Manishearth
You should squash PRs. What you shouldn't do is indiscriminately squash PRs.
Having a bunch of "oops, fix" and review fix commits lying around makes it
harder for everyone too. It also affects bisection much more. "don't squash
PRs" is just bad advice; it's more nuanced than that.

Split your work into bite sized pieces, and amend/squash the relevant fixups
into these before merging.

Since these tasks can need knowledge of rebase, I offer to do them for new
contributors too, which usually fixes that issue.

~~~
ashitlerferad
Use `git rebase -i` but don't squash PRs.

~~~
Manishearth
git rebase -i is one of the ways you can squash PRs and there are tons of
cases where you should.

------
geuis
I cannot disagree with this more. Commits should be feature-based, with
thorough commit notes. A feature can be a one line change all the way up to
something affecting hundreds of lines (or more).

High quality code standards (good variable naming, plentiful comments, etc)
tell you what's happening at the code level. The commit history tells you
what's happening at the feature level.

Don't pollute history with noise.

