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.

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

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
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

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.


Reply via email to