Re: what is the best way to update a geode pull request

[Kirk Lund]

+1 for doing whatever facilitates reviewing and is best for the PR (which
may vary by PR or even reviewers)
-1 to disallowing or even strongly discouraging force pushing in a PR (go
ahead and merge or rebase as each person prefers but don't force that
preference on others)

I prefer to separate cleanup/refactor commits from behavior change commits
but I accept that it's on a per-case or per-author basis. I care more about
cleaning up code and adding unit tests than making the reviewing easier.

On Fri, May 31, 2019 at 5:32 PM Aaron Lindsey  wrote:

> +1 to updating the PR template. I've noticed that few people actually
> follow it and sometimes they just remove it altogether.
> +1 to pushing PR changes as separate commits. This makes PR review easier.
>
> Sometimes it's helpful to me as a reviewer for the initial PR to be split
> into multiple commits as well. Also, I like Robert's suggestion of merge
> instead of rebase when the PR is out-of-date with develop.
>
> - Aaron
>
>
> On Fri, May 31, 2019 at 2:50 PM Jacob Barrett  wrote:
>
> >
> >
> > > On May 31, 2019, at 2:40 PM, Udo Kohlmeyer  wrote:
> > >
> > > If we are concerned about the single line that can break the product,
> > then our testing has failed us on so many levels, that there is no hope.
> >
> > Sorry, I used a hyperbolic statement about looking for 1 line out of
> 1000.
> > The point was “formatting” or “cleanup” style commits are better left
> > separate because looking for the real change in that sea of change is
> hard.
> >
> > > But looking forward to see how long one can sustain the "factor ->
> > commit -> make changes required to fulfill JIRA -> commit -> manual
> merge"…
> >
> > It’s only a problem if you are cleaning up lots a code. Not a bad problem
> > to have and the future looks brighter each time.
> >
> > > Also, who reviews the refactor, because even that can introduce
> > unintentional bugs... same effort as single commit. In single commit, if
> > the refactor has made code cleaner, clearer and simpler, maybe 1 commit
> is
> > easier to follow.
> >
> > I think there is a distinction between a refactor and cleanup. Consider
> > the time we decide to reformat all the code, that was a cleanup. Now as
> we
> > are going through the code and IJ reports every other line as a static
> > analyzer warning, fixing that is a cleanup. All these cleanups have been
> > reviews like any other PR. Th point being made was that they are done in
> a
> > way that allows the reviewer to review the clean and the change
> > independently.
> >
> > A refactor would would be a complete reorganization of code and should
> > have the tests, reviews, etc. that go with it.
> >
> > Regardless, all are reviewed.
> >
> > -Jake
> >
> >
>

Re: what is the best way to update a geode pull request

[Aaron Lindsey]

+1 to updating the PR template. I've noticed that few people actually
follow it and sometimes they just remove it altogether.
+1 to pushing PR changes as separate commits. This makes PR review easier.

Sometimes it's helpful to me as a reviewer for the initial PR to be split
into multiple commits as well. Also, I like Robert's suggestion of merge
instead of rebase when the PR is out-of-date with develop.

- Aaron


On Fri, May 31, 2019 at 2:50 PM Jacob Barrett  wrote:

>
>
> > On May 31, 2019, at 2:40 PM, Udo Kohlmeyer  wrote:
> >
> > If we are concerned about the single line that can break the product,
> then our testing has failed us on so many levels, that there is no hope.
>
> Sorry, I used a hyperbolic statement about looking for 1 line out of 1000.
> The point was “formatting” or “cleanup” style commits are better left
> separate because looking for the real change in that sea of change is hard.
>
> > But looking forward to see how long one can sustain the "factor ->
> commit -> make changes required to fulfill JIRA -> commit -> manual merge"…
>
> It’s only a problem if you are cleaning up lots a code. Not a bad problem
> to have and the future looks brighter each time.
>
> > Also, who reviews the refactor, because even that can introduce
> unintentional bugs... same effort as single commit. In single commit, if
> the refactor has made code cleaner, clearer and simpler, maybe 1 commit is
> easier to follow.
>
> I think there is a distinction between a refactor and cleanup. Consider
> the time we decide to reformat all the code, that was a cleanup. Now as we
> are going through the code and IJ reports every other line as a static
> analyzer warning, fixing that is a cleanup. All these cleanups have been
> reviews like any other PR. Th point being made was that they are done in a
> way that allows the reviewer to review the clean and the change
> independently.
>
> A refactor would would be a complete reorganization of code and should
> have the tests, reviews, etc. that go with it.
>
> Regardless, all are reviewed.
>
> -Jake
>
>

Re: what is the best way to update a geode pull request

[Jacob Barrett]

> On May 31, 2019, at 2:40 PM, Udo Kohlmeyer  wrote:
> 
> If we are concerned about the single line that can break the product, then 
> our testing has failed us on so many levels, that there is no hope.

Sorry, I used a hyperbolic statement about looking for 1 line out of 1000. The 
point was “formatting” or “cleanup” style commits are better left separate 
because looking for the real change in that sea of change is hard.

> But looking forward to see how long one can sustain the "factor -> commit -> 
> make changes required to fulfill JIRA -> commit -> manual merge"…

