[llvm] r260488 - [readobj] Handle ELF files with no section table or with no program headers.

[Chris Lattner via llvm-commits]

On Feb 16, 2016, at 12:29 PM, Michael Spencer <bigcheesegs at gmail.com> wrote:
> Per developer policy: Commits that violate these quality standards
> (e.g. are very broken) may be reverted. This is necessary when the
> change blocks other developers from making progress.
> 
> Reverting is reserved for very broken commits that block progress.
> This was not a very broken commit, and it did not block progress. What
> has blocked progress is you reverting the commit and having to argue
> with you about it.


I don’t know the details of what is going on here, but I don’t think either of your perspectives is “obviously" right: the ideal is somewhere in between.  The intent of the LLVM development approach is to optimize for the case where patches are uncontentious and “obviously correct”, but to make room for mistakes.

Patch review would be meaningless if no problems/improvements were ever found, and as such, this necessarily means that problems/improvements will be found after the patch goes in.  The developer policies specifically allow for either to problem to be fixed in place, or for the patch to be reverted and a better fix committed on top of it.  Which path is taken is something of a judgement call, which is why I don’t think either of you is “obviously” right.

The essential part of living with this model is to realize that the reversion of a patch is something that *will* happen, and shouldn’t be taken personally.  When I personally drop in patches that break things, or are contentious, I’m generally happy to have someone revert it and sort it out with them later - even if it means that I end up committing exactly the same patch again.

On Feb 16, 2016, at 12:39 PM, Rafael Espíndola <rafael.espindola at gmail.com> wrote:
> It *reverted* work that I had done, so you reverted code first. I got
> it back to what it was. So yes, it was blocking progress. It was
> blocking making sure we still had the features we had before you
> decided you could just delete tests.

I see what you’re saying Rafael, but this isn’t the spirit of what the dev policy is talking about.  Reversion is talking about a specific patch in question, not about removing or changing code.   I don’t think anyone would reasonably consider Michael’s patch to be a patch reversion in the sense that the dev policy means.

-Chris