"Kapade, Mrunal" <mrunal.kap...@intel.com> writes: > Hi, > > Recently, Github enabled much needed squash and merge functionality > for Pull Requests[1]. This leaves the decision to squash commits or > not upto the user. That way you can see the history of all the > intermediate changes which led to the final commit. > I think we should all stop squashing commits while the PR is still > under review. This should be similar to what we are accustomed to in > Chromium's code review tool. > > What do you think? > > Mrunal > [1] https://github.com/blog/2141-squash-your-commits
I don't think the comparison to Chromium's code review tool is fair, as in Rietveld one CL must correspond to exactly one commit (there is no way for one to associate several commits to one CL at all) whereas with GitHub that limitation does not exist. One could even say that with Rietveld one _must_ squash all changes when sending or updating a CL. With that said, I decided to give the squash option a try with a test repository as I had only read about it and did not know how certain issues would be handled: would it create a merge commit + a squashed commit or just add a squashed patch on top of HEAD (it does the latter)? What would the commit message look like (by default, a concatenation of all commit messages)? How could we determine the pull request a change came from (the number is included in the commit title)? For a repository handling contributions from many different people such as Crosswalk, at the very least we need to opt for one style or another IMO. If we decide that both are valid and can be used at each contributor's discretion, we can end up with A) merge commits with several "fix previous commit" changes that should not have been committed B) single squashed commits of things that should have been committed separately (e.g. pull request #3645). And if I were to choose between squashing or merging, I would stay with merging: - I really like it that GitHub can use git better than Rietveld (which was conceived at a time when git was not so widespread) including allowing separate commits for things that should be committed separately (like the PR I mentioned above). Squashing inevitably creates one big patch with everything. - This is a problem more specific to our own workflow: it puts a higher burden on the person clicking the green button on the pull request page to squash the commits because they need to choose the final commit message. By default it's going to consist of all the commit messages concatenated, meaning that we will end up with commit messages saying "fix previous mistake", "now fix it for real" etc unless the person merging the PR takes the time to either write their own commit message, or copy-paste the pull request message etc. _______________________________________________ Crosswalk-dev mailing list Crosswalk-dev@lists.crosswalk-project.org https://lists.crosswalk-project.org/mailman/listinfo/crosswalk-dev
