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.
>> 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. >> 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. >> 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. >> 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... >> 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. Yours, Petr. _______________________________________________ darcs-users mailing list [email protected] http://lists.osuosl.org/mailman/listinfo/darcs-users
