I created <https://github.com/apache/calcite/pull/790> for this purpose.
You can have a look and let me know. 2018-08-12 3:08 GMT+03:00 Michael Mior <[email protected]>: > Would someone be willing to put together a PR for the contributor > documentation? That way we have some concrete and specific changes to make. > > -- > Michael Mior > [email protected] > > > > Le sam. 11 août 2018 à 07:02, Stamatis Zampetakis <[email protected]> a > écrit : > > > 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 <[email protected]>: > > > > > 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 < > > > [email protected]> 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 > > > > > > > > >
