I agree with most of what Vladimir said. But briefly: * It isn’t often necessary for the contributor to rebase. The reviewer will ask for a rebase if it’s really needed. * If you add a commit after an initial review, it’s really important that you do not squash (or amend). The reviewer wants to see the delta. * Reviewers are (in my opinion) at liberty to perform "copy-editing” style tasks (squash, rebase, fix typos, add a couple of extra tests). We don’t want the process to provide friction to higher quality.
Julian' > On Aug 10, 2018, at 3:00 AM, Vladimir Sitnikov <sitnikov.vladi...@gmail.com> > wrote: > > Stamatis>Personally, I always perform rebase followed by a forced push > > I was inclined to use that policy in early days, yet I think it should not > be the main way. > > Bellow assumes GitHub. If we happen to use Gerrit things might shine with a > different colour. > > I suggest the following. > > FAQ: > Q: I want to rebase/squash to make the PR shiny. Should I? > A: No. It would complicate > > Q: I'm afraid all those oops/fixup commits will clutter git history. Should > I rebase? > A: No. Rebase/squash can be performed by committer if there's no other > issues > > Q: Travis CI failed, but the failure is not caused by my changes (e.g. > failed to download from Maven). Should I force-push to re-trigger the CI? > A: No. Please create empty commit (git commit --allow-empty) and push it. > > Q: My PR is quite old, and I am afraid it is no longer valid. Should I > rebase it? > A: Yes. > > Rules for contributor: > R1) Use feature branch when creating PR. Do not use yours master branch for > PR. > R2) Consider squashing the commits into meaningful ones before you create > the PR. Do not expect "oops/fix/fixup" commits to land to Calcite master. > R3) Feel free to force-push and squash commits during the first 10 minutes > of PR lifetime > R4) If PR was created more than 10 minutes ago, refrain from force-push > R5) Do not force-push in case there's a pending discussion (in the PR > and/or in JIRA) regarding the changes. Pending is vague, so I would suggest > tp consider the discussion to be in pending state if the latest comment is > within 2 weeks > R6) Consider using appropriate commit message for the first commit in > series. Consider duplicating the message to JIRA/PR, so it gets clear what > is the nature of the change > R7) Consider rebasing the PR on top of master if there are lots of new > commits there > > Rules for committer/reviewer: > R8) Consider squashing the commits manually rather than asking PR author to > do that. If "commit is not squashed" is the sole comment, then both author > and reviewer would have to spend time on one more review iteration with > just a mechanical changes. Note: committer cannot just use "squash and > merge" button in the GitHub UI > > Reasoning > 1) Prefer non-rebase push, prefer regular commits on top of previously > existing ones. > It does make it easier to review. Review is async in its nature, and having > a commit (or multiple of them) with new changes > enables to review the changes later. > "rebase + squash" makes it very complicated to review, especially if the > diff is very small. > On top of that, if new commits are just added, then reviewer can just point > which of the variations is better. > > 2) I suppose "squash everything in single commit" can be performed by > committer assuming the first commit has meaningful message. > Squash is trivial, however crafting a message takes some time. > > 3) Sometimes it makes sense to squash the PR into several commits (there > might be several fixes that relate to the same JIRA ticket), > and I suggest that to be made after there's a consensus in general, and > after all the other bits are resolved. > > 4) If the PR gets very old, it might make sense to rebase it on top of > current master. That might be very valid point to squash commits. > > 5) Adding a dummy commit is the only option to re-launch Travis CI tests. > Making dummy commit is way better than force-pushing all the changes with > just different commit date. > > > Vladimir
