something

Attachments

• 1624.dpatch
• unnamed

Relate to issue. It's an important issue as it was marked for 2.4 at the Vienna
sprint.

Hi all,

Op zondag 15 november 2009 17:17 schreef Luca Molteni:
> New submission from Luca Molteni <volothamp@gmail.com>:

Looks like it's about time for a review. I have a lot of questions about it.
In a large part, that is because the existing cache code seems a bit dense
to read through.

>[issue1624 break  global cache up into subdirectories
>Luca Molteni <volothamp@gmail.com>**20091115160511
> Ignore-this: ae4555fd3172a53549d90e42993dbce4
>] hunk ./src/Darcs/Repository/Cache.hs 23
> import System.Directory ( removeFile, doesFileExist, getDirectoryContents )
> import System.Posix.Files ( linkCount, getSymbolicLinkStatus )
> import System.IO ( hPutStrLn, stderr )
>+import System.FilePath (takeDirectory)

Nitpick: as you can see, most of darcs  source leaves spaces between the
names of the imported functions and the parenthesis.

> hunk ./src/Darcs/Repository/Cache.hs 113
>  -- | @hashedFilePath cachelocation subdir hash@ returns the physical filename of
>  -- hash @hash@ in the @subdir@ section of @cachelocation@.
>  hashedFilePath :: CacheLoc -> HashedDir -> String -> String
> -hashedFilePath (Cache Directory _ d) s f = d ++ "/" ++ (hashedDir s) ++ "/" ++ f
> +hashedFilePath (Cache Directory _ d) s f = d ++ "/" ++ (hashedDir s) ++ "/" ++ (hashedBucket f) ++ "/" ++ f 
> +          where hashedBucket f = take 2 $ case dropWhile (/='-') f of
> +                                                [] -> f
> +                                                s -> drop 1 s

I initally assumed that f was a filename because of its name, but it is a
hash. But that is not your mistake.

Where does the dash ('-') in the hash come from? In what situation won't it
be present, invoking the [] case in hashedBucket?

What happens with old caches that still use the older names? Aren't they made useless by this change?

> hunk ./src/Darcs/Repository/Cache.hs 146
>  copyFileUsingCache oos (Ca cache) subdir f =
>      do debugMessage $ "I'm doing copyFileUsingCache on "++(hashedDir subdir)++"/"++f
>         Just stickItHere <- cacheLoc cache
> -       createDirectoryIfMissing False (reverse $ dropWhile (/='/') $ reverse stickItHere)
> +       createBucket stickItHere
>         sfuc cache stickItHere
>      `catchall` return ()
>      where cacheLoc [] = return Nothing

I'm willing to believe that this change does the right thing, but the
copyFileUsingCache function is hermetic poetry to me.

> hunk ./src/Darcs/Repository/Cache.hs 168
>  
>  data FromWhere = LocalOnly | Anywhere deriving ( Eq )
>  
> +createBucket :: String -> IO ()
> +createBucket s = createDirectoryIfMissing True (takeDirectory s)
> +

Why do you change the first argument of createDirectoryIfMissing to True?

> hunk ./src/Darcs/Repository/Cache.hs 206
>                   return (fn c, x)
>                `catchall` do (fname,x) <- ffuc cs
>                              do createCache c subdir
> +                               createBucket (fn c)
>                                 createLink fname (fn c)
>                                 return (fn c, x)
>                               `catchall`

Why is this necessary?

> hunk ./src/Darcs/Repository/Cache.hs 247
>      where hash = cacheHash ps
>            wfuc (c:cs) | not $ writable c = wfuc cs
>                        | otherwise = do createCache c subdir
> +                                       createBucket (fn c)
>                                         write compr (fn c) ps -- FIXME: create links in caches
>                                         return hash
>            wfuc [] = debugFail $ "No location to write file `" ++ (hashedDir subdir) ++"/"++hash ++ "'"

And this?

> hunk ./src/Darcs/Repository/Cache.hs 252
>            fn c = hashedFilePath c subdir hash
> +          

This looks like a spurious whitespace change. They may cause unnecessary
conflicts later on, hence they are not so popular.

Bye,
Reinier

Reinier Lamers <tux_rocker@reinier.de> writes:

> Where does the dash ('-') in the hash come from? In what situation won't it
> be present, invoking the [] case in hashedBucket?
The dash separates the size (formatted to 10 decimal digits) from the
hash. There was no dash in old darcs 2 (probably pre-2.0) versions and
there will be no dash in future darcs versions. We should handle both
cases by properly bucketing. (If there is no dash, the filename is the
hash alone (maybe a suffix, but probably not important, that).) Reading
the code, it seems to me that this should work just fine for both cases
(although the no-dash version might be made a little more efficient, I'd
guess).

