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

[Petr Rockai]

Hi,

Ganesh Sittampalam <ganesh at earth.li> writes:
> I think we need to have a separate discussion on darcs-users to establish the
> standard one way or another.
(about hardcoded _darcs-paths... I'll post a separate mail later)

> 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.
Well, this will then need some tracking down, since darcs remove of
nonexistent file is a no-op with the current code, for whatever
reason. Might be my remove restructuring that broke something. I'll look
into it later.

>> : - ( 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.
:( ... amended

>> 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?
The question should probably stand "why it's there at all". I'd say
that's an omission, I'll remove the parameter.

>> 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
amended

>> 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?
It avoids extra parameter in an already long list. The opts are those
that are passed to withRepository used to obtain the repo object, which
is, IIUIC, the same.

>> 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?
>> 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

Well, existsAnycase could probably go to hashed-storage. For the rest, I
guess Darcs.Utils is as good place as any. We could create TreeUtils for
the 4 functions but sounds like overkill. I'd eventually like to flip
more of the code to use AnchoredPath directly, and the 4 helpers can go
to hashed-storage in AnchoredPath variant. Or maybe just move them right
away and put up with the extra floatPath ugliness on client side of
code. At least it'll provide further incentive to convert things about.

For those *Really* bits, I have no idea why they are there at
all. Should be removed eventually.

Yours,
   Petr.