1 patch for repository http://darcs.net:

Sat Jun  5 20:39:05 COT 2010  builes.adolfo@googlemail.com
  * Resolve Issue1210: global cache gets recorded in _darcs/prefs/sources

Attachments

• resolve-issue1210_-global-cache-gets-recorded-in-_darcs_prefs_sources.dpatch
• unnamed

The sources files is written each time we are copying a repository, what
happens is that when darcs tries to identify a repository it calls
Repository.Prefs.getCache , this function always include in  the cache the
global one unless --no-cache is given, and then when saving it to sources,
it just copies everything that is in the Cache of the repository.

My first approach was appending NoCache to the list of flags, but then I
realized that darcs wouldn't use the global cache, because it wasn't in the
Cache, so in that case we were losing the advantages offered by it,
specially if we were getting a remote one. So I modified the cache just
before saving it to the sources file, dropping the global cache out of the
list of sources.

--
Adolfo

On Sat, Jun 5, 2010 at 8:57 PM, Adolfo Builes <bugs@darcs.net> wrote:

>
> New submission from Adolfo Builes <builes.adolfo@googlemail.com>:
>
> 1 patch for repository http://darcs.net:
>
> Sat Jun  5 20:39:05 COT 2010  builes.adolfo@googlemail.com
>  * Resolve Issue1210: global cache gets recorded in _darcs/prefs/sources
>
> ----------
> files:
> resolve-issue1210_-global-cache-gets-recorded-in-_darcs_prefs_sources.dpatch,
> unnamed
> messages: 11248
> nosy: builes.adolfo
> status: needs-review
> title: Resolve Issue1210: global cache gets recorded in _darc...
>
> __________________________________
> Darcs bug tracker <bugs@darcs.net>
> <http://bugs.darcs.net/patch266>
> __________________________________

Attachments

• unnamed

Hi Adolfo,

On Sun, Jun 06, 2010 at 01:57:50 +0000, Adolfo Builes wrote:
> Sat Jun  5 20:39:05 COT 2010  builes.adolfo@googlemail.com
>   * Resolve Issue1210: global cache gets recorded in _darcs/prefs/sources

Probably a good idea to lowercase that 'Issue1210'.  Not a big deal, but it
increases the probability of somebody finding this patch later on when they're
looking for it.

The actual reason I'm asking for a (possible) amend is in my review below.

> The sources files is written each time we are copying a repository, what                                                                                   
> happens is that when darcs tries to identify a repository it calls                                                                                         
> Repository.Prefs.getCache , this function always include in  the cache the                                                                                 
> global one unless --no-cache is given, and then when saving it to sources,                                                                                 
> it just copies everything that is in the Cache of the repository.                                                                                          
>                                                                                                                                                            
> My first approach was appending NoCache to the list of flags, but then I                                                                                   
> realized that darcs wouldn't use the global cache, because it wasn't in the                                                                                
> Cache, so in that case we were losing the advantages offered by it,                                                                                        
> specially if we were getting a remote one. So I modified the cache just                                                                                    
> before saving it to the sources file, dropping the global cache out of the                                                                                 
> list of sources.                                                                                                                                           

Resolve Issue1210: global cache gets recorded in _darcs/prefs/sources
---------------------------------------------------------------------
>  import Darcs.Commands ( DarcsCommand(..), nodefaults, commandAlias, putInfo )
>  import Darcs.Arguments ( DarcsFlag( NewRepo, Partial, Lazy,
>                                      UseFormat2, UseOldFashionedInventory, UseHashedInventory,
> -                                    SetScriptsExecutable, OnePattern ),
> +                                    SetScriptsExecutable, OnePattern, NoCache ),

Presumably this is leftover and should be removed?

> -    appendBinFile (outr++"/"++darcsdir++"/prefs/sources") (show $ repo2cache inr `unionCaches` extractCache repo)
> +    (Just d) <- globalCacheDir
> +    let globalCache = Cache DarcsCache.Directory Writable d
> +    let repoCache   = extractCache $ modifyCache repo (dropLocalCache globalCache)
> +    appendBinFile (outr++"/"++darcsdir++"/prefs/sources") (show $ repo2cache inr `unionCaches` repoCache )

Most of this makes sense, but what if the global cache dir is Nothing?
Does this whole function fail?

-- 
Eric Kow <http://www.nltg.brighton.ac.uk/home/Eric.Kow>
PGP Key ID: 08AC04F9

1 patch for repository http://darcs.net:

Sun Jun  6 16:19:15 COT 2010  builes.adolfo@googlemail.com
  * Resolve issue1210: global cache gets recorded in _darcs/prefs/sources

Attachments

• resolve-issue1210_-global-cache-gets-recorded-in-_darcs_prefs_sources.dpatch
• unnamed

On Sun, Jun 06, 2010 at 21:36:16 +0000, Adolfo Builes wrote:
> Sun Jun  6 16:19:15 COT 2010  builes.adolfo@googlemail.com
>   * Resolve issue1210: global cache gets recorded in _darcs/prefs/sources

Applied, thanks!  Trivial comments below (not a request for amending,
just me making sort of useless comments)

Resolve issue1210: global cache gets recorded in _darcs/prefs/sources
---------------------------------------------------------------------
> +    globalcacheDirectory <- globalCacheDir

There's a useful Hungarian-ish pseudo-convention in Darcs to prefix
Maybe (or similar things like Either) with m.  Here for example, I
might have said something like mglobal.

> +    let repoCache   = extractCache $
> +                      case globalcacheDirectory of
> +                         Just d -> modifyCache repo (dropLocalCache (Cache DarcsCache.Directory Writable d))
> +                         _      -> repo

Two things I would have done differently here are (a) to use an explicit
Nothing and (b) to put the Cache DarcsCache.Directory Writable in the
dropLocalCache function.  People may disagree with me on (b), though.

It doesn't matter much.  For pattern matching on these sum types,
particularly types of my own, I've grown somewhat leery of the catch-all
pattern because what inevitably happens is that I catch something
unexpected (for example, if I later tweak the type and introduce a new
branch).  I don't know what the right answer is, to be honest, but
lately I tend to lean slightly away from the '_'

> hunk ./src/Darcs/Repository/Prefs.lhs 34
>                     boringRegexps, boringFileFilter, darcsdirFilter,
>                     FileType(..), filetypeFunction,
>                     getCaches,
> -                   binariesFileHelp
> +                   binariesFileHelp,
> +                   globalCacheDir
>                   ) where

I *think* you can actually use the trick of appending a comma to
globalCacheDir, which keeps future patches minimal.

-- 
Eric Kow <http://www.nltg.brighton.ac.uk/home/Eric.Kow>
PGP Key ID: 08AC04F9

This patch bundle (with 1 patches) was just applied to the repository http://darcs.net/.
This message was brought to you by DarcsWatch
http://darcswatch.nomeata.de/repo_http:__darcs.net_.html#bundle-fb8a3c35169a17b986920b7ace6ec81c69777257

This patch bundle (with 1 patches) was just applied to the repository http://darcs.net/reviewed.
This message was brought to you by DarcsWatch
http://darcswatch.nomeata.de/repo_http:__darcs.net_reviewed.html#bundle-fb8a3c35169a17b986920b7ace6ec81c69777257