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

[Nicolai Hähnle via llvm-dev]

On Fri, Jan 24, 2020 at 7:22 PM Doerfert, Johannes <jdoerfert at anl.gov> wrote:
> 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.

Agreed.


> > 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 find it much more important to have a clean history and would
therefore frown upon the first option; keeping the fixup commits
around is almost never a good idea.

One of the most important goals of history management should be to
have a useful `git bisect`. This requires that every individual commit
leaves the codebase in a "clean" state. (Ideally we'd have bots that
check each individual commit, but that's a resourcing problem.)


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

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.

I don't quite follow. Yes, gratuitous useless changes tend to create
merge and rebase problems and should generally be avoided. Why does it
make a difference whether they're in multiple commits though?

I agree that it's good practice to have such reorderings etc. in
separate, NFC commits. That's actually one of the reasons why I think
a good flow for patch-series-centric review is so important, because
it facilitates this good practice.

Cheers,
Nicolai


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



-- 
Lerne, wie die Welt wirklich ist,
aber vergiss niemals, wie sie sein sollte.