Re: [Audacity-devel] Bug 137
A free multi-track audio editor and recorder
Brought to you by:
aosiniao
From: Michael C. <mc...@gm...> - 2011-08-10 14:57:37
|
On Wed, Aug 10, 2011 at 7:47 AM, Edgar <edg...@wa...> wrote: > @ Michael... > > Thanks you for the in-depth review, I really appreciate it! In reply, I want > to reinforce that this code is not designed to stay in Audacity, it is just > there briefly (hopefully) in an effort to pin down bug137 if it relates to > Write errors. As it stands, Audacity ignores almost all Write errors and > goes on (e. g. saving a Project) just as though the error had not occurred. > This can clearly lead to bug 137-like symptoms as proved by the full disc > tests performed by Martyn, Gale and myself. > > My concept was to commit some reviewed-for-errors (not style) version as > 1.3.14beta, get it out so y'all can get real-world testing and see if you > can "blame" some of bug 137 on Write() errors. It would be nice to do a > companion readReturn patch but not until this one is committed (otherwise it > would be way too big a patch). > > I would leave this code "out there" for "a while" (6-8 weeks??) to see what > kind of results you get on bug reports. At that time y'all would either "fix > the problem" because you knew that Read()/Write() errors was the root cause > of bug 137 (see my fullDisc patch earlier in this thread for a brief start > on recovering from failed Write()s) or demote bug 137 to P3 and release 2.0. > QA/Team could then make a policy decision on a global "fix" to the ignored > Read()/Write() returns. Hi Ed, I understand what you are saying. But I would prefer the code would be permanent since we want to check for write errors. Also, it could be messy to revert a large patch like this 6-8 weeks later. In either case (fix or demote), the code from the patch could and I think should remain. In the fix case it would be used as a starting point for the fix or to check for write errors. In the demote case it could be left as is because these are useful checks and notifications. These are my main reasons for reviewing it somewhat in depth. Also I think most of my comments were not purely or mainly cosmetic style. While I prefer that my complete review is responded to (not necessarily adopted), if we are going to commit this I didn't see any obvious errors, but I hope you can respond at least to the comments on potentially dangerous code re cleanup. Furthermore, it doesn't always matter that there are no obvious errors, since a reviewer generally won't completely check and test each use case of such a large patch, but using a refactorization or macro for code replicated 10 or 20 times could minimize that risk. (Incidentally, this can help cosmetics/readability). I of course don't want to obstruct any justice with a patch review so please others speak up if you think we should get the patch in as is asap to get immediate testing results. But I am simply saying after reviewing it I think we might invest a bit more time to iterate on it to get something much more useful and permanent out of it. Best, Michael > > I do not believe that Read()/Write() errors can account for all the reports > of bug 137. I also think that failure to Lock() and/or Open() caused by > collision with OS directory scanning &/or Virus scanners might also impact > this bug. Audacity almost always also ignores these and rarely does any > recovery. > > -- > View this message in context: http://audacity.238276.n2.nabble.com/Bug-137-tp6614577p6671108.html > Sent from the audacity-devel mailing list archive at Nabble.com. > > ------------------------------------------------------------------------------ > uberSVN's rich system and user administration capabilities and model > configuration take the hassle out of deploying and managing Subversion and > the tools developers use with it. Learn more about uberSVN and get a free > download at: http://p.sf.net/sfu/wandisco-dev2dev > _______________________________________________ > audacity-devel mailing list > aud...@li... > https://lists.sourceforge.net/lists/listinfo/audacity-devel > |