Hi, On 5 December 2016 at 09:46, René J.V. Bertin wrote: > Hi, > > This has come up on the QtCurve PR and I cannot seem to find an explicit > answer in the wiki (https://trac.macports.org/wiki/WorkingWithGit#pr) > > If a pull request has seen some evolution and thus commits due to the review > process and/or somehow related reasons (for instance, because the process > dragged on), should the commit history be squashed or not?
We discussed this quite a bit (I'm not sure where) and the conclusion was that: - we want a linear history (therefore we do rebasing) - we want to avoid "weird commits" (so yes, we want to squash the commits) BUT: multiple commits are still sometimes desired / OK. Usually "larryv" is the one who takes most care to split commits into "reasonable atomic commits". A typical example would be when you fix whitespaces in the first commit, fix typos in the second commit and add a variant in the third commit. Or when you fix a port in the first commit and then revbump dependent ports in the second commit. Why do we want to squash? Imagine a complete newbie that submits a pull request and accidentally modifies two other ports and makes some other stupid mistakes which might even break MacPorts (if the code of a Portfile doesn't compile, portindex fails etc.). Then those changes are gradually reversed and the ports gets improved in 20 commits (many of those commits with screwed up commit messages) until an acceptable state is reached. When maintainers of those ports that were accidentally modified would later browse through the history and potentially do a bisection to find when some bug was introduced, those additional unrelated commits would be pretty annoying. With the old workflow we would also keep cleaning up the code until it was ready and then a committer would make a single commit or two to represent potential months of brainstorming. I'm not saying that GitHub makes it easy to do the squashing (it has to be done manually in the command line), but well ... > I would expect not, certainly not if squashing to a single atomic commit > means that whatever remains of the PR ticket after merging will no longer > allow to see the evolution of the commit, and if that evolution is deemed to > be of interest. Most other projects I know that accept patches through review > use a separate review mechanism or simply a bug tracker, and then include a > reference to the review request and/or bug ticket in the final, atomic commit > to the repository. You may rewrite history to include *some* evolution, but not all of it and certainly not all the accidental errors that had to be fixed later. You may include addition of new subports or introduction of new variants as separate commits. The PR still keeps quite some history (I'm unable to tell you how much exactly). I guess that "larryv" is our biggest expert in this topic. > BTW, does a PR remain accessible via github.com after the source branch from > which the pull was made has been deleted? I don't have a scientific answer yet, but it does to quite some extent (probably not 100% of it). In many PRs you can see code blocks that say "outdated" where you can click to see the source code of old / rewritten commits. (You can probably try to open a PR to your own repository and test.) Mojca
