CoC: allow maintainers to make 'value judgments' on correct patches

[tobtoht]

Discussion: #9738 (comment)

[jeffro256]

To play devil's advocate, "value judgements" without sufficient testing are what allowed a PR to be merged into the release branch that nuked the entire blockchain file in Windows: #9804. It would've been caught with a modicum of actual testing

[iamamyth]

To play devil's advocate, "value judgements" without sufficient testing are what allowed a PR to be merged into the release branch that nuked the entire blockchain file in Windows: #9804. It would've been caught with a modicum of actual testing

You have it backwards: That PR (#9664) had three sign-offs; the value-judgment rule allowed a swift patch for it to be merged.

[tobtoht]

@jeffro256 I'm not sure I understand the point you're trying to make.

[tobtoht]

I don't see how this rule relates to either the PR that introduced the problem or the PR that fixed it.

[iamamyth]

@tobtoht Technically you're right because the patch had two sign-offs and fixed the issue, so no value judgment involved. I merely wanted to highlight that the three-hour turnaround on the patch was orders of magnitude faster than the original PR, and, had reviewers been unavailable, the fast turnaround would have been beneficial. In other words: This patch was the type of scenario where a value judgment could have been beneficial, though it proved unnecessary (e.g. if it had been harder to fix, and less catastrophic, a revert would be a value judgment as to which buggy behavior to prefer).

[tobtoht]

What exactly is a 'value judgment'? I feel like our understanding differs in an unexpected way. I'm thinking of something like a comment on the usefulness / necessity of a patch. The reason why I proposed the removal of this rule is because I don't see why maintainers wouldn't be allowed to make such comments in good faith. Nothing to do with sign-offs or how fast something was merged.

[iamamyth]

A value judgment can include many things, but, the relevant category here would be prioritizing one use case over another. If, for example, the windows compression PR had a bug with a more limited scope, reverting it would break things for some users and fix the behavior for others; which one you pick proves a judgment as to the value of each use case.

[iamamyth]

I expect this is type of value judgment you have in mind: A core contributor objects to something, even though it "works", because of a maintainability concern, etc, so it doesn't get merged. That scenario matches the word "correct" in the CoC, and what I imagine was the intent behind the CoC rule. But, correctness tends to be murky. For example, if you revert a broken commit which fixed some other breakage, is the revert "correct"? The software still doesn't work, just in a different way.

[tobtoht]

The original rule (from C4) is: "Maintainers SHALL NOT make value judgments on correct patches.".

The rule was changed (before the CoC was introduced) to what it is now after mooo commented "Objectionable".

As I mentioned in the discussion preceding this PR, it isn't clear to me who the "core code developers" are. Is this the Monero Core Team or any Contributor? This should be clarified if the rule is kept.

I'm still unsure what this rule means in practice. Review by multiple contributors precedes all merges. And I don't get to decide which PRs are eligible for merging.

[iamamyth]

I don't find the original C4 rule helpful, and I therefore also disagree with the present language. Correctness doesn't imply soundness, and soundness requires a value judgment. Just because something does what it claims doesn't make it a good idea to include.

[iamamyth]

A better recent example: Adding connection control features to the daemon (#9765). One could argue, based on the unix philosophy of "do one thing and do it well", that a firewall (or similar purpose-built tool) should tackle this task for network-based software, rather than baking it into every program. I happen to agree with that argument, but I didn't mention it, not owing to the CoC rule, but a cost-benefit analysis. Trying to short-circuit such analyses with a blanket "if someone built it, now we must merge and maintain it" rule makes for bad software (to be clear, this is not intended as a criticism of the PR in question).

[jeffro256]

@jeffro256 I'm not sure I understand the point you're trying to make.

Sorry, I misunderstood what the CoC meant by "value judgements" here. I skimmed it and misinterpreted it as something along the lines of "an eyeballing of a patch for correctness based on personal judgement MUST NOT be the only criteria". You can ignore my comment.