Hi,

these are mostly test wibbling. May need some review though. Also, the last
patch would certainly benefit from a test run on a case-insensitive system.

Yours,
   Petr.

5 patches for repository darcs-unstable@darcs.net:darcs:

Thu Jun  3 14:29:57 CEST 2010  Petr Rockai <me@mornfall.net>
  * Resolve issue1014: mark the test as passing. Likely addressed by NewSet.

Thu Jun  3 14:31:33 CEST 2010  Petr Rockai <me@mornfall.net>
  * Resolve issue1337: mark the test as passing. Likely addressed by noslurps.

Thu Jun  3 14:34:00 CEST 2010  Petr Rockai <me@mornfall.net>
  * Resolve issue1401: mark the test as passing. Likely fixed by NewSet.

Thu Jun  3 14:37:18 CEST 2010  Petr Rockai <me@mornfall.net>
  * Resolve issue1610: mark the test as passing. Likely fixed by NewSet.

Thu Jun  3 14:37:35 CEST 2010  Petr Rockai <me@mornfall.net>
  * Skip the case folding test on case-sensitive systems.

Attachments

• resolve-issue1014_-mark-the-test-as-passing_-likely-addressed-by-newset_.dpatch
• unnamed

On Thu, Jun 03, 2010 at 15:32:43 +0000, Petr Ročkai wrote:
> these are mostly test wibbling. May need some review though. Also, the last
> patch would certainly benefit from a test run on a case-insensitive system.

Hmm, sorry to sit on this (probably this weekend).

It's certainly nice to see these being moved out (and not just
forgotten), but I'd like to read a little more on the tracker.

