Where communities thrive


  • Join over 1.5M+ people
  • Join over 100K+ communities
  • Free without limits
  • Create your own community
People
Repo info
Activity
  • Apr 15 15:03
    Client build 4731 deployed
  • Apr 15 14:09
    Server build 2999 deployed
  • Apr 15 06:44
    pkaminski closed #690
  • Apr 15 06:44
    pkaminski commented #690
  • Apr 15 03:05
    Client build 4729 deployed
  • Apr 14 17:13
    Client build 4728 deployed
  • Apr 14 17:08
    Server build 2997 deployed
  • Apr 14 16:35
    Client build 4727 deployed
  • Apr 14 14:36
    Server build 2996 deployed
  • Apr 14 14:25
    Client build 4726 deployed
  • Apr 12 23:06
    anthonykawa assigned #825
  • Apr 12 11:52
    Server build 2990 deployed
  • Apr 12 11:43
    Client build 4719 deployed
  • Apr 08 11:56
    Server build 2988 deployed
  • Apr 07 15:53
    pkaminski edited #798
  • Apr 07 15:53
    pkaminski commented #798
  • Apr 07 15:52
    pkaminski closed #804
  • Apr 06 03:08
    pkaminski commented #507
  • Apr 06 03:04
    pkaminski labeled #827
  • Apr 06 03:04
    pkaminski opened #827
Piotr Kaminski
@pkaminski
The API begs to differ:
curl -H "Authorization: <redacted>" https://api.github.com/repos/ridge/tectonic/commits/f2df7cf17095eb6637efa1b6e581b7cf897f1ec0/status
gives:
{
  "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?
Alexey Feldgendler
@feldgendler
Yes, d3d701b597e1bb866c91941c73c53a1b9887f5b3
Piotr Kaminski
@pkaminski
We folded some commits with no changes together and now we're updating the wrong commit. <facepalm> I understand the bug now, we'll get it sorted out ASAP. Thanks!
Alexey Feldgendler
@feldgendler
Thanks a lot! Can I force the PR to merge now?
You don't need it anymore?
Piotr Kaminski
@pkaminski
Absolutely, I'm certain we can reproduce this.
Alexey Feldgendler
@feldgendler
Great, thanks for the quick response, as always!
Misha Gusarov
@dottedmag
@pkaminski Hello Piotr. Is the problem Alexey Feldgendler reported several days ago (missing GH status on a branch with collapsed commits) fixed? I see it again on a different PR.
This time ridge/tectonic#9885 (yes, the number is different, last time it was 9855), branch is on bd4391b7463227f0c175d85ed42af7bb02841676
Alexey Feldgendler
@feldgendler
@pkaminski Hi! We have a PR where an actual change made by the PR author is incorrectly identified as “base commit changes” and hidden.
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.
Piotr Kaminski
@pkaminski
@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.
Misha Gusarov
@dottedmag
@pkaminski Thanks!
Piotr Kaminski
@pkaminski
@feldgendler There's something else unusual in that PR: the merge commit 1146c4e is merging (some other version of?) the branch into itself? I wonder if that confused things. Is this something your team does pretty often and it worked fine in the past?
Piotr Kaminski
@pkaminski
@feldgendler I assume the file we're talking about is moonstone/api/moonstone.yaml. Is it OK if I download the contents of that file to my machine so I can reproduce and debug the issue? (I need to ask per Reviewable's terms of use.) Thanks!
Alexey Feldgendler
@feldgendler
@pkaminski Indeed, there is a merge commit between two points on the same branch. I have no idea how that happened, or what they had in mind while doing it. But it happened before the line removal in question, and didn't affect moonstone.yaml.
@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.
Piotr Kaminski
@pkaminski
I can download the file: all your tokens are belong to us! 😁 Thanks, I'll keep digging.
Piotr Kaminski
@pkaminski
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...
Piotr Kaminski
@pkaminski
@feldgendler Attempted fix deployed and retroactive. Please give it a try and let me know if it addressed your issue, at least. I've had to upgrade the logic from a 3-way to a 4-way diff with line mapping galore but I think it makes more sense now.
Alexey Feldgendler
@feldgendler
@pkaminski Thanks a lot! The diff is now shown correctly in that affected PR. I'll be on the lookout for regressions, hope there won't be any!
Chris Giroux
@cgiroux86
@feldgendler @dottedmag The fix for a missing GH status on a branch with collapsed commits is now live. The code is relatively tricky and it's difficult to anticipate every possible state, so please let us know if you experience any further issues. Thanks :smile:
Alexey Feldgendler
@feldgendler
@cgiroux86 Thank you very much!
@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.
Piotr Kaminski
@pkaminski
I suspect the other person is just repeatedly acknowledging the discussion which puts it back in your court. I don't believe that a new revision should do it but I might be wrong there. Are you able to ask the other person what, if anything, he's doing with the discussion?
Alexey Feldgendler
@feldgendler
@pkaminski He says he doesn't remember. He might have acknowledged it.
But even if he did, why would that matter? A reviewer is not satisfied => it should keep being the author's problem.
Piotr Kaminski
@pkaminski
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".
Alexey Feldgendler
@feldgendler
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”?)
Alexey Feldgendler
@feldgendler
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).
Piotr Kaminski
@pkaminski
Looking at the rules table I think we're falling through to rule 6: https://docs.reviewable.io/discussions.html#unreplied-discussions
Alexey Feldgendler
@feldgendler
Looking at those rules always made my head hurt… Alright, rule 6. What's the rationale behind it?
Piotr Kaminski
@pkaminski
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).
Alexey Feldgendler
@feldgendler
If none of the rules 1–5 is triggered, the thread is declared to be the blocker's problem. Why not the author's problem instead?
Piotr Kaminski
@pkaminski
The rationale is "we're out of likely suspects, let's just pick whoever's blocking -- hopefully we never reach this state". :)
Alexey Feldgendler
@feldgendler
So this is a typical use case: the reviewer doesn't accept the fix, demanding it be fixed differently.
Piotr Kaminski
@pkaminski
Well, it's atypical in that the other participant is refusing to engage and has marked themselves satisfied.
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.
Alexey Feldgendler
@feldgendler
They have marked themselves satisfied earlier, when they committed their first fix and clicked “Done”. They are satisfied ever since.
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.
Piotr Kaminski
@pkaminski
(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.
Alexey Feldgendler
@feldgendler
@pkaminski I haven't considered dismissing. Will they be able to participate in the discussion after that? What I expect to happen is that they either push a new fix and say “Done” (becoming Satisfied) or argue why their original fix is right.
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.