Yours,
   Petr.

On Wed, Dec 23, 2009 at 12:00:43 +0100, Luca Molteni wrote:
> Have you already applied this? I may have found a bug on it.

The patch tracker says it was still being reviewed (and it's
not in darcs changes).

Ah-hah! Here's another chance to try an amendment.
Hopefully, it's just a "simple" matter of doing

  darcs send --subject '[patch72]'

I'll just cross my fingers :-)

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

I did a review and I head a number of questions. Petr answered one of them, but
I'd like to hear from Luca about the others.

Also, I'd like to know what Luca thinks is a problem with this implementation.

Hi Luca,

May I nag you for your patch?  I'd like to see this make it into darcs 2.4, if
possible.

Eric,

I didn't manage to reproduce the bug, yet.

It should be a good sign. I'll try again this evening, and I'll see if
I'm missing something.

L.M.



2010/1/5 Eric Kow <bugs@darcs.net>:
>
> Eric Kow <kowey@darcs.net> added the comment:
>
> Hi Luca,
>
> May I nag you for your patch?  I'd like to see this make it into darcs 2.4, if
> possible.
>
> __________________________________
> Darcs bug tracker <bugs@darcs.net>
> <http://bugs.darcs.net/patch72>
> __________________________________
>

Ok, no problem found using those steps I wrote.

Since no one else reported something similar, I can reasonably think
that there is no bug, and that folder I found on my machine was
created by another bug, much harder to reproduce.

Let me know if there are open questions on this patch, I thought I
answered all of them.

L.M.



2010/1/7 Luca Molteni <volothamp@gmail.com>:
> Eric,
>
> I didn't manage to reproduce the bug, yet.
>
> It should be a good sign. I'll try again this evening, and I'll see if
> I'm missing something.
>
> L.M.
>
>
>
> 2010/1/5 Eric Kow <bugs@darcs.net>:
>>
>> Eric Kow <kowey@darcs.net> added the comment:
>>
>> Hi Luca,
>>
>> May I nag you for your patch?  I'd like to see this make it into darcs 2.4, if
>> possible.
>>
>> __________________________________
>> Darcs bug tracker <bugs@darcs.net>
>> <http://bugs.darcs.net/patch72>
>> __________________________________
>>
>

I think Reinier asked several here which aren't yet answered: http://bugs.darcs.net/msg9504

Luca Molteni wrote:
> Ok, no problem found using those steps I wrote.
> 
> Since no one else reported something similar, I can reasonably think
> that there is no bug, and that folder I found on my machine was
> created by another bug, much harder to reproduce.  
> 
> Let me know if there are open questions on this patch, I thought I
> answered all of them. 
> 
> L.M.
> 
> 
> 
> 2010/1/7 Luca Molteni <volothamp@gmail.com>:
>> Eric,
>> 
>> I didn't manage to reproduce the bug, yet.
>> 
>> It should be a good sign. I'll try again this evening, and I'll see
>> if I'm missing something. 
>> 
>> L.M.
>> 
>> 
>> 
>> 2010/1/5 Eric Kow <bugs@darcs.net>:
>>> 
>>> Eric Kow <kowey@darcs.net> added the comment:
>>> 
>>> Hi Luca,
>>> 
>>> May I nag you for your patch?  I'd like to see this make it into
>>> darcs 2.4, if possible. 
>>> 
>>> __________________________________
>>> Darcs bug tracker <bugs@darcs.net>
>>> <http://bugs.darcs.net/patch72>
>>> __________________________________
>>> 
>> 
> _______________________________________________
> darcs-users mailing list
> darcs-users@darcs.net
> http://lists.osuosl.org/mailman/listinfo/darcs-users


=============================================================================== 
 Please access the attached hyperlink for an important electronic communications disclaimer: 
 http://www.credit-suisse.com/legal/en/disclaimer_email_ib.html 
 ===============================================================================

I just checked. I replied to that email, but unfortunately it doesn't
appear in the bug tracker.

The main problem is: did anyone receive that email? :)

And my amended-patch?

I hope so.

Bye, thanks.

L.M.



