darcs

Issue 1026 wish: improved advice for bug in get_extra commuting patch (2.x)

Title wish: improved advice for bug in get_extra commuting patch (2.x)
Priority feature Status wont-fix
Milestone 2.1.x Resolved in
Superseder patch ids are not collision-free
View: 27
Nosy List Serware, arjanb, darcs-devel, dmitry.kurochkin, jaredj, kowey, simonpj, thorkilnaur
Assigned To
Topics Darcs2, ProbablyEasy

Created on 2008-08-22.11:59:03 by simonpj, last changed 2010-06-15.21:48:16 by admin.

Files
File name Uploaded Type Edit Remove
improve-reporting-for-bug-in-get_extra.dpatch dagit, 2008-08-25.01:22:13 text/x-darcs-patch
repo1.tar.bz2 dmitry.kurochkin, 2008-09-18.11:02:08 application/octet-stream
repo2.tar.bz2 dmitry.kurochkin, 2008-09-18.11:02:34 application/octet-stream
unnamed simonpj, 2008-08-22.11:59:00 text/html
Messages
msg5633 (view) Author: simonpj Date: 2008-08-22.11:59:00
I am continuing to have trouble with Darcs.  Since the darcs community is now active again, I'll try to continue to report my troubles.  I cannot promise that I am reporting distinct problems.

On Windows, using darcs2.

Simon

See the trace below.  I'm using the darcs-all script to pull in patches from a peer directory.  I suspect (but I do not know) that the script isn't doing the Right Thing (see the line in red below).  But regardless, darcs should not crash with "bug in get_extra".   Doing so does not induce confidence.

sh-2.04$  ./darcs-all pull c:/simonpj/darcs/HEAD
== running darcs pull c:/simonpj/darcs/HEAD --repodir .
No remote changes to pull in!
== Required repo utils/hsc2hs is missing! Skipping
== running darcs pull c:/simonpj/darcs/HEAD --repodir libraries/array
Reading inventory of repository c:/simonpj/darcs/HEAD
darcs2.exe: bug in get_extra commuting patch:
Fri Apr 22 18:00:49 GMT Daylight Time 2005  sof
  * [project @ 2005-04-22 17:00:49 by sof]
  [mingw only]
  Better handling of I/O request abortions upon throwing an exception
  to a Haskell thread. As was, a thread blocked on an I/O request was
  simply unblocked, but its corresponding worker thread wasn't notified
  that the request had been abandoned.

  This manifested itself in GHCi upon Ctrl-C being hit at the prompt -- the
  worker thread blocked waiting for input on stdin prior to Ctrl-C would
  stick around even though its corresponding Haskell thread had been
  thrown an Interrupted exception. The upshot was that the worker would
  consume the next character typed in after Ctrl-C, but then just dropping
  it. Dealing with this turned out to be even more interesting due to
  Win32 aborting any console reads when Ctrl-C/Break events are delivered.

  The story could be improved upon (at the cost of portability) by making
  the Scheduler able to abort worker thread system calls; as is, requests
  are cooperatively abandoned. Maybe later.

  Also included are other minor tidyups to Ctrl-C handling under mingw.

  Merge to STABLE.
darcs failed: 256 at ./darcs-all line 49, <IN> line 6.
sh-2.04$

   Try it without the script

sh-2.04$ darcs2 pull c:/simonpj/darcs//HEAD --repodir libraries/array
Pulling from "c:/simonpj/darcs/HEAD"...
darcs2.exe: bug in get_extra commuting patch:
Fri Apr 22 18:00:49 GMT Daylight Time 2005  sof
  * [project @ 2005-04-22 17:00:49 by sof]
  [mingw only]
  Better handling of I/O request abortions upon throwing an exception
  to a Haskell thread. As was, a thread blocked on an I/O request was
  simply unblocked, but its corresponding worker thread wasn't notified
  that the request had been abandoned.

  This manifested itself in GHCi upon Ctrl-C being hit at the prompt -- the
  worker thread blocked waiting for input on stdin prior to Ctrl-C would
  stick around even though its corresponding Haskell thread had been
  thrown an Interrupted exception. The upshot was that the worker would
  consume the next character typed in after Ctrl-C, but then just dropping
  it. Dealing with this turned out to be even more interesting due to
  Win32 aborting any console reads when Ctrl-C/Break events are delivered.

  The story could be improved upon (at the cost of portability) by making
  the Scheduler able to abort worker thread system calls; as is, requests
  are cooperatively abandoned. Maybe later.

  Also included are other minor tidyups to Ctrl-C handling under mingw.

  Merge to STABLE.
sh-2.04$
Attachments
msg5637 (view) Author: kowey Date: 2008-08-22.12:11:23
Thanks, Simon.  Could we have your darcs --version, please?

darcs folk, I need a volunteer to
 1. reproduce the crash with 2.0.2, preferably without using the darcs-all script
 2. reproduce it with darcs 1.0.9 to make sure this is not a regression
 3. reproduce it with the latest darcs from trunk to see if it has been fixed.
The repository in question is libraries/array

Marking urgent because of crash, and adding darcs-devel because of
need-volunteer request.
msg5644 (view) Author: simonpj Date: 2008-08-22.12:55:01
darcs2 --version
2.0.1rc2 (2.0.1rc2 (+ -1 patch))
sh-2.04$

