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.
> +/* 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.)
> + * 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?)."
> + * 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.
> + */
> +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.")
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.
]]]
- Julian
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>
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2414409
Received on 2009-11-04 14:42:54 CET