I also believe that rebase and force-push makes the review process more
difficult and that is why I raised the question in order to formalize the
procedure.

I think the email of Vladimir is quite complete towards that direction so
it would be helpful to update the site accordingly.

Concretely, I think it would be nice to add R3 to R6 in <
https://calcite.apache.org/develop/#contributing> at the end of the Section
(R1 and R2 are more or less already there).
The empty commit trick could also fit in the same section.

R8 already exists in <
https://calcite.apache.org/docs/howto.html#merging-pull-requests-for-calcite-committers>
so I don't think something more is needed.

Best,
Stamatis

2018-08-10 19:39 GMT+03:00 Julian Hyde <jhyde.apa...@gmail.com>:

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

Reply via email to