| -----Original Message-----
| From: Eric Kow [mailto:bugs@darcs.net]
| Sent: 22 August 2008 13:11
| To: dagit@codersbase.com; darcs-devel@darcs.net; eric.kow@gmail.com;
| serware@msn.com; Simon Peyton-Jones
| Subject: [issue1026] pull => bug in get_extra commuting patch (2.x)
|
|
| Eric Kow <eric.kow@gmail.com> added the comment:
|
| Thanks, Simon.  Could we have your darcs --version, please?
|
| darcs folk, I need a volunteer to
|  1. reproduce the crash with 2.0.2, preferably without using the darcs-all
| script
|  2. reproduce it with darcs 1.0.9 to make sure this is not a regression
|  3. reproduce it with the latest darcs from trunk to see if it has been
| fixed.
| The repository in question is libraries/array
|
| Marking urgent because of crash, and adding darcs-devel because of
| need-volunteer request.
|
| ----------
| nosy: +Serware, darcs-devel
| priority:  -> urgent
| status: unread -> need-volunteer
| title: Darcs trouble -> pull => bug in get_extra commuting patch (2.x)
| topic: +Darcs2
|
| __________________________________
| Darcs bug tracker <bugs@darcs.net>
| <http://bugs.darcs.net/issue1026>
| __________________________________
msg5658 (view) Author: dagit Date: 2008-08-23.06:02:23
On Fri, Aug 22, 2008 at 4:59 AM, Simon Peyton-Jones <bugs@darcs.net> wrote:
>
> New submission from Simon Peyton-Jones <simonpj@microsoft.com>:
>
> I am continuing to have trouble with Darcs.  Since the darcs community is now active again, I'll try to continue to report my troubles.  I cannot promise that I am reporting distinct problems.
>
> On Windows, using darcs2.
>
> Simon
>
> See the trace below.  I'm using the darcs-all script to pull in patches from a peer directory.  I suspect (but I do not know) that the script isn't doing the Right Thing (see the line in red below).  But regardless, darcs should not crash with "bug in get_extra".   Doing so does not induce confidence.

For some reason the red has been stripped out, but in this case it's
still quite clear you're having a problem.

> sh-2.04$ darcs2 pull c:/simonpj/darcs//HEAD --repodir libraries/array
> Pulling from "c:/simonpj/darcs/HEAD"...
> darcs2.exe: bug in get_extra commuting patch:

Eric,

Weren't we discussing last week the circumstances under which a user
could get a bug in get_extra?  What did we decide at that time?  This
is what I recall (pasting realvant parts of the code):

-- | Returns a sub-sequence from @patches@, where all the elements of
@common@ have
-- been removed by commuting them out.
get_extra :: RepoPatch p => [PatchInfo] -- ^ @common@
          -> RL (PatchInfoAnd p) C(u x) -- ^ @patches@
          -> Either MissingPatch (FlippedSeal (RL (PatchInfoAnd p)) C(y))

              case commuteFL (p :> skpd) of
                Just (skipped_patch' :> p') -> do
                    FlippedSeal x <- get_extra_aux (return
skipped_patch') common pps
                    return $ flipSeal (info hp `piap` p' :<: x)
                -- Failure to commute indicates a bug because it means
                -- that a patch was interspersed between the common
                -- patches.  This should only happen if that patch was
                -- commuted there.  This uses 2 properties:
                -- 1) commute is its own inverse
                -- 2) if patches commute in one adjacent context then
                --    they commute in any context where they are
                --    adjacent
                Nothing -> errorDoc $ text "bug in get_extra commuting patch:"
                           $$ human_friendly (info hp)

Triggering this bug basically means, your repositories merged together
at some point in the past, but now the patches fail to commute like
they used to.  This code be a bug in commutation, but I think it's
more likely to be due to index or patch corruption in one or both
copies of the repository.

Thinking about the above, I guess another way this could happen is if
the user amend records a patch and then later tries to merge their
repo with one containing the original patch?

