> 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 <jbarr...@pivotal.io> 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 <aba...@pivotal.io> 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 <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. > >>> > > >
