wc-ng patch review
|
View:
New views
11 Messages
—
Rating Filter:
Alert me
|
 |
wc-ng patch review
by Neels Janosch Hofmeyr-2
::
Rate this Message:
Hi Hyrum, Bert or other wc-ng pros,
could someone please glance over this patch and point out stupid use of wc-ng, if any? Thanks, ~Neels ------------------------------------------------------ http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2414261 Remove use of svn_wc_entry_t from tree-conflict detection during update (wc-ng). * subversion/libsvn_wc/update_editor.c (SVN_WC_CONFLICT_REASON_NONE): New local helper #define. (get_node_working_state, get_node_uri): New helper 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 40364) +++ subversion/libsvn_wc/update_editor.c (working copy) @@ -1516,6 +1516,178 @@ tree_has_local_mods(svn_boolean_t *modif return SVN_NO_ERROR; } + +#define SVN_WC_CONFLICT_REASON_NONE (svn_wc_conflict_reason_t)(-1) + +/* Helper for check_tree_conflict(): query a node's local status. + * Use svn_wc__db_read_info() and others to return selected bits of info + * useful for check_tree_conflict(). + * + * Return POSSIBLE_CONFLICT_REASON, which is the current status/schedule of + * the node paraphrased into an svn_wc_conflict_reason_t. For example, if + * the node has been replaced in the working copy, this returns + * svn_wc_conflict_reason_replaced. This value 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 KIND, which is the svn_wc__db_kind_t returned by + * svn_wc__db_read_info() translated to an svn_node_kind_t. + * + * See svn_wc__db_read_info() for the remaining arguments DB, LOCAL_ABSPATH, + * RESULT_POOL and SCRATCH_POOL. + */ +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 *result_pool, + 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, + result_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; +} + +/* Helper for check_tree_conflict() which finds the repository and node + * paths for recording a tree-conflict. + * + * REVISON, REPOS_ROOT_URL and REPOS_RELPATH are return values, and + * DB, LOCAL_ABSPATH, RESULT_POOL and SCRATCH_POOL are input parameters, all + * of which are the same as in svn_wc__db_read_info(). + */ +static svn_error_t* +get_node_uri(svn_revnum_t *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; + *revision = SVN_INVALID_REVNUM; + svn_boolean_t do_scan = FALSE; + + /* First cover all the cases that have a base present. + * The only ones that lack a base are a add, copied, moved_here. (*not* + * replaced, replaced is covered by this one here as well.) */ + err = svn_wc__db_base_get_info(NULL, NULL, + 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(*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, + 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. */ + 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. @@ -1551,86 +1723,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, 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 @@ -1660,35 +1810,34 @@ 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 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_uri(&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, + 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 @@ -1723,7 +1872,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; |
|
|
Re: wc-ng patch review
by Julian Foad
::
Rate this Message:
|
|
|
Re: wc-ng patch review
by Neels Janosch Hofmeyr-2
::
Rate this Message:
Julian Foad wrote:
> Neels Janosch Hofmeyr wrote: >> Hi Hyrum, Bert or other wc-ng pros, >> >> could someone please glance over this patch and point out stupid use of >> wc-ng, if any? > > Hi Neels. Some comments on the doc strings. Hey Julian, thanks for your quality review. I'd like to note apologetically that I concentrated on making the behavior exactly the same as it was with ENTRY, not quite looking at the design, and it shows in the log messages. So, this one is supposed to be the step where behavior matches exactly, but I think we should also change some behavior. Previously, we had the ENTRY and tried to get all information from that. Now, we have more fine-grained and distinct information on BASE and WORKING readily available. So we should roll it out from that side. And I think that should be done in a separate commit. > >> +/* Helper for check_tree_conflict(): query a node's local status. >> + * Use svn_wc__db_read_info() and others to return selected bits of info >> + * useful for check_tree_conflict(). > > I don't think there's any benefit in saying what functions it calls. > (The impl. is likely to change soonish, I guess.) Agreed. Right now it isn't anything more than a wrapper around the __db_read_info(), so I guess that's what I wrote in the comment (vs. just writing it out inline in check_tree_conflict() and no comment at all). > >> + * Return POSSIBLE_CONFLICT_REASON, which is the current status/schedule of >> + * the node paraphrased into an svn_wc_conflict_reason_t. For example, if >> + * the node has been replaced in the working copy, this returns >> + * svn_wc_conflict_reason_replaced. This value 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 KIND, which is the svn_wc__db_kind_t returned by >> + * svn_wc__db_read_info() translated to an svn_node_kind_t. > > Please could you describe KIND without reference to the implementation? > "Set KIND to the node kind of the (working node? base node? something > else?)." Note that we don't have any tests on the kind/path/peg rev shown in svn info with a tree conflict, so we apparently have no clear definition of what it should say. > >> + * See svn_wc__db_read_info() for the remaining arguments DB, LOCAL_ABSPATH, >> + * RESULT_POOL and SCRATCH_POOL. > > We could start the doc string with "Query the local status of the node > at LOCAL_ABSPATH, using DB." SCRATCH_POOL is a universal parameter, so > we often don't bother to document it in static functions. Any use of > RESULT_POOL would be specific to this function... but I don't see that a > RESULT_POOL is needed by this function, because it only writes its > outputs into caller-supplied memory spaces. __db_read_info(), and I don't want to make assumptions on what that function needs the result pool for. Or should we? Hm, thought about it, removed RESULT_POOL. > >> + */ >> +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 *result_pool, >> + apr_pool_t *scratch_pool) > [...] > >> +/* Helper for check_tree_conflict() which finds the repository and node >> + * paths for recording a tree-conflict. >> + * >> + * REVISON, REPOS_ROOT_URL and REPOS_RELPATH are return values, and >> + * DB, LOCAL_ABSPATH, RESULT_POOL and SCRATCH_POOL are input parameters, all >> + * of which are the same as in svn_wc__db_read_info(). >> + */ >> +static svn_error_t* >> +get_node_uri(svn_revnum_t *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) > > I am left wondering what revision, and what repository paths, this > function gets. svn_wc__db_read_info() doesn't document its REVISION, > REPOS_ROOT_URL and REPOS_RELPATH parameters explicitly. (It says, "The > information returned comes from the BASE tree, as possibly modified by > the WORKING and ACTUAL trees.") time being, because there will be more and more wc-ng-newbies weaving the code in. > > I guess it's "the base", but the meanings of "the node" and "the base" > and so on are not always clear, especially now that the concepts for > describing such states are different in WC-1 and WC-NG. > > Let's try to state very clearly what this function does, so that we can > all judge whether it actually does it. > > How about, > > [[[ > Set *REVISION, *REPOS_RELPATH and *REPOS_ROOT_URL to the revision, > path-in-repository and repository root URL (respectively) of the > base node implied by LOCAL_ABSPATH from the local filesystem, > looked up in DB. > > Allocate the result strings in RESULT_POOL. > ]]] it returns stuff. This is svn_wc__get_entry() code stripped down. check_tree_conflict()'s inline comments say that it reflects both source-left and source-right for all except the REVISION, because they will be the same for all cases... something in that line. We better come from the other side: what *should* it say? <design-mode> This code determines the tree-conflict information shown, i.e.: [[[ Source left: () ^/foo@1 Source right: (file) ^/foo@2 ]]] But, what is this "Source" anyway? AFAIK, it's the incoming change; in the case of an update to HEAD: - "Source left" should be BASE, - "Source right" should be HEAD, and - The difference between "Source left" and "Source right" is the "action". Furthermore, - "Target" should be WORKING. - The difference between BASE and WORKING (think "Target left" and "Target right", respectively) is the "schedule", or the "conflict reason". (NOTE: If updating to a specific revision like 'update -r5', replace HEAD with that revision number.) What are the implications during an update? I think: - In the case action=="edit", "Source left" and "Source right" can only differ in REVISION. "edit" does not change the kind. --> left: PATH@BASE with KIND == kind-in-BASE ( == kind-in-HEAD) right: PATH@HEAD with KIND == kind-in-BASE ( == kind-in-HEAD) - In the case action=="replace", "Source left" and "Source right" can only differ in REVISION and KIND. --> left: PATH@BASE with KIND == kind-in-BASE right: PATH@HEAD with KIND == kind-in-HEAD - In action=="simple add", technically, "Source left" would be <nothing>. However, it is more accurate to state the non-existent PATH with the revision at which it did not exist, i.e. the revision of its nearest parent folder in BASE. The "left" "kind" should be svn_node_none. --> left: PATH@BASE with KIND == svn_node_none. right: PATH@HEAD with KIND == kind-in-HEAD - In action=="simple delete", "Source *right*" would be <nothing>. Again, it is more accurate to say: --> left: PATH@BASE with KIND == kind-in-BASE right: PATH@HEAD with KIND == svn_node_none. - The "add" part of a "copied/moved here" could be seen as a "simple add", so that "Source left" is <nothing> and more accurately PATH@BASE. On the other hand, it might make sense to state the PATH@REV of the copied_from location of that copy/move operation. ...? - The "delete" part of a "moved away" could be seen as a "simple delete", so that "Source right" is <nothing> and more accurately PATH@HEAD. On the other hand, it might make sense to state the PATH@REV of the moved_to location of that move operation. ...? I know that the current code does return KIND == svn_node_unknown in certain cases, but I can't think of any justification for that. Either it is 'none' or 'file'/'dir', there shouldn't be an 'unknown' case, right? Right, what about 'switch', then? 'switch' is the same as 'update', but now "Source left", being BASE, reflects the path where the user's working copy is at, and "Source right" reflects the path of the switch target at HEAD. So while the REPOS_ROOT_URL will stay the same (cross-repos switch not supported), the REPOS_RELPATH should be different between "Source left" and "Source right". --> left: CURRENT_WC_PATH@BASE with KIND == kind-in-BASE right: SWITCH_TO_PATH@HEAD with KIND == kind-in-SWITCH_TO_PATH@HEAD. </design-mode> ...but with this patch, I'd like to not even consider the design yet, only cut out the ENTRY bits. It would be nice to have a design refactoring ready and commit it right after this one, but I'd rather not wait and commit soon. So I'll see if I can improve the comments... > p.s. <peeve kind="pet"> We shouldn't need to be told what calls it, and > never mind that it's a "helper": every function except "main()" is a > helper. Redundant info. </peeve> "Helper", to me, means: this function is only called by that other function, and that is how it is meant to be. The reason why it is a separate function is merely cosmetic. No consideration has gone into code re-usability. So if someone came along and wanted to use it for something different, she should take a close look and possibly refactor / change the comment. Not good? I'll cut it out. How about this one? ~Neels ------------------------------------------------------ http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2414448 Remove use of svn_wc_entry_t from tree-conflict detection during update (wc-ng). * subversion/libsvn_wc/update_editor.c (SVN_WC_CONFLICT_REASON_NONE): New local helper #define. (get_node_working_state, get_node_uri): New helper 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 40372) +++ subversion/libsvn_wc/update_editor.c (working copy) @@ -1516,6 +1516,176 @@ tree_has_local_mods(svn_boolean_t *modif return SVN_NO_ERROR; } + +#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_uri(svn_revnum_t *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; + *revision = SVN_INVALID_REVNUM; + svn_boolean_t do_scan = FALSE; + + /* First cover all the cases that have a base present. + * The only ones that lack a base are a add, copied, moved_here. (*not* + * replaced, replaced is covered by this one here as well.) */ + err = svn_wc__db_base_get_info(NULL, NULL, + 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(*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, + 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. */ + 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. @@ -1551,86 +1721,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 @@ -1660,35 +1808,34 @@ 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 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_uri(&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, + 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 @@ -1723,7 +1870,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; |
|
|
Re: wc-ng patch review
by Julian Foad
::
Rate this Message:
|
|
|
|
|
|
Re: wc-ng patch review
by Neels Janosch Hofmeyr-2
::
Rate this Message:
Committed the tweaked patch in r40424.
More changes/fixes welcome, as always. ~Neels Julian Foad wrote: > On Wed, 2009-11-04, Neels J Hofmeyr wrote: >> Julian Foad wrote: >>> Neels Janosch Hofmeyr wrote: >>>> Hi Hyrum, Bert or other wc-ng pros, >>>> >>>> could someone please glance over this patch and point out stupid use of >>>> wc-ng, if any? >>> Hi Neels. Some comments on the doc strings. >> Hey Julian, >> >> thanks for your quality review. I'd like to note apologetically that I >> concentrated on making the behavior exactly the same as it was with ENTRY, >> not quite looking at the design, and it shows in the log messages. > > No problem. Sorry it's taken me a couple of days to respond. > >> So, this one is supposed to be the step where behavior matches exactly, but >> I think we should also change some behavior. Previously, we had the ENTRY >> and tried to get all information from that. Now, we have more fine-grained >> and distinct information on BASE and WORKING readily available. So we should >> roll it out from that side. And I think that should be done in a separate >> commit. >> >>>> +/* Helper for check_tree_conflict(): query a node's local status. >>>> + * Use svn_wc__db_read_info() and others to return selected bits of info >>>> + * useful for check_tree_conflict(). >>> I don't think there's any benefit in saying what functions it calls. >>> (The impl. is likely to change soonish, I guess.) >> Agreed. Right now it isn't anything more than a wrapper around the >> __db_read_info(), so I guess that's what I wrote in the comment (vs. just >> writing it out inline in check_tree_conflict() and no comment at all). > > Oh, but it was already more than a wrapper around __db_read_info: it > used __db_base_get_info as well. > > [...] >>> Please could you describe KIND without reference to the implementation? >>> "Set KIND to the node kind of the (working node? base node? something >>> else?)." >> Agreed. >> >> Note that we don't have any tests on the kind/path/peg rev shown in svn info >> with a tree conflict, so we apparently have no clear definition of what it >> should say. > > In my mind it's clear. There is an incoming change to this node, and the > change is the difference between two repository locations, PATH1@REV1 > and PATH2@REV2. REV1 and REV2 both exist. PATH1@REV1 and/or PATH2@REV2 > might not exist. The node kind is 'none' if a node doesn't exist. > > [...] >>>> +/* Helper for check_tree_conflict() which finds the repository and node >>>> + * paths for recording a tree-conflict. >>>> + * >>>> + * REVISON, REPOS_ROOT_URL and REPOS_RELPATH are return values, and >>>> + * DB, LOCAL_ABSPATH, RESULT_POOL and SCRATCH_POOL are input parameters, all >>>> + * of which are the same as in svn_wc__db_read_info(). >>>> + */ >>>> +static svn_error_t* >>>> +get_node_uri(svn_revnum_t *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) > [...] >>> How about, >>> >>> [[[ >>> Set *REVISION, *REPOS_RELPATH and *REPOS_ROOT_URL to the revision, >>> path-in-repository and repository root URL (respectively) of the >>> base node implied by LOCAL_ABSPATH from the local filesystem, >>> looked up in DB. >>> >>> Allocate the result strings in RESULT_POOL. >>> ]]] >> That's not enough, because in case of an added node, there is no BASE, yet >> it returns stuff. This is svn_wc__get_entry() code stripped down. >> check_tree_conflict()'s inline comments say that it reflects both >> source-left and source-right for all except the REVISION, because they will >> be the same for all cases... something in that line. >> >> We better come from the other side: what *should* it say? > > Yes, it's better to approach from that side. > >> <design-mode> >> >> This code determines the tree-conflict information shown, i.e.: >> [[[ >> Source left: () ^/foo@1 >> Source right: (file) ^/foo@2 >> ]]] >> But, what is this "Source" anyway? AFAIK, it's the incoming change; in the >> case of an update to HEAD: >> - "Source left" should be BASE, >> - "Source right" should be HEAD, and >> - The difference between "Source left" and "Source right" is the "action". > > Yes, exactly. > >> Furthermore, >> - "Target" should be WORKING. >> - The difference between BASE and WORKING (think "Target left" and "Target >> right", respectively) is the "schedule", or the "conflict reason". > > Yes. > >> (NOTE: If updating to a specific revision like 'update -r5', replace HEAD >> with that revision number.) >> >> What are the implications during an update? I think: >> >> - In the case action=="edit", "Source left" and "Source right" can only >> differ in REVISION. "edit" does not change the kind. >> --> left: PATH@BASE with KIND == kind-in-BASE ( == kind-in-HEAD) >> right: PATH@HEAD with KIND == kind-in-BASE ( == kind-in-HEAD) >> >> - In the case action=="replace", "Source left" and "Source right" can only >> differ in REVISION and KIND. >> --> left: PATH@BASE with KIND == kind-in-BASE >> right: PATH@HEAD with KIND == kind-in-HEAD >> >> - In action=="simple add", technically, "Source left" would be <nothing>. >> However, it is more accurate to state the non-existent PATH with the >> revision at which it did not exist, i.e. the revision of its nearest parent >> folder in BASE. The "left" "kind" should be svn_node_none. >> --> left: PATH@BASE with KIND == svn_node_none. >> right: PATH@HEAD with KIND == kind-in-HEAD > > Nearly. Where you said "i.e. the revision of its nearest parent folder > in BASE", normally you can't assume that the parent folder is at the > same BASE revision as one of its children. In this case, where the > schedule is "add", the parent's BASE is the relevant revision UNLESS the > child has metadata recording the special state "I'm updated to rX at > which I'm deleted". > >> - In action=="simple delete", "Source *right*" would be <nothing>. Again, it >> is more accurate to say: >> --> left: PATH@BASE with KIND == kind-in-BASE >> right: PATH@HEAD with KIND == svn_node_none. > > Yes. > >> - The "add" part of a "copied/moved here" could be seen as a "simple add", >> so that "Source left" is <nothing> and more accurately PATH@BASE. > > Yes. > >> On the >> other hand, it might make sense to state the PATH@REV of the copied_from >> location of that copy/move operation. ...? > > No. When we update a non-existent child so that it comes into existence, > the incoming change is adding a whole node. The copied_from information > is also available, separately, but is not the left-hand side of the > incoming change. > >> - The "delete" part of a "moved away" could be seen as a "simple delete", so >> that "Source right" is <nothing> and more accurately PATH@HEAD. > > Yes. > >> On the other >> hand, it might make sense to state the PATH@REV of the moved_to location of >> that move operation. ...? > > No. Similar to the "copied/moved here" case above. > >> I know that the current code does return KIND == svn_node_unknown in certain >> cases, but I can't think of any justification for that. Either it is 'none' >> or 'file'/'dir', there shouldn't be an 'unknown' case, right? > > Correct. > >> Right, what about 'switch', then? >> 'switch' is the same as 'update', but now "Source left", being BASE, >> reflects the path where the user's working copy is at, and "Source right" >> reflects the path of the switch target at HEAD. So while the REPOS_ROOT_URL >> will stay the same (cross-repos switch not supported), the REPOS_RELPATH >> should be different between "Source left" and "Source right". >> --> left: CURRENT_WC_PATH@BASE with KIND == kind-in-BASE >> right: SWITCH_TO_PATH@HEAD with KIND == kind-in-SWITCH_TO_PATH@HEAD. > > Yes. > >> </design-mode> >> >> ...but with this patch, I'd like to not even consider the design yet, only >> cut out the ENTRY bits. It would be nice to have a design refactoring ready >> and commit it right after this one, but I'd rather not wait and commit soon. >> >> So I'll see if I can improve the comments... > > OK. > >>> p.s. <peeve kind="pet"> We shouldn't need to be told what calls it, and >>> never mind that it's a "helper": every function except "main()" is a >>> helper. Redundant info. </peeve> >> "Helper", to me, means: this function is only called by that other function, >> and that is how it is meant to be. The reason why it is a separate function >> is merely cosmetic. No consideration has gone into code re-usability. So if >> someone came along and wanted to use it for something different, she should >> take a close look and possibly refactor / change the comment. > > Sure, I understand, but I think that style of programming leads to > unmaintainable code: when the next person comes along and could > potentially re-use some functionality, it's too hard to untangle the > re-usable part from the caller-specific details and so they just create > another caller-specific "helper" function and don't bother to improve > the old one. > > Whenever some code is factored out into a separate function, that's > usually a good sign that it's worth making it a clearly defined > re-usable chunk of code, even if it isn't currently used in more than > one place. > >> Not good? I'll >> cut it out. >> >> How about this one? > > I'm going to send this reply now because I'm not sure how long before I > review the new patch you attached. > > Thanks for the detailed response and designing. > > - Julian > > ------------------------------------------------------ > http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2415071 http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2415617 |
|
|
tree-conflicts with 'add' -- was: Re: wc-ng patch review
by Neels Janosch Hofmeyr-2
::
Rate this Message:
Hi tree-conflicts folks,
I'm intrigued by the tree-conflicts code in update_editor.c that checks for TCs involving add. Using 'update' and 'switch', I can't find any way to produce a tree-conflict. No-one asked for tests about it, so we don't have any. Does anyone remember what our aim was? In case anyone is interested, my test scripts for all file|dir combinations of a simple add vs. add during update and during switch are attached. None report a tree-conflict. edit|delete vs. add don't make sense anyway. It feels like we can simply remove all current 'add' TC code from update_editor.c... So what *should* it be like? At least the 'switch' tests of 'add vs. add' don't look very good, especially when they try to switch back again. So... I guess... we need to fix those TCs...? ~Neels Neels Janosch Hofmeyr wrote: > Committed the tweaked patch in r40424. > More changes/fixes welcome, as always. > > ~Neels > > Julian Foad wrote: >> On Wed, 2009-11-04, Neels J Hofmeyr wrote: >>> Julian Foad wrote: >>>> Neels Janosch Hofmeyr wrote: >>>>> Hi Hyrum, Bert or other wc-ng pros, >>>>> >>>>> could someone please glance over this patch and point out stupid use of >>>>> wc-ng, if any? >>>> Hi Neels. Some comments on the doc strings. >>> Hey Julian, >>> >>> thanks for your quality review. I'd like to note apologetically that I >>> concentrated on making the behavior exactly the same as it was with ENTRY, >>> not quite looking at the design, and it shows in the log messages. >> No problem. Sorry it's taken me a couple of days to respond. >> >>> So, this one is supposed to be the step where behavior matches exactly, but >>> I think we should also change some behavior. Previously, we had the ENTRY >>> and tried to get all information from that. Now, we have more fine-grained >>> and distinct information on BASE and WORKING readily available. So we should >>> roll it out from that side. And I think that should be done in a separate >>> commit. >>> >>>>> +/* Helper for check_tree_conflict(): query a node's local status. >>>>> + * Use svn_wc__db_read_info() and others to return selected bits of info >>>>> + * useful for check_tree_conflict(). >>>> I don't think there's any benefit in saying what functions it calls. >>>> (The impl. is likely to change soonish, I guess.) >>> Agreed. Right now it isn't anything more than a wrapper around the >>> __db_read_info(), so I guess that's what I wrote in the comment (vs. just >>> writing it out inline in check_tree_conflict() and no comment at all). >> Oh, but it was already more than a wrapper around __db_read_info: it >> used __db_base_get_info as well. >> >> [...] >>>> Please could you describe KIND without reference to the implementation? >>>> "Set KIND to the node kind of the (working node? base node? something >>>> else?)." >>> Agreed. >>> >>> Note that we don't have any tests on the kind/path/peg rev shown in svn info >>> with a tree conflict, so we apparently have no clear definition of what it >>> should say. >> In my mind it's clear. There is an incoming change to this node, and the >> change is the difference between two repository locations, PATH1@REV1 >> and PATH2@REV2. REV1 and REV2 both exist. PATH1@REV1 and/or PATH2@REV2 >> might not exist. The node kind is 'none' if a node doesn't exist. >> >> [...] >>>>> +/* Helper for check_tree_conflict() which finds the repository and node >>>>> + * paths for recording a tree-conflict. >>>>> + * >>>>> + * REVISON, REPOS_ROOT_URL and REPOS_RELPATH are return values, and >>>>> + * DB, LOCAL_ABSPATH, RESULT_POOL and SCRATCH_POOL are input parameters, all >>>>> + * of which are the same as in svn_wc__db_read_info(). >>>>> + */ >>>>> +static svn_error_t* >>>>> +get_node_uri(svn_revnum_t *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) >> [...] >>>> How about, >>>> >>>> [[[ >>>> Set *REVISION, *REPOS_RELPATH and *REPOS_ROOT_URL to the revision, >>>> path-in-repository and repository root URL (respectively) of the >>>> base node implied by LOCAL_ABSPATH from the local filesystem, >>>> looked up in DB. >>>> >>>> Allocate the result strings in RESULT_POOL. >>>> ]]] >>> That's not enough, because in case of an added node, there is no BASE, yet >>> it returns stuff. This is svn_wc__get_entry() code stripped down. >>> check_tree_conflict()'s inline comments say that it reflects both >>> source-left and source-right for all except the REVISION, because they will >>> be the same for all cases... something in that line. >>> >>> We better come from the other side: what *should* it say? >> Yes, it's better to approach from that side. >> >>> <design-mode> >>> >>> This code determines the tree-conflict information shown, i.e.: >>> [[[ >>> Source left: () ^/foo@1 >>> Source right: (file) ^/foo@2 >>> ]]] >>> But, what is this "Source" anyway? AFAIK, it's the incoming change; in the >>> case of an update to HEAD: >>> - "Source left" should be BASE, >>> - "Source right" should be HEAD, and >>> - The difference between "Source left" and "Source right" is the "action". >> Yes, exactly. >> >>> Furthermore, >>> - "Target" should be WORKING. >>> - The difference between BASE and WORKING (think "Target left" and "Target >>> right", respectively) is the "schedule", or the "conflict reason". >> Yes. >> >>> (NOTE: If updating to a specific revision like 'update -r5', replace HEAD >>> with that revision number.) >>> >>> What are the implications during an update? I think: >>> >>> - In the case action=="edit", "Source left" and "Source right" can only >>> differ in REVISION. "edit" does not change the kind. >>> --> left: PATH@BASE with KIND == kind-in-BASE ( == kind-in-HEAD) >>> right: PATH@HEAD with KIND == kind-in-BASE ( == kind-in-HEAD) >>> >>> - In the case action=="replace", "Source left" and "Source right" can only >>> differ in REVISION and KIND. >>> --> left: PATH@BASE with KIND == kind-in-BASE >>> right: PATH@HEAD with KIND == kind-in-HEAD >>> >>> - In action=="simple add", technically, "Source left" would be <nothing>. >>> However, it is more accurate to state the non-existent PATH with the >>> revision at which it did not exist, i.e. the revision of its nearest parent >>> folder in BASE. The "left" "kind" should be svn_node_none. >>> --> left: PATH@BASE with KIND == svn_node_none. >>> right: PATH@HEAD with KIND == kind-in-HEAD >> Nearly. Where you said "i.e. the revision of its nearest parent folder >> in BASE", normally you can't assume that the parent folder is at the >> same BASE revision as one of its children. In this case, where the >> schedule is "add", the parent's BASE is the relevant revision UNLESS the >> child has metadata recording the special state "I'm updated to rX at >> which I'm deleted". >> >>> - In action=="simple delete", "Source *right*" would be <nothing>. Again, it >>> is more accurate to say: >>> --> left: PATH@BASE with KIND == kind-in-BASE >>> right: PATH@HEAD with KIND == svn_node_none. >> Yes. >> >>> - The "add" part of a "copied/moved here" could be seen as a "simple add", >>> so that "Source left" is <nothing> and more accurately PATH@BASE. >> Yes. >> >>> On the >>> other hand, it might make sense to state the PATH@REV of the copied_from >>> location of that copy/move operation. ...? >> No. When we update a non-existent child so that it comes into existence, >> the incoming change is adding a whole node. The copied_from information >> is also available, separately, but is not the left-hand side of the >> incoming change. >> >>> - The "delete" part of a "moved away" could be seen as a "simple delete", so >>> that "Source right" is <nothing> and more accurately PATH@HEAD. >> Yes. >> >>> On the other >>> hand, it might make sense to state the PATH@REV of the moved_to location of >>> that move operation. ...? >> No. Similar to the "copied/moved here" case above. >> >>> I know that the current code does return KIND == svn_node_unknown in certain >>> cases, but I can't think of any justification for that. Either it is 'none' >>> or 'file'/'dir', there shouldn't be an 'unknown' case, right? >> Correct. >> >>> Right, what about 'switch', then? >>> 'switch' is the same as 'update', but now "Source left", being BASE, >>> reflects the path where the user's working copy is at, and "Source right" >>> reflects the path of the switch target at HEAD. So while the REPOS_ROOT_URL >>> will stay the same (cross-repos switch not supported), the REPOS_RELPATH >>> should be different between "Source left" and "Source right". >>> --> left: CURRENT_WC_PATH@BASE with KIND == kind-in-BASE >>> right: SWITCH_TO_PATH@HEAD with KIND == kind-in-SWITCH_TO_PATH@HEAD. >> Yes. >> >>> </design-mode> >>> >>> ...but with this patch, I'd like to not even consider the design yet, only >>> cut out the ENTRY bits. It would be nice to have a design refactoring ready >>> and commit it right after this one, but I'd rather not wait and commit soon. >>> >>> So I'll see if I can improve the comments... >> OK. >> >>>> p.s. <peeve kind="pet"> We shouldn't need to be told what calls it, and >>>> never mind that it's a "helper": every function except "main()" is a >>>> helper. Redundant info. </peeve> >>> "Helper", to me, means: this function is only called by that other function, >>> and that is how it is meant to be. The reason why it is a separate function >>> is merely cosmetic. No consideration has gone into code re-usability. So if >>> someone came along and wanted to use it for something different, she should >>> take a close look and possibly refactor / change the comment. >> Sure, I understand, but I think that style of programming leads to >> unmaintainable code: when the next person comes along and could >> potentially re-use some functionality, it's too hard to untangle the >> re-usable part from the caller-specific details and so they just create >> another caller-specific "helper" function and don't bother to improve >> the old one. >> >> Whenever some code is factored out into a separate function, that's >> usually a good sign that it's worth making it a clearly defined >> re-usable chunk of code, even if it isn't currently used in more than >> one place. >> >>> Not good? I'll >>> cut it out. >>> >>> How about this one? >> I'm going to send this reply now because I'm not sure how long before I >> review the new patch you attached. >> >> Thanks for the detailed response and designing. >> >> - Julian >> >> ------------------------------------------------------ >> http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2415071 > > ------------------------------------------------------ > http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2415617 http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2415633 #!/bin/bash ## CONFIGURE THIS: ## The next line is the only line you should need to adjust. ## (and uncomment all four lines) #SVNDIR=/my/svn/trunk #alias svn=${SVNDIR}/subversion/svn/svn #alias svnserve=${SVNDIR}/subversion/svnserve/svnserve #alias svnadmin=${SVNDIR}/subversion/svnadmin/svnadmin svn --version REPOS="`pwd`/repos" URL="file://$REPOS" rm -rf repos wc svnadmin create repos svn co -q ${URL} wc set -x ## ACTUAL TEST svn mkdir -m boo $URL/a echo a > wc/a svn add wc/a svn up wc svn st wc svn info wc/a #!/bin/bash ## CONFIGURE THIS: ## The next line is the only line you should need to adjust. ## (and uncomment all four lines) #SVNDIR=/my/svn/trunk #alias svn=${SVNDIR}/subversion/svn/svn #alias svnserve=${SVNDIR}/subversion/svnserve/svnserve #alias svnadmin=${SVNDIR}/subversion/svnadmin/svnadmin svn --version REPOS="`pwd`/repos" URL="file://$REPOS" rm -rf repos wc wc2 svnadmin create repos set -x ## ACTUAL TEST svn co -q ${URL} wc echo a > wc/a svn add wc/a svn co -q ${URL} wc2 echo b > wc2/a svn add wc2/a svn ci -mm wc2 svn up --accept=postpone wc svn st wc svn info wc/a #!/bin/bash ## CONFIGURE THIS: ## The next line is the only line you should need to adjust. ## (and uncomment all four lines) #SVNDIR=/my/svn/trunk #alias svn=${SVNDIR}/subversion/svn/svn #alias svnserve=${SVNDIR}/subversion/svnserve/svnserve #alias svnadmin=${SVNDIR}/subversion/svnadmin/svnadmin svn --version REPOS="`pwd`/repos" URL="file://$REPOS" rm -rf repos wc wc2 svnadmin create repos set -x ## ACTUAL TEST svn co -q ${URL} wc mkdir wc/a svn add wc/a svn co -q ${URL} wc2 mkdir wc2/a svn add wc2/a svn ci -mm wc2 svn up wc svn st wc svn info wc/a #!/bin/bash ## CONFIGURE THIS: ## The next line is the only line you should need to adjust. ## (and uncomment all four lines) #SVNDIR=/my/svn/trunk #alias svn=${SVNDIR}/subversion/svn/svn #alias svnserve=${SVNDIR}/subversion/svnserve/svnserve #alias svnadmin=${SVNDIR}/subversion/svnadmin/svnadmin svn --version REPOS="`pwd`/repos" URL="file://$REPOS" rm -rf repos wc svnadmin create repos svn co -q ${URL} wc set -x cd wc ## ACTUAL TEST mkdir trunk echo a > trunk/a svn add trunk svn ci -mm svn cp -mm ^/trunk ^/branch svn up svn mkdir branch/b svn ci -mm svn up echo b > trunk/b svn add trunk/b cd trunk svn st svn switch ^/branch svn info b #!/bin/bash ## CONFIGURE THIS: ## The next line is the only line you should need to adjust. ## (and uncomment all four lines) #SVNDIR=/my/svn/trunk #alias svn=${SVNDIR}/subversion/svn/svn #alias svnserve=${SVNDIR}/subversion/svnserve/svnserve #alias svnadmin=${SVNDIR}/subversion/svnadmin/svnadmin svn --version REPOS="`pwd`/repos" URL="file://$REPOS" rm -rf repos wc svnadmin create repos svn co -q ${URL} wc set -x cd wc ## ACTUAL TEST mkdir trunk svn add trunk svn ci -mm svn cp -mm ^/trunk ^/branch svn up svn mkdir branch/b svn ci -mm svn up mkdir trunk/b echo important data > trunk/b/c svn add trunk/b cd trunk svn st svn switch ^/branch svn st svn info b cat b/c #echo "just for interest, what if we switch back?" #svn switch ^/trunk #svn st #svn info b #ls #!/bin/bash ## CONFIGURE THIS: ## The next line is the only line you should need to adjust. ## (and uncomment all four lines) #SVNDIR=/my/svn/trunk #alias svn=${SVNDIR}/subversion/svn/svn #alias svnserve=${SVNDIR}/subversion/svnserve/svnserve #alias svnadmin=${SVNDIR}/subversion/svnadmin/svnadmin svn --version REPOS="`pwd`/repos" URL="file://$REPOS" rm -rf repos wc svnadmin create repos svn co -q ${URL} wc set -x cd wc ## ACTUAL TEST mkdir trunk svn add trunk svn ci -mm svn cp -mm ^/trunk ^/branch svn up echo a > branch/a svn add branch/a svn ci -mm svn up wc echo important data > trunk/a svn add trunk/a cd trunk svn st svn switch --accept=postpone ^/branch svn st svn info a cat a #echo "just for interest, what if we switch back?" #svn switch ^/trunk #svn st #svn info a #cat a |
|
|
Re: tree-conflicts with 'add' -- was: Re: wc-ng patch review
by Julian Foad
::
Rate this Message:
|
|
|
Re: wc-ng patch review
by Julian Foad
::
Rate this Message:
|
|
|
Re: tree-conflicts with 'add'
by Neels Janosch Hofmeyr-2
::
Rate this Message:
Julian Foad wrote:
> Neels J Hofmeyr wrote: >> Hi tree-conflicts folks, >> >> I'm intrigued by the tree-conflicts code in update_editor.c that checks for >> TCs involving add. Using 'update' and 'switch', I can't find any way to >> produce a tree-conflict. >> >> No-one asked for tests about it, so we don't have any. Sorry, I only looked in update_tests.py and switch_tests.py; thanks for this hint: > > Tests exist: > > tree_conflict_tests.py 4 8 12 16 But I see now that although these are successful, we still don't have tests for the simple cases. Add-vs.-add during update/switch only become tree-conflicts when the local add is an add-with-history. My tests with *simple* adds produce no tree-conflicts, nor where the *incoming* add is with-history. Did we at some point decide to not flag simple local adds as tree-conflicts? > Add vs. add: should be a tree conflict. > > (Future enhancements: Implement an option to silently merge the two > things if they are identical, and probably make that the default mode of > operation.) (agreed) ~Neels P.S.: A permutation map of my tests, if you're interested. So we currently only flag an add-vs.-add in 6 out of these 18 cases that I tested: permutations: kinds (local vs. incoming) | | operation | | add-with-history? ---------------------------| |-----------| |------------------- file vs. file | | update | | none dir vs. dir | * | switch | * | local node (1) file vs. dir | | incoming node 3 * 2 * 3 == 18 (1): only the permutations with local node added with history are detected as tree-conflicts (3 * 2 * 1 = 6). (dir vs. file, i.e. local dir & incoming file, still untested, I assume is same as file vs. dir) (*both* local and incoming with history still untested, I assume is same as only local with history) (With these assumptions, we actually catch 16 out of 32 cases) ------------------------------------------------------ http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2415804 |
|
|
Re: tree-conflicts with 'add'
by Julian Foad
::
Rate this Message:
|
signature.asc (205 bytes) 
| Free embeddable forum powered by Nabble | Forum Help |
