[darcs-users] [patch156] Remove unused force_replace_slurpy. (and 3 more)

[Ganesh Sittampalam]

On Sat, 20 Feb 2010, Petr Rockai wrote:

> Hi,
>
> Ganesh Sittampalam <bugs at darcs.net> 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 <me at mornfall.net>
>>>   * Use a more canonic way to create empty hashed pristine.
>>
>>> Fri Feb 12 10:18:44 GMT 2010  Petr Rockai <me at mornfall.net>
>>>   * 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 <me at mornfall.net>
>>>   * 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 <me at mornfall.net>
>>>   * 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 <me at mornfall.net>
>>>   * 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 <me at mornfall.net>
>>>   * 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 <me at mornfall.net>
>   * Remove SlurpDirectory.

OK

> Fri Feb 12 10:33:22 GMT 2010  Petr Rockai <me at mornfall.net>
>   * Replace slurp_recorded with readRecorded in make_new_pending.

OK

> Fri Feb 12 10:28:42 GMT 2010  Petr Rockai <me at mornfall.net>
>   * 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 <me at mornfall.net>
>   * 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 <me at mornfall.net>
>   * Port Commands.Move from Slurpy to Tree.

OK

> Thu Feb 11 00:13:53 GMT 2010  Petr Rockai <me at mornfall.net>
>   * Re-implement optimize --relink using Trees instead of Slurpies.

More hardcoded paths

otherwise OK

> Thu Feb 11 00:13:26 GMT 2010  Petr Rockai <me at mornfall.net>
>   * 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 <me at mornfall.net>
>   * Purge unused fileExists from Commands.Add.

description wrong - should be Commands.Record

> Wed Feb 10 23:57:54 GMT 2010  Petr Rockai <me at mornfall.net>
>   * Purge Slurpy usage in Commands.Rollback (use announceFiles from whatsnew).

OK

> Wed Feb 10 23:50:00 GMT 2010  Petr Rockai <me at mornfall.net>
>   * 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 <me at mornfall.net>
>   * 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 <me at mornfall.net>
>   * 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 <me at mornfall.net>
>   * Replace SlurpDirectory usage in Commands.Add with Tree-based code.

Likewise for Darcs.Utils

otherwise OK