It’s only a problem if you are cleaning up lots a code. Not a bad problem to 
have and the future looks brighter each time.

> Also, who reviews the refactor, because even that can introduce unintentional 
> bugs... same effort as single commit. In single commit, if the refactor has 
> made code cleaner, clearer and simpler, maybe 1 commit is easier to follow.

I think there is a distinction between a refactor and cleanup. Consider the 
time we decide to reformat all the code, that was a cleanup. Now as we are 
going through the code and IJ reports every other line as a static analyzer 
warning, fixing that is a cleanup. All these cleanups have been reviews like 
any other PR. Th point being made was that they are done in a way that allows 
the reviewer to review the clean and the change independently.

A refactor would would be a complete reorganization of code and should have the 
tests, reviews, etc. that go with it. 

Regardless, all are reviewed.

-Jake

Re: what is the best way to update a geode pull request

[Nabarun Nag]

>
> This sounds to me like "all my friends are jumping off bridges" and an
> incorrect application or understanding of your toolset.  In my own
> experience, I have had much more trouble trying to track a bug through
> massive squashed commits than I have identifying which branch was
> responsible for a code change.


-> learning from long standing successful Apache projects is not equivalent
of  "all my friends are jumping off bridges". You need to re-think that.
This is what is done because it is convenient and makes our lives easier,
and this is a sentiment shared by a lot of developers.


This might just be a bit of philosophy, but I have to disagree with you.


Sure, but in practice things are easier the other way.


@Udo : I agree.

Regards
Naba





On Fri, May 31, 2019 at 2:23 PM Udo Kohlmeyer  wrote:

> I think a single (squashed if required) commit for the start of a PR.
> ANY changes due to comments should be reflected as separate commits on
> the PR.
> Once approved, that PR should squash all commits into 1 commit with a
> message that describes what was done,etc...
>
> In the past I've heard developers ask for the following:
>
> 1x commit to address the problem/feature
> 1x commit with refactors.
>
> I must be honest, but I am yet to find 1 developer that keeps a list of
> all changes they want to be refactored separate from the bug/feature
> code. OR better stated I am yet to find where this was sustainable AND
> productive.
>
> It would definitely discourage the community to refactor, because who
> keeps track of all the code they would like to refactor and change the
> code after completing the task. Sounds like double if not triple work to
> me.
>
> Also, how would this work if there are comments on the code i.e better
> approaches, have you thought about... or that might lead to errors. Are
> these changes applied to the core/base commit or against the refactor or
> both?
>
> Also, a good practice for all is to commit (and push) code on a daily
> basis on their branches. "WIP - completed x,y,z. need to address: a,g,r"
> comments are not uncommon and safeguard against loss of equipment,
> illness of members,etc...
>
> My personal coding style is, I make changes to code as I pass through
> it.. (change variable names when they don't make sense, refactor
> methods, etc...) So, I rarely have the time to retrace my steps and
> post-factum re-apply my refactored changes.
>
> I think the checklist could be reworded, but as everything in this
> project, we take it in the spirit was intended and don't follow it to
> the letter. Would the rewording of the checklist enhance/improve/make
> the intent clearer or not? I don't care either way, but when someone
> merges a PR into a main branch with many intermediate commits, it just
> clusters the commit log with "contextually irrelevant" noise. (also
> makes it harder to follow all the changes that are in a branch). What if
> I want to rollback a commit into develop, how many does one have to
> revert, 1 is always simpler than 2-10... Also.. cherry picking becomes
> simpler...
>
> --Udo
>
> On 5/31/19 13:43, Nabarun Nag wrote:
> > In my opinion, I am okay will multiple commits within a PR.
> > But please do squash them to a single  commit when it is pushed to
> develop.
> > This helps us a ton if it is single commit.
> > - bisect operations are easier when it is a single commit during major
> > failure analysis.
> > - cherrypick is easier when it is one commit.
> >
> >
> > I even don’t prefer merge commit messages :
> >   -  none of the big ASF projects do it.
> >   -  visualizing on tools is bit skewed.
> >   -  difficult to analyze failures .
> >
> >
> > I would like to reiterate Dan’s statement on emphasis on people and
> empathy
> > over blanket process and rules.
> >
> > Regards
> > Naba
> >
> >
> >
> > On Fri, May 31, 2019 at 1:32 PM Helena Bales  wrote:
> >
> >> +1. I would guess that it is the checklist as part of the PR that is
> >> confusing people.
> >>
> >> The other reason that history gets rewritten is when force pushing
> after a
> >> rebase. While fast-forwarding is necessary on occasion, this can be
> >> accomplished without rewriting history by using a merge.
> >>
> >> As part of our document on making PRs, we should include instructions on
> >> how to handle the situation where fast-forwarding is necessary,
> explicitly
> >> discourage the use of merges and force-pushes once a PR has been opened,
> >> and some guidelines regarding the appropriate number of commits when
> the PR
> >> is initially opened. Once we have these guidelines, it would be helpful
> to
> >> link to them from the PR checklist that we currently have, and rework
> the
> >> checklist so that it is in line with our desired process.
> >>
> >> On Fri, May 31, 2019 at 1:20 PM Darrel Schneider  >
> >> wrote:
> >>
> >>> Something I have noticed is that often when I have requested changes be
> >>> made to a pull request is that the changes are force pushed ask a
> single

