[darcs-users] [patch260] Implement darcs optimize --http (and 2 more)

[Alexey Levan]

Alexey Levan <exlevan at gmail.com> added the comment:

> It's more common to use ' (prime) as a suffix in Haskell than 2 (the
> latter usually means 2-argument, like liftM2...)

Using prime here breaks the build:

src/Darcs/Repository.hs:173:30:
    Not in scope: type constructor or class `C'

It seems that some preprocessor magic doesn't work with primes well.

> I'd say copyAnyToOld is a better name than copyAnythingToOld. (But now I
> see this is not a name you introduced -- you can still rename it if you
> are amending or extending this, though...)

Done.

> I think toCache is OK, since it's what the original code did.
> …
> Again, toRepo should be OK.

OK, leaving the original version.


> Create a function for lazy fetching files
> -----------------------------------------
>
> (maybe fix the patch title here to say "fetching of files"?)

Fixed.

> Ok, although it should be noted that the lazy readFile may constitute a
> resource (fd) leak -- a haddock explaining that would be certainly
> appropriate. (I.e. this behaves the same as Prelude.readFile -- see
> contrib/darcs-errors.hlint in your darcs source tree for explanation.)

Done.

>> +import Control.Applicative ( (<$>) )
> : - )

That one should definetely go to Prelude (-:

> The error message should explicitly say what was expected: "Only hashed
> repositories can be optimized for HTTP" or something in that vein.

Done.

> (about reformatting: I am not complaining about how it looks now, but it
> helps review to do formatting changes in separate patch that says it's
> just formatting)

I had to touch that line anyway, because of variable renaming.

>> hunk ./src/Darcs/Repository.hs 249
>> -  withRepoLock opts $- \torepository@(Repo _ _ rfto (DarcsRepository _ c)) ->
>> +  withRepoLock opts $- \torepository@(Repo _ _ rfto _) ->
>>        if formatHas HashedInventory rffrom && formatHas HashedInventory rfto
>>        then do debugMessage "Writing working directory contents..."
>>                createPristineDirectoryTree torepository "."
> Is this just a warning fix?

Yes, I moved it to separate patch for clarity.

> Hm, this is my sin, but the findCommonAndUncommon function actually does
> not return any "common" patches. I will rename it later... You probably
> want to rename "comm" and "unc" to "us'" and "them'".

Done.

>> +  revertTentativeChanges
> This might be redundant, but let's keep it in for a good measure.

Darcs crashes if it's not there, so yes, let's keep it :)

> Great. We should also make this interruptible later, like normal "get"
> is, with the result of getting a lazy repository. You can do this in a
> followup patch and I won't hold up pushing this just for that.

I'll do this in another patch.


5 patches for repository http://darcs.net/:

Thu Jun  3 14:59:00 EEST 2010  Alexey Levan <exlevan at gmail.com>
  * Add --http flag for optimize

Thu Jun 24 20:28:22 EEST 2010  Alexey Levan <exlevan at gmail.com>
  * Refactor Darcs.Repository.copyInventory (consistent naming)

Thu Jun 24 21:01:46 EEST 2010  Alexey Levan <exlevan at gmail.com>
  * Create a function for lazy fetching of files

Thu Jun 24 21:38:11 EEST 2010  Alexey Levan <exlevan at gmail.com>
  * Implement darcs optimize --http

Thu Jun 24 21:39:10 EEST 2010  Alexey Levan <exlevan at gmail.com>
  * Fix warning in Darcs.Repository

__________________________________
Darcs bug tracker <bugs at darcs.net>
<http://bugs.darcs.net/patch260>
__________________________________
-------------- next part --------------
A non-text attachment was scrubbed...
Name: add-__http-flag-for-optimize.dpatch
Type: text/x-darcs-patch
Size: 89964 bytes
Desc: not available
URL: <http://lists.osuosl.org/pipermail/darcs-users/attachments/20100624/61bf8554/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: unnamed
Type: application/octet-stream
Size: 5 bytes
Desc: not available
URL: <http://lists.osuosl.org/pipermail/darcs-users/attachments/20100624/61bf8554/attachment-0001.obj>