"Bert Huijben" <b...@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.
>> +/* 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.
> (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?
>> +/* 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".
> 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.
--
Philip