Re: what is the best way to update a geode pull request

[Udo Kohlmeyer]

If we are concerned about the single line that can break the product, 
then our testing has failed us on so many levels, that there is no hope.


But looking forward to see how long one can sustain the "factor -> 
commit -> make changes required to fulfill JIRA -> commit -> manual 
merge"...


Also, who reviews the refactor, because even that can introduce 
unintentional bugs... same effort as single commit. In single commit, if 
the refactor has made code cleaner, clearer and simpler, maybe 1 commit 
is easier to follow.


Once again... mileage will vary for EVERYONE.

On 5/31/19 14:31, Jacob Barrett wrote:



On May 31, 2019, at 2:23 PM, Udo Kohlmeyer  wrote:
I must be honest, but I am yet to find 1 developer that keeps a list of all 
changes they want to be refactored separate from the bug/feature code. OR 
better stated I am yet to find where this was sustainable AND productive.

Challenge accepted and complete… I have done this on a few PRs recently. I have 
taken to cleaning up the code first, leaving that as a separate commit on the 
PR, then fixing the code in question as another commit. The trick has come when 
merging the PR I wanted to squash all feedback commits. This was a manual task 
outside of GitHub. Lately I have taken to filing a PR fo the area I am cleaning 
up, merging that, then a PR for the fix and merging that. I haven’t decided 
which I find more tedious yet.

I do know that it sucks to find the one line change buried in a 1000 line 
cleanup task. They should always be separate commits into develop.

-Jake

Re: what is the best way to update a geode pull request

[Jacob Barrett]

> On May 31, 2019, at 2:23 PM, Udo Kohlmeyer  wrote:
> I must be honest, but I am yet to find 1 developer that keeps a list of all 
> changes they want to be refactored separate from the bug/feature code. OR 
> better stated I am yet to find where this was sustainable AND productive.

Challenge accepted and complete… I have done this on a few PRs recently. I have 
taken to cleaning up the code first, leaving that as a separate commit on the 
PR, then fixing the code in question as another commit. The trick has come when 
merging the PR I wanted to squash all feedback commits. This was a manual task 
outside of GitHub. Lately I have taken to filing a PR fo the area I am cleaning 
up, merging that, then a PR for the fix and merging that. I haven’t decided 
which I find more tedious yet.

I do know that it sucks to find the one line change buried in a 1000 line 
cleanup task. They should always be separate commits into develop.

-Jake

Re: what is the best way to update a geode pull request

[Udo Kohlmeyer]

I think a single (squashed if required) commit for the start of a PR.
ANY changes due to comments should be reflected as separate commits on 
the PR.
Once approved, that PR should squash all commits into 1 commit with a 
message that describes what was done,etc...


In the past I've heard developers ask for the following:

1x commit to address the problem/feature
1x commit with refactors.

I must be honest, but I am yet to find 1 developer that keeps a list of 
all changes they want to be refactored separate from the bug/feature 
code. OR better stated I am yet to find where this was sustainable AND 
productive.


It would definitely discourage the community to refactor, because who 
keeps track of all the code they would like to refactor and change the 
code after completing the task. Sounds like double if not triple work to me.


Also, how would this work if there are comments on the code i.e better 
approaches, have you thought about... or that might lead to errors. Are 
these changes applied to the core/base commit or against the refactor or 
both?


Also, a good practice for all is to commit (and push) code on a daily 
basis on their branches. "WIP - completed x,y,z. need to address: a,g,r" 
comments are not uncommon and safeguard against loss of equipment, 
illness of members,etc...


My personal coding style is, I make changes to code as I pass through 
it.. (change variable names when they don't make sense, refactor 
methods, etc...) So, I rarely have the time to retrace my steps and 
post-factum re-apply my refactored changes.


I think the checklist could be reworded, but as everything in this 
project, we take it in the spirit was intended and don't follow it to 
the letter. Would the rewording of the checklist enhance/improve/make 
the intent clearer or not? I don't care either way, but when someone 
merges a PR into a main branch with many intermediate commits, it just 
clusters the commit log with "contextually irrelevant" noise. (also 
makes it harder to follow all the changes that are in a branch). What if 
I want to rollback a commit into develop, how many does one have to 
revert, 1 is always simpler than 2-10... Also.. cherry picking becomes 
simpler...


--Udo

On 5/31/19 13:43, Nabarun Nag wrote:

In my opinion, I am okay will multiple commits within a PR.
But please do squash them to a single  commit when it is pushed to develop.
This helps us a ton if it is single commit.
- bisect operations are easier when it is a single commit during major
failure analysis.
- cherrypick is easier when it is one commit.


I even don’t prefer merge commit messages :
  -  none of the big ASF projects do it.
  -  visualizing on tools is bit skewed.
  -  difficult to analyze failures .


I would like to reiterate Dan’s statement on emphasis on people and empathy
over blanket process and rules.

Regards
Naba



On Fri, May 31, 2019 at 1:32 PM Helena Bales  wrote:


