On Sun, Dec 17, 2006 at 03:26:12AM +0100, Eric Y. Kow wrote: > I'm accepting these; notes below. > > > Sat Dec 9 14:39:16 PST 2006 David Roundy <[EMAIL PROTECTED]> > > * fix pending bug that broke several_commands.sh. > > > > Sun Dec 10 13:18:46 PST 2006 David Roundy <[EMAIL PROTECTED]> > > * resolve conflict in white space. > > Ok, I don't entirely understand this, but I'll trust that David actually > understands the pending stuff really well, in any case much better than > me and I do see that tests do now pass where they didn't before. > > Notes > ----- > 1) Remove.lhs - I smell a refactor. Maybe?
This is indeed ugly code. I'm hesitant to touch it, though. > 2) What exactly does moving add_to_pending before the moving/replacing/etc > accomplish? Is this about correctness, robustness or something else? It's about correctness. The key is that we can only correctly add_to_pending changes that could be applied to the current working directory. Most of the recent bugs showed up because we incorrectly first made the change to the working directory, and then added it to pending. This worked with a buggy version of add_to_pending, but that version had its own issues, in that it ignored the working directory entirely, so the two could get out of sync. Pending handling is definitely the place where I'm most thoroughly wishing for the GADT type witnesses to verify correctness. The problem is that we've actually got three states of the repository, the recorded state, the post-pending state, and the "unrecorded" state, which includes changes made in the working directory. For any given patch sequence, it's getting increasingly difficult to see in which context said patch can be applied. This is less of an issue if people don't do things like manually removing files, which introduces an ambiguity about where that particular patch lives, but of course that's no solution. > 3) add_to_pending now uses get_unrecorded, which includes read_pending that > we used to invoke. I'm guess this is the bit that fixes the bugs, but is > there any chance that this introduces a new category of bugs? Is there > such a thing as unrecorded changes that shouldn't go on pending being > dumped there by accident? It's possible that unrecorded changes will accidentally go into pending, but that's not a very serious bug, and I don't think it'll actually happen, because add_to_pending explicitely doesn't pass [DarcsFlag], so LookForAdds will never get triggered on this code path, and sift_for_pending should filter out any other "undesirable" patches. > And now a general remark > ------------------------ > One thing that seems to be missing in our comments is an explanation how > pending really works. Maybe there is none, because there is nothing to > explain. I would suspect that other developers would appreciate some sort of > hand-holding on the subtleties behind pending vs. regular old unrecord > changes, even a basic list of gotchas and sanity checks we should make when > mucking around in there. > > I claim also that with David having recently cleaned up all this stuff and > pushed it into the Repository module, now would be a good time to work on said > documentation, while it's still fresh in our collective head. That would be good, but I'd like to finish first (there's still a couple of tests that I don't think pass). > > Sun Dec 10 13:35:36 PST 2006 David Roundy <[EMAIL PROTECTED]> > > * change Maybe Patch to Hopefully Patch. > > > > Sun Dec 10 15:16:23 PST 2006 David Roundy <[EMAIL PROTECTED]> > > * don't use HopefullyPrivate outside of Hopefully. > > > > Sun Dec 10 15:44:53 PST 2006 David Roundy <[EMAIL PROTECTED]> > > * fix bug in haskell_policy check for HopefullyPrivate. > > This version of the Hopefully stuff answers some of my previous > questions, so I've copied over the ones I still find relevant and > added some more > > Notes 2 > ------- > 4) should hopefully be expressed in terms of conscientiously? Done (in a later patch). > 5) There is one disadvantage to this new code, and that is that we lose > the source code line numbers that tell use where the fromJust/ > hopefully error was triggered. I would argue that this is worth > getting back, because hopefully still gets called in many different > places, so it might not be entirely clear why/when patch stuff > becomes Unavailable. > > Would it be possible for hopefully to have some of the same > impossible.h line number magic? That would cover the top of the > chain, just like fromJust did. What would also be nice is if we > could cover the bottom of the chain by putting the same magic in our > calls to the Unavailable constructors. Indeed, I wondered about that a bit, whether it would be a good idea or not. My current leaning is to primarily treat these errors as IO errors, which they really are. Adding a line number doesn't necesarily gain much, and makes it look more like a bug report. What I really want to be clear from the error message is which repository doesn't have which patch, and any IO errors (lest it turn out to be an ssh error or something). Also, note that the code that generated the Unavailable is in many ways more interesting than the code that discovered it. But with uncertainly, we're able to get information from both locations. I'd actually prefer to use variants of the messages passed to uncertainly to indicate where it is used, so that users wouldn't be overwhelmed with information, but a grep of the code would show us where the trouble spot is. We certainly could get line numbers for the use of hopefully, but I'm not convinced it's worth the effort. I suppose if we managed to reduce all the hopefullies to scenarios where a failure really is a bug... but then it'd probably most often be an actual IO error that we're seeing. After flipping back and forth, I'd say wait and see if we start seeing lots of confusing hopefully errors. > 6) Perhaps we should have the policy checker "enforce" that Unavailable > should never be passed the empty string. We now have much more > capacity for error tracking, so it seems like we should adopt a > discipline of actually using it. Hmmmm. If you could figure out how to make the policy checker do that, that'd be great. There's always you, the human policy checker... but a computer program is more reliable! :-] > 7) Maybe refactor hopefullyNoParseError? > I see it in two places; maybe there'll be more? Perhaps. We could have the parse function return a hopefully? -- David Roundy Department of Physics Oregon State University
signature.asc
Description: Digital signature
_______________________________________________ darcs-devel mailing list [email protected] http://www.abridgegame.org/cgi-bin/mailman/listinfo/darcs-devel
