1. Yep. 2. I would phrase this as "merge is the default, but we may (should?) sqaush/rewrite obvious fixup commits." 3. If separable commits belong to the same PR, let's keep them in the same PR.
Sounds like we made a mistake for this PR, and should err more on the side of not squashing. On Tue, Apr 24, 2018 at 10:29 AM Kenneth Knowles <k...@google.com> wrote: > I would like to push back on 2 & 3 and replace it with this: > (not 2, not 3) Don't squash/rebase someone's pull request unless it is obvious that they are OK with it > If someone has used git in the standard way, and you squash their commits, you may have broken them and it is (minor) data loss for the repo. Don't institute a policy that stops power users from doing more powerful things. > Kenn > On Tue, Apr 24, 2018 at 8:19 AM Andrew Pilloud <apill...@google.com> wrote: >> Thanks for the feedback. Sounds like there are a few takeaways from the discussion: >> 1. Not everyone is a git power user, squash and merge is commonly used and can't be disabled. >> 2. As a non-committer I should expect my commits to be squashed, rewritten, or otherwise changed at the discretion of the committer. >> 3. I should be breaking up separate commits into separate PRs. >> Andrew >> On Wed, Apr 18, 2018 at 12:00 PM Kenneth Knowles <k...@google.com> wrote: >>> One thing that is available is the "allow maintainers to edit" which can let any committer push to the PR and do all of this. It is either on by default, or I have simply checked it in the past, since I have recently had a maintainer push to my PR on accident :-) >>> On Tue, Apr 17, 2018 at 5:57 PM Ahmet Altay <al...@google.com> wrote: >>>> I agree with Robert. In this case one size does not fit all. There are times, another round trip with a contributor would be frustrating to the author. Especially for new contributors. Having the option to squash and merge is useful in those cases. (For reference in the past we even helped new contributors by doing small fixes at merge time.) >>>> On Tue, Apr 17, 2018 at 2:28 PM, Robert Bradshaw <rober...@google.com> wrote: >>>>> I think the two options are useful, because we have different kinds of >>>>> contributors. Sophisticated users curate their own history, create >>>>> logically useful commits, build atop it, etc. and merge is by far the >>>>> better option. Others have a single commit followed by any number of >>>>> "lint," "fixup," and "reviewer comments" ones that should clearly be >>>>> squashed, and given that it takes a round trip to ask them to squash it, >>>>> it's nice to be able to do it once there's an LGTM as part of the merge. At >>>>> least making this fact explicit and pointing it out in the docs may be >>>>> useful. >>>>> On Tue, Apr 17, 2018 at 1:43 PM Mingmin Xu <mingm...@gmail.com> wrote: >>>>> > Not strongly against `Create a merge commit`, but I use `squash and >>>>> merge` by default. I understand the potential impact mentioned by Andrew, >>>>> it's still a better option IMO: >>>>> > 1. if a PR contains several parts, it can be documented in commit message >>>>> instead of several commits; --If it's a big task, let's split it into >>>>> several PRs if possible; >>>>> > 2. when several PRs are changing the same file, I would ask contributor >>>>> to fix it; >>>>> > 3. most commits are introduced by reviewer's ask, it's not necessary to >>>>> do another squash(by contributors) before merge; >>>>> > On Tue, Apr 17, 2018 at 1:09 PM, Robert Burke <rob...@frantil.com> wrote: >>>>> >> +1 Having made a few web commits and been frustrated by the options, >>>>> anything to standardize on a single option seems good to me. >>>>> >> On Tue, 17 Apr 2018 at 01:49 Etienne Chauchot <echauc...@apache.org >>>>> wrote: >>>>> >>> +1 to enforce the behavior recommended in the committer guide. I >>>>> usually ask the author to manually squash before committing. >>>>> >>> Etienne >>>>> >>> Le lundi 16 avril 2018 à 22:19 +0000, Robert Bradshaw a écrit : >>>>> >>> +1, though I'll admit I've been an occasional user of the "squash and >>>>> merge" button when a small PR has a huge number of small, fixup changes >>>>> piled on it. >>>>> >>> On Mon, Apr 16, 2018 at 3:07 PM Kenneth Knowles <k...@google.com> wrote: >>>>> >>> It is no secret that I agree with this. When you don't rewrite history, >>>>> distributed git "just works". I didn't realize we could mechanically >>>>> enforce it. >>>>> >>> Kenn >>>>> >>> On Mon, Apr 16, 2018 at 2:55 PM Andrew Pilloud < apill...@google.com> >>>>> wrote: >>>>> >>> The Github UI provides several options for merging a PR hidden behind >>>>> the “Merge pull request” button. Only the “Create a merge commit” option >>>>> does what most users expect, which is to merge by creating a new merge >>>>> commit. This is the option recommended in the Beam committer’s guide, but >>>>> it is not necessarily the default behavior of the merge button. >>>>> >>> A small cleanup PR I made was recently merged via the merge button >>>>> which generated a squash merge instead of a merge commit, breaking two >>>>> other PRs which were based on it. See >>>>> https://github.com/apache/beam/pull/4991 >>>>> >>> I would propose that we disable the options for both rebase and squash >>>>> merging via the Github UI. This will make the behavior of the merge button >>>>> unambiguous and consistent with our documentation, but will not prevent a >>>>> committer from performing these operations from the git cli if they desire. >>>>> >>> Andrew >>>>> > -- >>>>> > ---- >>>>> > Mingmin
