On Sun, Jan 28, 2007 at 07:43:12PM +0100, Eric Y. Kow wrote: > Hey David, Tommy, all. > > David, could you have a second look at these? > > > The result is that now a Repository "knows" if it should be ignoring > > the time stamps or not. This refactoring could lead to others, since > > there are certainly functions that should no longer need to have the > > [DarcsFlag] passed. > > Yeah, not having both sounds good, but... > > In the Repository.lhs module, there is a get_unrecorded function that > now has oldopts and moreopts. Can moreopts go away?
Alas, not just yet. I removed it, but then added it back when I realized
that when you run record --all, AnyOrder gets added into the options, which
is a major space optimization by making the construction and consumption of
the patch lazy. There might be another way to represent this, such as a
special flag, so that
data OrderedOrNot = Ordered | AnyOrder
get_unrecorded :: Repository -> OrderedOrNot -> IO Slurpy
> Also, I'm slightly unhappy about passing [] to identifyRepository in
> cases where the flags are irrelevant. I don't have a clear objections
> to it just misgivings about it coming back to haunt us in the future.
> Maybe it's a more general feeling that passing flags around just isn't
> explicit enough for Repository?
I agree that this is ugly, and I made an attempt to verify that I only pass
[] to the repository when the actual file contents are never used. It's
still ugly, but I don't see a particularly better option. Ideally, we'd
always just pass in whatever flags the user has specified, and let the
Repository code figure out what's important, but right now that would
require a deeper refactoring than I wanted to get into.
> I was considering maybe a Bool or a record, but this seems also
> inconvenient and messy because you'd have to convert from the flags.
> As a slight consolation, I'll note that Diff.gendiff uses a tuple
> of Bools, so if we were to come up with some kind of record, maybe
> we could refactor against that.
I'm not sure I like this idea either, but I'm torn. We could create
something like
data RepositoryAccessOptions = RAO { ignoretimes :: Bool,
compresspatches :: Bool,
verbosity :: VerbosityLevel,
usecontrolmaster :: Bool,
}
defaultRAO :: RepositoryAccessOptions
generateRAO :: [DarcsFlag] -> IO RepositoryAccessOptions
which is considerably cleaner, and more flexible than something like a
tuple in terms of pattern matching and refactorings (provided we always and
only use field access when pattern matching, or decide just to hide its
constructor).
I put generateRAO in the IO monad, in case some of these options may be
configurable by environment variables. Even if they aren't now, such
configurations may be added later, so this is good defensive programming,
and this is a function one'll want to call once anyways.
> > Sat Jan 27 16:22:06 PST 2007 David Roundy <[EMAIL PROTECTED]>
> > * fix bug triggered in replace.sh
>
> I think this one introduces a bug:
>
> # so far so good
> darcs init
> echo hej > a
> darcs add a
> darcs record -am hej
> # uh-oh
> sed -e s/hej/xxx/ a > a.tmp; mv -f a.tmp a
> darcs replace --ignore-times xxx hej a
>
> This succeeds, and I don't think it should. I believe the problem is
> that we've become too permissive in that we treat the apply suceeding
> in working as being good enough when we should really be checking that
> it suceeds in both working and pristine.
No, it only needs to check against pristine. If there are changes in
working that correspond to some of the changes we want to make, that's
fine. Very often, this will be the case, as a developer will start making
a change before realizing that it could be done more cleanly with a darcs
replace. This behavior is also consistent with older darcs. It's more
work than the behavior you expect, but I belive it's a better behavior.
If you look into it, I believe if you insert a sleep (or type by hand)
you'll see the same behavior with older darcs as with my bugfix.
> Also, I instinctively agree that replace should accept --ignore-times,
> but I don't understand where it's getting used. If it is getting used,
> should it maybe be used all the time?
No, we don't want to use --ignore-times unless absolutely necesary--that is
to say, unless the user asks for it--as it greatly increases the cost of
the diff operation, both in space and time.
--ignore-times is getting used in add_to_pending, which is where the bug
was showing up. What was showing up as an invalid pending was caused by
add_to_pending failing to realize that there were any pending changes,
because the file modification times were the same in pristine and working.
It does seem like a good idea to ensure that --ignore-times is available in
any darcs command that either takes a lock or checks the unrecorded
changes. Which is all but a few, such as changes, annotate, query
manifest, and maybe a few more. I'm not sure how best to enforce this. We
could make it universally available, like --help, but that doesn't really
appeal to me. I don't like the idea of adding meaningless options to any
of our commands.
> Unrelated: I notice also a smart_diff in the Replace code. Should it be
> paranoid_diff, rather?
No, I think replace is quite slow enough without always reading every file
in the repository. And, in fact, in this use we don't even want to pass in
the "real" opts, since force_replace_slurpy invalidates the timestamps on
the modified file, so the code as written does exactly what we want (albeit
in a somewhat contorted way, to reuse code).
--
David Roundy
http://www.darcs.net
signature.asc
Description: Digital signature
_______________________________________________ darcs-devel mailing list [email protected] http://lists.osuosl.org/mailman/listinfo/darcs-devel