2010/1/8 Sittampalam, Ganesh <ganesh.sittampalam@credit-suisse.com>:
> I think Reinier asked several here which aren't yet answered: http://bugs.darcs.net/msg9504
>
> Luca Molteni wrote:
>> Ok, no problem found using those steps I wrote.
>>
>> Since no one else reported something similar, I can reasonably think
>> that there is no bug, and that folder I found on my machine was
>> created by another bug, much harder to reproduce.
>>
>> Let me know if there are open questions on this patch, I thought I
>> answered all of them.
>>
>> L.M.
>>
>>
>>
>> 2010/1/7 Luca Molteni <volothamp@gmail.com>:
>>> Eric,
>>>
>>> I didn't manage to reproduce the bug, yet.
>>>
>>> It should be a good sign. I'll try again this evening, and I'll see
>>> if I'm missing something.
>>>
>>> L.M.
>>>
>>>
>>>
>>> 2010/1/5 Eric Kow <bugs@darcs.net>:
>>>>
>>>> Eric Kow <kowey@darcs.net> added the comment:
>>>>
>>>> Hi Luca,
>>>>
>>>> May I nag you for your patch?  I'd like to see this make it into
>>>> darcs 2.4, if possible.
>>>>
>>>> __________________________________
>>>> Darcs bug tracker <bugs@darcs.net>
>>>> <http://bugs.darcs.net/patch72>
>>>> __________________________________
>>>>
>>>
>> _______________________________________________
>> darcs-users mailing list
>> darcs-users@darcs.net
>> http://lists.osuosl.org/mailman/listinfo/darcs-users
>
>
> ===============================================================================
>  Please access the attached hyperlink for an important electronic communications disclaimer:
>  http://www.credit-suisse.com/legal/en/disclaimer_email_ib.html
>  ===============================================================================
>
>

That's annoying :-( Can you find it in the mailing list archives and post a link, or otherwise resend it?

Luca Molteni wrote:
> I just checked. I replied to that email, but unfortunately it doesn't
> appear in the bug tracker. 
> 
> The main problem is: did anyone receive that email? :)
> 
> And my amended-patch?
> 
> I hope so.
> 
> Bye, thanks.
> 
> L.M.
> 
> 
> 
> 2010/1/8 Sittampalam, Ganesh <ganesh.sittampalam@credit-suisse.com>:
>> I think Reinier asked several here which aren't yet answered:
>> http://bugs.darcs.net/msg9504
>> 
>> Luca Molteni wrote:
>>> Ok, no problem found using those steps I wrote.
>>> 
>>> Since no one else reported something similar, I can reasonably think
>>> that there is no bug, and that folder I found on my machine was
>>> created by another bug, much harder to reproduce.
>>> 
>>> Let me know if there are open questions on this patch, I thought I
>>> answered all of them. 
>>> 
>>> L.M.
>>> 
>>> 
>>> 
>>> 2010/1/7 Luca Molteni <volothamp@gmail.com>:
>>>> Eric,
>>>> 
>>>> I didn't manage to reproduce the bug, yet.
>>>> 
>>>> It should be a good sign. I'll try again this evening, and I'll
>>>> see if I'm missing something. 
>>>> 
>>>> L.M.
>>>> 
>>>> 
>>>> 
>>>> 2010/1/5 Eric Kow <bugs@darcs.net>:
>>>>> 
>>>>> Eric Kow <kowey@darcs.net> added the comment:
>>>>> 
>>>>> Hi Luca,
>>>>> 
>>>>> May I nag you for your patch?  I'd like to see this make it into
>>>>> darcs 2.4, if possible. 
>>>>> 
>>>>> __________________________________
>>>>> Darcs bug tracker <bugs@darcs.net>
>>>>> <http://bugs.darcs.net/patch72>
>>>>> __________________________________
>>>>> 
>>>> 
>>> _______________________________________________
>>> darcs-users mailing list
>>> darcs-users@darcs.net
>>> http://lists.osuosl.org/mailman/listinfo/darcs-users
>> 
>> 
>> ======================================================================
>> ========= 
>>  Please access the attached hyperlink for an important electronic
>> communications disclaimer:
>> http://www.credit-suisse.com/legal/en/disclaimer_email_ib.html 
>> 
>> ======================================================================
>> =========


=============================================================================== 
 Please access the attached hyperlink for an important electronic communications disclaimer: 
 http://www.credit-suisse.com/legal/en/disclaimer_email_ib.html 
 ===============================================================================

Here's the link to the amended patch:

http://lists.osuosl.org/pipermail/darcs-users/2009-December/022506.html

I will forward my email again, sorry.

L.M.



