[darcs-users] [patch262] Resolve issue1014: mark the test as pass... (and 4 more)
me at mornfall.net
Mon Jun 7 10:19:42 UTC 2010
Eric Kow <kowey at darcs.net> writes:
> On Fri, Jun 04, 2010 at 15:22:17 +0200, Petr Rockai wrote:
>> Well, if this still induced commutation failures, they would still crash
>> darcs. So no, we are not silently ignoring the bug. I don't quite
>> understand how this bug actually came into existence, since I don't
>> think the problem is in any way inherent. It indeed looks like an
>> implementation barf in get_extra. Which no longer exists...
> So we've discussed this a bit on IRC with Ian this morning,
> I'm not sure I followed all of the discussion, but I think we've come around to
> thinking that issue1014 needs a closer look (and more aggressive testing). This
> may be one of those succeeding-for-the-wrong-reason bugs which are way way worse
> than crashing. If I understand correctly, now we have a situation where
> 1. named patches A and B contain only identical primitive patches.
> 2. named patch C depends on A (or B)
> 3. merging AC+BC now leads to a weird repo where we have the named
> patch C appearing *twice* (not a set of patches anymore, but a
> bag :-/)
> Please correct me if I'm mistaken. Perhaps we could extend the issue1014
> one by grepping the changes --xml output for the patchinfo and passing it
> to wc -l.
Well, I have so far extended the code to maintain a list of patches that
failed to commute into "common" even though should have. Now at the
point of findCommonAndUncommon and friends, it is too early to tell if
this is really a problem.
So what we probably need is wibbling the merge code to pass this extra
list into the merge function, which can then decide whether this is a
problem. Dup'd named patches that turn out to be merely duplicates can
then be safely discarded during the merge.
In addition, this should allow us to produce much better error messages
in case of PatchInfo collisions: we can clearly say that there was
actually a PatchInfo collision on these two patches, and we can print
them out (in some context, unfortunately we can't guarantee identical,
or even similar context for them).
All in all, this approach should allow us to fix issue1014 safely, while
at the same time improving the situation with corrupt repositories: we
should now be able to tell apart corrupt patches from PatchInfo
collisions, which both previously lead to get_extra failure.
Of course, it is not coming for free -- the code will get a little more
complex, although I think it should still be a lot easier to follow than
the pre-newset gcau.
As for a plan of action wrt. our pending alpha release, I think that I
will restore the original "failing" behaviour for those possibly
dangerous merges. I will then work on a more reasonable implementation
for alpha 2.
[snip text about legit get_extra failures]
> (Sounds like we should have an example of this cooked up, maybe not as a test
> case but just so we have something to point to as an example of what could go
We should actually have this a as a test, requiring that darcs fails or
warns or something, if it runs into bogus patch combinations. Even if
they may never arise, theoretically, they do in practice.
More information about the darcs-users