geode-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Nabarun Nag <n...@apache.org>
Subject Re: what is the best way to update a geode pull request
Date Fri, 31 May 2019 21:41:50 GMT
>
> 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 <udo@apache.org> 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 <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
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message