This seems to run contrary to the commonly cited rule of "Commit early, commit often". By committing often and pushing often, other developers can see your work and offer informal review, while ensuring that there is not duplication of effort.rarified wrote:Here is where I would disagree, on the basis that we're trying to follow a single-commit-per-fix pattern. And why I was debating using patch queues until deciding it may be more confusing to less experienced developers than the straightforward rule.Christian Walther wrote:I don’t see why this is an issue. On the contrary, by committing the change to a repository before it gets reviewed, you ensure that what’s reviewed is exactly what’s going to end up in the mainline if approved. If not approved, I wouldn’t say it needs to be patched out either. It’s either amended by more commits on top of it, then merged into the mainline after all, or it’s replaced or definitely rejected and can just stay there as an open head in the submitter’s repository.rarified wrote:it avoids the issue of committing a change to the repository before it gets review exposure, so it doesn't have to be patched out
It's also usually preferable to have a bunch of small atomic commits than a single giant patch, because if there's a bug in any of those small commits, they can easily be reverted without losing the entire changeset.
As an example, Deledrius's fixes for localization: https://github.com/H-uru/Plasma/pull/55
Rather than a single large commit that changed several unrelated pieces of code, he kept each change as its own commit so that there would both be context in the commit message, and easily reversible changes.
You can also see an example of our code review process there (although that was a more general discussion than a code review).
A better example might be https://github.com/H-uru/Plasma/pull/52 or https://github.com/H-uru/Plasma/pull/163.
I'd also like to add that we frequently rewrite history or rebase commits in our personal forks, and doing so often results in a cleaner merge, and a more organized history. One of the main reasons that we switched a lot of our projects from Mercurial over to git is because of how difficult Mercurial makes it to shuffle commits around.
As a general question, has anyone tried to have a discussion with Cyan regarding how they actually want changes handled? I think trying to get some sort of answer is better than guessing.
Even if they aren't using hg internally, it shouldn't be difficult to generate a patch that contains a number of commits.