On Fri, Apr 15, 2011 at 16:20, Bert Huijben <bert_at_qqmail.nl> wrote:
>> -----Original Message-----
>> From: Greg Stein [mailto:gstein_at_gmail.com]
>> Sent: vrijdag 15 april 2011 22:00
>> To: Bert Huijben
>> Cc: dev_at_subversion.apache.org
>> Subject: Re: svn commit: r1092660 - in
>> /subversion/trunk/subversion/libsvn_wc: adm_ops.c crop.c wc_db.c
>> wc_db.h
>
>> But I still don't think this change is right. You immediately remove
>> nodes, or you queue up a commit operation (in order to batch the
>> entire commit). What am I missing here?
>
> The rest of the batch of commits ;-)
hehe :-) ... it is just that things like this can also be viewed in
isolation, and didn't seem right.
> I started by moving that remove deleted operation which had to be loggy in
> the entries+access baton world, but is 100% database now. (We don't leave
> directories with data, to be removed later).
>
> This is essentialy the db-only side of svn_wc_remove_from_revision_control2,
> which is just a recursive delete
>
> So this handles committing svn_wc__db_status_deleted.
>
> And I replaced the queuing of this operation with performing this operation
> itself. (We explicitly ran the wq to avoid recursing over deleted nodes. So
> this was a safe change)
Gotcha. It was that queue-run that we did which makes these equivalent.
That said... we don't do the queue-run any more (you took out those,
leaving one final run, iirc), so it would seem that we still want to
(eventually) reach one big set of wq operations before *any* db or
filesystem changes are made. Right?
> Then I handled the specialized case where we shadow nodes. (The old
> 'replaced' and the new cases where we can have more than 2 layers). I moved
> this into the db_op, as it is a very easy recursive sql operation: Delete
> all nodes at shadowed op_depths.
> This is a safe operation once the op_root defining the old top level
> operation is committed (read: moved to BASE / op_depth 0). And that happens
> in the same transaction.
Alrighty.
> And then we had just two commit operations left:
> svn_wc__db_status_added and svn_wc__db_status_normal.
>
> And you already implemented the support for these two operations in wc_db
> :-)
heh.
> Before my patches, the commit for these statee was handled in the
> post_commit wq operation:
>
> - This fetched old information from the db
> - Changed the DB
> - Applied differences to the working copy.
>
> This order of events made it a not restartable wq operation. (Once the db is
> changed, the differences may be lost)
Right. It is effectively the old loggy operation, just cut and paste
into the workqueue. The things you're doing now were "to be done at
some future point".
> In r1092708, I changed this into a wq operation with only these steps:
> - Fetch information
> - Change the DB *and* install wq operation.
> And a separate wq operation for applying.
Yup, looks great. I will also note that OP_FILE_COMMIT "should"
possibly go away in favor of a simple OP_FILE_INSTALL. IOW, is there
something specific that makes OP_FILE_COMMIT different, and if so...
why? Logically, it would seem INSTALL is all that is necessary.
> But with that change it was also possible to do the fetch, change and
> install outside the wq.
>
> So that is the change I applied in r1092712.
Ah. Very nice. I just updated to r1092711, and yeah...
log_do_committed is a simple shell around the db operation. So this is
a pretty straightforward shift.
> (And then Mark Phippard ran into a few problems in svn_client_commit, which
> I tried to solve in the last hour before rushing to the daycare for my
> children...)
*nod*
This stuff is really looking great. We'll finally have some sanity to
the commit process, and the code looks like it is getting much simpler
(and thus, understandable!) ... the commit code pre-wc-ng had so many
corner cases, it was really hard to follow.
I do believe that the endpoint is when
svn_wc_process_committed_queue2() queues up all of the individual
commit processing, and then runs the whole batch. That way, we can
perform the entire commit... or nothing. The current code can still
leave the working copy in a partially-committed state, which isn't
really all that cool. My original concern with this commit (r1092660)
is that moving from workqueue-to-not, but that is effectively where we
already were (ie. partial-commits). But this commit certainly helps to
improve the flow and understandability. Thus, it is certainly better
than before!
Thanks for taking the time, and all the detailed explanation on this
stuff. It has really helped.
Cheers,
-g
Received on 2011-04-16 00:12:04 CEST