Re: Review and update pull requests

[Stamatis Zampetakis]

I created  for this purpose.

You can have a look and let me know.

2018-08-12 3:08 GMT+03:00 Michael Mior :

> 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
> mm...@apache.org
>
>
>
> Le sam. 11 août 2018 à 07:02, Stamatis Zampetakis  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 :
> >
> > > 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

Re: Review and update pull requests

[Michael Mior]

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
mm...@apache.org



Le sam. 11 août 2018 à 07:02, Stamatis Zampetakis  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 :
>
> > 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

Re: Review and update pull requests

[Stamatis Zampetakis]

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 :

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

Re: Review and update pull requests

[Julian Hyde]

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

Re: Review and update pull requests

[Vladimir Sitnikov]

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

Re: Review and update pull requests

[Michael Mior]

IMO, it depends on how extensive the changes are. Eventually we certainly
prefer the rebase and force push approach . However, if the PR is complex
and a lot of changes have been made, it can be helpful to temporarily add a
commitment (which will be squashed later) to help the reviewer see what has
changed.

I'll let others also chime in with their thoughts.

On Fri, Aug 10, 2018, 4:36 AM Stamatis Zampetakis  wrote:

> Hi,
>
> Contributing to Calcite is quite well documented in the website (
> https://calcite.apache.org/develop/#contributing). One minor thing that
> might be missing is how to update the pull request when changes are
> required by the reviewers.
>
> Personally, I always perform rebase followed by a forced push in an attempt
> to always keep the PR with one commit. However, this may make the changes
> more difficult to review.
>
>1. Is there a preferred way to handle updates in PRs?
>2. Should we update the site with a few extra lines documenting this
>case?
>
> Best,
> Stamatis
>

Review and update pull requests

[Stamatis Zampetakis]

Hi,

Contributing to Calcite is quite well documented in the website (
https://calcite.apache.org/develop/#contributing). One minor thing that
might be missing is how to update the pull request when changes are
required by the reviewers.

Personally, I always perform rebase followed by a forced push in an attempt
to always keep the PR with one commit. However, this may make the changes
more difficult to review.

   1. Is there a preferred way to handle updates in PRs?
   2. Should we update the site with a few extra lines documenting this
   case?

Best,
Stamatis