(If you're feeling impatient, feel free to just apply them)

> Thu Jun  3 14:29:57 CEST 2010  Petr Rockai <me@mornfall.net>
>   * Resolve issue1014: mark the test as passing. Likely addressed by NewSet.

I find this a bit too good to be true... but here's David's patch
* resolve issue1014: use merge_them in pull

I can understand that the bug in get_extra just goes away in the NewSet
work.  But is the fundamental bug solved actually solved, or has it
swept under the rug?  

The wiki says that bug in get_extra happens when 

  This bug is triggered if for whatever reason Darcs finds itself in a position
  where a patch which is shared by both repos appears to depend on a patch
  which is local to one of them only. Such a dependency would be absurd. We can
  divide this error into three known cases:
  
  1. Darcs thinks a patch is shared, but it's actually local to one of the repositories
  2. The contrary: Darcs mistakenly believes a shared patch is local
  3. There is a real dependency and it indicates a deep Darcs bug (see issue1014)

Sorry if that's in confusing kowey-language.  Anyway the point is that
issue1014 is one of the few cases where the bug in get_extra really is
legitimate and not just some incidental nonsense thing like an encoding
problem.

So I'm a bit surprised that it's just been fixed like that, since it's about
the duplicate patch shenanigans.  If we're not making error messages anymore,
then it almost sounds like we just need to be testing more aggressively to
reveal the real problem.  :-/

Hmm!  Hopefully you'll come back to me with a sort of "ah, but NewSet *does*
fundamentally address these issues because X Y and Z" and I'll just apply these
patches and be happy.

> Thu Jun  3 14:31:33 CEST 2010  Petr Rockai <me@mornfall.net>
>   * Resolve issue1337: mark the test as passing. Likely addressed by noslurps.

Will read about this later.
 
> Thu Jun  3 14:34:00 CEST 2010  Petr Rockai <me@mornfall.net>
>   * Resolve issue1401: mark the test as passing. Likely fixed by NewSet.
> 
> Thu Jun  3 14:37:18 CEST 2010  Petr Rockai <me@mornfall.net>
>   * Resolve issue1610: mark the test as passing. Likely fixed by NewSet.

I'm less concerned about these, but would like to read more about NewSet and
get_extra sometime

> Thu Jun  3 14:37:35 CEST 2010  Petr Rockai <me@mornfall.net>
>   * Skip the case folding test on case-sensitive systems.

I confirm that this still runs (and fails) on my Mac

-- 
Eric Kow <http://www.nltg.brighton.ac.uk/home/Eric.Kow>
PGP Key ID: 08AC04F9

Re,

Eric Kow <kowey@darcs.net> writes:
> On Thu, Jun 03, 2010 at 15:32:43 +0000, Petr Ročkai wrote:
>> Thu Jun  3 14:29:57 CEST 2010  Petr Rockai <me@mornfall.net>
>>   * Resolve issue1014: mark the test as passing. Likely addressed by NewSet.
>
> I find this a bit too good to be true... but here's David's patch
> * resolve issue1014: use merge_them in pull
>
> I can understand that the bug in get_extra just goes away in the NewSet
> work.  But is the fundamental bug solved actually solved, or has it
> swept under the rug?
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...

Also, my implementation is somewhat different, since in our branch,
darcs actually needs the common/uncommon sequences for features that did
not exist at the time David did this. So I have re-implemented
findCommonAndUncommon (used to be get_common_and_uncommon) in terms of
findCommonWithThem which is just a glorified partitionFL. I.e. this
should all be safe. There's no way really it could care the least about
duplicate patches.

If it was however an inherent bug, the partitionFL would break, since
the partitioning point would be ill-defined: there would be patches that
need to go to the left while at the same time they depend on patches
that need to go to the right. This however does not happen. It would
still happen in cases 1 and 2 that you describe below.

> The wiki says that bug in get_extra happens when 
>
>   This bug is triggered if for whatever reason Darcs finds itself in a position
>   where a patch which is shared by both repos appears to depend on a patch
>   which is local to one of them only. Such a dependency would be absurd. We can
>   divide this error into three known cases:
>   
>   1. Darcs thinks a patch is shared, but it's actually local to one of the repositories
>   2. The contrary: Darcs mistakenly believes a shared patch is local
>   3. There is a real dependency and it indicates a deep Darcs bug (see issue1014)
>
> Sorry if that's in confusing kowey-language.  Anyway the point is that
> issue1014 is one of the few cases where the bug in get_extra really is
> legitimate and not just some incidental nonsense thing like an encoding
> problem.
>
> So I'm a bit surprised that it's just been fixed like that, since it's about
> the duplicate patch shenanigans.  If we're not making error messages anymore,
> then it almost sounds like we just need to be testing more aggressively to
> reveal the real problem.  :-/
I don't think so. Hopefully, the above explanation is good enough.

> Hmm!  Hopefully you'll come back to me with a sort of "ah, but NewSet *does*
> fundamentally address these issues because X Y and Z" and I'll just apply these
> patches and be happy.
I think there's no fundamental issue in the first place.

[snip]

Yours,
   Petr.

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.

> If it was however an inherent bug, the partitionFL would break, since
> the partitioning point would be ill-defined: there would be patches that
> need to go to the left while at the same time they depend on patches
> that need to go to the right. This however does not happen. It would
> still happen in cases 1 and 2 that you describe below.

For the interested, Petr later on realised that actually,

| It won't fail as get_extra used to in the remaining cases, it will err on the
| "common" side -- i.e. some patches that are actually common may fail to be
| recognized as such if there's a buggy patchinfo somewhere.

(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).

Anyway, as for the patches in this bundle...

| Thu Jun  3 14:29:57 CEST 2010  Petr Rockai <me@mornfall.net>                                                                                               
|   * Resolve issue1014: mark the test as passing. Likely addressed by NewSet.                                                                               

Needs looking into some more.
                                                                                                                                                            
| Thu Jun  3 14:31:33 CEST 2010  Petr Rockai <me@mornfall.net>                                                                                               
|   * Resolve issue1337: mark the test as passing. Likely addressed by noslurps.                                                                             

I'm happy to see this is solved (applied!)
                                                                                                                                                            
| Thu Jun  3 14:34:00 CEST 2010  Petr Rockai <me@mornfall.net>                                                                                               
|   * Resolve issue1401: mark the test as passing. Likely fixed by NewSet.                                                                                   

Ian said this was the same as 1014, so it seems we should working to
extend this test too (or maybe getting rid of it if it really tells
us nothing new).

| Thu Jun  3 14:37:18 CEST 2010  Petr Rockai <me@mornfall.net>                                                                                               
|   * Resolve issue1610: mark the test as passing. Likely fixed by NewSet.                                                                                   

Hmm, I guess I should just assume this really is fixed.  Yay!

| Thu Jun  3 14:37:35 CEST 2010  Petr Rockai <me@mornfall.net>                                                                                               
|   * Skip the case folding test on case-sensitive systems.   

Already applied.

-- 
Eric Kow <http://www.nltg.brighton.ac.uk/home/Eric.Kow>
PGP Key ID: 08AC04F9

Eric Kow <kowey@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,
>
> 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.

OK, I'll be marking these as rejected for now (bah).
Hopefully in the future!