Hi Greg, I committed a wc-ng patch weeeks ago, where there was a helper function that you said was duplicate functionality to svn_wc__internal_node_get_url(). Because I was busy with a customer at the time I reverted the commit.
Looking at it again, I think it's not a duplicate but a misnomer. I'd like to get your approval on this patch since you complained about it initially. A helper function in the original patch was called get_node_uri(). I renamed that to get_node_base_rev_and_url_components(), because that's what it really does. svn_wc__internal_node_get_url() is not suitable because - It returns the URL in one string, but it's needed in separate repos-root and path-in-repos strings. - It does not return the base revision, which is also needed in that context, and which can be obtained in the same call to wc_db. - It uses svn_wc__db_read_info(), but it looks to me like svn_wc__db_base_get_info() is the better choice, since that context explicitly wants the BASE information. Here is the entire patch again based on current trunk. Thanks, ~Neels
wc-ng: Remove use of svn_wc_entry_t from tree-conflict detection during update. Keep the outcome identical, avoid fixing. Changes in behavior are pending and discussed on dev@: http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2415071 Subject: Re: wc-ng patch review Date: Fri, 06 Nov 2009 11:43:05 +0000 Message-ID: <1257507785.8865.46.ca...@edith.foad.me.uk> * subversion/libsvn_wc/update_editor.c (SVN_WC_CONFLICT_REASON_NONE): New local #define. (get_node_working_state, get_node_base_rev_and_url_components): New static functions, for check_tree_conflict(). (check_tree_conflict): Replace svn_wc_entry_t use with calls to the WC DB via the new helper functions. Remove obsolete check for duplicate tree conflict involving an add of a file external (cannot reproduce). --This line, and those below, will be ignored-- Index: subversion/libsvn_wc/update_editor.c =================================================================== --- subversion/libsvn_wc/update_editor.c (revision 899602) +++ subversion/libsvn_wc/update_editor.c (working copy) @@ -1537,6 +1537,175 @@ tree_has_local_mods(svn_boolean_t *modif } +#define SVN_WC_CONFLICT_REASON_NONE (svn_wc_conflict_reason_t)(-1) + +/* Query a node's local status found in DB at LOCAL_ABSPATH. + * + * Return POSSIBLE_CONFLICT_REASON, which is the current status / schedule / + * difference-between-BASE-and-WORKING of the node, paraphrased into an + * svn_wc_conflict_reason_t. For example, if the node is replaced by + * WORKING, this returns svn_wc_conflict_reason_replaced. If this node is + * not changed by WORKING, this returns SVN_WC_CONFLICT_REASON_NONE. If it's + * not SVN_WC_CONFLICT_REASON_NONE, POSSIBLE_CONFLICT_REASON can be plugged + * directly into a tree-conflict descriptor struct (i.e. + * svn_wc_conflict_description2_t) once this reason is found to conflict + * with an incoming action. + * + * Return LOCAL_ABSPATH's KIND in the BASE tree, which is + * - svn_node_file for a file or symlink, + * - svn_node_dir for a directory and + * - svn_node_none if not found in BASE; + * - svn_node_unknown in all other cases. ### TODO: check each of those + * "other cases" (e.g. DB reports "unknown", ...) and make this + * svn_node_none if possible. + */ +static svn_error_t* +get_node_working_state(svn_wc_conflict_reason_t *possible_conflict_reason, + svn_node_kind_t *kind, + svn_wc__db_t *db, + const char *local_abspath, + apr_pool_t *scratch_pool) +{ + svn_error_t *err; + svn_wc__db_status_t status; + svn_wc__db_kind_t db_kind; + svn_boolean_t base_shadowed; + + *possible_conflict_reason = SVN_WC_CONFLICT_REASON_NONE; + *kind = svn_node_unknown; + + err = svn_wc__db_read_info(&status, + &db_kind, + NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, + NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, + NULL, NULL, NULL, + &base_shadowed, + NULL, NULL, + db, + local_abspath, + scratch_pool, + scratch_pool); + + if (err && err->apr_err == SVN_ERR_WC_PATH_NOT_FOUND) + { + svn_error_clear(err); + *possible_conflict_reason = svn_wc_conflict_reason_missing; + *kind = svn_node_none; + } + else + { + SVN_ERR(err); + + /* Find the "reason" (local schedule) of the potential conflict. */ + if (status == svn_wc__db_status_added + || status == svn_wc__db_status_obstructed_add) + { + *possible_conflict_reason = svn_wc_conflict_reason_added; + /* Is it a replace? */ + if (base_shadowed) + { + svn_wc__db_status_t base_status; + SVN_ERR(svn_wc__db_base_get_info(&base_status, NULL, NULL, + NULL, NULL, NULL, NULL, NULL, + NULL, NULL, NULL, NULL, NULL, + NULL, NULL, + db, local_abspath, + scratch_pool, + scratch_pool)); + if (base_status != svn_wc__db_status_not_present) + *possible_conflict_reason = svn_wc_conflict_reason_replaced; + } + } + else + if (status == svn_wc__db_status_deleted + || status == svn_wc__db_status_obstructed_delete) + *possible_conflict_reason = svn_wc_conflict_reason_deleted; + + /* Translate the node kind. */ + if (db_kind == svn_wc__db_kind_file + || db_kind == svn_wc__db_kind_symlink) + *kind = svn_node_file; + else + if (db_kind == svn_wc__db_kind_dir + || db_kind == svn_wc__db_kind_subdir) + *kind = svn_node_dir; + else + *kind = svn_node_unknown; + } + + return SVN_NO_ERROR; +} + +/* Find the source-left REVISON, REPOS_RELPATH and REPOS_ROOT_URL for + * recording a tree-conflict on node LOCAL_ABSPATH in DB. + */ +static svn_error_t* +get_node_base_rev_and_url_components(svn_revnum_t *base_revision, + const char **repos_relpath, + const char **repos_root_url, + svn_wc__db_t *db, + const char *local_abspath, + apr_pool_t *result_pool, + apr_pool_t *scratch_pool) +{ + svn_error_t *err; + svn_boolean_t do_scan = FALSE; + *base_revision = SVN_INVALID_REVNUM; + + /* First cover all the cases that have a base present. + * The only ones that lack a base are added, copied, moved_here. + */ + err = svn_wc__db_base_get_info(NULL, NULL, + base_revision, + repos_relpath, + repos_root_url, + NULL, NULL, NULL, NULL, NULL, NULL, + NULL, NULL, NULL, NULL, + db, + local_abspath, + result_pool, + scratch_pool); + + if (err && err->apr_err == SVN_ERR_WC_PATH_NOT_FOUND) + { + svn_error_clear(err); + do_scan = TRUE; + } + else + SVN_ERR(err); + + if (do_scan || !SVN_IS_VALID_REVNUM(*base_revision) || !*repos_root_url) + { + /* No base found. Assume this is an 'add'. */ + svn_wc__db_status_t added_status; + + SVN_ERR(svn_wc__db_scan_addition(&added_status, NULL, + repos_relpath, + repos_root_url, + NULL, NULL, NULL, NULL, + base_revision, + db, + local_abspath, + result_pool, + scratch_pool)); + + /* If we ever hit a non-add here, we need to query the actual status + * of the node and handle other cases accordingly. */ + SVN_ERR_ASSERT(added_status == svn_wc__db_status_added + || added_status == svn_wc__db_status_obstructed_add + || added_status == svn_wc__db_status_copied + || added_status == svn_wc__db_status_moved_here); + } + + /* Legacy behaviour from svn_wc__get_entry() use. + * ### TODO check if this is still needed, and, if yes, why. */ + if (*repos_root_url && !*repos_relpath) + *repos_relpath = ""; + + return SVN_NO_ERROR; +} + + /* Check whether the incoming change ACTION on FULL_PATH would conflict with * LOCAL_ABSPATH's scheduled change. If so, then raise a tree conflict with * LOCAL_ABSPATH as the victim. @@ -1572,86 +1741,64 @@ check_tree_conflict(svn_wc_conflict_desc svn_boolean_t inside_deleted_subtree, apr_pool_t *pool) { - svn_wc_conflict_reason_t reason = (svn_wc_conflict_reason_t)(-1); + svn_wc_conflict_reason_t reason = SVN_WC_CONFLICT_REASON_NONE; svn_boolean_t all_mods_are_deletes = FALSE; - const svn_wc_entry_t *entry; - svn_error_t *err; - - err = svn_wc__get_entry(&entry, eb->db, local_abspath, TRUE, - svn_node_unknown, FALSE, pool, pool); - - if (err && err->apr_err == SVN_ERR_NODE_UNEXPECTED_KIND) - svn_error_clear(err); - else - SVN_ERR(err); - - if (entry) - { - svn_boolean_t hidden; - SVN_ERR(svn_wc__db_node_hidden(&hidden, eb->db, local_abspath, pool)); + svn_wc_conflict_reason_t possible_conflict_reason; + svn_node_kind_t kind; - if (hidden) - entry = NULL; - } + SVN_ERR(get_node_working_state(&possible_conflict_reason, &kind, + eb->db, local_abspath, pool)); switch (action) { + case svn_wc_conflict_action_edit: /* Use case 1: Modifying a locally-deleted item. If LOCAL_ABSPATH is an incoming leaf edit within a local tree deletion then we will already have recorded a tree conflict on the locally deleted parent tree. No need to record a conflict within the conflict. */ - if ((entry->schedule == svn_wc_schedule_delete - || entry->schedule == svn_wc_schedule_replace) - && !inside_deleted_subtree) - reason = entry->schedule == svn_wc_schedule_delete - ? svn_wc_conflict_reason_deleted - : svn_wc_conflict_reason_replaced; + if (!inside_deleted_subtree + && (possible_conflict_reason == svn_wc_conflict_reason_deleted + || possible_conflict_reason == svn_wc_conflict_reason_replaced)) + reason = possible_conflict_reason; break; case svn_wc_conflict_action_add: - /* Use case "3.5": Adding a locally-added item. - * - * When checking out a file-external, add_file() is called twice: - * 1.) In the main update, a minimal entry is created. - * 2.) In the external update, the file is added properly. - * Don't raise a tree conflict the second time! */ - if (entry && !entry->file_external_path) - reason = svn_wc_conflict_reason_added; + /* Use case "3.5": Adding a locally-added item. */ + if (possible_conflict_reason == svn_wc_conflict_reason_added) + reason = possible_conflict_reason; break; case svn_wc_conflict_action_delete: case svn_wc_conflict_action_replace: /* Use case 3: Deleting a locally-deleted item. */ - if (entry->schedule == svn_wc_schedule_delete - || entry->schedule == svn_wc_schedule_replace) + if (possible_conflict_reason == svn_wc_conflict_reason_deleted + || possible_conflict_reason == svn_wc_conflict_reason_replaced) { /* If LOCAL_ABSPATH is an incoming leaf deletion within a local tree deletion then we will already have recorded a tree conflict on the locally deleted parent tree. No need to record a conflict within the conflict. */ if (!inside_deleted_subtree) - reason = entry->schedule == svn_wc_schedule_delete - ? svn_wc_conflict_reason_deleted - : svn_wc_conflict_reason_replaced; + reason = possible_conflict_reason; } else { svn_boolean_t modified = FALSE; /* Use case 2: Deleting a locally-modified item. */ - if (entry->kind == svn_node_file) + if (kind == svn_node_file) { - if (entry->schedule != svn_wc_schedule_normal) + if (possible_conflict_reason != SVN_WC_CONFLICT_REASON_NONE) modified = TRUE; else SVN_ERR(entry_has_local_mods(&modified, eb->db, local_abspath, - entry->kind, pool)); - if (entry->schedule == svn_wc_schedule_delete) + kind, pool)); + if (possible_conflict_reason == svn_wc_conflict_reason_deleted) all_mods_are_deletes = TRUE; } - else if (entry->kind == svn_node_dir) + else if (kind == svn_node_dir) { /* We must detect deep modifications in a directory tree, * but the update editor will not visit the subdirectories @@ -1681,35 +1828,37 @@ check_tree_conflict(svn_wc_conflict_desc /* If a conflict was detected, append log commands to the log accumulator * to record it. */ - if (reason != (svn_wc_conflict_reason_t)(-1)) + if (reason != SVN_WC_CONFLICT_REASON_NONE) { svn_wc_conflict_description2_t *conflict; const svn_wc_conflict_version_t *src_left_version; const svn_wc_conflict_version_t *src_right_version; + svn_revnum_t base_revision; const char *repos_url = NULL; const char *path_in_repos = NULL; - svn_node_kind_t left_kind = (entry->schedule == svn_wc_schedule_add) + svn_node_kind_t left_kind = (reason == svn_wc_conflict_reason_added) ? svn_node_none - : (entry->schedule == svn_wc_schedule_delete) + : (reason == svn_wc_conflict_reason_deleted) ? svn_node_unknown - : entry->kind; + : kind; /* Source-left repository root URL and path in repository. * The Source-right ones will be the same for update. * For switch, only the path in repository will differ, because * a cross-repository switch is not possible. */ - repos_url = entry->repos; - path_in_repos = svn_uri_is_child(repos_url, entry->url, pool); - if (path_in_repos == NULL) - path_in_repos = ""; + SVN_ERR(get_node_base_rev_and_url_components(&base_revision, + &path_in_repos, + &repos_url, eb->db, + local_abspath, pool, + pool)); src_left_version = svn_wc_conflict_version_create(repos_url, path_in_repos, - entry->revision, + base_revision, left_kind, pool); - /* entry->kind is both base kind and working kind, because schedule + /* kind is both base kind and working kind, because schedule * replace-by-different-kind is not supported. */ /* ### TODO: but in case the entry is locally removed, entry->kind * is svn_node_none and doesn't reflect the older kind. Then we @@ -1744,7 +1893,7 @@ check_tree_conflict(svn_wc_conflict_desc pool); conflict = svn_wc_conflict_description_create_tree2( - local_abspath, entry->kind, + local_abspath, kind, eb->switch_url ? svn_wc_operation_switch : svn_wc_operation_update, src_left_version, src_right_version, pool); conflict->action = action;
signature.asc
Description: OpenPGP digital signature