+1. I would guess that it is the checklist as part of the PR that is
confusing people.

The other reason that history gets rewritten is when force pushing after a
rebase. While fast-forwarding is necessary on occasion, this can be
accomplished without rewriting history by using a merge.

As part of our document on making PRs, we should include instructions on
how to handle the situation where fast-forwarding is necessary, explicitly
discourage the use of merges and force-pushes once a PR has been opened,
and some guidelines regarding the appropriate number of commits when the PR
is initially opened. Once we have these guidelines, it would be helpful to
link to them from the PR checklist that we currently have, and rework the
checklist so that it is in line with our desired process.

On Fri, May 31, 2019 at 1:20 PM Darrel Schneider 
wrote:


Something I have noticed is that often when I have requested changes be
made to a pull request is that the changes are force pushed ask a single
commit to the pr. I would actually prefer that the changes show up as a

new

commit on the pr instead of everything being rebased into one commit.

That

makes the history of the pr easier to follow and make it easy to see what
has changed since the previous review. What do others think? Have we done
something that makes contributors think the pull request has to be single
commit? I know the initial pull request is supposed to be but from then

on

I'd prefer that we wait to squash when we merge it to develop.

Re: what is the best way to update a geode pull request

[Patrick Rhomberg]

> Not sure if some people just force-push out of habit, or if the
requirement for initial commit to be squashed creates some fear.

If anyone here finds they have that habit, it would be a good one to break.

> I would only do a rebase and force-push if github tells me that there is a
> conflict, but my rebase would keep my original commits (no squashing). I
do
> not like to do a merge, since it will pull in other people's commits to my
> PR.

If you are merging from origin/develop (or whatever branch against which
you have opened your PR), the PR diff doesn't show the work that is already
present on the targeted branch.  When we aren't actively engaging in
revisionist history, Git / GitHub is smart enough, to know where changes
originated and only display the new diffset.
Even if you just "Accept Mine" for your whole merge conflict, the fact that
it is tracked as a merge will help Git and the replayability of the history
down the road.

> But please do squash them to a single  commit when it is pushed to
develop.
> This helps us a ton if it is single commit
> [...]

This might just be a bit of philosophy, but I have to disagree with you.

I personally think a commit is "one atomic unit of work."  The product
should be robust, correct, and buildable at each commit.  And while most
PRs should also be an atomic action and therefore a single commit, there
have been plenty of times in my own experience where the scope of a PR grew
as the result of discussion.  Perhaps in these cases, the PR should have
been split out into multiple PRs, but if the additional work is the direct
result of discussion on that PR, it feels strange to close it and open
multiple others.

> I even don’t prefer merge commit messages :
>  -  none of the big ASF projects do it.
>  -  visualizing on tools is bit skewed.
> -  difficult to analyze failures .

This sounds to me like "all my friends are jumping off bridges" and an
incorrect application or understanding of your toolset.  In my own
experience, I have had much more trouble trying to track a bug through
massive squashed commits than I have identifying which branch was
responsible for a code change.

> I think either is fine as long as it is deliberate and makes it easier to
understand what someone is doing.

My sentiment exactly.  The history is for the reader.

Also, because Concourse runs precheckin and stores history based on a
particular SHA, if you are pushing on top of an open PR rather than forcing
an apparent new PR, you preserve your testing history in a clean and
readable way.

Lastly, as long as I'm on my soapbox, all this should be happening on your
fork.  There are some people still pushing their things to the public
space, and I would appreciate if people were more diligent about working on
forks and opening PRs from there.

On Fri, May 31, 2019 at 1:44 PM Jacob Barrett  wrote:

> Would that be PR of a the PR template. Might that cause a black hole to
> form?
>
> I’m in favor of updating this wall of text you smack your face on for each
> PR. Let’s pair it down to discourage people (myself strongly included) from
> ignoring or deleting it.
>
> -jake
>
>
> > On May 31, 2019, at 1:33 PM, Anthony Baker  wrote:
> >
> > Let’s update the checklist to match the outcome of this thread:
> >
> https://github.com/apache/geode/blob/develop/.github/PULL_REQUEST_TEMPLATE.md
> <
> https://github.com/apache/geode/blob/develop/.github/PULL_REQUEST_TEMPLATE.md
> >
> >
> > Anthony
> >
> >
> >> On May 31, 2019, at 1:31 PM, Helena Bales  wrote:
> >>
> >> +1. I would guess that it is the checklist as part of the PR that is
> >> confusing people.
> >>
> >> The other reason that history gets rewritten is when force pushing
> after a
> >> rebase. While fast-forwarding is necessary on occasion, this can be
> >> accomplished without rewriting history by using a merge.
> >>
> >> As part of our document on making PRs, we should include instructions on
> >> how to handle the situation where fast-forwarding is necessary,
> explicitly
> >> discourage the use of merges and force-pushes once a PR has been opened,
> >> and some guidelines regarding the appropriate number of commits when
> the PR
> >> is initially opened. Once we have these guidelines, it would be helpful
> to
> >> link to them from the PR checklist that we currently have, and rework
> the
> >> checklist so that it is in line with our desired process.
> >>
> >> On Fri, May 31, 2019 at 1:20 PM Darrel Schneider  >
> >> wrote:
> >>
> >>> Something I have noticed is that often when I have requested changes be
> >>> made to a pull request is that the changes are force pushed ask a
> single
> >>> commit to the pr. I would actually prefer that the changes show up as
> a new
> >>> commit on the pr instead of everything being rebased into one commit.
> That
> >>> makes the history of the pr easier to follow and make it easy to see
> what
> >>> has changed since the previous review. What do others think? Have we
> done
> >>> something that 

