On Tue, 2009-01-06 at 03:00 -0800, Greg Stein wrote:
> Author: gstein
> Date: Tue Jan 6 03:00:30 2009
> New Revision: 35036
>
> Log:
> Switch tree conflict storage over to skels, rather than a custom format.
> Solves issue #3343.
Greg,
Thanks ever so much for doing this!
Reviewing...
> Review and additional work by: hwright
>
> * subversion/libsvn_wc/tree_conflicts.h:
> (svn_wc__write_tree_conflicts): constify a parameter
>
> * subversion/libsvn_wc/tree_conflicts.c:
> (field_separator, desc_separator, escape_char, read_field_separator,
> read_desc_separator, read_string_field, write_string_field,
> write_integer_field): removed
> (is_valid_version_info_skel, is_valid_conflict_skel): new
> (read_enum_field): read the field from a skel, rather than a character
> start/end position. the pool param is no longer required.
> (read_node_version_info, read_one_tree_conflict): read from a skel
> instead of a string. split the single pool param into both scratch and
> result pools.
> (svn_wc__read_tree_conflicts): parse the input into a skel, and then
> walk that to construct the tree conflict structures.
> (write_enum_field): renamed to ...
> (skel_prepend_enum): ... this. prepend the mapped value to a given skel.
> (prepend_version_info_skel): build a skel for the version info, and
> prepend it to the provided skel.
> (svn_wc__write_tree_conflicts): constify a parameter, and switch to
> skel-based preparation.
> (svn_wc__loggy_del_tree_conflict): remove temp variable, writing the
> conflict data result directly into the entry (save a strdup).
>
> * subversion/libsvn_wc/log.c:
> (run_log): remove temp variable, writing directly into the entry
> structure, saving a strdup.
> (svn_wc__loggy_add_tree_conflict): constify temp variable.
>
> * subversion/tests/libsvn_wc/tree-conflict-data-test.c:
> (test_read_tree_conflict, test_read_2_tree_conflicts): change the
> expected string representation of the tree conflicts.
> (test_write_tree_conflict, test_write_2_tree_conflicts): constify a
> param and change the expected string rep of the conflict.
> (test_write_invalid_tree_conflicts): constify a variable.
> Modified: trunk/subversion/libsvn_wc/tree_conflicts.c
> URL: http://svn.collab.net/viewvc/svn/trunk/subversion/libsvn_wc/tree_conflicts.c?pathrev=35036&r1=35035&r2=35036
> ==============================================================================
> --- trunk/subversion/libsvn_wc/tree_conflicts.c Tue Jan 6 02:39:11 2009 (r35035)
> +++ trunk/subversion/libsvn_wc/tree_conflicts.c Tue Jan 6 03:00:30 2009 (r35036)
We need to update the big comment near the beginning of this file that
describes the data format.
> +
> +static svn_boolean_t
> +is_valid_version_info_skel(const svn_skel_t *skel)
Please give the new function a doc string even if it is brief. What's
the intended level of validation here?
> +{
> + return (svn_skel__list_length(skel) == 5
> + && svn_skel__matches_atom(skel->children, "version")
> + && skel->children->next->is_atom
> + && skel->children->next->next->is_atom
> + && skel->children->next->next->next->is_atom
> + && skel->children->next->next->next->next->is_atom);
> +}
> +
> +
> +static svn_boolean_t
> +is_valid_conflict_skel(const svn_skel_t *skel)
Ditto.
> +{
> + int i;
> +
> + if (svn_skel__list_length(skel) != 8
> + || !svn_skel__matches_atom(skel->children, "conflict"))
> + return FALSE;
> +
> + /* 5 atoms ... */
> + skel = skel->children->next;
> + for (i = 5; i--; skel = skel->next)
> + if (!skel->is_atom)
> + return FALSE;
> +
> + /* ... and 2 version info skels. */
> + return (is_valid_version_info_skel(skel)
> + && is_valid_version_info_skel(skel->next));
> +}
> +
> +/* Parse the enumeration value in VALUE into a plain
> * 'int', using MAP to convert from strings to enumeration values.
> - * In MAP, a null STR field marks the end of the map.
> - * Don't read further than END.
> - * After reading, make *START point to the character after the field.
> + * In MAP, a null .str field marks the end of the map.
Please update the doc string to match the arguments. Especially, what is
SKEL on entry?
> */
> static svn_error_t *
> read_enum_field(int *result,
> const enum_mapping_t *map,
> - const char **start,
> - const char *end,
> - apr_pool_t *pool)
> + const svn_skel_t *skel)
> {
> - const char *str;
> int i;
>
> - SVN_ERR(read_string_field(&str, start, end, pool));
> -
> /* Find STR in MAP; error if not found. */
> for (i = 0; ; i++)
> {
> @@ -209,7 +156,10 @@ read_enum_field(int *result,
> return svn_error_create(SVN_ERR_WC_CORRUPT, NULL,
> _("Unknown enumeration value in tree conflict "
> "description"));
> - if (strcmp(str, map[i].str) == 0)
> + /* ### note: theoretically, a corrupt skel could have a long value
> + ### whose prefix is one of our enumerated values, and so we'll
> + ### choose that enum. fine. we'll accept these "corrupt" values. */
> + if (strncmp(skel->data, map[i].str, skel->len) == 0)
> break;
The "strncmp" compares up to the skel length. That means the sense of
the comment is reversed: a corrupt skel could have a short value which
matches a prefix of one of our enumerated values. Either way around,
this could be acceptable if we are careful that no enumerator is a
prefix of another, but that's not a guarantee I really want to make for
future maintenance purposes. (To illustrate the concept, imagine we want
both "added" and "added-with-history" as two distinct values.) Without
that guarantee, we could falsely match a short enumerator against a
longer one.
Can we ensure that only a full match is detected? e.g.
if (strncmp(...)
&& skel->len == strlen(map[i].str))
> }
>
> @@ -217,37 +167,40 @@ read_enum_field(int *result,
> return SVN_NO_ERROR;
> }
>
> -/* Parse the conflict info fields pointed to by *START into *VERSION_INFO.
> - * Don't read further than END.
> - * After reading, make *START point to the character after the field.
> - */
> +/* Parse the conflict info fields from SKEL into *VERSION_INFO. */
> static svn_error_t *
> read_node_version_info(svn_wc_conflict_version_t *version_info,
> - const char **start,
> - const char *end,
> - apr_pool_t *pool)
> + const svn_skel_t *skel,
> + apr_pool_t *scratch_pool,
> + apr_pool_t *result_pool)
> {
> - const char *str;
> int n;
>
> - /* repos_url */
> - SVN_ERR(read_string_field(&str, start, end, pool));
> - version_info->repos_url = (str[0] == '\0') ? NULL : str;
> - SVN_ERR(read_field_separator(start, end));
> -
> - /* peg_rev */
> - SVN_ERR(read_string_field(&str, start, end, pool));
> - version_info->peg_rev = (str[0] == '\0') ? SVN_INVALID_REVNUM
> - : SVN_STR_TO_REV(str);
> - SVN_ERR(read_field_separator(start, end));
> + if (!is_valid_version_info_skel(skel))
> + return svn_error_create(SVN_ERR_WC_CORRUPT, NULL,
> + _("Invalid version info in tree conflict "
> + "description"));
>
> - /* path_in_repos */
> - SVN_ERR(read_string_field(&str, start, end, pool));
> - version_info->path_in_repos = (str[0] == '\0') ? NULL : str;
> - SVN_ERR(read_field_separator(start, end));
> + version_info->repos_url = apr_pstrmemdup(result_pool,
> + skel->children->next->data,
> + skel->children->next->len);
> + if (*version_info->repos_url == '\0')
> + version_info->repos_url = NULL;
> +
> + version_info->peg_rev =
> + SVN_STR_TO_REV(apr_pstrmemdup(scratch_pool,
> + skel->children->next->next->data,
> + skel->children->next->next->len));
> +
> + version_info->path_in_repos =
> + apr_pstrmemdup(result_pool,
> + skel->children->next->next->next->data,
> + skel->children->next->next->next->len);
> + if (*version_info->path_in_repos == '\0')
> + version_info->path_in_repos = NULL;
>
> - /* node_kind */
> - SVN_ERR(read_enum_field(&n, node_kind_map, start, end, pool));
> + SVN_ERR(read_enum_field(&n, node_kind_map,
> + skel->children->next->next->next->next));
> version_info->node_kind = (svn_node_kind_t)n;
>
> return SVN_NO_ERROR;
> @@ -262,10 +215,10 @@ read_node_version_info(svn_wc_conflict_v
> */
Please update the doc string.
> static svn_error_t *
> read_one_tree_conflict(svn_wc_conflict_description_t **conflict,
> - const char **start,
> - const char *end,
> + const svn_skel_t *skel,
> const char *dir_path,
> - apr_pool_t *pool)
> + apr_pool_t *scratch_pool,
> + apr_pool_t *result_pool)
> {
> const char *victim_basename;
> svn_node_kind_t node_kind;
> @@ -274,60 +227,66 @@ read_one_tree_conflict(svn_wc_conflict_d
> svn_wc_conflict_version_t *src_right_version;
> int n;
>
> - SVN_ERR_ASSERT(*start < end);
> -
> - /* Each read_...() call modifies *START ! */
> + if (!is_valid_conflict_skel(skel))
> + return svn_error_create(SVN_ERR_WC_CORRUPT, NULL,
> + _("Invalid conflict info in tree conflict "
> + "description"));
>
> /* victim basename */
> - SVN_ERR(read_string_field(&victim_basename, start, end, pool));
> + victim_basename = apr_pstrmemdup(scratch_pool,
> + skel->children->next->data,
> + skel->children->next->len);
> if (victim_basename[0] == '\0')
> return svn_error_create(SVN_ERR_WC_CORRUPT, NULL,
> _("Empty 'victim' field in tree conflict "
> "description"));
> - SVN_ERR(read_field_separator(start, end));
>
> /* node_kind */
> - SVN_ERR(read_enum_field(&n, node_kind_map, start, end, pool));
> + SVN_ERR(read_enum_field(&n, node_kind_map, skel->children->next->next));
> node_kind = (svn_node_kind_t)n;
> if (node_kind != svn_node_file && node_kind != svn_node_dir)
> return svn_error_create(SVN_ERR_WC_CORRUPT, NULL,
> _("Invalid 'node_kind' field in tree conflict description"));
> - SVN_ERR(read_field_separator(start, end));
>
> /* operation */
> - SVN_ERR(read_enum_field(&n, operation_map, start, end, pool));
> + SVN_ERR(read_enum_field(&n, operation_map,
> + skel->children->next->next->next));
> operation = (svn_wc_operation_t)n;
> - SVN_ERR(read_field_separator(start, end));
>
> /* Construct the description object */
> src_left_version = svn_wc_conflict_version_create(NULL, NULL,
> SVN_INVALID_REVNUM,
> - svn_node_none, pool);
> + svn_node_none,
> + result_pool);
> src_right_version = svn_wc_conflict_version_create(NULL, NULL,
> SVN_INVALID_REVNUM,
> - svn_node_none, pool);
> + svn_node_none,
> + result_pool);
> *conflict = svn_wc_conflict_description_create_tree(
> - svn_path_join(dir_path, victim_basename, pool),
> - NULL, node_kind, operation, src_left_version, src_right_version, pool);
> + svn_path_join(dir_path, victim_basename, result_pool),
> + NULL, node_kind, operation, src_left_version, src_right_version,
> + result_pool);
>
> /* action */
> - SVN_ERR(read_enum_field(&n, action_map, start, end, pool));
> + SVN_ERR(read_enum_field(&n, action_map,
> + skel->children->next->next->next->next));
> (*conflict)->action = (svn_wc_conflict_action_t)n;
> - SVN_ERR(read_field_separator(start, end));
>
> /* reason */
> - SVN_ERR(read_enum_field(&n, reason_map, start, end, pool));
> + SVN_ERR(read_enum_field(&n, reason_map,
> + skel->children->next->next->next->next->next));
> (*conflict)->reason = (svn_wc_conflict_reason_t)n;
> - SVN_ERR(read_field_separator(start, end));
> +
> + /* Let's just make it a bit easier on ourself here... */
> + skel = skel->children->next->next->next->next->next->next;
For maintainability I would do that a bit more often in smaller steps!
>
> /* src_left_version */
> - SVN_ERR(read_node_version_info((*conflict)->src_left_version, start, end,
> - pool));
> - SVN_ERR(read_field_separator(start, end));
> + SVN_ERR(read_node_version_info((*conflict)->src_left_version, skel,
> + scratch_pool, result_pool));
>
> /* src_right_version */
> - SVN_ERR(read_node_version_info((*conflict)->src_right_version, start, end,
> - pool));
> + SVN_ERR(read_node_version_info((*conflict)->src_right_version, skel->next,
> + scratch_pool, result_pool));
>
> return SVN_NO_ERROR;
> }
> @@ -338,7 +297,8 @@ svn_wc__read_tree_conflicts(apr_array_he
> const char *dir_path,
> apr_pool_t *pool)
> {
> - const char *start, *end;
> + const svn_skel_t *skel;
> + apr_pool_t *iterpool;
>
> *conflicts = apr_array_make(pool, 0,
> sizeof(svn_wc_conflict_description_t *));
> @@ -346,53 +306,34 @@ svn_wc__read_tree_conflicts(apr_array_he
> if (conflict_data == NULL)
> return SVN_NO_ERROR;
>
> - start = conflict_data;
> - end = start + strlen(start);
> + skel = svn_skel__parse(conflict_data, strlen(conflict_data), pool);
> + if (skel == NULL)
> + return svn_error_create(SVN_ERR_WC_CORRUPT, NULL,
> + _("Error parsing tree conflict skel"));
>
> - while (start < end)
> + iterpool = svn_pool_create(pool);
> + for (skel = skel->children; skel != NULL; skel = skel->next)
> {
> svn_wc_conflict_description_t *conflict;
>
> - SVN_ERR(read_one_tree_conflict(&conflict, &start, end, dir_path, pool));
> + svn_pool_clear(iterpool);
> + SVN_ERR(read_one_tree_conflict(&conflict, skel, dir_path, iterpool,
> + pool));
> if (conflict != NULL)
> APR_ARRAY_PUSH(*conflicts, svn_wc_conflict_description_t *) = conflict;
> -
> - /* *START should now point to a description separator
> - * if there are any descriptions left. */
> - if (start < end)
> - SVN_ERR(read_desc_separator(&start, end));
> }
> + svn_pool_destroy(iterpool);
>
> return SVN_NO_ERROR;
> }
>
> -/* Append to BUF the string STR, escaping it as necessary. */
> -static void
> -write_string_field(svn_stringbuf_t *buf,
> - const char *str)
> -{
> - int len = strlen(str);
> - int i;
> -
> - /* Escape separator chars. */
> - for (i = 0; i < len; i++)
> - {
> - if ((str[i] == field_separator)
> - || (str[i] == desc_separator)
> - || (str[i] == escape_char))
> - {
> - svn_stringbuf_appendbytes(buf, &escape_char, 1);
> - }
> - svn_stringbuf_appendbytes(buf, &str[i], 1);
> - }
> -}
> -
> -/* Append to BUF the string corresponding to enumeration value N, as found
> +/* Prepend to SKEL the string corresponding to enumeration value N, as found
> * in MAP. */
> static svn_error_t *
> -write_enum_field(svn_stringbuf_t *buf,
> - const enum_mapping_t *map,
> - int n)
> +skel_prepend_enum(svn_skel_t *skel,
> + const enum_mapping_t *map,
> + int n,
> + apr_pool_t *result_pool)
> {
> int i;
>
> @@ -402,40 +343,44 @@ write_enum_field(svn_stringbuf_t *buf,
> if (map[i].val == n)
> break;
> }
> - svn_stringbuf_appendcstr(buf, map[i].str);
> +
> + svn_skel__prepend(svn_skel__str_atom(map[i].str, result_pool), skel);
> return SVN_NO_ERROR;
> }
>
> -/* Append to BUF the denary form of the number N. */
> -static void
> -write_integer_field(svn_stringbuf_t *buf,
> - int n,
> - apr_pool_t *pool)
> +/* Prepend to PARENT_SKEL the several fields that represent VERSION_INFO, */
> +static svn_error_t *
> +prepend_version_info_skel(svn_skel_t *parent_skel,
> + const svn_wc_conflict_version_t *version_info,
> + apr_pool_t *pool)
> {
> - const char *str = apr_psprintf(pool, "%d", n);
> + svn_skel_t *skel = svn_skel__make_empty_list(pool);
>
> - svn_stringbuf_appendcstr(buf, str);
> -}
> + /* node_kind */
> + SVN_ERR(skel_prepend_enum(skel, node_kind_map, version_info->node_kind,
> + pool));
>
> -/* Append to BUF the several fields that represent VERSION_INFO, */
> -static svn_error_t *
> -write_node_version_info(svn_stringbuf_t *buf,
> - const svn_wc_conflict_version_t *version_info,
> - apr_pool_t *pool)
> -{
> - if (version_info->repos_url)
> - write_string_field(buf, version_info->repos_url);
> - svn_stringbuf_appendbytes(buf, &field_separator, 1);
> -
> - if (SVN_IS_VALID_REVNUM(version_info->peg_rev))
> - write_integer_field(buf, version_info->peg_rev, pool);
> - svn_stringbuf_appendbytes(buf, &field_separator, 1);
> -
> - if (version_info->path_in_repos)
> - write_string_field(buf, version_info->path_in_repos);
> - svn_stringbuf_appendbytes(buf, &field_separator, 1);
> + /* path_in_repos */
> + svn_skel__prepend(svn_skel__str_atom(version_info->path_in_repos
> + ? version_info->path_in_repos
> + : "", pool), skel);
> +
> + /* peg_rev */
> + svn_skel__prepend(svn_skel__str_atom(apr_psprintf(pool, "%ld",
> + version_info->peg_rev),
> + pool), skel);
> +
> + /* repos_url */
> + svn_skel__prepend(svn_skel__str_atom(version_info->repos_url
> + ? version_info->repos_url
> + : "", pool), skel);
> +
> + svn_skel__prepend(svn_skel__str_atom("version", pool), skel);
> +
> + SVN_ERR_ASSERT(is_valid_version_info_skel(skel));
> +
> + svn_skel__prepend(skel, parent_skel);
>
> - SVN_ERR(write_enum_field(buf, node_kind_map, version_info->node_kind));
> return SVN_NO_ERROR;
> }
>
> @@ -444,72 +389,68 @@ write_node_version_info(svn_stringbuf_t
> * in a unit test in tests/libsvn_wc/, so it isn't.
> */
> svn_error_t *
> -svn_wc__write_tree_conflicts(char **conflict_data,
> +svn_wc__write_tree_conflicts(const char **conflict_data,
> apr_array_header_t *conflicts,
> apr_pool_t *pool)
> {
> /* A conflict version struct with all fields null/invalid. */
> static const svn_wc_conflict_version_t null_version = {
> NULL, SVN_INVALID_REVNUM, NULL, svn_node_unknown };
> - svn_stringbuf_t *buf = svn_stringbuf_create("", pool);
> int i;
> + svn_skel_t *skel = svn_skel__make_empty_list(pool);
>
> - for (i = 0; i < conflicts->nelts; i++)
> + /* Iterate backwards so that the list-prepend will build the skel in
> + proper order. */
> + for (i = conflicts->nelts; --i >= 0; )
> {
> const char *path;
> const svn_wc_conflict_description_t *conflict =
> APR_ARRAY_IDX(conflicts, i, svn_wc_conflict_description_t *);
> + svn_skel_t *c_skel = svn_skel__make_empty_list(pool);
>
> - /* Victim path (escaping separator chars). */
> - path = svn_path_basename(conflict->path, pool);
> - SVN_ERR_ASSERT(strlen(path) > 0);
> - write_string_field(buf, path);
> -
> - svn_stringbuf_appendbytes(buf, &field_separator, 1);
> -
> - /* node_kind */
> - SVN_ERR_ASSERT(conflict->node_kind == svn_node_dir
> - || conflict->node_kind == svn_node_file);
> - SVN_ERR(write_enum_field(buf, node_kind_map, conflict->node_kind));
> -
> - svn_stringbuf_appendbytes(buf, &field_separator, 1);
> + /* src_right_version */
> + if (conflict->src_right_version)
> + SVN_ERR(prepend_version_info_skel(c_skel, conflict->src_right_version,
> + pool));
> + else
> + SVN_ERR(prepend_version_info_skel(c_skel, &null_version, pool));
>
> - /* operation */
> - SVN_ERR(write_enum_field(buf, operation_map, conflict->operation));
> + /* src_left_version */
> + if (conflict->src_left_version)
> + SVN_ERR(prepend_version_info_skel(c_skel, conflict->src_left_version,
> + pool));
> + else
> + SVN_ERR(prepend_version_info_skel(c_skel, &null_version, pool));
>
> - svn_stringbuf_appendbytes(buf, &field_separator, 1);
> + /* reason */
> + SVN_ERR(skel_prepend_enum(c_skel, reason_map, conflict->reason, pool));
>
> /* action */
> - SVN_ERR(write_enum_field(buf, action_map, conflict->action));
> -
> - svn_stringbuf_appendbytes(buf, &field_separator, 1);
> + SVN_ERR(skel_prepend_enum(c_skel, action_map, conflict->action, pool));
>
> - /* reason */
> - SVN_ERR(write_enum_field(buf, reason_map, conflict->reason));
> + /* operation */
> + SVN_ERR(skel_prepend_enum(c_skel, operation_map, conflict->operation,
> + pool));
>
> - svn_stringbuf_appendbytes(buf, &field_separator, 1);
> + /* node_kind */
> + SVN_ERR_ASSERT(conflict->node_kind == svn_node_dir
> + || conflict->node_kind == svn_node_file);
> + SVN_ERR(skel_prepend_enum(c_skel, node_kind_map, conflict->node_kind,
> + pool));
>
> - /* src_left_version */
> - if (conflict->src_left_version)
> - SVN_ERR(write_node_version_info(buf, conflict->src_left_version,
> - pool));
> - else
> - SVN_ERR(write_node_version_info(buf, &null_version, pool));
> + /* Victim path (escaping separator chars). */
> + path = svn_path_basename(conflict->path, pool);
> + SVN_ERR_ASSERT(strlen(path) > 0);
> + svn_skel__prepend(svn_skel__str_atom(path, pool), c_skel);
>
> - svn_stringbuf_appendbytes(buf, &field_separator, 1);
> + svn_skel__prepend(svn_skel__str_atom("conflict", pool), c_skel);
>
> - /* src_right_version */
> - if (conflict->src_right_version)
> - SVN_ERR(write_node_version_info(buf, conflict->src_right_version,
> - pool));
> - else
> - SVN_ERR(write_node_version_info(buf, &null_version, pool));
> + SVN_ERR_ASSERT(is_valid_conflict_skel(c_skel));
>
> - if (i < (conflicts->nelts - 1))
> - svn_stringbuf_appendbytes(buf, &desc_separator, 1);
> + svn_skel__prepend(c_skel, skel);
> }
>
> - *conflict_data = apr_pstrdup(pool, buf->data);
> + *conflict_data = svn_skel__unparse(skel, pool)->data;
>
> return SVN_NO_ERROR;
> }
Other than that, on a read-through review, it looks good.
- Julian
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=1009851
Received on 2009-01-07 12:42:32 CET