On 03/27/2015 08:05 PM, Ian wrote: > On Wed, Mar 25, 2015 at 12:55 PM, David Roden <bo...@pterodactylus.net> > wrote: >> On 25.03.2015, at 06:14, Steve Dougherty <st...@asksteved.com> wrote: >> >>> My inclination is to merge the unit of review. If the diff can be easily >>> described in a single commit summary, the pull request is squashed and >>> merged as a single commit. >> >> I really wish people would stop doing this. Commits are atomic units of >> work, building on one another (unless the person creating them is rather >> unorganized or is mixing several things in which case a pull request should >> probably also not be merged until those situations are cleaned up). >> Squashing commits destroys valuable information for no practical reason. >> > > I completely agree. It serves no useful purpose, and is entirely contrary > to widely established best-practices for using Github.
The intent of squashing commits is not to destroy useful history. It's
to make history easier to read and allow reviewing changes incrementally
by presenting them in high-level commits which reflect the final state
of the code which passes review.
To be more specific, this is about turning something with changes made
during development / code review like
Clarify test reasoning
Add more tests for new feature
Fix NPE in new feature
Fix inverted branch condition in new feature
Add tests for new feature
Refactor support classes and new feature
Add exciting new feature
Add support needed by new feature
into
Add tests for new feature
Add exciting new feature
Add support needed by new feature
not into
Add exciting new feature
It is my opinion that it is not worth maintaining in repo history a
distinction between an initial implementation and changes made during
code review or refinement. Of course only feature / bugfix branches are
subject to squashing - next/master/release branches are not. git
supports this workflow - git commit has --fixup and --squash. We use
this at my job - a reviewer who is up to date can see the incremental
part in response to review, and a reviewer coming up to date can rebase
autosquash before reading.
We seem to have arrived at two approaches to this. I'm willing to go
with either at this point, and I'd appreciate more perspectives. We'll
update CONTRIBUTING.md with what we agree on.
# Review pull request as a diff; merge without squashing.
David and Ian favor this.
Benefits:
* Common use of GitHub pull requests.
* Does not require the effort / complication of squashing commits.
Disadvantages:
* Keeps changes made toward the final version of the pull request in
the repo history - harder to review later.
* Difficult to review incrementally. This prevents working with very
large changes, not that we want them anyway. It displays changes in
multiple layers simultaneously.
* Hard to review commit messages.
# Authors squash commits into high-level changes making up final
version; review pull request as commits.
Arne and I favor this.
Benefits:
* Easy to review in repo history - high-level context is with each
commit; avoids storing incremental changes toward final version in
the stable repo history.
* Allows reviewing changes incrementally - commits can be ordered and
given messages to aid understanding; diff ordering is not
customizable.
Disadvantages:
* Atypical use of GitHub pull requests - higher bar of entry for larger
changes.
* More effort / complication for authors.
Is this an accurate summary?
I'm not clear on the opinions of Florent, Matthew, Roland, (is Bert
around?) or others on these. Thoughts?
Thanks,
Steve
signature.asc
Description: OpenPGP digital signature
_______________________________________________ Devl mailing list Devl@freenetproject.org https://emu.freenetproject.org/cgi-bin/mailman/listinfo/devl
