> > 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 <u...@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 <hba...@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 <dschnei...@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. > >>> >
