[darcs-users] darcs patch: check if default options are valid

David Roundy droundy at darcs.net
Wed Oct 1 18:11:28 UTC 2008


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


More information about the darcs-users mailing list