Where communities thrive


  • Join over 1.5M+ people
  • Join over 100K+ communities
  • Free without limits
  • Create your own community
People
Repo info
Activity
  • 06:23
    Server build 2224 deployed
  • Dec 06 18:38
    pkaminski commented #708
  • Dec 06 18:35
    pkaminski commented #709
  • Dec 06 10:22
    aedelstein closed #709
  • Dec 06 10:22
    aedelstein commented #709
  • Dec 06 10:02
    DamienCassou commented #708
  • Dec 06 10:01
    DamienCassou commented #708
  • Dec 06 09:28
    aedelstein opened #709
  • Dec 06 03:20
    Server build 2222 deployed
  • Dec 06 03:09
    Client build 3872 deployed
  • Dec 05 19:31
    pkaminski commented #708
  • Dec 05 11:24
    DamienCassou opened #708
  • Dec 05 03:18
    Client build 3870 deployed
  • Dec 03 21:19
    pkaminski labeled #707
  • Dec 03 20:08
    ibrahima edited #707
  • Dec 03 20:07
    ibrahima opened #707
  • Dec 03 18:58
    pkaminski commented #706
  • Dec 03 09:43
    misha-tectonic commented #706
  • Dec 03 09:40
    misha-tectonic commented #706
  • Dec 03 05:50
    pkaminski closed #706
Dan Halperin
@dhalperi
Or rather, is there a recommended keyboard flow for reviewing commit by commit? my usual is .x.x.x.x
Piotr Kaminski
@pkaminski
f will trigger the purple "show <whatever> diffs" button by default, might be what you're looking for.
Commit message will show up in the Changes dropdown. These are specific to the current range of revisions, but not the current file.
Dan Halperin
@dhalperi
Are you talking about this one?
Screen Shot 2019-11-06 at 12.28.29 PM.png
Piotr Kaminski
@pkaminski
Yes. But it's not showing commits because you have mixed diff bounds...
Dan Halperin
@dhalperi
Hmm. This is just "next unreviewed diffs"
How can you have mixed bounds for "base->r1"
Piotr Kaminski
@pkaminski
Good question.
Dan Halperin
@dhalperi
I hit f and now it's (base or r1) -> r2, and it looks like it's using base for files added in r2
Piotr Kaminski
@pkaminski
Are some files undiffed, ie have no diff bounds showing?
Dan Halperin
@dhalperi
Screen Shot 2019-11-06 at 12.31.01 PM.png
It actually looks like in commit-by-commit it's just r2?
Piotr Kaminski
@pkaminski
Just r2 doesn't make sense, since you'll have nil diffs.
If that's what got selected automatically then there's a bug in the bounds picking algorithm. :(
And there might me a bug in the algorithm that tries to figure out what the "global" diff bounds are as well.
Dan Halperin
@dhalperi
That's what happened when I hit f
Piotr Kaminski
@pkaminski
Wait, did f do something when the "show next diffs" button was disabled? It shouldn't.
Dan Halperin
@dhalperi
No, I had to mark all as reviewed first
Basically if I toggle between these two buttons it keeps advancing to (rN+1,rN+1)
Screen Shot 2019-11-06 at 12.34.59 PM.png
Screen Shot 2019-11-06 at 12.35.07 PM.png
"these two" == "mark as reviewed" <-> "next unreviewed diffs"
Piotr Kaminski
@pkaminski
Oh. I think that's OK, since those files have no changes at the given revision, so their diff should be nil. Does it pick more reasonable bounds for files that do have a change?
Dan Halperin
@dhalperi
Screen Shot 2019-11-06 at 12.36.53 PM.png
Looks like it does, but this keeps me from seeing the commit desc?
Piotr Kaminski
@pkaminski
Yes, likely. It's weird that there's no bounds at all for the first few files -- I would've expected the same treatment for all of them.
Dan Halperin
@dhalperi
The first few were all deleted
So maybe it only happens for files that still exist later?
Piotr Kaminski
@pkaminski
Yeah, that could make a difference. I think I just need to dig into that algorithm again and have it not emit nil bounds at all. That will show the same diffs but should fix the overall bounds aggregator.
Piotr Kaminski
@pkaminski
OK, I pushed the simplest fix I could think of but there might be unexpected side-effects. Can you give it a try? Also, I just realized that the list of commits in the dropdown won't show up until you have a file focused.
Dan Halperin
@dhalperi
I'll TAL
Looks like it working
Nice, thanks
We'll be starting another review cycle soon
Dan Halperin
@dhalperi
I don't know if it's just the size of the review or if there is actually a more fundamental problem, but some interactions seem to crash chrome. https://reviewable.io/reviews/batfish/batfish/5129#-
Piotr Kaminski
@pkaminski
That's certainly a very big PR and technically outside the scope of what Reviewable is meant to handle, but I played with it a bit and didn't run into any major difficulties. Can you narrow down "some interactions"?
Ibrahim Awwal
@ibrahima

@pkaminski

Bug filed: https://bugzilla.mozilla.org/show_bug.cgi?id=1591579

yay looks like this should be fixed in Firefox 72, which goes to beta next week: https://wiki.mozilla.org/Releases/Firefox_72

Dan Halperin
@dhalperi
Does github or reviewable handle diff generation? I'm guessing Reviewable, since it's much better than I've ever seen on GH?
If the latter, wondering if bug reports / enhancement requests are useful :)
Piotr Kaminski
@pkaminski
Reviewable does its own diffs, so yes, bug reports are useful. I thought there was an issue open to gather up all smaller enhancement requests for diffs but I can't find it at the moment...
Dan Halperin
@dhalperi
Screen Shot 2019-11-25 at 3.39.59 PM.png
Seems like indent detection behaves radically different if the line starts with punctuation
Piotr Kaminski
@pkaminski
That sent me down a rabbithole... I'll follow up when I manage to claw my way out.
Dan Halperin
@dhalperi
hahaha sorry :)
Piotr Kaminski
@pkaminski
OK, the good news is that the yak has been shaved and I have a repro. The bad news is that the repro is incredibly touchy, and the issue lies deep in the diff algorithm's "semantic cleanup" code -- i.e., un-grokkable heuristics that are very likely to break other scenarios if I change anything. I'll step through the code tomorrow and see if I get lucky and it's something straightforward where I can predict the side-effects... But probably not.
BTW, this algorithm is not content-sensitive AFAICT but it is extremely sensitive to the structure and length of raw surrounding deltas.
Dan Halperin
@dhalperi
Ack. Well, thanks for trying and I hope the journey was useful.
Piotr Kaminski
@pkaminski
@dhalperi OK, I tried tweaking the algorithm. Basically, what was happening is that short equal deltas that were flanked by longer insertion/deletion deltas were being collapsed into the insertion/deletion. I changed the code to ignore space-only insertions/deletions for the purposes of this computation. I believe it fixes the problem you brought up with hopefully minimal side-effects but I can't be sure. Please keep an eye out for weird regressions!
Dan Halperin
@dhalperi
@pkaminski Back to work now, will let you know. Thanks!