Re: what is the best way to update a geode pull request

[Robert Houghton]

I really don't like force-push to PRs  after comments have been made. At
least GitHub shows that the force-push occured now, but that still blocks
us from seeing the incremental change.

For needing to update a PR due to develop having progressed, I recommend
using `git merge origin/develop ` rather than a rebase.

For merging back to develop, I am torn between squash (one commit per
feature) and rebase-merge (linearizes history a-la SVN). I like the idea of
true-history playing out in our code, but I understand Naba's point that
squashed commit-per-feature is easier to bisect.

My 2c

On Fri, May 31, 2019 at 1:44 PM Jacob Barrett  wrote:

> Would that be PR of a the PR template. Might that cause a black hole to
> form?
>
> I’m in favor of updating this wall of text you smack your face on for each
> PR. Let’s pair it down to discourage people (myself strongly included) from
> ignoring or deleting it.
>
> -jake
>
>
> > On May 31, 2019, at 1:33 PM, Anthony Baker  wrote:
> >
> > Let’s update the checklist to match the outcome of this thread:
> >
> https://github.com/apache/geode/blob/develop/.github/PULL_REQUEST_TEMPLATE.md
> <
> https://github.com/apache/geode/blob/develop/.github/PULL_REQUEST_TEMPLATE.md
> >
> >
> > Anthony
> >
> >
> >> On May 31, 2019, at 1:31 PM, Helena Bales  wrote:
> >>
> >> +1. I would guess that it is the checklist as part of the PR that is
> >> confusing people.
> >>
> >> The other reason that history gets rewritten is when force pushing
> after a
> >> rebase. While fast-forwarding is necessary on occasion, this can be
> >> accomplished without rewriting history by using a merge.
> >>
> >> As part of our document on making PRs, we should include instructions on
> >> how to handle the situation where fast-forwarding is necessary,
> explicitly
> >> discourage the use of merges and force-pushes once a PR has been opened,
> >> and some guidelines regarding the appropriate number of commits when
> the PR
> >> is initially opened. Once we have these guidelines, it would be helpful
> to
> >> link to them from the PR checklist that we currently have, and rework
> the
> >> checklist so that it is in line with our desired process.
> >>
> >> On Fri, May 31, 2019 at 1:20 PM Darrel Schneider  >
> >> wrote:
> >>
> >>> Something I have noticed is that often when I have requested changes be
> >>> made to a pull request is that the changes are force pushed ask a
> single
> >>> commit to the pr. I would actually prefer that the changes show up as
> a new
> >>> commit on the pr instead of everything being rebased into one commit.
> That
> >>> makes the history of the pr easier to follow and make it easy to see
> what
> >>> has changed since the previous review. What do others think? Have we
> done
> >>> something that makes contributors think the pull request has to be
> single
> >>> commit? I know the initial pull request is supposed to be but from
> then on
> >>> I'd prefer that we wait to squash when we merge it to develop.
> >>>
> >
>

Re: what is the best way to update a geode pull request

[Alexander Murmann]

+1 to updating the checklist.

About single vs multiple commits: I think either is fine as long as it is
deliberate and makes it easier to understand what someone is doing. If
there is a large refactor that enables a small change, it is often easier
to understand when these changes are in separate commits. At that point one
might consider splitting this even out into separate PRs, but sometimes
seeing that the change is following the refactor provides valuable context.
It also reduces review latency. As so often: Think about what you are doing
and why and do what makes sense rather than blindly follow a simplistic
rule.

On Fri, May 31, 2019 at 1:44 PM Jacob Barrett  wrote:

