PR final steps (to squash or not to squash)

[Mojca Miklavec]

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