Eric Kow <[email protected]> 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, > > http://irclog.perlgeek.de/darcs/2010-06-06#i_2408192 > > 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 > wrong). 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. [snip] Yours, Petr. _______________________________________________ darcs-users mailing list [email protected] http://lists.osuosl.org/mailman/listinfo/darcs-users