> Would that be PR of a the PR template. Might that cause a black hole to
> form?
>
> I’m in favor of updating this wall of text you smack your face on for each
> PR. Let’s pair it down to discourage people (myself strongly included) from
> ignoring or deleting it.
>
> -jake
>
>
> > On May 31, 2019, at 1:33 PM, Anthony Baker  wrote:
> >
> > Let’s update the checklist to match the outcome of this thread:
> >
> https://github.com/apache/geode/blob/develop/.github/PULL_REQUEST_TEMPLATE.md
> <
> https://github.com/apache/geode/blob/develop/.github/PULL_REQUEST_TEMPLATE.md
> >
> >
> > Anthony
> >
> >
> >> On May 31, 2019, at 1:31 PM, Helena Bales  wrote:
> >>
> >> +1. I would guess that it is the checklist as part of the PR that is
> >> confusing people.
> >>
> >> The other reason that history gets rewritten is when force pushing
> after a
> >> rebase. While fast-forwarding is necessary on occasion, this can be
> >> accomplished without rewriting history by using a merge.
> >>
> >> As part of our document on making PRs, we should include instructions on
> >> how to handle the situation where fast-forwarding is necessary,
> explicitly
> >> discourage the use of merges and force-pushes once a PR has been opened,
> >> and some guidelines regarding the appropriate number of commits when
> the PR
> >> is initially opened. Once we have these guidelines, it would be helpful
> to
> >> link to them from the PR checklist that we currently have, and rework
> the
> >> checklist so that it is in line with our desired process.
> >>
> >> On Fri, May 31, 2019 at 1:20 PM Darrel Schneider  >
> >> wrote:
> >>
> >>> Something I have noticed is that often when I have requested changes be
> >>> made to a pull request is that the changes are force pushed ask a
> single
> >>> commit to the pr. I would actually prefer that the changes show up as
> a new
> >>> commit on the pr instead of everything being rebased into one commit.
> That
> >>> makes the history of the pr easier to follow and make it easy to see
> what
> >>> has changed since the previous review. What do others think? Have we
> done
> >>> something that makes contributors think the pull request has to be
> single
> >>> commit? I know the initial pull request is supposed to be but from
> then on
> >>> I'd prefer that we wait to squash when we merge it to develop.
> >>>
> >
>

Re: what is the best way to update a geode pull request

[Nabarun Nag]

In my opinion, I am okay will multiple commits within a PR.
But please do squash them to a single  commit when it is pushed to develop.
This helps us a ton if it is single commit.
- bisect operations are easier when it is a single commit during major
failure analysis.
- cherrypick is easier when it is one commit.


I even don’t prefer merge commit messages :
 -  none of the big ASF projects do it.
 -  visualizing on tools is bit skewed.
 -  difficult to analyze failures .


I would like to reiterate Dan’s statement on emphasis on people and empathy
over blanket process and rules.

Regards
Naba



On Fri, May 31, 2019 at 1:32 PM Helena Bales  wrote:

> +1. I would guess that it is the checklist as part of the PR that is
> confusing people.
>
> The other reason that history gets rewritten is when force pushing after a
> rebase. While fast-forwarding is necessary on occasion, this can be
> accomplished without rewriting history by using a merge.
>
> As part of our document on making PRs, we should include instructions on
> how to handle the situation where fast-forwarding is necessary, explicitly
> discourage the use of merges and force-pushes once a PR has been opened,
> and some guidelines regarding the appropriate number of commits when the PR
> is initially opened. Once we have these guidelines, it would be helpful to
> link to them from the PR checklist that we currently have, and rework the
> checklist so that it is in line with our desired process.
>
> On Fri, May 31, 2019 at 1:20 PM Darrel Schneider 
> wrote:
>
> > Something I have noticed is that often when I have requested changes be
> > made to a pull request is that the changes are force pushed ask a single
> > commit to the pr. I would actually prefer that the changes show up as a
> new
> > commit on the pr instead of everything being rebased into one commit.
> That
> > makes the history of the pr easier to follow and make it easy to see what
> > has changed since the previous review. What do others think? Have we done
> > something that makes contributors think the pull request has to be single
> > commit? I know the initial pull request is supposed to be but from then
> on
> > I'd prefer that we wait to squash when we merge it to develop.
> >
>

Re: what is the best way to update a geode pull request

[Jacob Barrett]

Would that be PR of a the PR template. Might that cause a black hole to form? 

I’m in favor of updating this wall of text you smack your face on for each PR. 
Let’s pair it down to discourage people (myself strongly included) from 
ignoring or deleting it.

-jake


> On May 31, 2019, at 1:33 PM, Anthony Baker  wrote:
> 
> Let’s update the checklist to match the outcome of this thread:
> https://github.com/apache/geode/blob/develop/.github/PULL_REQUEST_TEMPLATE.md 
> 
> 
> Anthony
> 
> 
>> On May 31, 2019, at 1:31 PM, Helena Bales  wrote:
>> 
>> +1. I would guess that it is the checklist as part of the PR that is
>> confusing people.
>> 
>> The other reason that history gets rewritten is when force pushing after a
>> rebase. While fast-forwarding is necessary on occasion, this can be
>> accomplished without rewriting history by using a merge.
>> 
>> As part of our document on making PRs, we should include instructions on
>> how to handle the situation where fast-forwarding is necessary, explicitly
>> discourage the use of merges and force-pushes once a PR has been opened,
>> and some guidelines regarding the appropriate number of commits when the PR
>> is initially opened. Once we have these guidelines, it would be helpful to
>> link to them from the PR checklist that we currently have, and rework the
>> checklist so that it is in line with our desired process.
>> 
>> On Fri, May 31, 2019 at 1:20 PM Darrel Schneider 
>> wrote:
>> 
>>> Something I have noticed is that often when I have requested changes be
>>> made to a pull request is that the changes are force pushed ask a single
>>> commit to the pr. I would actually prefer that the changes show up as a new
>>> commit on the pr instead of everything being rebased into one commit. That
>>> makes the history of the pr easier to follow and make it easy to see what
>>> has changed since the previous review. What do others think? Have we done
>>> something that makes contributors think the pull request has to be single
>>> commit? I know the initial pull request is supposed to be but from then on
>>> I'd prefer that we wait to squash when we merge it to develop.
>>> 
> 

