2 ms·
> If one submits a PR and it has a messy history, what is the reviewer supposed to do? Make them go through the commits and clean them up? That's up to the pr
by mikegerwitz 10y ago
> If one submits a PR and it has a messy history, what is the reviewer supposed to do? Make them go through the commits and clean them up?
That's up to the project. Some do, yes, for the reasons that I described---it's much easier to review a series of patches that can be comprehended, and has benefits later on (e.g. bisecting). If the patch is large from a contributor, and a bisect arrives at that commit, and the contributor is not the one doing the debugging, it can be a frustrating and inefficient experience. I've wasted many nights on something that could have otherwise been immediately obvious.
> If you only have squash commits from PRs and run CI on them, you are sure that every commit compiles.
It's not difficult to loop through commits and make sure they compile.
In fact, bisect can do it for you, if you aren't fond of command-line loops.
- stepanhruda 10y agoFor larger projects this takes literally hours
- mikegerwitz 10y agoIt's not a one-size-fits-all type of thing; if it doesn't work for your project, then you need to try something else. Where I work, we don't enforce that each commit build---it's known to be a good practice, and they're going to have someone flip out on them if they're trying to debug something and they have to skip a dozen commits when bisecting (incidentally, I had that problem today). In that case, it's a cultural thing. If someone consistently commits code that doesn't build, then they should be addressed. If it's a random contributor to a project, more care should probably be taken. If building each commit isn't feasible, then maybe only building modified files (that is, a normal `make` for example rather than a fresh workspace) is better than nothing. If that's too long, maybe build a few sample commits. Etc. In any case, even if you don't build each commit, the history is still useful.