geode-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Udo Kohlmeyer <...@apache.org>
Subject Re: what is the best way to update a geode pull request
Date Fri, 31 May 2019 21:23:44 GMT
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 <hbales@pivotal.io> 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 <dschneider@pivotal.io>
>> 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.
>>>

Mime
View raw message