Re: what is the best way to update a geode pull request

[Jinmei Liao]

+1 for initial PR needs to be a single commit
+1 for making intermittent commits to the PR to show history of revision.

I would only do a rebase and force-push if github tells me that there is a
conflict, but my rebase would keep my original commits (no squashing). I do
not like to do a merge, since it will pull in other people's commits to my
PR.

On Fri, May 31, 2019 at 1:33 PM Anthony Baker  wrote:

> Let’s update the checklist to match the outcome of this thread:
>
> https://github.com/apache/geode/blob/develop/.github/PULL_REQUEST_TEMPLATE.md
> <
> https://github.com/apache/geode/blob/develop/.github/PULL_REQUEST_TEMPLATE.md
> >
>
> Anthony
>
>
> > On May 31, 2019, at 1:31 PM, Helena Bales  wrote:
> >
> > +1. I would guess that it is the checklist as part of the PR that is
> > confusing people.
> >
> > The other reason that history gets rewritten is when force pushing after
> a
> > rebase. While fast-forwarding is necessary on occasion, this can be
> > accomplished without rewriting history by using a merge.
> >
> > As part of our document on making PRs, we should include instructions on
> > how to handle the situation where fast-forwarding is necessary,
> explicitly
> > discourage the use of merges and force-pushes once a PR has been opened,
> > and some guidelines regarding the appropriate number of commits when the
> PR
> > is initially opened. Once we have these guidelines, it would be helpful
> to
> > link to them from the PR checklist that we currently have, and rework the
> > checklist so that it is in line with our desired process.
> >
> > On Fri, May 31, 2019 at 1:20 PM Darrel Schneider 
> > wrote:
> >
> >> Something I have noticed is that often when I have requested changes be
> >> made to a pull request is that the changes are force pushed ask a single
> >> commit to the pr. I would actually prefer that the changes show up as a
> new
> >> commit on the pr instead of everything being rebased into one commit.
> That
> >> makes the history of the pr easier to follow and make it easy to see
> what
> >> has changed since the previous review. What do others think? Have we
> done
> >> something that makes contributors think the pull request has to be
> single
> >> commit? I know the initial pull request is supposed to be but from then
> on
> >> I'd prefer that we wait to squash when we merge it to develop.
> >>
>
>

-- 
Cheers

Jinmei

Re: what is the best way to update a geode pull request

[Owen Nichols]

I’ve almost never seen a PR where this checklist was filled out.  Either the PR 
is created with the original boilerplate intact (or sometimes it’s deleted 
entirely).

I feel like this wall of boilerplate discourages contributors from instead 
using that space to describe their change and add other context that would be 
helpful to a reviewer...


> On May 31, 2019, at 1:33 PM, Anthony Baker  wrote:
> 
> Let’s update the checklist to match the outcome of this thread:
> https://github.com/apache/geode/blob/develop/.github/PULL_REQUEST_TEMPLATE.md 
> 
> 
> Anthony
> 
> 
>> On May 31, 2019, at 1:31 PM, Helena Bales  wrote:
>> 
>> +1. I would guess that it is the checklist as part of the PR that is
>> confusing people.
>> 
>> The other reason that history gets rewritten is when force pushing after a
>> rebase. While fast-forwarding is necessary on occasion, this can be
>> accomplished without rewriting history by using a merge.
>> 
>> As part of our document on making PRs, we should include instructions on
>> how to handle the situation where fast-forwarding is necessary, explicitly
>> discourage the use of merges and force-pushes once a PR has been opened,
>> and some guidelines regarding the appropriate number of commits when the PR
>> is initially opened. Once we have these guidelines, it would be helpful to
>> link to them from the PR checklist that we currently have, and rework the
>> checklist so that it is in line with our desired process.
>> 
>> On Fri, May 31, 2019 at 1:20 PM Darrel Schneider 
>> wrote:
>> 
>>> Something I have noticed is that often when I have requested changes be
>>> made to a pull request is that the changes are force pushed ask a single
>>> commit to the pr. I would actually prefer that the changes show up as a new
>>> commit on the pr instead of everything being rebased into one commit. That
>>> makes the history of the pr easier to follow and make it easy to see what
>>> has changed since the previous review. What do others think? Have we done
>>> something that makes contributors think the pull request has to be single
>>> commit? I know the initial pull request is supposed to be but from then on
>>> I'd prefer that we wait to squash when we merge it to develop.
>>> 
> 

Re: what is the best way to update a geode pull request

[Anthony Baker]

Let’s update the checklist to match the outcome of this thread:
https://github.com/apache/geode/blob/develop/.github/PULL_REQUEST_TEMPLATE.md 


Anthony


