[darcs-users] darcs patch: test that tentative leftover is cleared (and 2 more)

Jason Dagit dagit at codersbase.com
Sun Sep 6 18:22:19 UTC 2009


Hi Kamil,

Trying to keep the discussion on the list so others can give feedback
as they want.

On Sun, Sep 6, 2009 at 5:03 AM, Kamil Dworakowski<kamil at dworakowski.name> wrote:
> On Sun, Sep 6, 2009 at 5:05 AM, Jason Dagit<dagit at codersbase.com> wrote:
>> Kamil,
>>
>> You've really tackled an unruly bit of code.  It's forced me to read over
>> this part and really try to understand it.  I hope my understanding is
>> correct and hopefully you're also learning a lot from this too.  This
>> version is better than the last, but I suspect I've found some problems with
>> it (see below).
>
> Jason,
>
> Thanks for the time taken on this review. Below I write how I
> understand the transactions in darcs, which I hope will help to
> resolve some of the doubts you have about the patch.

You're welcome and thank you for trying to fix this bug!

> Tentative state corresponds to the real state of the repository. The
> representation is the same except for the tentative_pristine, which is
> stored as a sequence of patches, a diff from the real pristine.

Sure.  That makes sense.

> Operations in a transaction (one such operation is tentatvelyRemovePatches)
> should operate on the tentative state only.

Sounds good.

> The failure described in the bug report was caused by one of the
> functions called by tentativeRemovePatches, namely
> DarcsRepo.remove_from_tentative_inventory, changing the real state of
> the repository. As it was written it was preserving consistency but
> breaking atomicity of any transaction it was used in. I have
> rewritten the function to make changes only to the tentative state and
> thus preserve both properties.

Right.

> The consistency of the tentative state after a failure does not
> matter. The tentative state is cleared before every transaction, see
> my description above.

Hmm...Okay.

>> Looking at add_to_tentative_pristine, I see that it updates a file
>> (tentative_pristine) instead of the actual pristine.  I guess that makes it
>> safer than what I was talking about above.  It could be safer yet by writing
>> to a new file and then move that to be tentative_pristine.
>
> That would break consistency. remove_from_tentative_inventory should
> not assume that it is the only or first operation in a transaction
> (which actually holds, but nevertheless). There might have been an
> arbitrary number of operations on the tentative state before it, each
> of which could have modified the tentative_pristine. In other words,
> we should update the tentative_pristine, not create it based on the
> real pristine, which would drop all the changes previous operations in
> the transaction made.

In that case, the patches we want to remove from the
tentative_pristine need to be removed carefully.  For example,
commuting them to the end and discarding them.  The code as written
writes the inverses to the tentative_pristine.  Do we make any effort
to later collapse inverses and remove the resulting identity patches?
The ideal case is that patches to be removed simply do not exist in
the tentative_pristine when we're done.  Failing that, we need to
simulate a 'rollback' which is what applying their inverse is all
about.

>> If I recall
>> correctly that's how you can do atomic file changes on unix.  Not sure how
>> well that works on windows.  We already do this in
>> finalize_tentative_changes for inventories.
>
> I am not sure if we need the updating of the tentative_pristine file
> to be atomic. We want to have atomicity, and other ACID properties, on
> the level of performing an amend-record for example. Atomicity of
> updating the tentative_pristing file does not matter for the ACID
> properties of the of darcs transactions.

Fair enough.

> Yes. Doesn't darcs optimize remove orphaned patches? I just assumed it
> is the way with darcs because obliterate does not make any effort to
> remove orphaned patches either. But now I can't easily find anything
> in the code that does it. This might be a separate issue.

I'm not aware of any command to clean up garbage patches.  Having it
be an option in optimize makes some sense.  If we can avoid creating
garbage here (I'm treating this issue like a garbage collection issue,
the inventory is our set of root pointers and the patches exist in our
'heap'.), then I think we should try to do so.

> Indeed. The bug looked easy to begin with, but the code changes required
> are quite extensive.

As I understand it, this bug was mistaken for a UI fix and hence
labeled ProbablyEasy in the bug tracker.  Oops.  Mistakes happen.

Thanks,
Jason


More information about the darcs-users mailing list