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?)."
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.
>
>> + * 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.
You're right, in practice it doesn't. But the result pool is passed to
__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.")
Yeah, I think we should press the wc-ng guys to properly document it for the
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.
> ]]]
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?
<design-mode>
This code determines the tree-conflict information shown, i.e.:
[[[
Source left: () ^/foo_at_1
Source right: (file) ^/foo_at_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_at_BASE with KIND == kind-in-BASE ( == kind-in-HEAD)
right: PATH_at_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_at_BASE with KIND == kind-in-BASE
right: PATH_at_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_at_BASE with KIND == svn_node_none.
right: PATH_at_HEAD with KIND == kind-in-HEAD
- In action=="simple delete", "Source *right*" would be <nothing>. Again, it
is more accurate to say:
--> left: PATH_at_BASE with KIND == kind-in-BASE
right: PATH_at_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_at_BASE. On the
other hand, it might make sense to state the PATH_at_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_at_HEAD. On the other
hand, it might make sense to state the PATH_at_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_at_BASE with KIND == kind-in-BASE
right: SWITCH_TO_PATH_at_HEAD with KIND == kind-in-SWITCH_TO_PATH_at_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;
Received on 2009-11-04 17:34:57 CET