> On May 31, 2019, at 1:31 PM, Helena Bales  wrote:
> 
> +1. I would guess that it is the checklist as part of the PR that is
> confusing people.
> 
> The other reason that history gets rewritten is when force pushing after a
> rebase. While fast-forwarding is necessary on occasion, this can be
> accomplished without rewriting history by using a merge.
> 
> As part of our document on making PRs, we should include instructions on
> how to handle the situation where fast-forwarding is necessary, explicitly
> discourage the use of merges and force-pushes once a PR has been opened,
> and some guidelines regarding the appropriate number of commits when the PR
> is initially opened. Once we have these guidelines, it would be helpful to
> link to them from the PR checklist that we currently have, and rework the
> checklist so that it is in line with our desired process.
> 
> On Fri, May 31, 2019 at 1:20 PM Darrel Schneider 
> wrote:
> 
>> Something I have noticed is that often when I have requested changes be
>> made to a pull request is that the changes are force pushed ask a single
>> commit to the pr. I would actually prefer that the changes show up as a new
>> commit on the pr instead of everything being rebased into one commit. That
>> makes the history of the pr easier to follow and make it easy to see what
>> has changed since the previous review. What do others think? Have we done
>> something that makes contributors think the pull request has to be single
>> commit? I know the initial pull request is supposed to be but from then on
>> I'd prefer that we wait to squash when we merge it to develop.
>> 

Re: what is the best way to update a geode pull request

[Helena Bales]

+1. I would guess that it is the checklist as part of the PR that is
confusing people.

The other reason that history gets rewritten is when force pushing after a
rebase. While fast-forwarding is necessary on occasion, this can be
accomplished without rewriting history by using a merge.

As part of our document on making PRs, we should include instructions on
how to handle the situation where fast-forwarding is necessary, explicitly
discourage the use of merges and force-pushes once a PR has been opened,
and some guidelines regarding the appropriate number of commits when the PR
is initially opened. Once we have these guidelines, it would be helpful to
link to them from the PR checklist that we currently have, and rework the
checklist so that it is in line with our desired process.

On Fri, May 31, 2019 at 1:20 PM Darrel Schneider 
wrote:

> Something I have noticed is that often when I have requested changes be
> made to a pull request is that the changes are force pushed ask a single
> commit to the pr. I would actually prefer that the changes show up as a new
> commit on the pr instead of everything being rebased into one commit. That
> makes the history of the pr easier to follow and make it easy to see what
> has changed since the previous review. What do others think? Have we done
> something that makes contributors think the pull request has to be single
> commit? I know the initial pull request is supposed to be but from then on
> I'd prefer that we wait to squash when we merge it to develop.
>

Re: what is the best way to update a geode pull request

[Dan Smith]

+1 to pushing PR changes as separate commits. Also +1 to creating PRs with
multiple commits where it makes sense.

-Dan

On Fri, May 31, 2019 at 1:28 PM Owen Nichols  wrote:

> Personally, I do not force-push to my PRs once any review comments have
> been accumulated, for the reasons you mention.
>
> Not sure if some people just force-push out of habit, or if the
> requirement for initial commit to be squashed creates some fear.
>
> I would go a step further and suggest that there is no reason to even
> require the initial PR to be a single, squashed commit.  If you have made a
> small fix and also done some refactoring or cleanup, as a reviewer I would
> much rather you submit that PR with multiple distinct commits, so I don’t
> have to comb through a gigantic diff trying to spot what is the real change
> and what is just renames.
>
> GitHub already very nicely presents the combined delta by default, as if
> it was a single squashed commit, so to me there is only negative value in
> encouraging/requiring pre-squashing.
>
> > On May 31, 2019, at 1:20 PM, Darrel Schneider 
> wrote:
> >
> > Something I have noticed is that often when I have requested changes be
> > made to a pull request is that the changes are force pushed ask a single
> > commit to the pr. I would actually prefer that the changes show up as a
> new
> > commit on the pr instead of everything being rebased into one commit.
> That
> > makes the history of the pr easier to follow and make it easy to see what
> > has changed since the previous review. What do others think? Have we done
> > something that makes contributors think the pull request has to be single
> > commit? I know the initial pull request is supposed to be but from then
> on
> > I'd prefer that we wait to squash when we merge it to develop.
>
>

Re: what is the best way to update a geode pull request

[Owen Nichols]

Personally, I do not force-push to my PRs once any review comments have been 
accumulated, for the reasons you mention.

Not sure if some people just force-push out of habit, or if the requirement for 
initial commit to be squashed creates some fear.

I would go a step further and suggest that there is no reason to even require 
the initial PR to be a single, squashed commit.  If you have made a small fix 
and also done some refactoring or cleanup, as a reviewer I would much rather 
you submit that PR with multiple distinct commits, so I don’t have to comb 
through a gigantic diff trying to spot what is the real change and what is just 
renames.

GitHub already very nicely presents the combined delta by default, as if it was 
a single squashed commit, so to me there is only negative value in 
encouraging/requiring pre-squashing.

> On May 31, 2019, at 1:20 PM, Darrel Schneider  wrote:
> 
> Something I have noticed is that often when I have requested changes be
> made to a pull request is that the changes are force pushed ask a single
> commit to the pr. I would actually prefer that the changes show up as a new
> commit on the pr instead of everything being rebased into one commit. That
> makes the history of the pr easier to follow and make it easy to see what
> has changed since the previous review. What do others think? Have we done
> something that makes contributors think the pull request has to be single
> commit? I know the initial pull request is supposed to be but from then on
> I'd prefer that we wait to squash when we merge it to develop.