On Mon, Sep 29, 2008 at 06:38:00PM +0400, Dmitry Kurochkin wrote: > Hello. > > Attached is fix for issue1105 and test. I am not very happy with it. > IMHO code became more complex. Suggestions how to make it better or > fix the bug another way are welcomed.
Hmmm. This code doesn't look so bad to me. The only ugly bit is the passing of all_opts around, but it's hard to see any fix that would be less than a major module refactor. For those who haven't read the patch, the issue is that we would like to warn users (crash?) if they have a broken ALL default in their defaults file, but to do that when parsing the defaults, we need to know what *all* the possible flags are. This list of flags is a global constant, but the functions that need it can't import the list of all commands, since they are in modules that are imported by the commands themselves, so this would cause a dependency loop. :( I think the best solution to this issue would be to refactor Darcs.Commands, which is to big anyways. I think that we could separate the logic to run commands (which includes looking up the defaults) from the data and helper-function definitions that the commands themselves need to import. But this is a job for another day and/or another person than myself (as are so many jobs!). I also found one bug, which was in how we combine the global and per-repository defaults. I'm sending a separate patch bundle with a fix for that bug (and a new regression test for the test suite), so I won't discuss it more here. That was the only issue I found, so I'm pushing with my fix, but further review by other developers would be most welcome! David _______________________________________________ darcs-users mailing list darcs-users@darcs.net http://lists.osuosl.org/mailman/listinfo/darcs-users
