[PATCH 1 of 2 STABLE] mq: qpush fails with infinite recursion in _findtags when status file is wrong (issue2664)

Tuukka Tolvanen tuukka.tolvanen at gmail.com
Mon Feb 28 12:04:32 CST 2011


28.2.2011 10:07, Matt Mackall kirjoitti:
> On Mon, 2011-02-28 at 08:57 +0200, timeless wrote:
>> On Mon, 2011-02-28 at 06:23 +0200, timeless wrote:
>>> i did:
>>>
>>> d) add a test which will prevent people from doing it again (if they run tests).
>>
>> On Mon, Feb 28, 2011 at 8:29 AM, Matt Mackall<mpm at selenic.com>  wrote:
>>> Oops, so you did.
>>>
>>> Ok, I guess the ball is in my court to fix this all up.
>>
>> yeah, sorry. i managed a bad workaround (not submitted) for the crash.
>> tuukka tracked down the regression and suggested the backout as a
>> solution (which was much more elegant than my fix -- although
>> admittedly not ideal).
>>
>> as to oscillation, your commit message was "mq: avoid using
>> revlog.nodemap unnecessarily". which to me seemed like an attempt at
>> an optimization instead of some other bug fix.
>
> Indeed, a very successful optimization attempt except for your
> interesting test case.

Re optimization -- I can't tell offhand what the caveats for cl.rev are 
exactly vs checking cl.nodemap, but the first fix (for this testcase, 
and other tests pass) before finding the regressing cset was to take an 
exception instead of the check that looks potentially redundant here:

-            if qbasenode not in self:
+            try:
+                qbase = cl.rev(qbasenode)
+            except LookupError:
                  self.ui.warn(_('mq status file refers to unknown node 
%s\n')
                               % short(qbasenode))
                  return super(mqrepo, self)._branchtags(partial, lrev)

-            qbase = cl.rev(qbasenode)

(That doesn't address the other part, _findtags called from __contains__ 
potentially calling __contains__ recursively, but it won't get hit by 
this testcase.)

't.


More information about the Mercurial-devel mailing list