> -----Original Message-----
> From: Philip Martin [mailto:philip.martin_at_wandisco.com]
> Sent: woensdag 24 februari 2010 12:08
> To: Bert Huijben
> Cc: dev_at_subversion.apache.org; philip_at_apache.org
> Subject: Re: svn commit: r915378 - in /subversion/trunk: notes/wc-
> ng/transitions subversion/libsvn_wc/wc-queries.sql
> subversion/libsvn_wc/wc_db.c
>
> "Bert Huijben" <bert_at_vmoo.com> writes:
>
> >> +-- STMT_INSERT_WORKING_NODE_FROM_BASE_NODE
> >> +INSERT INTO WORKING_NODE (
> >> + wc_id, local_relpath, parent_relpath, presence, kind, checksum,
> >> + translated_size, changed_rev, changed_date, changed_author,
> depth,
> >> + symlink_target, last_mod_time )
> >> +SELECT wc_id, local_relpath, parent_relpath, presence, kind,
> checksum,
> >> + translated_size, changed_rev, changed_date, changed_author,
> depth,
> >> + symlink_target, last_mod_time FROM BASE_NODE WHERE wc_id = ?1
> >> AND
> >> +local_relpath = ?2;
> >
> > I'm not sure how generic this copy from base to working is. I think
> > it is only valid for a few of the presence states. (Maybe use a more
> > specific name?)
>
> I'd really like an SQL statement that allows me to set the presence as
> ?3 while pulling the other values from BASE, but I don't know if that
> is possible.
I think you should be able to use SELECT wc_id, local_relpath,
parent_relpath, ?3 AS presence, ...
for this case. (I know you can just select a constant for the insert, but I
didn't check if you can fetch the value from a bound value)
>
> >> +/* A WORKING version of svn_wc__db_base_get_info, stripped down to
> >> + just the status column. */
> >> +static svn_error_t *
> >> +db_working_get_status(svn_wc__db_status_t *status,
> >> + svn_wc__db_t *db,
> >> + const char *local_abspath,
> >> + apr_pool_t *result_pool,
> >> + apr_pool_t *scratch_pool) {
> >
> > Why reimplement this whole function if a
> >
> > return svn_error_return(
> > svn_wc__db_read_status(status, ..... several NULLs..., db,
> local_abspath, scratch_pool...) would suffice?
> >
> > _read_info is optimized for the case with many NULLs and has tests
> > on it. (And we didn't see performance reasons yet to do this same
> > split for several other helpers)
>
> I want both the base and the working presence, the base presence comes
> from base_get_info but I couldn't see how to reliably distinguish
> working presence from base presence when using _read_info. I get
> SVN_ERR_WC_PATH_NOT_FOUND for no base and no working, and
> base_shadowed TRUE for base and working, but it was not clear how to
> distinguish base and no working from no base and working.
Greg?
I think the idea was that you always should be able to tell the difference
from the status?
Deleting incomplete, excluded or absent should be a user error.
>
> > (The result_pool argument from your function is not necessary as
> > status is no structure that needs allocation)
>
> Noted.
>
> >> +/* Update the working node for LOCAL_ABSPATH setting
> presence=STATUS
> >> */
> >> +static svn_error_t * db_working_update_presence(svn_wc__db_status_t
> >> +status,
> >> + svn_wc__db_t *db,
> >> + const char *local_abspath,
> >> + apr_pool_t *scratch_pool)
> >
> > For which presence states does this function work?
>
> It uses presence_map.
>
> > I think the supported list should be in the docstring and asserted
> > on top of the function, like all other svn_wc__db functions do.
> >
> > (I reviewed the base-deleted case... I don't think it is safe for
> > any other presence?)
>
> It's used to set base-deleted or not-present. What do you mean by safe?
Passing absent, excluded, incomplete, would give a +- undefined database
state.
>
> >> +/* Insert a working node for LOCAL_ABSPATH with presence=STATUS. */
> >> +static svn_error_t * db_working_insert(svn_wc__db_status_t status,
> >> + svn_wc__db_t *db,
> >> + const char *local_abspath,
> >> + apr_pool_t *scratch_pool) {
> >
> > Which presences does this support?
> >
> > Only base-delete?
>
> I believe so, I could have hard-coded it.
>
> > It certainly can't handle cases where the node kind between BASE and
> > WORKING differ and many other cases.
> >
> >> +static svn_error_t*
> >> +is_root_of_copy(svn_boolean_t *root_of_copy,
> >> + svn_wc__db_t *db,
> >> + const char *local_abspath,
> >> + apr_pool_t *scratch_pool) {
> >> + svn_wc__db_status_t status;
> >> + const char *op_root_abspath;
> >> + const char *original_repos_relpath, *original_repos_root;
> >> + const char *original_repos_uuid;
> >> + svn_revnum_t original_revision;
> >> +
> >> + SVN_ERR(svn_wc__db_scan_addition(&status, &op_root_abspath,
> >> + NULL, NULL, NULL,
> >> + &original_repos_relpath,
> >> + &original_repos_root,
> >> + &original_repos_uuid,
> >> + &original_revision,
> >> + db, local_abspath,
> >> + scratch_pool, scratch_pool));
> >>
> >> - if (!deleted)
> >> - {
> >> - /* This case is easy.. Just delete the entry */
> >> - SVN_ERR(svn_wc__entry_remove(db, local_abspath,
> scratch_pool));
> >> - SVN_ERR(svn_wc__db_temp_forget_directory(db,
> local_abspath,
> >> scratch_pool));
> >> + SVN_ERR_ASSERT(status == svn_wc__db_status_added
> >> + || status == svn_wc__db_status_copied);
> >>
> >> - return SVN_NO_ERROR;
> >> + *root_of_copy = op_root_abspath && !strcmp(local_abspath,
> >> + op_root_abspath);
> >> +
> >> + if (*root_of_copy && status == svn_wc__db_status_copied)
> >> + {
> >> + /* ### merge sets the wrong copyfrom when adding a tree and
> so
> >> + the root detection above is unreliable. I'm "fixing"
> it
> >> + here because I just need to detect whether this is an
> >> + instance of the merge bug, and that's easier than
> fixing
> >> + scan_addition or merge. */
> >
> > Adds can be merged by design, so you don't just see this behavior
> > below merges!
>
> I saw an explict merge bug that caused op_root_abspath to have the
> wrong value. I didn't see that behaviour with add but perhaps the
> regresssion tests don't trigger it.
>
> I think I should rename this function. It's about making the "remove
> or set not-present" decision, I've been using "root of copy" but
> really it's "add or root of copy" versus "within copy".
In WC-1.0 we had a hard time describing simple copies. With WC-NG we can
describe more advanced copies and we certainly need more test here...
(I added a few simple ones to show current failures)
>
> > Calls like
> > svn cp ^/trunk A
> > svn cp ^/branches/B A/B
> >
> > give you two copy roots below each other (and you can have N of
> > them). You really need to loop/recurse upwards to find the place
> > where the copy is added to or replaces a base node.
> >
> > Each of these needs different handling on delete with WC-NG. (A/B
> > was not really handled that well via entries especially if it was a
> > replacement).
> >
> > This specific case also touches the issue that we can't describe a
> > local add below a copy yet. (I think you raised that issue yourself)
>
> I've also noticed that I left in the STMT_UPDATE_WORKING_KIND code
> which is not needed. It was temporary code I put in while trying to
> get the parent stub stuff to work.
I really like that your implementation can safely ignore those parent
stubs...
I was still thinking about how we should fix that to get this working the
right way... but for a recorded delete you can ignore those states and you
can't accidentally remove them if they are in the parent stub.
Bert
Received on 2010-02-24 13:16:03 CET