"Failed to validate commented lines" after rebase

[mbitsnbites]

After rebasing (new upstream), any attempt to add an issue to a commit (older than the rebase in this case) will result in a dialog saying "Failed to validate commented lines" (this happens as soon as you release the mouse button after marking source code lines).

The admin mail reads (some stuff cut away):

Subject: wsgi[validatecommentchain]: IndexError: list index out of range

Data: {
  "origin": "new",
  "count": 1,
  "review_id": 37,
  "parent_id": 1100,
  "file_id": 80,
  "offset": 167,
  "child_id": 1108
}

Traceback (most recent call last):
  File "/usr/share/critic/operation/__init__.py", line 100, in __call__
    return self.process(db, user, **value)
  File "/usr/share/critic/operation/createcomment.py", line 36, in process
    verdict, data = validateCommentChain(db, review, origin, parent_id, child_id, file_id, offset, count)
  File "/usr/share/critic/reviewing/comment/__init__.py", line 509, in validateCommentChain
    addressed_by = propagation.addressed_by[0]
IndexError: list index out of range

Note, this has not always been the case, but it happens more frequently lately on our setup.

[mbitsnbites]

A simple workaround that seems to work (haven't tested it much, but it gets past the "Failed..." dialog at least), but I suppose that the problem goes deeper than this:

diff --git a/src/reviewing/comment/__init__.py b/src/reviewing/comment/__init__.py
index 8b618bc..170c0fa 100644
--- a/src/reviewing/comment/__init__.py
+++ b/src/reviewing/comment/__init__.py
@@ -506,11 +506,15 @@ def validateCommentChain(db, review, origin, parent_id, child_id, file_id, offse
         else:
             return "clean", {}
     else:
-        addressed_by = propagation.addressed_by[0]
+        # HACK: Workaround for https://github.com/jensl/critic/issues/89
+        if len(propagation.addressed_by) >= 1:
+            addressed_by = propagation.addressed_by[0]
 
-        return "modified", { "parent_sha1": addressed_by.parent.sha1,
-                             "child_sha1": addressed_by.child.sha1,
-                             "offset": addressed_by.location.first_line }
+            return "modified", { "parent_sha1": addressed_by.parent.sha1,
+                                 "child_sha1": addressed_by.child.sha1,
+                                 "offset": addressed_by.location.first_line }
+        else:
+            return "clean", {}
 
 def propagateCommentChains(db, user, review, commits, replayed_rebases={}):
     import reviewing.comment.propagate

[jensl]

With that workaround (which is fine as a workaround, though the situation isn't supposed to occur, of course,) does actually creating the issue work? The same kind of propagation with similar expectations runs when you actually create an issue.

[mbitsnbites]

Yes, it actually does. Got a "Updated review" message from Critic after submitting, will try to address the issue too...

[mbitsnbites]

...and the issues got addressed by a fixup commit too.

[jensl]

Interesting. What kind of rebase was it? Looking at the code, I can imagine this sort of thing could be a problem if you have the branch A -> B -> C, where C is a revert of B, and then do a history rewrite leaving only A.

[mbitsnbites]

The problem occurred after a new upstream rebase (clean, no dropped or tampered commits) - no history rewrite.

One thing that's a bit special with our setup (I'm not sure if this has anything to do with it) is that I've configured (or hacked, rather) Critic to poll origin every two minutes or so (the default is every 24h), since I don't have any commit hooks on origin. It's ugly, but does the trick. I imagine this can trigger a few uncommon states / code paths?

[jensl]

Ah, yes, I can reproduce this by doing a plain, clean new upstream rebase. I strongly suspect this is a regression from https://critic-review.org/r/314. Shouldn't be too hard to fix. Stay tuned. :-)

Your setup didn't contribute; that modification seems rather benign to me, as long as wherever you're polling from doesn't mind being polled that often.

[jensl]

This should fix the problem: https://critic-review.org/334

I won't land it before I've added a test, which I doubt I'll get around to today, but you're of course welcome to try the fix out. (Note in that case that the initial commit in the review was quite broken; don't try that.)

[mbitsnbites]

Nice! If I'm doing a review before it's on master I'll give it a shot, otherwise I think I'll just wait it out. Thanks!