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

[Doerfert, Johannes via llvm-dev]

On 01/24, Renato Golin wrote:
> 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.

Arguably, having clean self contained commits is for this use case even
better. While you can cherry-pick N commits after N rounds of review it
is easier and more convenient to have a single one.

> 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 can see that a suitable model can be found with the last two, or a
combination of the three.


> 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.

Agreed.


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

I don't understand:
  - Single commits per feature should make it easier to find cause of
    breakage. What do I misunderstand?

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

I think most people are in agreement that those should be avoided if
possible, e.g., as part of a review, but are fine if they come alone as
NFC commits.


> > 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.

I agree with you completely. I was surprised and unhappy that this was
possible myself. If we move to GH this should be disabled if possible
and "shunned upon" if we cannot disable it. (To be fair, it might be the
case that only owners can do it so there is little risk it happens to
us.)

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 228 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20200124/6c33c4a9/attachment.sig>