Squashing sounds reasonable to me, although I don’t really mind the rebasing problems. Individual commits should be small and well isolated, so I don’t think it’s going to be a big problem in practice. I think you have overstated many of the issues:
Resolving conflicts with rebasing is a lot more difficult if there are multiple conflicting commits and/or if the conflicting upstream commit would have required substantial changes in the feature branch from the beginning. This is a bad situation in general, but I find that it’s easier to rebase because it requires fixing individual commits that are smaller. And because those commits were already reviewed, they should have test cases to verify the rebase changes. Rebasing leads to rewriting history, which goes against the every change is reviewed on the feature branch approach. Again, reviews should validate tests that can be used here. The squashing approach that we’ve used for a long time relies on the same principle. Rebasing the feature branch leads to loss of review comments on existing and merged pull requests, since those are associated with specific git hashes. Comments should be attached to PRs, not hashes. The feature branch can only be rebased by a committer. Anyone can create a rebased branch, it’s just committers that can update the feature branch in the Apache repository. I think that’s reasonable. On Thu, Oct 18, 2018 at 8:47 AM Zoltan Ivanfi <z...@cloudera.com.invalid> wrote: > Dear Parquet Developers, > > On yesterday's Parquet Sync we discussed merging vs. rebasing vs. > squashing again. (Please note that I use the word "merge" both in the > sense of incorporating a change or feature branch in the main branch > as well as one specific way of doing so: using merge commits. This > unfortunate overloading of "merge" comes from github's terminology.) > > Ryan presented a new argument against merging that we haven't > considered before: reverting behaves strangely with merge commits. > Combined with the chaotic way in which git and github present merges > in their log/history views, this makes merging much less compelling > for handling feature branches. > > We considered rebasing as well, but thinking further about it after > the sync we came to the conclusion that it is not feasible for feature > branch development due to several reasons: > > - Resolving conflicts with rebasing is a lot more difficult if there > are multiple conflicting commits and/or if the conflicting upstream > commit would have required substantial changes in the feature branch > from the beginning. > - Rebasing leads to rewriting history, which goes against the every > change is reviewed on the feature branch approach. > - Rebasing the feature branch leads to loss of review comments on > existing and merged pull requests, since those are associated with > specific git hashes. > - The feature branch can only be rebased by a committer. > > Based on these considerations, we deemed both merging and rebasing > unfeasible for being used as a feature branch merging strategy. As a > compromise, we settled on Ryan's original suggestion: squashing. This > is also the regular way of merging individual pull requests that are > not developed on feature branches. This practice can easily be adapted > to feature branches as follows: > > - The feature branch is handled similar to the master branch: > developers open pull requests, committers review, approve and merge > them by squashing each pull request into a single commit on the > feature branch. > - The feature branch developers may occasionally merge the master > branch into the feature branch. (This needs some extra manual work > from the committer, since merges are disabled on the github UI on > purpose.) > - When the feature branch is ready, its developers open a pull request > for merging it into master. Committers should merge such a pull > request by squashing it but mentioning the feature branch name and > keeping the list of commits in the commit message at the same time. > This allows interested parties to trace back the history of affected > code lines by consulting the feature branch history and/or pull > request reviews. > > You can see an example of such a commit (the first one and as such the > only one so far) here: > > https://github.com/apache/parquet-mr/commit/e7db9e20f52c925a207ea62d6dda6dc4e870294e > > Please let us know any concerns or suggestions you may have to improve > this practice even further. > > Thanks, > > Zoltan > -- Ryan Blue Software Engineer Netflix
