[darcs-users] darcs patch: Parametrize "pristine.subdir" in a bunch... (and 10 more)

Petr Rockai me at mornfall.net
Sun Aug 10 23:36:08 UTC 2008


Hi,

first of all, thanks for review. Heads-up summary: I will pull latest upstream
and work on the patchset a bit, amending/re-recording as needed. Will resend,
hopefully by next evening CEST (it's middle of the night now).

David Roundy <droundy at darcs.net> writes:
> See below for the comments written while reviewing your patch.  After
> writing these comments up, I found that the very first patch won't compile
> in my repository.  I suspect this is due to bad interactions with another
> patch I've applied, but am not sure which.  Could you try pulling the
> latest from darcs.net and see if you can fix what's broken? Thanks!
Will do.

>> As the bundle is, it makes one test fail, and dunno if it's a
>> problem with the test (pending_has_conflicts.pl), or elsewhere. Please review
>> on that.
>
> Given this caveat, I'm going to review carefully (which I'd hopefully do
> anyhow, since this is a pretty invasive patch).  I also may apply only
> subsets, if I run out of time to read the entire sequence.
Just to sum up the issue: this test seems to assume that repair will work with
inapplicable pending, which for some reason doesn't break with the traditional
repair and seems to break with hashed one. I'll try to investigate.

>> PS: Just small notes on working with the code: I find the import rigidity to
>> be quite annoying. Not sure how to fix that though. Also, it seems that make
>> is taking a lot of time to run... anyone tried to compare that against ghc
>> --make, eg?
>
> I'm not sure I follow what you mean by import rigidity.  If you compile
> without optimization the make will go much faster (it'll recompile fewer
> files) due to fewer dependencies (i.e. less inlining).
Ah, will try that trick out (I'm rather new to all this...).  As for imports:
it seems to usually take me two or three compile passes to fix all imports, as
everything is explicit and you get warnings for unused bits. So I guess that's
fine, but updating the imports is time-consuming. Maybe I just need to learn
new tricks.

>> Sat Aug  9 17:52:17 CEST 2008  me at mornfall.net
>>   * Add copyoutSlurpy to roll out a copy of slurpy into a filesystem.
>
> I'm not sure that I like the behavior of copyoutSlurpy.  I think I'd rather
> it created a new directory, and I'd probably rather call it writeSlurpy.
> These changes would greatly reduce the likelihood of errors cropping up
> that you'll have to deal with (in particular, if the directory specified
> doesn't exist or is not empty).
Yes, I've been at a loss with regard to naming copyoutSlurpy. Indeed, making it
create the directory sounds like a good idea too (and failing if it already
exists). Will do.

>> Sat Aug  9 18:03:03 CEST 2008  me at mornfall.net
>>   * Add Repository.replacePristineFromSlurpy.
>
> I see that this is where you first copyoutSlurpy.  The idea is fine, and
> the code isn't bad, but it's got one major problem, which is that when
> you're adding support to Pristine, you've added a race condition:
>
> +replacePristineFromSlurpy s (PlainPristine n) =
> +    do rm_recursive nold
> +           `catchall` return ()
> +       renameDirectory n nold
> +       createDirectory n
> +       copyoutSlurpy s n
> +       return ()
> +           where nold = darcsdir ++ "/" ++ pristineName ++ "-old"
>
> Here you can see that if darcs is killed (or dies) after the
> renameDirectory but before the copyoutSlurpy is completed, our users are
> guaranteed to be left with a corrupt repository.  This may not seem like
> such a big deal--since the primary intended use is in repair, and they've
> probably already got a corrupt repository--but you never know where else
> this code might get used sometime in the future.  So far as I know, there's
> no way to atomically update a directory, but we can at least arrange it so
> that a there is a minimum period during which the repository could be
> corrupted by first writing out the new directory (with a new name) and then
> quickly renaming the old and new.
>
> I'm going to stop here (as this reviewing is taking quite a bit of time).
> If you find time to rewrite and amend copyoutSlurpy and this patch, that'd
> be great.  This is going to be a very busy (grant-writing) week, so I might
> take a while before I have another chance to look at this (which gives you
> some time to amend-record if you'd like--I'd appreciate that).
Yes, no problem with that. It didn't occur to me, but I have seen the rename
trick elsewhere.

>
> The rest of the patch sequence I'm only going to review your
> descriptions...
>
>> Sat Aug  9 18:05:24 CEST 2008  me at mornfall.net
>>   * First working (albeit slow) version of repair that uses hashed newpristine.
>> 
>> Sat Aug  9 18:11:03 CEST 2008  me at mornfall.net
>>   * Add forceHashSlurped that hashes the slurpy even if it already contains hashes.
>
> What is the purpose of this? The point of caching the hashes was to avoid
> doing this extra IO.  If there's a bug in the caching, we should fix that
> bug rather than working around it!
Ah, this was a rather tricky problem: hashSlurped skips writing out files that
already have their hashes. And if you are trying to hashSlurped a Slurpy that
came from slurpHashed, that's a noop (!), even if you try to hash it into a
different directory. This seemed like the least-intrusive way to fix that for
the only case where I needed to do that (replacePristineFromSlurpy, which needs
to work with slurpHashed slurpies).

>> Sat Aug  9 18:40:06 CEST 2008  me at mornfall.net
>>   * Be a little more paranoid, IgnoreTimes when doing checkPristineAgainstSlurpy.
>
> Paranoia is good in check and repair.  It might also be worth having an
> additional old-fashioned check that actually does do the writes to an
> ordinary directory (as the existing code does), just so we can verify that
> there isn't some sort of bug in the writing-to-disk code of darcs (which
> could lead to a very subtly corrupt repository).
What I have been thinking was adding a --hashed flag to repair and check as
well (and probably make it default). Use the original code if it's --no-hashed
or so. Would that work?

>> Sat Aug  9 18:40:49 CEST 2008  me at mornfall.net
>>   * Fix about half zillion warnings I introduced inadverently.
>
> Hmmmm.  It'd be nicer to fix these with amend-record where possible.
Yes, I have tried, but it's been running late and it's been a little too messy
all around. I'll give it a new run and will try to make all the patches clean.

>> Sat Aug  9 19:00:25 CEST 2008  me at mornfall.net
>>   * Only "update" (sync to disk) the slurpy ever so often.
>
> On the whole, this looks like a nice sequence.  You've got the right idea,
> and apart from minor techincal issues, your code looks great.  Thanks!
Thanks. : - )

Yours,
   Petr.

-- 
Peter Rockai | me()mornfall!net | prockai()redhat!com
 http://blog.mornfall.net | http://web.mornfall.net

"In My Egotistical Opinion, most people's C programs should be
 indented six feet downward and covered with dirt."
     -- Blair P. Houghton on the subject of C program indentation


More information about the darcs-users mailing list