[lldb-dev] [cfe-dev] [RFC] Deprecate pre-commit email code reviews in favor of Phabricator

[David Blaikie via lldb-dev]

On Mon, May 17, 2021 at 11:12 AM Krzysztof Parzyszek via cfe-dev <
cfe-dev at lists.llvm.org> wrote:

> This is a revision of the previous RFC[1].  This RFC limits the scope to
> pre-commit reviews only.
>
>
>
> *Statement:*
>
> Our current code review policy states[2]:
>
> “Code reviews are conducted, in order of preference, on our web-based
> code-review tool (see Code Reviews with Phabricator), by email on the
> relevant project’s commit mailing list, on the project’s development list,
> or on the bug tracker.”
>
> This proposal is to limit pre-commit code reviews only to Phabricator.
> This would apply to all projects in the LLVM monorepo.  With the change in
> effect, the amended policy would read:
>
> “Pre-commit code reviews are conducted on our web-based code-review tool
> (see Code Reviews with Phabricator).
>
I'm with you here ^, this seems to document/formalize existing practice -
though does this accurately reflect all the projects in the mororepo? I get
the impression that mlir, maybe flang, etc might be doing reviews
differently?

> Post-commit reviews are conducted, in order of preference, on Phabricator,
>
This still seems like a change in practice that I'm not in favor of,
personally - due to the current divergence between email and phab review
feedback. Yes, this would be one way to unify it - but I'm not sure it's
necessarily the best one.

I'd suggest leaving this to a separate proposal so as not to
complicate/muddy the waters of the formalization of pre-commit review
practice.

> by email on the relevant project’s commit mailing list, on the project’s
> development list, or on the bug tracker.”
>
>
>
> *Current situation:*
>
>    1. In a recent llvm-dev thread[3], Christian Kühnel pointed out that
>    pre-commit code reviews rarely originate via an email (most are started on
>    Phabricator), although, as others pointed out, email responses to an
>    ongoing review are not uncommon.  (That thread also contains examples of
>    mishaps related to the email-Phabricator interactions, or email handling
>    itself.)
>    2. We have Phabricator patches that automatically apply email comments
>    to the Phabricator reviews, although reportedly this functionality is not
>    fully reliable[4,5].  This can cause review comments to be lost in the
>    email traffic.
>
>
>
> *Benefits:*
>
>    1. Single way of doing pre-commit code reviews: these code reviews are
>    a key part of the development process, and having one way of performing
>    them would make the process clearer and unambiguous.
>    2. Review authors and reviewers would only need to monitor one source
>    of comments without the fear that a review comment may end up overlooked.
>    3. This change simply codifies an existing practice.
>
>
>
> *Concerns:*
>
>    1. Because of the larger variety, email clients may offer better
>    accessibility options than web browsers.
>
>
>
>
>
> [1] https://lists.llvm.org/pipermail/llvm-dev/2021-May/150344.html
>
> [2]
> https://llvm.org/docs/CodeReview.html#what-tools-are-used-for-code-review
>
> [3] https://lists.llvm.org/pipermail/llvm-dev/2021-April/150129.html
>
> [4] https://lists.llvm.org/pipermail/llvm-dev/2021-April/150136.html
>
> [5] https://lists.llvm.org/pipermail/llvm-dev/2021-April/150139.html
>
>
>
>
>
> --
>
> Krzysztof Parzyszek  kparzysz at quicinc.com   AI tools development
>
>
> _______________________________________________
> cfe-dev mailing list
> cfe-dev at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/lldb-dev/attachments/20210517/f03f85e1/attachment-0001.html>