2010/1/8 Sittampalam, Ganesh <ganesh.sittampalam@credit-suisse.com>:
> That's annoying :-( Can you find it in the mailing list archives and post a link, or otherwise resend it?
>
> Luca Molteni wrote:
>> I just checked. I replied to that email, but unfortunately it doesn't
>> appear in the bug tracker.
>>
>> The main problem is: did anyone receive that email? :)
>>
>> And my amended-patch?
>>
>> I hope so.
>>
>> Bye, thanks.
>>
>> L.M.
>>
>>
>>
>> 2010/1/8 Sittampalam, Ganesh <ganesh.sittampalam@credit-suisse.com>:
>>> I think Reinier asked several here which aren't yet answered:
>>> http://bugs.darcs.net/msg9504
>>>
>>> Luca Molteni wrote:
>>>> Ok, no problem found using those steps I wrote.
>>>>
>>>> Since no one else reported something similar, I can reasonably think
>>>> that there is no bug, and that folder I found on my machine was
>>>> created by another bug, much harder to reproduce.
>>>>
>>>> Let me know if there are open questions on this patch, I thought I
>>>> answered all of them.
>>>>
>>>> L.M.
>>>>
>>>>
>>>>
>>>> 2010/1/7 Luca Molteni <volothamp@gmail.com>:
>>>>> Eric,
>>>>>
>>>>> I didn't manage to reproduce the bug, yet.
>>>>>
>>>>> It should be a good sign. I'll try again this evening, and I'll
>>>>> see if I'm missing something.
>>>>>
>>>>> L.M.
>>>>>
>>>>>
>>>>>
>>>>> 2010/1/5 Eric Kow <bugs@darcs.net>:
>>>>>>
>>>>>> Eric Kow <kowey@darcs.net> added the comment:
>>>>>>
>>>>>> Hi Luca,
>>>>>>
>>>>>> May I nag you for your patch?  I'd like to see this make it into
>>>>>> darcs 2.4, if possible.
>>>>>>
>>>>>> __________________________________
>>>>>> Darcs bug tracker <bugs@darcs.net>
>>>>>> <http://bugs.darcs.net/patch72>
>>>>>> __________________________________
>>>>>>
>>>>>
>>>> _______________________________________________
>>>> darcs-users mailing list
>>>> darcs-users@darcs.net
>>>> http://lists.osuosl.org/mailman/listinfo/darcs-users
>>>
>>>
>>> ======================================================================
>>> =========
>>>  Please access the attached hyperlink for an important electronic
>>> communications disclaimer:
>>> http://www.credit-suisse.com/legal/en/disclaimer_email_ib.html
>>>
>>> ======================================================================
>>> =========
>
>
> ===============================================================================
>  Please access the attached hyperlink for an important electronic communications disclaimer:
>  http://www.credit-suisse.com/legal/en/disclaimer_email_ib.html
>  ===============================================================================
>
>

L.M.



---------- Forwarded message ----------
From: Luca Molteni <volothamp@gmail.com>
Date: 2009/12/28
Subject: Re: [patch72] resolve issue1624 break global cache up into
subdirectories
To: darcs-users@darcs.net
Cc: kowey@darcs.net, me@mornfall.net, tux_rocker@reinier.de


2009/11/29 Reinier Lamers <tux_rocker@reinier.de>:

> What happens with old caches that still use the older names? Aren't they made useless by this change?

The old cache will stay in the same position as before. This patch
doesn't delete any files. From now on, the new cache will be read
instead of the old one.
You can manually delete the cache, or leave it as it is.
Maybe in the a new release we can force the deletion of the old files.


>> hunk ./src/Darcs/Repository/Cache.hs 168
>>
>>  data FromWhere = LocalOnly | Anywhere deriving ( Eq )
>>
>> +createBucket :: String -> IO ()
>> +createBucket s = createDirectoryIfMissing True (takeDirectory s)
>> +
>
> Why do you change the first argument of createDirectoryIfMissing to True?

This is the function that creates the directories of the cache.
We create a subdir for each group and we have to create the parent
cache directory too. This is the meaning of True in
createDirectoryIfMissing.

The createBucket function is called before writing in the cache. If
you don't create the directory, you can't write in it.

