> -----Original Message-----
> From: Greg Stein [mailto:gstein_at_gmail.com]
> Sent: zaterdag 16 april 2011 0:12
> 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
>
> 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.
In some ways it should go away, and in some ways it shouldn't.
It should be safe to completely replace the op with the install, but that
doesn't look at two things the original commit processing pre wc-ng did
* What if somebody changes the file while we perform the commit?
(The to be committed file is already in the pristine store (used to be in
.svn/tmp); so this doesn't affect the commit itself)
* Can we help our user by not changing the timestamp of their working copy
file?
I'm not sure how relevant the first part is, but I certainly like to keep
the second part.
Take our own working copy:
I edit subversion/includes/svn_types.h
Run a full testrun, etc. etc.
And then commit.
The current code notices that the in-wc actual file is the same as the
committed file and leaves this in the working copy with the current
timestamp (and records the timestamp+size as the unmodified state). So
running make just sees that the file isn't changed, and you can compile a
simple change in a .c file in a few seconds.
When I would perform OP_FILE_INSTALL, the file would be rewritten at commit
time.
And when the next 'make' is started everything is recompiled.
(But this support can certainly be added to the same WQ operation)
<snip>
> > (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.
Thanks :)
(And thanks for all your work to make this possible)
> 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.
Thanks for asking!
These questions and the answers will certainly help others :)
Bert
Received on 2011-04-16 20:44:27 CEST