The API begs to differ:
gives:
curl -H "Authorization: <redacted>" https://api.github.com/repos/ridge/tectonic/commits/f2df7cf17095eb6637efa1b6e581b7cf897f1ec0/statusgives:
{
"state": "success",
"statuses": [
{
"url": "https://api.github.com/repos/ridge/tectonic/statuses/f2df7cf17095eb6637efa1b6e581b7cf897f1ec0",
"avatar_url": "https://avatars.githubusercontent.com/oa/128315?v=4",
"id": 12526718959,
"node_id": "MDEzOlN0YXR1c0NvbnRleHQxMjUyNjcxODk1OQ==",
"state": "success",
"description": "all files reviewed",
"target_url": "https://reviewable.io/reviews/ridge/tectonic/9855",
"context": "code-review/reviewable",
"created_at": "2021-03-18T16:09:27Z",
"updated_at": "2021-03-18T16:09:27Z"
}
],
Perhaps that's not the right commit SHA?
Ooh, wait, I bet I know what's happening. Is the branch at
d3d701b597e1bb866c91941c73c53a1b9887f5b3?
You don't need it anymore?
This time ridge/tectonic#9885 (yes, the number is different, last time it was 9855), branch is on bd4391b7463227f0c175d85ed42af7bb02841676
In https://reviewable.io/reviews/ridge/tectonic/9742, a line was removed from
moonstone/api/moonstone.yaml by the author in commit d0f2a5c72c439be3abc2706c48ab326049c91e2a, which Reviewable has grouped as r11 together with two merges. The diff between r10 and r11 for that file is fully collapsed, saying “COLLAPSED 1705 lines with base commit changes and resolved discussions”. Clicking the “base commit changes” expands the diff, showing the line removal.
@dottedmag Not fixed yet, still putting the finishing touches on it. Once we started pulling on the thread it revealed more shortcomings in the code so we're addressing all of them together. We'll likely push the fix candidate in a few hours, or tomorrow at the latest though.
@feldgendler Ooh, that's bad. I'm guessing it's because of the weird grouping with the merges but let me dig around a bit.
@pkaminski You won't be able to download the file from GitHub because it's in a private repository. The file itself is not top secret, I can share it with you.
I believe you're correct and the somewhat complicated commit history is irrelevant; this is a fundamental flaw in base change elision when lines are added/removed (rather than modified) and the base has changed in unrelated ways. I'm trying to figure out a fix but this logic always makes my head hurt...
@pkaminski As a reviewer, I posted a blocking comment. The PR author fixed the bug and said “Done”, which changed his status to Satisfied. I looked at the fix and disagreed that it doesn't fix the problem, so instead of clicking “Resolve” on the discussion, I posted a further comment explaining what's wrong. Now his status is Satisfied, my status is Blocking, but for some reason Reviewable brings that discussion to my attention every now and then (I think it happens whenever a new revision is added to the PR), even though the last message is mine, and the ball should be in the author's court. Posting one more message like “Still not fixed” flips the table, but only temporarily.
The thread is https://reviewable.io/reviews/ridge/tectonic/9815#-MW08QapERKApvn3CANy if you're interested.
But even if he did, why would that matter? A reviewer is not satisfied => it should keep being the author's problem.
Hmm, I'm not sure. First, I don't think it matters whether the other person is the author or not -- once there are 2+ participants in the discussion they're pretty much equal. If the satisfied participant has no further comment, then acknowledging basically says "I see your comment and that you're not happy, but I have nothing further to say; back to you".
Posting a blocking comment in the first place does make it the author's problem. The author can't just brush it off “I've got nothing to say”. They have to fix it or make a point why it shouldn't be fixed. But if the reviewer isn't happy with the fix, why should it be different?
Currently, I work around this by posting a new comment criticising the fix instead of continuing the discussion. But resolving the old discussion collapses it, so the context is lost in the new thread.
In a two-way conversation with a blocking comment, it's pretty natural that one has to say something in order to “flip the table”; one shouldn't be able to do it silently. (What would that mean? “I hear you but I ignore you”?)
Three-way conversations are trickier because the order of messages is not necessarily A-B-A-B. But still, the participants of a three-way conversation aren't equal. The PR author has a special role: they are the one targeted by the criticism, and it's them, not someone else, who has to do something about it (make fixes and/or defend their solution).
Looking at the rules table I think we're falling through to rule 6: https://docs.reviewable.io/discussions.html#unreplied-discussions
I don't believe the author should have special billing, other than when initiating a discussion. The roles people play in discussions are just too flexible, and who's the "author" can change through the course of a PR (and right now Reviewable has no good way of recognizing that change).
Let me think about this a bit, but right now I'm really not convinced that singling out the author is a good way to go. I think Reviewable/Reviewable#820 might help too, though it's not a full solution.
So it's not like the author ignores my by setting himself to satisfied. He's already satisfied — the problem is that I'm not.
(Sorry, I'm very distracted with other stuff at the moment.) I think this is an instance of the more general problem I've noticed, which is "the other participant has the wrong disposition, but I'm the one being hurt by it". reviewable/reviewable#820 was my best idea so far on how to deal with it without chasing special cases all over the place but it definitely has limitations, in part because I don't want to let one person set/reset another person's disposition. But perhaps that is indeed the correct answer in this case -- maybe you should try dismissing the author? This will revert their disposition from Satisfied to Dismissed and should activate rule 2.
Also, whose attention will the discussion require right after I dismiss the author from it? My expectation is that it requires the author's attention but not mine.
#820 is a great idea, and I've run into use cases for it many times. I just don't understand how I would use it here.