[llvm-dev] [cfe-dev] Phabricator -> GitHub PRs?

[Renato Golin via llvm-dev]

On Fri, 24 Jan 2020 at 17:11, Doerfert, Johannes <jdoerfert at anl.gov> wrote:
> As I understand it, the GH model is to amend a new commit to your PR
> which addresses the review comments. The "problem" is that "we" are used
> to the force push model in which each commit is always as "clean and self
> contained" as possible (this is not only because of Phab I'd argue).

With Git, *adding* commits to a pull request that address the comments
is good practice.

This does not change the the original commits, and in the web UI,
doesn't disturb the comments.

New comments can be added, and fixups can be approved with a comment:
"commit X is good, wait for the series to be approved to push".

Better still, it's very easy for anyone (with git) to checkout,
cherry-pick, rebase and try things locally during code review.

In the end, when the series is approved, there are a number of options
before pushing:
* You can merge as is (works if the author had good commit messages
for all fixups)
* You can squash-and-merge, which will add all commit messages to the
one patch (merger can edit)
* You can request the author to squash the fixups on the original commits

I prefer the last one. It's more work, but it lands "as the author
intended" because it gives them more time to rework local history.

Single commits are harder to find cause of breakages and the commit
message may lose info if the merger is not the author.

Multiple useless commits (fix typo, whitespace, reordering
switch-case) can create rebase problems for everyone else.

> The first experience I had with GH, someone modified my pull request
> through the web interface. (There is a way to disallow this but I didn't
> know.) Then I force pushed my updates to my private branch and the
> person was angry that I undid all the work. We did manage to recover it
> but it is not easily accessible as far as I know.

Editing on the web is a horrendous practice and I was very cross when
someone did that on my patch before merging, because they got it wrong
and I had to do another fix.

Patch review is intended as quality control. They're not inalienable
right to do whatever you want it someone else's work. I find the
practice deeply disrespectful.

So, if there's a way to disable that, I'm strongly in favour of doing so.

--renato