[PATCH] D89995: Make the post-commit review expectations more explicit with respect to revert

[Mehdi AMINI via Phabricator via llvm-commits]

mehdi_amini added inline comments.


================
Comment at: llvm/docs/CodeReview.rst:49-51
+author, unless they are unresponsive). Developers often disagree, and erring on
+the side of the developer asking for more review prevents any lingering
+disagreement over code in the tree. This does not indicate any fault from the
----------------
sivachandra wrote:
> The wording seems to imply that this would apply even for opinion driven disagreements. I would prefer a wording which implies something more concrete. Also, in a way, it seems like the post-commit review practice is in conflict with the wording here. As in, if concerns are raised post-commit, then the post-commit review practice requires the original author to address those concerns even post-commit. I would think a revert will only happen if the original author is not acting on/addressing the concerns. Even in such cases, the revert should be justified. An example justifiable reason can be that //progress// cannot be made with out a revert.
> 
> There are of course other obvious reasons to revert, but I am assuming that this discussion is not about those reasons.
> The wording seems to imply that this would apply even for opinion driven disagreement

How to you handle this in pre-commit review? Feel free to suggest an alternative wording, but keep in mind the spirit (see below).

In general the mindset is that there shouldn't be a difference whether I express my opposition to a patch 10min before you want to push it or 10 min after you pushed it.

>  As in, if concerns are raised post-commit, then the post-commit review practice requires the original author to address those concerns even post-commit. 

This is a case-by-case: if someone has the kind of concerns that would block a patch from landing (like design concerns), revert is the right approach I believe.

> Even in such cases, the revert should be justified. An example justifiable reason can be that progress cannot be made with out a revert.

There is an obvious justification: the patch should not have been landed. A practical concern is that revert become harder quickly as other part of the codebase start to build on top of the landed change.
In the extreme what you're describing would justify that I could land anything and we would not revert it unless it is breaking or blocking someone else. This does not seem consistent with the rest of this doc, neither with the actual practices.





================
Comment at: llvm/docs/CodeReview.rst:57
+
+Before being recommitted, the patch generally undergoes further review,
+including by the community member who identified the problem and, in cases
----------------
rengolin wrote:
> I'd say "should undergo". The way it's written sounds like something that "happens" not something that "should happen".
Will update! (Note that this is carried over from the existing doc, I didn't touch it other than reflow)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D89995/new/

https://reviews.llvm.org/D89995