I think we should try printing the patch in skpd that didn't commute
with p.  This could be done by writing a function (untested, but
should be right; and more elegant combining of the Maybe and Either
monads is welcome):
commuteFL2 :: Commute p => (p :> FL p) C(x y) -> Either (Sealed2 p)
((FL p :> p) C(x y))
commuteFL2 (p :> NilFL) = return (NilFL :> p)
commuteFL2 (q :> p :>: ps) = case commute (q :> p) of
                             Just (p' :> q') ->
                               case commuteFL2 (q' :> ps) of
                               Right (ps' :> p'') -> return (p' :>: ps' :> q'')
                               p'' -> p''
                             Nothing -> fail $ seal2 p

Then we could see not only the patch that should have commuted, but
the patch it failed to commute with.  If this is an amend-record
problem it might make it really obvious that it is.  Otherwise, it may
just help with debugging.

Yet another debugging trick to apply is to figure out if this is
happening in the first or second call to get_extra in the function
gcau_simple.  It's especially interesting if it happens in one and not
the other.  I think this could be implemented via a catch in
gcau_simple, but if not we can rewrite get_extra to return the error
message too.  Then we try to compare the patches in question to see if
we can tell why the commute failed.

Another possibility is that we're wrong about the assumptions above
some how, but that is trickery to understand.

We may need to fold David into the discussion on this one.

Jason
msg5659 (view) Author: kowey Date: 2008-08-23.07:03:27
Jason, indeed we were just asking ourselves that question, why do we consider it
a bug for an 'extra' patch not to commute past the list of skipped ones.

I agree that we should augment our debugging to say more about commute failures.
  Could something like commuteFL2 just become the new commuteFL?  Could we just
upgrade the commute functions to always return Either?  Would it maybe be
appropriate to upgrade to a MonadError (p,p) m => m 
(modulo witnesses)?
msg5660 (view) Author: kowey Date: 2008-08-23.07:06:20
Simon:

Could you a tarball of your libraries/array on darcs.haskell.org?  I'm hoping
that if I download it and do a darcs pull from it, it will produce the get_extra
crash
msg5668 (view) Author: thorkilnaur Date: 2008-08-23.16:39:19
What Simon PJ seems to be doing here (or rather: What the darcs-all script is 
doing on his behalf) is to do a darcs pull from the GHC HEAD repository into the 
libraries/array repository. I tried that on an Intel Mac OS X 10.5 and got this 
reaction:

> thorkil-naurs-intel-mac-mini:repo thorkilnaur$ darcs --exact-version
> darcs compiled on Aug  7 2008, at 21:50:40
> # configured Mon Jun 23 18:19:52 PDT 2008
> ./configure /usr/local/share/config.site /usr/local/etc/config.site
> 
> Context:
> 
> [TAG 2.0.2
> David Roundy <droundy@darcs.net>**20080624012041] 
> 
> thorkil-naurs-intel-mac-mini:repo thorkilnaur$ cp -R 
/Users/thorkilnaur/tn/GHCDarcsRepository/ghc-HEAD-complete-for-pulling-and-
copying-20070713_1212/ghc/libraries/array array
> thorkil-naurs-intel-mac-mini:repo thorkilnaur$ cd array/
> thorkil-naurs-intel-mac-mini:array thorkilnaur$ darcs pull 
/Users/thorkilnaur/tn/GHCDarcsRepository/ghc-HEAD-complete-for-pulling-and-
copying-20070713_1212/ghc --repodir .
> This is the GHC darcs repository (HEAD branch)
> 
> For more information, visit the GHC developer wiki at
>   http://hackage.haskell.org/trac/ghc
> **********************
> darcs: bug in get_extra commuting patch:
> Fri Apr 22 19:00:49 CEST 2005  sof
>   * [project @ 2005-04-22 17:00:49 by sof]
>   [mingw only]
>   Better handling of I/O request abortions upon throwing an exception
>   to a Haskell thread. As was, a thread blocked on an I/O request was
>   simply unblocked, but its corresponding worker thread wasn't notified
>   that the request had been abandoned.
>   
>   This manifested itself in GHCi upon Ctrl-C being hit at the prompt -- the
>   worker thread blocked waiting for input on stdin prior to Ctrl-C would
>   stick around even though its corresponding Haskell thread had been
>   thrown an Interrupted exception. The upshot was that the worker would
>   consume the next character typed in after Ctrl-C, but then just dropping
>   it. Dealing with this turned out to be even more interesting due to
>   Win32 aborting any console reads when Ctrl-C/Break events are delivered.
>   
>   The story could be improved upon (at the cost of portability) by making
>   the Scheduler able to abort worker thread system calls; as is, requests
>   are cooperatively abandoned. Maybe later.
>   
>   Also included are other minor tidyups to Ctrl-C handling under mingw.
>   
>   Merge to STABLE.
> thorkil-naurs-intel-mac-mini:array thorkilnaur$ 

A similar thing happened on a PPC Mac OS X with:

> Thorkil-Naurs-Computer:~/tn/test/darcs/Issue/1026/work/repo/array thorkilnaur$ 
darcs --exact-version
> darcs compiled on May  8 2007, at 09:50:57
> # configured Fri Jun 16 14:55:21 EDT 2006
> ./configure --no-create --no-recursion
> 
> Context:
> 
> [TAG 1.0.8
> Tommy Pettersson <ptp@lysator.liu.se>**20060616160213] 

Finally, I tried the latest (I presume) darcs on (another) PPC Mac OS X that 
hosts a darcs buildbot slave:

> thorkil-naurs-mac-mini:array thorkilnaur$ 
'/Users/thorkilnaur/tn/buildbot/darcs/thorkil_mac/thorkil tn20/build/darcs' --
version
> 2.0.2 (+ 119 patches)

Same reaction. So I would conclude: This reaction has been present for a while 
and it is not gone.
msg5669 (view) Author: kowey Date: 2008-08-23.16:57:03
Hi,

Thanks for helping us to reproduce this!

Thorkil, could you make a copy of just the libraries array repository you are
pulling with?  My goal is to have two tarballs, one to pull with, and one to
pull from (i.e. the libraries/array HEAD) without dealing with darcs-all or any
intervening GHC stuff.

The reason I'm asking for tarballs is that I'm still not sure I understand where
the local copy of libraries/array is coming from.  I realise that the darcs-all
script can be used to recursively get subrepositories, but I don't know how the
libraries/array subrepository got into this state (and a reproducible one at that)

Many thanks!
msg5670 (view) Author: kowey Date: 2008-08-23.17:23:57
Speaking with Thorkil on IRC, I now understand the situation better.

Ok, so there is no darcs-all fanciness in this whatsoever... and the pull really
just a crazy one of GHC from libraries/array, i.e. two completely unrelated
repositories.

Sorry for the noise, I just kept assuming that there was darcs-all magic
underneath, and that what everybody was talking about was a libraries/array to
libraries/array pull.

Ok, so now we just need to boil this down into a test case!  Thanks!
msg5675 (view) Author: dagit Date: 2008-08-24.07:37:11
On Sat, Aug 23, 2008 at 12:03 AM, Eric Kow <bugs@darcs.net> wrote:
>
> Eric Kow <eric.kow@gmail.com> added the comment:
>
> Jason, indeed we were just asking ourselves that question, why do we consider it
> a bug for an 'extra' patch not to commute past the list of skipped ones.
>
> I agree that we should augment our debugging to say more about commute failures.
>  Could something like commuteFL2 just become the new commuteFL?  Could we just
> upgrade the commute functions to always return Either?  Would it maybe be
> appropriate to upgrade to a MonadError (p,p) m => m
> (modulo witnesses)?

Standard commute probably doesn't need upgraded.  When you call
commute, you have all the patches available and you could then deal
with it (by throwing them up the chain as needed).  But, yes this
should become the new commuteFL.  I can do that.  I can also look at
making gcau_simple tell us which call to get_extra is failing or if
both are.

Jason
msg5677 (view) Author: arjanb Date: 2008-08-24.14:49:05
Considering a failed commute in get_extra a bug is based on the assumption
that commutation of named patches is consistent over time.
This assumption can be broken by: amended patches, duplicate patches
(see Ian's example / issue 1014), repository corruption or a bug in darcs.

I think get_extra should be changed to not bug out on this and try to commute
out as many as possible common patches instead of all. Hopefully the case of
corruption or bugs will be caught elsewhere. I'm not familiar enough with
darcs internals to see if this change has any impact on higher level code.

-- Arjan
msg5686 (view) Author: dagit Date: 2008-08-25.01:22:13
I can't reproduce Simon's bug.  I have tried fetching a fresh copy from:
http://hackage.haskell.org/trac/ghc

As well as starting from this tarball:
http://darcs.haskell.org/ghc-STABLE-2007-09-11-ghc-corelibs-testsuite.tar.bz2

The attached patch can be used to improve the error reporting of darcs.  I was
hoping to try it out and see which two patches fail to commute.

Can someone that can reproduce this try for me?  Thorkil could you look at it
please?

Thanks!
Attachments
msg5688 (view) Author: thorkilnaur Date: 2008-08-25.10:10:17
I will try, within the next couple of days.

Best regards
Thorkil
msg5700 (view) Author: simonpj Date: 2008-08-26.08:03:30
| Could you a tarball of your libraries/array on darcs.haskell.org?  I'm hoping
| that if I download it and do a darcs pull from it, it will produce the get_extra
| crash

I think Thorkil has it.  I think he's right that the script is pulling from the wrong repo entirely; and that the problem is really that darcs should complain politely rather than run off and pull patches, and then crash.

So I won't tar up the repo for now unless you need me to.  (I'm not even sure I have it.)

Simon

| -----Original Message-----
| From: Eric Kow [mailto:bugs@darcs.net]
| Sent: 23 August 2008 17:57
| To: dagit@codersbase.com; darcs-devel@darcs.net; eric.kow@gmail.com; naur@post11.tele.dk;
| serware@msn.com; serware@msn.com; Simon Peyton-Jones
| Subject: [issue1026] pull => bug in get_extra commuting patch (2.x)
|
|
| Eric Kow <eric.kow@gmail.com> added the comment:
|
| Hi,
|
| Thanks for helping us to reproduce this!
|
| Thorkil, could you make a copy of just the libraries array repository you are
| pulling with?  My goal is to have two tarballs, one to pull with, and one to
| pull from (i.e. the libraries/array HEAD) without dealing with darcs-all or any
| intervening GHC stuff.
|
| The reason I'm asking for tarballs is that I'm still not sure I understand where
| the local copy of libraries/array is coming from.  I realise that the darcs-all
| script can be used to recursively get subrepositories, but I don't know how the
| libraries/array subrepository got into this state (and a reproducible one at that)
|
| Many thanks!
|
| ----------
| assignedto:  -> thorkilnaur
| status: need-volunteer -> need-info
|
| __________________________________
| Darcs bug tracker <bugs@darcs.net>
| <http://bugs.darcs.net/issue1026>
| __________________________________
msg5717 (view) Author: thorkilnaur Date: 2008-08-26.23:51:09
On Monday 25 August 2008 03:22, Jason Dagit wrote:
> ...
> The attached patch can be used to improve the error reporting of darcs.  I was
> hoping to try it out and see which two patches fail to commute.
>
> Can someone that can reproduce this try for me?

With your patch applied, the reaction extends to:

> Thorkil-Naurs-Computer:~/tn/test/darcs/Issue/1026/work/repo/array thorkilnaur$ 
/Users/thorkilnaur/tn/darcsDarcsRepository/darcs.net/darcs pull /Users/thorkilnaur/tn/GHCDarcsRepository/ghc-HEAD-complete-for-pulling-
and-copying-20070713_1212/ghc --repodir .
> Pulling from "/Users/thorkilnaur/tn/GHCDarcsRepository/ghc-HEAD-complete-for-pulling-and-copying-20070713_1212/ghc"...
> This is the GHC darcs repository (HEAD branch)
> 
> For more information, visit the GHC developer wiki at
>   http://hackage.haskell.org/trac/ghc
> **********************
> darcs: bug in get_extra commuting patches:                                      
> First patch is:
> Fri Apr 22 19:00:49 CEST 2005  sof
>   * [project @ 2005-04-22 17:00:49 by sof]
>   [mingw only]
>   Better handling of I/O request abortions upon throwing an exception
>   to a Haskell thread. As was, a thread blocked on an I/O request was
>   simply unblocked, but its corresponding worker thread wasn't notified
>   that the request had been abandoned.
>   
>   This manifested itself in GHCi upon Ctrl-C being hit at the prompt -- the
>   worker thread blocked waiting for input on stdin prior to Ctrl-C would
>   stick around even though its corresponding Haskell thread had been
>   thrown an Interrupted exception. The upshot was that the worker would
>   consume the next character typed in after Ctrl-C, but then just dropping
>   it. Dealing with this turned out to be even more interesting due to
>   Win32 aborting any console reads when Ctrl-C/Break events are delivered.
>   
>   The story could be improved upon (at the cost of portability) by making
>   the Scheduler able to abort worker thread system calls; as is, requests
>   are cooperatively abandoned. Maybe later.
>   
>   Also included are other minor tidyups to Ctrl-C handling under mingw.
>   
>   Merge to STABLE.
> Second patch is:
> Fri May  6 02:30:57 CEST 2005  sof
>   * [project @ 2005-05-06 00:30:56 by sof]
>   [mingw only]
>   Work around bug in win32 Console API which showed up in the GHCi UI:
>   if the user typed in characters prior to the appearance of the prompt,
>   the first of these characters always came out as a 'g'. The GHCi UI does
>   for good reasons one-character reads from 'stdin', which causes the
>   underlying APIs to become confused. A simple repro case is the following
>   piece of C code:
>   
>   /*----------------------*/
>   #include <stdio.h>
>   #include <windows.h>
>   int main()
>   {
>       char ch1,ch2;
>       HANDLE hStdIn = GetStdHandle(STD_INPUT_HANDLE);
>       DWORD dw;
>   
>       /* Type in some characters before the prompt appears and be amused.. */
>       sleep(1000); printf("? ");
>       ReadConsoleA(hStdIn,&ch1,1,&dw,NULL);
>       ReadConsoleA(hStdIn,&ch2,1,&dw,NULL);
>   /*  or, if you want to use libc:
>       read(0,&ch1,1); read(0,&ch2,1); */
>   
>       printf("%c%c\n", ch1,ch2);
>       return 0;
>   }
>   /*----------------------*/
>   
>   This happens across win32 OSes, and I can't see anything untoward as far
>   as API usage goes (the GHC IO implementation uses read(), but that
>   reduces to ReadConsoleA() calls.) People inside the Behemoth might want
>   to have a closer look at this..
>   
>   Not much we can do about this except work around the problem by flushing
>   the input buffer prior to reading from stdin. Not ideal, as type-ahead
>   is a useful feature. Flushing is handled by GHC.ConsoleHandler.flushConsole
>   
>   Merge to STABLE.
> Thorkil-Naurs-Computer:~/tn/test/darcs/Issue/1026/work/repo/array thorkilnaur$ 

Best regards
Thorkil
msg5718 (view) Author: simonpj Date: 2008-08-27.07:30:24
User view again: the new error still looks like debug output from a darcs crash.

But actually the error is: "user error: you are pulling from the wrong repository".

In the darcs model, a repository is just a collection of patches, and doesn't have an "identity" per se.  But somehow it really is wrong to pull patches from a repository of GHC into a repository for an unrelated library.  That is a very plausible user error and darcs should, I believe, report it in a civilised way.

This might in turn require thinking about what it means to for two repos to be "the same".  But that is useful thinking to do.

Simon

| -----Original Message-----
| From: Thorkil Naur [mailto:bugs@darcs.net]
| Sent: 27 August 2008 00:51
| To: arjan.boeijink@gmail.com; dagit@codersbase.com; darcs-devel@darcs.net; eric.kow@gmail.com;
| naur@post11.tele.dk; serware@msn.com; serware@msn.com; Simon Peyton-Jones
| Subject: [issue1026] pull => bug in get_extra commuting patch (2.x)
|
|
| Thorkil Naur <naur@post11.tele.dk> added the comment:
|
| On Monday 25 August 2008 03:22, Jason Dagit wrote:
| > ...
| > The attached patch can be used to improve the error reporting of darcs.  I was
| > hoping to try it out and see which two patches fail to commute.
| >
| > Can someone that can reproduce this try for me?
|
| With your patch applied, the reaction extends to:
|
| > Thorkil-Naurs-Computer:~/tn/test/darcs/Issue/1026/work/repo/array thorkilnaur$
| /Users/thorkilnaur/tn/darcsDarcsRepository/darcs.net/darcs pull
| /Users/thorkilnaur/tn/GHCDarcsRepository/ghc-HEAD-complete-for-pulling-
| and-copying-20070713_1212/ghc --repodir .
| > Pulling from "/Users/thorkilnaur/tn/GHCDarcsRepository/ghc-HEAD-complete-for-pulling-and-copying-
| 20070713_1212/ghc"...
| > This is the GHC darcs repository (HEAD branch)
| >
| > For more information, visit the GHC developer wiki at
| >   http://hackage.haskell.org/trac/ghc
| > **********************
| > darcs: bug in get_extra commuting patches:
| > First patch is:
| > Fri Apr 22 19:00:49 CEST 2005  sof
| >   * [project @ 2005-04-22 17:00:49 by sof]
| >   [mingw only]
| >   Better handling of I/O request abortions upon throwing an exception
| >   to a Haskell thread. As was, a thread blocked on an I/O request was
| >   simply unblocked, but its corresponding worker thread wasn't notified
| >   that the request had been abandoned.
| >
| >   This manifested itself in GHCi upon Ctrl-C being hit at the prompt -- the
| >   worker thread blocked waiting for input on stdin prior to Ctrl-C would
| >   stick around even though its corresponding Haskell thread had been
| >   thrown an Interrupted exception. The upshot was that the worker would
| >   consume the next character typed in after Ctrl-C, but then just dropping
| >   it. Dealing with this turned out to be even more interesting due to
| >   Win32 aborting any console reads when Ctrl-C/Break events are delivered.
| >
| >   The story could be improved upon (at the cost of portability) by making
| >   the Scheduler able to abort worker thread system calls; as is, requests
| >   are cooperatively abandoned. Maybe later.
| >
| >   Also included are other minor tidyups to Ctrl-C handling under mingw.
| >
| >   Merge to STABLE.
| > Second patch is:
| > Fri May  6 02:30:57 CEST 2005  sof
| >   * [project @ 2005-05-06 00:30:56 by sof]
| >   [mingw only]
| >   Work around bug in win32 Console API which showed up in the GHCi UI:
| >   if the user typed in characters prior to the appearance of the prompt,
| >   the first of these characters always came out as a 'g'. The GHCi UI does
| >   for good reasons one-character reads from 'stdin', which causes the
| >   underlying APIs to become confused. A simple repro case is the following
| >   piece of C code:
| >
| >   /*----------------------*/
| >   #include <stdio.h>
| >   #include <windows.h>
| >   int main()
| >   {
| >       char ch1,ch2;
| >       HANDLE hStdIn = GetStdHandle(STD_INPUT_HANDLE);
| >       DWORD dw;
| >
| >       /* Type in some characters before the prompt appears and be amused.. */
| >       sleep(1000); printf("? ");
| >       ReadConsoleA(hStdIn,&ch1,1,&dw,NULL);
| >       ReadConsoleA(hStdIn,&ch2,1,&dw,NULL);
| >   /*  or, if you want to use libc:
| >       read(0,&ch1,1); read(0,&ch2,1); */
| >
| >       printf("%c%c\n", ch1,ch2);
| >       return 0;
| >   }
| >   /*----------------------*/
| >
| >   This happens across win32 OSes, and I can't see anything untoward as far
| >   as API usage goes (the GHC IO implementation uses read(), but that
| >   reduces to ReadConsoleA() calls.) People inside the Behemoth might want
| >   to have a closer look at this..
| >
| >   Not much we can do about this except work around the problem by flushing
| >   the input buffer prior to reading from stdin. Not ideal, as type-ahead
| >   is a useful feature. Flushing is handled by GHC.ConsoleHandler.flushConsole
| >
| >   Merge to STABLE.
| > Thorkil-Naurs-Computer:~/tn/test/darcs/Issue/1026/work/repo/array thorkilnaur$
|
| Best regards
| Thorkil
|
| __________________________________
| Darcs bug tracker <bugs@darcs.net>
| <http://bugs.darcs.net/issue1026>
| __________________________________
msg5719 (view) Author: dagit Date: 2008-08-27.07:57:24
On Wed, Aug 27, 2008 at 12:30 AM, Simon Peyton-Jones
<simonpj@microsoft.com> wrote:
> User view again: the new error still looks like debug output from a darcs crash.

Well, that's exactly what that output is for.  I want to understand
the problem, and I can't reproduce it (see below).

>
> But actually the error is: "user error: you are pulling from the wrong repository".

Having fun with the error message:
Perhaps darcs should say, "Sorry my brain just exploded.  Patch does
not commute consistently."  Or maybe,  "The inferred changes are less
commutable than expected."  GHC seems to do fine with such type check
error messages :)

>
> In the darcs model, a repository is just a collection of patches, and doesn't have an "identity" per se.  But somehow it really is wrong to pull patches from a repository of GHC into a repository for an unrelated library.  That is a very plausible user error and darcs should, I believe, report it in a civilised way.

Well, darcs should be reporting it in a civilized way.  That's what
we're trying to work towards.  Making get_extra more transparent so we
can see why it's breaking is a direction we haven't tried yet, hence
my patch.  I was hoping to understand why those patches failed to
commute.  Unfortunately, I still don't know why.  If I could reproduce
the failure locally that might not be the case.  So far no one has
provided me with the files to do that, even though I've asked twice
now. Hint, hint.

> This might in turn require thinking about what it means to for two repos to be "the same".  But that is useful thinking to do.

Well, all repositories start at the same context, the empty
repository.  So from a theoretical standpoint, all repositories are
branches from a common root.  Pretty much anything after that should
just depend on the way patches commute.  But, what we're seeing here
is odd.  As Arjan pointed out, this bug that darcs is reporting is the
result of a fundamental assumption being violated.

Even "unrelated" repositories should be able to agree about the empty
repository and from that point figure out which patches they have in
common (which is the operation that's failing for you).  This
shouldn't lead to the bug you're seeing unless something is going
wrong.  I was looking at the way amend-record works and it seems to
generate a new PatchInfo, so I don't think this bug can be caused by
amend-record.  Can someone construct a counter example?

If our goal was just to make some repositories the same and some
different, probably the easiest thing to do is to make the empty
repository not be a globally unique context.  This would change quite
a few semantics of darcs, but I agree it would have some nice side
effects.  Such as saying, "Hey, array != GHC, stop trying to pull
here!"

Thanks for the feedback and bug reports,
Jason
msg5720 (view) Author: simonpj Date: 2008-08-27.08:16:50
| commute.  Unfortunately, I still don't know why.  If I could reproduce
| the failure locally that might not be the case.  So far no one has
| provided me with the files to do that, even though I've asked twice
| now. Hint, hint.

Are you directing that at me?  I did reply about this: Thorkil managed to reproduce it this bug I believe http://bugs.darcs.net/msg5668, and described how to do so.  Try this recipe

* Get a GHC repo (I'd start with a tarball from http://darcs.haskell.org)
* Get a packages/array repo
* Pull from the former into the latter

Simon
msg5722 (view) Author: thorkilnaur Date: 2008-08-27.10:22:02
> Try this recipe
> * Get a GHC repo (I'd start with a tarball from http://darcs.haskell.org)
> * Get a packages/array repo
> * Pull from the former into the latter

For example:

> $ tar xjf /Users/thorkilnaur/tn/download/GHC/darcs.haskell.org/ghc-HEAD-2008-08-17-ghc.tar.bz2
> $ darcs get http://darcs.haskell.org/packages/array
> Finished getting.
> $ ls
> array   ghc
> $ cd array/
> $ darcs pull ../ghc
> darcs: bug in get_extra commuting patch:
> Fri Apr 22 19:00:49 CEST 2005  sof
>   * [project @ 2005-04-22 17:00:49 by sof]
>   [mingw only]
>   Better handling of I/O request abortions upon throwing an exception
>   to a Haskell thread. As was, a thread blocked on an I/O request was
>   simply unblocked, but its corresponding worker thread wasn't notified
>   that the request had been abandoned.
>
>   This manifested itself in GHCi upon Ctrl-C being hit at the prompt -- the
>   worker thread blocked waiting for input on stdin prior to Ctrl-C would
>   stick around even though its corresponding Haskell thread had been
>   thrown an Interrupted exception. The upshot was that the worker would
>   consume the next character typed in after Ctrl-C, but then just dropping
>   it. Dealing with this turned out to be even more interesting due to
>   Win32 aborting any console reads when Ctrl-C/Break events are delivered.
>
>   The story could be improved upon (at the cost of portability) by making
>   the Scheduler able to abort worker thread system calls; as is, requests
>   are cooperatively abandoned. Maybe later.
>
>   Also included are other minor tidyups to Ctrl-C handling under mingw.
>
>   Merge to STABLE.
> $

Best regards
Thorkil
msg5747 (view) Author: dagit Date: 2008-08-28.07:27:42
Thanks Thorkil.  I have been able to reproduce the problem locally.

It turns out to be rather interesting!

Both repositories, array and GHC, have both the patchinfos that are shown in the
new error message.  Unfortunately, the patch contents vary depending on which
repository you view those patches from.

Darcs is rightly confused here.  I mentioned this to Ian and Eric, and they
seemed to think these patches were probably created by a tailorisation of the
old GHC cvs repository into the two darcs repositories array and GHC.

Darcs has a pretty pervasive assumption that when patchinfos are equal that the
patches represent the same changes even though the patches might be in different
contexts.  

Arjan suggested that we either take it in stride or throw a catchable error up
the chain.  The former option seems dangerous to me, and the latter approach
might allow us to suggest that the user has tried to merge two very different
repositories.

Eric mentioned that there is a issue already about making patchinfos more unique
than they currently are.  I just glanced but I don't know which issue that is. 
If you find it please connect it to this issue.
msg5748 (view) Author: kowey Date: 2008-08-28.08:12:32
That would be http://bugs.darcs.net/issue27

So it sounds like there is not much more we can do for this ticket, except to
give the user better advice when get_extra fails, explaining roughly why darcs
is confused -- it thinks that two patches are the "same" when they really are
not -- and what the user can do about it (not much? maybe this is a mistaken
pull anyway?)

Otherwise, it sounds like we should work on issue27 to prevent future
occurrences of this confusion.
msg5750 (view) Author: dagit Date: 2008-08-28.08:37:33
On Thu, Aug 28, 2008 at 1:12 AM, Eric Kow <bugs@darcs.net> wrote:
>
> Eric Kow <eric.kow@gmail.com> added the comment:
>
> That would be http://bugs.darcs.net/issue27
>
> So it sounds like there is not much more we can do for this ticket, except to
> give the user better advice when get_extra fails, explaining roughly why darcs
> is confused -- it thinks that two patches are the "same" when they really are
> not -- and what the user can do about it (not much? maybe this is a mistaken
> pull anyway?)
>
> Otherwise, it sounds like we should work on issue27 to prevent future
> occurrences of this confusion.

The only other thing I can think of to help this bug is to add a
feature where you put a "unique" token somewhere in _darcs/*, perhaps
in _darcs/format, that is assigned during 'init'.  If you try to merge
two repositories that vary by token, then darcs says, "These
repositories seem to be unrelated, proceed? [y/N]"

I don't know how others feel about this feature, but it seems like
what SimonPJ was requesting.  It also doesn't sound like it would be
too hard to add.

Jason
msg5752 (view) Author: kowey Date: 2008-08-28.08:44:09
Ok, I have split ancestry-checking off into http://bugs.darcs.net/issue1039

I am also downgrading this to a feature and marking the improved advice (along
with Jason's information commuteFL?) as critical for 2.0.3.

I also consider this to be ProbablyEasy in that the hard part is trying to
phrase our error message so that it is informative without being overwhelming.
msg5753 (view) Author: simonpj Date: 2008-08-28.09:45:09
...
|
| The only other thing I can think of to help this bug is to add a
| feature where you put a "unique" token somewhere in _darcs/*, perhaps
| in _darcs/format, that is assigned during 'init'.  If you try to merge
| two repositories that vary by token, then darcs says, "These
| repositories seem to be unrelated, proceed? [y/N]"
|
| I don't know how others feel about this feature, but it seems like
| what SimonPJ was requesting.  It also doesn't sound like it would be
| too hard to add.

Something like that, yes.  The point is this: if darcs falls over saying "bug in get_extra" then my reaction is "Darcs is unreliable and I should stop using it".  If it says politely "You are trying to merge patches from one repository into an utterly different one; you've probably got the wrong prefs/defaultrepo" or something like that, then I think "Oh I'm being an idiot, how great Darcs is".

You see the difference?  How it's achieved is a different matter.

Incidentally, even if Darcs *doesn't* crash, I *still don't* want to accidentally merge 10,000 patches from ghc's repo into libraries/array.  Yes, it might in principle be do-able, but I really want Darcs to say "this looks silly to me, are you really sure you want to do this?".

Simon
msg6045 (view) Author: dmitry.kurochkin Date: 2008-09-18.11:02:08
Hello!

I think I have reproduced this problem with two small repos attached. Run pull
from repo2 to repo1 and you get the bug.

Repo1 contains patch aaa and bbb. Repo2 contains the same patch aaa, then ccc,
and bbb with different content from repo1 bbb but the same id.

To make patch with the same id I edited inventory and patch by hand. Is there a
way to make dacas use a given date for patch? Then we can put this into a test.

Regards,
  Dmitry
Attachments
msg6068 (view) Author: simon Date: 2008-09-20.11:47:23
This worked for me; you may need to adjust the timezone:

(echo 'Mon Sep 18 10:37:06 GMT 2008'; echo 'Dmitry Kurochkin
<dmitry.kurochkin@gmail.com>') | darcs record --pipe -a -m 'aaa'
msg6251 (view) Author: kowey Date: 2008-10-06.18:52:42
So, speaking with Jason Dagit on IRC, we have determined that (a) this is a
corner case (unfortunately, one that bit a pretty big repository) and (b) it
will be virtually eliminated because issue27 (the underlying cause) is resolved.
 There isn't much more we can do to improve the advice anyway.  So I'm marking
this wont-fix.

Note that the wont-fix refers to my request to improve the advice given by
darcs.  Simon's suggestion, that darcs attempt to warn you if you are pulling
between unrelated repositories has been implemented in
http://bugs.darcs.net/issue1039 and will be released in darcs 2.1

Thanks, everyone!
History
Date User Action Args
2008-08-22 11:59:03simonpjcreate
2008-08-22 12:11:26koweysetstatus: unread -> needs-reproduction
topic: + Darcs2
title: Darcs trouble -> pull => bug in get_extra commuting patch (2.x)
nosy: + Serware, darcs-devel
messages: + msg5637
priority: urgent
2008-08-22 12:55:03simonpjsetnosy: + serware
messages: + msg5644
2008-08-23 06:02:28dagitsetnosy: kowey, darcs-devel, dagit, simonpj, serware, Serware
messages: + msg5658
title: pull => bug in get_extra commuting patch (2.x) -> Darcs trouble
2008-08-23 07:03:29koweysetnosy: kowey, darcs-devel, dagit, simonpj, serware, Serware
messages: + msg5659
2008-08-23 07:06:21koweysetnosy: kowey, darcs-devel, dagit, simonpj, serware, Serware
messages: + msg5660
title: Darcs trouble -> pull => bug in get_extra commuting patch (2.x)
2008-08-23 16:39:23thorkilnaursetnosy: + thorkilnaur
messages: + msg5668
2008-08-23 16:57:05koweysetstatus: needs-reproduction -> waiting-for
nosy: kowey, darcs-devel, dagit, simonpj, thorkilnaur, serware, Serware
messages: + msg5669
assignedto: thorkilnaur
2008-08-23 17:23:59koweysetstatus: waiting-for -> needs-reproduction
nosy: kowey, darcs-devel, dagit, simonpj, thorkilnaur, serware, Serware
messages: + msg5670
assignedto: thorkilnaur -> (no value)
2008-08-23 17:26:12koweysetpriority: urgent -> bug
nosy: kowey, darcs-devel, dagit, simonpj, thorkilnaur, serware, Serware
2008-08-24 07:37:13dagitsetnosy: kowey, darcs-devel, dagit, simonpj, thorkilnaur, serware, Serware
messages: + msg5675
title: pull => bug in get_extra commuting patch (2.x) -> Darcs trouble
2008-08-24 14:49:07arjanbsetnosy: + arjanb
messages: + msg5677
2008-08-25 01:22:15dagitsetfiles: + improve-reporting-for-bug-in-get_extra.dpatch
nosy: kowey, darcs-devel, dagit, simonpj, arjanb, thorkilnaur, serware, Serware
messages: + msg5686
2008-08-25 10:10:19thorkilnaursetstatus: needs-reproduction -> has-patch
nosy: kowey, darcs-devel, dagit, simonpj, arjanb, thorkilnaur, serware, Serware
messages: + msg5688
assignedto: thorkilnaur
2008-08-25 15:17:50droundysetnosy: - darcs-devel
2008-08-26 08:03:34simonpjsetnosy: + darcs-devel
messages: + msg5700
title: Darcs trouble -> pull => bug in get_extra commuting patch (2.x)
2008-08-26 23:51:13thorkilnaursetnosy: kowey, darcs-devel, dagit, simonpj, arjanb, thorkilnaur, serware, Serware
messages: + msg5717
2008-08-26 23:52:21thorkilnaursetnosy: kowey, darcs-devel, dagit, simonpj, arjanb, thorkilnaur, serware, Serware
assignedto: thorkilnaur ->
2008-08-27 07:30:30simonpjsetnosy: kowey, darcs-devel, dagit, simonpj, arjanb, thorkilnaur, serware, Serware
messages: + msg5718
2008-08-27 07:57:28dagitsetnosy: kowey, darcs-devel, dagit, simonpj, arjanb, thorkilnaur, serware, Serware
messages: + msg5719
2008-08-27 08:16:52simonpjsetnosy: kowey, darcs-devel, dagit, simonpj, arjanb, thorkilnaur, serware, Serware
messages: + msg5720
2008-08-27 10:22:05thorkilnaursetnosy: kowey, darcs-devel, dagit, simonpj, arjanb, thorkilnaur, serware, Serware
messages: + msg5722
2008-08-28 07:27:44dagitsetnosy: kowey, darcs-devel, dagit, simonpj, arjanb, thorkilnaur, serware, Serware
messages: + msg5747
2008-08-28 08:12:34koweysetnosy: kowey, darcs-devel, dagit, simonpj, arjanb, thorkilnaur, serware, Serware
superseder: + patch ids are not collision-free
messages: + msg5748
2008-08-28 08:37:35dagitsetnosy: kowey, darcs-devel, dagit, simonpj, arjanb, thorkilnaur, serware, Serware
messages: + msg5750
2008-08-28 08:44:18koweysetstatus: has-patch -> unknown
topic: + ProbablyEasy, Target-2.1
title: pull => bug in get_extra commuting patch (2.x) -> wish: better advice on bug in get_extra commuting patch (2.x)
nosy: + jaredj
messages: + msg5752
priority: bug -> feature
2008-08-28 09:07:08koweysetstatus: unknown -> needs-reproduction
nosy: kowey, darcs-devel, dagit, simonpj, arjanb, thorkilnaur, jaredj, serware, Serware
2008-08-28 09:45:12simonpjsetnosy: kowey, darcs-devel, dagit, simonpj, arjanb, thorkilnaur, jaredj, serware, Serware
messages: + msg5753
title: wish: better advice on bug in get_extra commuting patch (2.x) -> pull => bug in get_extra commuting patch (2.x)
2008-09-18 11:02:15dmitry.kurochkinsetfiles: + repo1.tar.bz2
nosy: + dmitry.kurochkin
messages: + msg6045
2008-09-18 11:02:41dmitry.kurochkinsetfiles: + repo2.tar.bz2
nosy: kowey, darcs-devel, dagit, simonpj, arjanb, thorkilnaur, jaredj, dmitry.kurochkin, serware, Serware
2008-09-20 11:47:30simonsetnosy: + simon
messages: + msg6068
2008-10-02 09:29:21koweysetnosy: kowey, darcs-devel, dagit, simonpj, simon, arjanb, thorkilnaur, jaredj, dmitry.kurochkin, serware, Serware
title: pull => bug in get_extra commuting patch (2.x) -> wish: improved advice for bug in get_extra commuting patch (2.x)
2008-10-06 18:52:49koweysetstatus: needs-reproduction -> wont-fix
nosy: kowey, darcs-devel, dagit, simonpj, simon, arjanb, thorkilnaur, jaredj, dmitry.kurochkin, serware, Serware
messages: + msg6251
2009-08-10 23:43:32adminsetnosy: - dagit
2009-08-25 17:24:56adminsetnosy: - simon
2009-08-27 14:12:39adminsetnosy: kowey, darcs-devel, simonpj, arjanb, thorkilnaur, jaredj, dmitry.kurochkin, serware, Serware
2009-10-23 22:44:17adminsetnosy: - Serware
2009-10-23 23:27:49adminsetnosy: + Serware, - serware
2010-05-31 14:13:17koweylinkissue1855 superseder
2010-06-15 21:48:14adminsetmilestone: 2.1.x
2010-06-15 21:48:16adminsettopic: - Target-2.1