On Sat, 20 Feb 2010, Petr Rockai wrote:
Hi,
Ganesh Sittampalam <[email protected]> writes:
I've started reviewing this. Here are some partial comments. Petr, are
you ok for me to any patches I'm already happy with (so long as darcs
compiles/the testsuite passes, of course) while the rest of the review
is still ongoing?
Sure, go ahead.
I've now pushed a few patches. Note that some of the remaining patches are
in conflict with current HEAD.
The rest of my review is at the bottom.
Fri Feb 12 10:32:58 GMT 2010 Petr Rockai <[email protected]>
* Use a more canonic way to create empty hashed pristine.
Fri Feb 12 10:18:44 GMT 2010 Petr Rockai <[email protected]>
* Use a more canonic way to create empty hashed pristine in optimize
(--upgrade).
hardcoded "_darcs/pristine.hashed" is not a good standard, IMO
Dunno. It's easier to write this way and it can't be changed most of the
time (only when we break backward compatibility) and is easy to grep
anyway. It also avoids an extra lookup when reading the code.
I think we need to have a separate discussion on darcs-users to establish
the standard one way or another.
Fri Feb 12 10:17:51 GMT 2010 Petr Rockai <[email protected]>
* Cannot "darcs remove" non-existent files.
What's the logic behind this? I agree that removing a file that's in
pristine but not working is redundant, but I don't see why there should
be a complaint. BTW while scratching my head at the remove code I found
a bug which I'll raise as a separate ticket.
Hm. The logic is that old darcs would write out a pending patch with the
removal even if the file was not there, which would then alter behaviour
of darcs show files --pending. Or somesuch (maybe the other way
around). The same patch should have a change to the testsuite to a
similar effect. I am not convinced either way, just seemed a sane thing
to do.
Thinking about it again, I can also see a use case for when you want to
make sure the file stays marked as removed but actually put it back later.
So I'm against this patch without further evidence on the other side.
Thu Feb 11 00:14:35 GMT 2010 Petr Rockai <[email protected]>
* Re-implement setScriptsExecutable using Trees instead of Slurpies.
+ tree <- expand =<< readPlainTree "." -- TODO filter out _darcs
This TODO needs doing
Well, it should be safe, even if less optimal. It could be argued boring
files should be ignored as well here. But then, this is only called in
contexts where we are constructing a new working tree
(get/convert/test/trackdown) and we have an untouched _darcs (which by
darcs action alone cannot contain anything starting with #! even if
there was any harm in +x-ing such files). Ok ok I'll do that.
OK, this is awaiting amendment/a follow-up.
Mon Feb 8 23:11:25 GMT 2010 Petr Rockai <[email protected]>
* Avoid use of SlurpDirectory in Commands.ShowFiles.
+import Prelude hiding ( all )
...
+all <- ...
Just call the variable a different name instead of breaking hiding
standard Prelude functions in the rest of the module
: - ( I hate the shadowing warning. All concise and reasonable names I
want to use are always taken...
I'd be in favour of turning off the shadowing warnings, because they block
legitimate deliberate uses of shadowing where you want to avoid
accidentally reusing the shadowed value.
But actually I'm still against using standard Prelude function names as
variables in anything but a very limited scope, because of the potential
for confusion for someone else editing the code later.
Mon Feb 8 22:47:04 GMT 2010 Petr Rockai <[email protected]>
* Remove implementation of --store-in-memory, simplifying matcher code.
This seems to be a live option. If you're dumping it, (a) there needs to
be some discussion and (b) you haven't actually removed it properly,
you're just ignoring it.
Yeah, I dumped it because I didn't think it was of any use. Discussion
can of course happen, my vote is to drop it. :) I agree it should be
removed completely if that's the decision.
The right way to remove features is to explicitly start a discussion, not
slip in a patch and hope it gets past the reviewer :-)
Fri Feb 12 10:36:41 GMT 2010 Petr Rockai <[email protected]>
* Remove SlurpDirectory.
OK
Fri Feb 12 10:33:22 GMT 2010 Petr Rockai <[email protected]>
* Replace slurp_recorded with readRecorded in make_new_pending.
OK
Fri Feb 12 10:28:42 GMT 2010 Petr Rockai <[email protected]>
* Reimplement applyHashed in terms of hashedTreeIO (Storage.Hashed.Monad).
Why does this ignore the cache parameter?
Note that we are switching the monad apply uses from SlurpMonad to TreeIO,
and as a result fail inside apply turns directly into fail in IO, which is
what the old code did explicitly.
I still don't like hardcoded directories.
Otherwise OK.
Fri Feb 12 10:19:46 GMT 2010 Petr Rockai <[email protected]>
* Purge unused bits of checkpoint reading.
OK
Note that there were two separate copies of read_checkpoints!
Thu Feb 11 09:31:53 GMT 2010 Petr Rockai <[email protected]>
* Port Commands.Move from Slurpy to Tree.
OK
Thu Feb 11 00:13:53 GMT 2010 Petr Rockai <[email protected]>
* Re-implement optimize --relink using Trees instead of Slurpies.
More hardcoded paths
otherwise OK
Thu Feb 11 00:13:26 GMT 2010 Petr Rockai <[email protected]>
* Use stock setScriptsExecutable from Darcs.Repository in Commands.Convert.
The old code worked on an explicit repository, while the new code relies
on getCurrentDirectory. Are we sure this is ok?
otherwise OK
Wed Feb 10 23:58:47 GMT 2010 Petr Rockai <[email protected]>
* Purge unused fileExists from Commands.Add.
description wrong - should be Commands.Record
Wed Feb 10 23:57:54 GMT 2010 Petr Rockai <[email protected]>
* Purge Slurpy usage in Commands.Rollback (use announceFiles from whatsnew).
OK
Wed Feb 10 23:50:00 GMT 2010 Petr Rockai <[email protected]>
* Generalize announceFiles used by whatsnew and use it in record as well.
This extracts options from the repository rather than having them passed
down to it. What difference does this make?
otherwise OK
Wed Feb 10 23:48:20 GMT 2010 Petr Rockai <[email protected]>
* Re-work Commands.Add (simplify, use the new treeHas* functions).
Replacing a chain of tests with a whole set at once - could any of the
later tests fail when previously they'd have been skipped?
otherwise OK
Wed Feb 10 23:47:11 GMT 2010 Petr Rockai <[email protected]>
* Re-implement the Slurp-based file/dir existence-check functions in
terms of Trees.
I think we should be trying to break up Darcs.Utils or restrict it to
really generic utility code, rather than dumping more stuff in it.
Can this stuff go elsewhere or in a new module?
otherwise OK
Wed Feb 10 12:26:01 GMT 2010 Petr Rockai <[email protected]>
* Replace SlurpDirectory usage in Commands.Add with Tree-based code.
Likewise for Darcs.Utils
otherwise OK
_______________________________________________
darcs-users mailing list
[email protected]
http://lists.osuosl.org/mailman/listinfo/darcs-users