>> hunk ./src/Darcs/Repository/Cache.hs 206
>>                   return (fn c, x)
>>                `catchall` do (fname,x) <- ffuc cs
>>                              do createCache c subdir
>> +                               createBucket (fn c)
>>                                 createLink fname (fn c)
>>                                 return (fn c, x)
>>                               `catchall`
>
> Why is this necessary?
>
>> hunk ./src/Darcs/Repository/Cache.hs 247
>>      where hash = cacheHash ps
>>            wfuc (c:cs) | not $ writable c = wfuc cs
>>                        | otherwise = do createCache c subdir
>> +                                       createBucket (fn c)
>>                                         write compr (fn c) ps -- FIXME: create links in caches
>>                                         return hash
>>            wfuc [] = debugFail $ "No location to write file `" ++ (hashedDir subdir) ++"/"++hash ++ "'"
>
> Bye,
> Reinier


The problem I found with this implementation is that the folder where
there the cache is is copied if you take the repository and copy it to
another machine, instead of using "darcs get".

I think this may be a side effect of changing createDirectoryIfMissing
to True. Now it creates the whole tree if it doesn't exixts, leading
to very strange results (I found a unix path with the darcs cache on
my windows machine :|).

I'm still not sure about that, I think I'll be able to investigate
that in these days.

Steps to reproduce that:

Copy a repository from a Unix machine using cp

Modify your repository on a windows machine and record a patch.

See if something bad happened (strange folders in your machine).


Bye,

L.M.

Hi Reinier,

If I understand correctly, the amended patch is on this thread:
http://lists.osuosl.org/pipermail/darcs-users/2009-December/022506.html

(just bumping this up).  Thanks!

Could we please get a status update on this patch?

Thanks!

I've got some different implementations. I think we're going to
discuss them on Saturday or Sunday. Will you be at the Zurihac?

L.M.



On 18 March 2010 03:38, Jason Dagit <bugs@darcs.net> wrote:
>
> Jason Dagit <dagitj@gmail.com> added the comment:
>
> Could we please get a status update on this patch?
>
> Thanks!
>
> ----------
> nosy: +dagit
>
> __________________________________
> Darcs bug tracker <bugs@darcs.net>
> <http://bugs.darcs.net/patch72>
> __________________________________
>

On Thu, Mar 18, 2010 at 2:27 AM, Luca Molteni <volothamp@gmail.com> wrote:

> I've got some different implementations. I think we're going to
> discuss them on Saturday or Sunday. Will you be at the Zurihac?
>

I won't, but many many other darcs devs will be!

Jason

Attachments

• unnamed

Luca (volothamp), Petr (mornfall) and I (tux_rocker) talked about this
at the Zürich sprint. We decided to use a sort of on-line cache
conversion leaving the old cache in place. It works as follows:

Store the bucketed cache and the old-style cache in different directories.
Every time you look up a file in the cache:
    If the new-style cache exists:
        use that.
    Else:
        If the old-style cache exists:
            Create a new bucketed cache, hardlinked to the old-style one
        Else:
            Create a new, empty bucketed cache

Luca, you mentioned on IRC on Sunday before leaving that you have
potentially found a new bug on this?  Please confirm.

I'm marking amend-in-progress in the meantime.

We're almost there! :-)

Yes I have almost completed the implementation, but unfortunately in
the current state it breaks the whole pull system with a very bad
error (I/O error) :)

I think I can finish it in a few days, otherwise I will call you guys for help.

Thanks for your understanding.

L.M.



On 22 March 2010 18:01, Eric Kow <bugs@darcs.net> wrote:
>
> Eric Kow <kowey@darcs.net> added the comment:
>
> Luca, you mentioned on IRC on Sunday before leaving that you have
> potentially found a new bug on this?  Please confirm.
>
> I'm marking amend-in-progress in the meantime.
>
> We're almost there! :-)
>
> ----------
> assignedto: tux_rocker -> volothamp
> nosy: +ganesh -darcs-users, ganesh.sittampalam
> status: needs-review -> amend-in-progress
>
> __________________________________
> Darcs bug tracker <bugs@darcs.net>
> <http://bugs.darcs.net/patch72>
> __________________________________
>

Hi Luca,

Op maandag 22 maart 2010 18:06 schreef je:
> Yes I have almost completed the implementation, but unfortunately in
> the current state it breaks the whole pull system with a very bad
> error (I/O error) :)
> 
> I think I can finish it in a few days, otherwise I will call you guys for 
help.

Any update? Do you think you can fix it before the darcs 2.5 code freeze on 
the 8th of July?

Regards,
Reinier

Could we please get an update on the status of this?  The last update from 
volothamp was 2010-03-22.

I think we should try to reassign the work if Luca is unavailable.  Do we 
have the most recent copy of these patches?

Closing this thread (marking the patch as 'rejected') since it is not a
work-in-progress since more than 1 year and a half. Reopen if you disagree.

Page of issue1624 is still open to discuss this issue.