[darcs-users] [patch310] Resolve issue 1599: automatically expire unused caches

[Eric Kow]

On Thu, Jul 22, 2010 at 20:38:35 -0500, Adolfo Builes wrote:
> > Is it really necessary for this to be String -> Bool?
> > Why not String -> IO Bool?
> >
> > Alternatively, why not IO (String -> Bool)?
> >
> I used  (String -> Bool) basically to escape the IO so that I could
> use it easily in the guards.

...

> I totally agree that I abused of unsafePerformIO, in this case I used
> for the same reason I gave before.

I think an IO (String -> Bool) approach could maybe still work because
you can grab the (String -> Bool) function within IO, and then use it
within a guard.

You'll have to decide if minimising the unsafePerformIO is worth the
extra code complexity or not.  I tend to say it is, but others may
disagree with me.

> >> +cacheSource (Cache _ _ s) = s
> >
> > Potentially useful helper function.  Seems like we could alternatively
> > modify the definition of Cache to use the curly brace syntax.
> >
> I would totally prefer curly brace syntax, do we have any reason for
> not using it ?

I'd say go for it.  I trust you mean something like

  data Cache = Cache { ...
                     , cacheSource :: String }

> I use the whitelist basically to don't test again if the repo is
> reachable or not. ( Given the fact that I can get an error just
> because certain file is not  in that repo, but the repo does exist )

OK, that makes some sense, that there may be a legitimate 404 on a
file that does not exist, but the repository is reachable.  Also, would
it be necessary to worry about other HTTP status codes, such as the
redirect ones (301 and 302?).  Is there a more robust way to do this?

[Being sort of an abstract, high-level kind of guy, working on Darcs
 has been a wonderful way for me to learn how things work a bit more
 on the lower level]

> I agree with you, I tried to work with the exception, unfortunately
> "fromException" was giving nothing to this error ( I will look at it
> again though).

Thanks.  System.IO.Error may help.

> > 3. Case in point, you're matching "Foo" against (map toLower "foo")
> >   which means that this code path is never visited.
> I used specifically for "timeout" since the error could show "Timeout"
> or "timeout".

Sorry, I was referring to this:

  case "No such file or directory" `isInfixOf`  (map toLower e) of

> > If the exception is not a timeout, we test if the URL is reachable and
> > whitelist it if so (the whitelist avoids future testing).  But why do we
> > do this?  And doesn't that confuse Darcs into thinking it successfully
> > fetched a file when it did not?  Have you tested the actual case where
> > you get an exception but not a timeout?
> >
> I did test it, and the reasons I add it to the white list, is to avoid
> inspecting again ( as Petr said).

Do you test it automatically?
 
> I added the trailing slash basically because of the reason Petr said,
> cause some servers treats foo/ and foo differently, but as Petr
> suggest what I should test is if foo/_darcs/hashed_inventory exist.

Yes.

> > In this particular case, you've chosen to break checkSshError into
> > a separate function, but not the HTTP stuff.  Why?
> >
> I did it just because it was getting too long ( yes, I tried to keep
> the columns <= 80, but I had that clumsy idea I mentioned before)

OK, well what I would suggest is to break the local and HTTP stuff out too if
you're going to break the SSH stuff out (now I may decide I was wrong after
looking at the results of this, because I don't yet have a good feel about
what is most readable)...
 
>  I guess I would have to do something like
> "existRemoteFileSsh" and check for foo/_darcs/hashed_inventory

Seems fair enough.

> > Do you really use this?
> >>  import Data.Maybe ( isJust, catMaybes )
> >>  import Control.Monad ( msum )
> >
> That's not mine. I just didn't clean that up, as I think that kind of
> things should go in a different patch.

I was referring to badCache :: CacheLoc -> Bool

This is one of these subtle and very subjective issues that I don't really
have an answer for, but I have a feeling that there is sometimes a point
where you're better off redoing application or composition than writing a
helper function.  An example of this would be a hypothetical

 putStrLnError = hPutStrLn stderr

(In fact, I use this sort of thing in some of my own code).  Seems like
there the cost of the extra helper function to think about is is higher
than that of just repeating 'hPutStrLn stderr'.  Maybe somebody else has
clearer thinking on this tradeoff.  These things aren't big deals, of course.
Maybe there is no right answer and there's no point talking about them.

> > The same sort of worry as above.  What happens if your exception is not
> > a timeout?  I'm concerned that you succeed here, where what you really
> > want to do maybe is re-throw the exception.
> 
> If the exception is not timeout that means that It could be a
> possibility that "repo++"/"++darcsdir++"/inventory" exist, if not, we
> throw the final error.

OK, I have to admit I don't really understand the problem yet (I think Petr
does) so I can trust the two of you.  What I was worried about was that you
were just absorbing the final error instead of throwing it.  Are you sure
you're actually throwing it?
 
> Now I have the doubt of how to go with the ssh server, I mean, how
> could I check that the SSH server is reachable or that the
> _darcs/hashed_inventory exist, a first idea come to my mind maybe with
> ssh, but I'm not sure of portability.

Why not use the preExisting copyFileOrUrl (or was it fetch? not too
sure I remember the difference) functions?

-- 
Eric Kow <http://www.nltg.brighton.ac.uk/home/Eric.Kow>
For a faster response, please try +44 (0)1273 64 2905.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.osuosl.org/pipermail/darcs-users/attachments/20100723/fcd84c45/attachment.pgp>