These APIs seems ideal for unit tests. Dan, do you already have some,
or shall I put some tests together?
A few comments inline.
On Wed, 17 May 2006, dberlin@tigris.org wrote:
...
> --- (empty file)
> +++ branches/merge-tracking/subversion/include/svn_mergeinfo.h Wed May 17 09:39:14 2006
> @@ -0,0 +1,48 @@
...
> + * @file svn_mergeinfo.h
> + * @brief mergeinfo handling and processing
...
> +/** Parse the mergeinfo stored @a *input (ending at @a end), into @a
> + * hash mapping from svn_pathrev_t to arrays of svn_merge_info_t.
> + * Perform temporary allocations in @a pool.
> + * @since New in 1.5.
> + */
> +svn_error_t *
> +parse_mergeinfo(const char **input, const char *end, apr_hash_t *hash,
> + apr_pool_t *pool);
...
Will svn_mergeinfo.h be scoped to a very tight API (as possiblly
indicated by the "info" in its name), or do you think it'll provide a
home for other merge tracking-related APIs? Perhaps this is a
non-issue, since many of the APIs would live in svn_client.h or a
server-side header (e.g. svn_repos.h)...?
> --- branches/merge-tracking/subversion/include/svn_types.h (original)
> +++ branches/merge-tracking/subversion/include/svn_types.h Wed May 17 09:39:14 2006
> @@ -628,41 +628,47 @@
> */
> typedef enum
> {
> - MERGE_RANGE_SINGLE,
> - MERGE_RANGE_RANGE
> -} merge_range_type_t;
> + SVN_MERGE_RANGE_SINGLE,
> + SVN_MERGE_RANGE_RANGE
> +} svn_merge_range_type_t;
"RANGE_SINGLE" could be confused with "single range" by a casual
reader. "RANGE_RANGE" is just namespaced, but reads a little
redundantly.
How about SVN_MERGE_RANGE_SINGLE_REV (or SVN_MERGE_RANGE_ONE_REV or
SVN_MERGE_RANGE_REV) and SVN_MERGE_RANGE_SPAN as alternate names?
> /**
> * Merge info representing a merge of a single revision.
> * @since New in 1.5
> */
> -typedef struct merge_single_t
> +typedef struct svn_merge_single_t
> {
> svn_revnum_t revision;
> -} merge_single_t;
> +} svn_merge_single_t;
>
> /**
> * Merge info representing a merge of a range of revisions.
> * @since New in 1.5
> */
> -typedef struct merge_range_t
> +typedef struct svn_merge_range_t
> {
> svn_revnum_t start;
> svn_revnum_t end;
> -} merge_range_t;
> +} svn_merge_range_t;
>
> /**
> * Structure containing all possible merge info types
> * @since New in 1.5
> */
> -typedef struct merge_info_t
> +typedef struct svn_merge_info_t
> {
> - merge_range_type_t type;
> + svn_merge_range_type_t type;
> union {
> - merge_range_t range;
> - merge_single_t single;
> + svn_merge_range_t range;
> + svn_merge_single_t single;
> } u;
Do we have to name this union, or can it be completely anonymous?
> -} merge_info_t;
> +} svn_merge_info_t;
> +
> +typedef struct svn_pathrev_pair_t
> +{
> + const char *path;
> + svn_revnum_t revnum;
> +} svn_pathrev_pair_t;
...
Surprising that we don't already have this data structure. :)
> --- (empty file)
> +++ branches/merge-tracking/subversion/libsvn_subr/mergeinfo.c Wed May 17 09:39:14 2006
> @@ -0,0 +1,186 @@
...
> static svn_error_t *
> parse_revision(const char **input, const char *end, svn_revnum_t *revision)
> {
> const char *curr = *input;
> char *endptr;
> svn_revnum_t result = strtol(curr, &endptr, 10);
>
> if (curr == endptr)
> return svn_error_create(SVN_ERR_MERGE_INFO_PARSE_ERROR, NULL, "Invalid revision number");
Long line here which could be wrapped. Error message text should have
_() markup for L10N.
> *revision = result;
>
> *input = endptr;
> return SVN_NO_ERROR;
> }
> +/* pathname -> PATHNAME@REVISION */
Why is PATHNAME@REVISION capitalized in the doc string? Should it
refer to the PATHREV parameter? If it's also part of the grammar, I
recommend indicating so to differentiate it from our internal doc
string formatting conventions.
> +static svn_error_t *
> +parse_pathname(const char **input, const char *end,
> + svn_pathrev_pair_t *pathrev, apr_pool_t *pool)
> +{
> + const char *curr = *input;
> + svn_stringbuf_t *pathname = svn_stringbuf_create("", pool);
> +
> + while (curr < end && *curr != '@')
> + {
> + svn_stringbuf_appendbytes(pathname, curr, 1);
> + curr++;
> + }
> + if (*curr != '@')
> + return svn_error_create(SVN_ERR_MERGE_INFO_PARSE_ERROR, NULL, "No '@' found in pathname");
Error message text should have _() markup.
> + pathrev->path = pathname->data;
> + curr++;
> +
> + if (curr >= end || !svn_ctype_isdigit(*curr))
> + return svn_error_create(SVN_ERR_MERGE_INFO_PARSE_ERROR, NULL,
> + "Revision number missing after ");
Trailing whitespace on this error message -- unfinished sentence?
(Looks like it should include the path name.) Error message text
should have _() markup.
> +
> + SVN_ERR(parse_revision(&curr, end, &pathrev->revnum));
> + *input = curr;
> +
> + return SVN_NO_ERROR;
> +}
> +
> +
> +/* revisionlist -> (revisionrange | REVISION)(COMMA revisioneelement)*
> + revisionrange -> REVISION "-" REVISION
> + revisioneelement -> revisionrange | REVISION
> + */
See comment for parse_pathname().
> +static svn_error_t *
> +parse_revlist(const char **input, const char *end, apr_array_header_t *revlist,
> + apr_pool_t *pool)
> +{
> + const char *curr = *input;
> +
> + if (curr == end)
> + return svn_error_create(SVN_ERR_MERGE_INFO_PARSE_ERROR, NULL,
> + "No revision list found ");
Trailing whitespace. Error message text should have _() markup.
> + while (curr < end)
> + {
> + svn_merge_info_t *minfo = apr_pcalloc(pool, sizeof(*minfo));
> + svn_revnum_t firstrev;
> +
> + SVN_ERR(parse_revision(&curr, end, &firstrev));
> + if (*curr != '-' && *curr != '\n' && *curr != ',' && curr != end)
> + return svn_error_create(SVN_ERR_MERGE_INFO_PARSE_ERROR, NULL,
> + "Invalid character found in revision list");
Error message text should have _() markup.
> + minfo->type = SVN_MERGE_RANGE_SINGLE;
> + minfo->u.single.revision = firstrev;
> +
> + if (*curr == '-')
> + {
> + svn_revnum_t secondrev;
> +
> + curr++;
> + SVN_ERR(parse_revision(&curr, end, &secondrev));
> + minfo->type = SVN_MERGE_RANGE_RANGE;
> + minfo->u.range.start = firstrev;
> + minfo->u.range.end = secondrev;
> + }
> +
> + /* XXX: Watch empty revision list problem */
> + if (*curr == '\n' || curr == end)
> + {
> + APR_ARRAY_PUSH(revlist, svn_merge_info_t *) = minfo;
> + *input = curr;
> + return SVN_NO_ERROR;
> + }
> + else if (*curr == ',')
> + {
> + APR_ARRAY_PUSH(revlist, svn_merge_info_t *) = minfo;
> + curr++;
> + }
> + else
> + {
> + return svn_error_create(SVN_ERR_MERGE_INFO_PARSE_ERROR, NULL,
> + "Invalid character found in revision list");
Error message text should have _() markup.
> + }
> +
> + }
> + if (*curr != '\n')
> + return svn_error_create(SVN_ERR_MERGE_INFO_PARSE_ERROR, NULL,
> + "Revision list parsing ended before hitting newline");
Error message text should have _() markup.
> + *input = curr;
> + return SVN_NO_ERROR;
> +}
> +
> +/* revisionline -> PATHNAME@REVISION COLON revisionlist */
See comment for parse_pathname().
> +static svn_error_t *
> +parse_revision_line(const char **input, const char *end, apr_hash_t *hash,
> + apr_pool_t *pool)
> +{
> + svn_pathrev_pair_t *pair = apr_pcalloc(pool, sizeof (*pair));
Does the space-before-paren policy apply to sizeof()? (I think so.)
> + apr_array_header_t *revlist = apr_array_make(pool, 1,
> + sizeof(svn_merge_info_t *));
> +
> + SVN_ERR(parse_pathname(input, end, pair, pool));
> +
> + if (*(*input) != ':')
> + return svn_error_create(SVN_ERR_MERGE_INFO_PARSE_ERROR, NULL,
> + "Pathname not terminated by ':'");
Error message text should have _() markup.
> + *input = *input + 1;
> +
> + SVN_ERR(parse_revlist(input, end, revlist, pool));
> +
> + if (*input != end && *(*input) != '\n')
> + return svn_error_create(SVN_ERR_MERGE_INFO_PARSE_ERROR, NULL,
> + "Could not find end of line in revision line");
Error message text should have _() markup.
> + if (*input != end)
> + *input = *input + 1;
> +
> + apr_hash_set (hash, pair, sizeof (*pair), revlist);
Shouldn't have a space-before-paren.
> + return SVN_NO_ERROR;
> +}
> +
> +/* top -> revisionline (NEWLINE revisionline)* */
Ditto.
> +static svn_error_t *
> +parse_top(const char **input, const char *end, apr_hash_t *hash,
> + apr_pool_t *pool)
> +{
> + while (*input < end)
> + SVN_ERR(parse_revision_line(input, end, hash, pool));
> +
> + return SVN_NO_ERROR;
> +}
> +
> +/* Parse mergeinfo. */
> +svn_error_t *
> +parse_mergeinfo(const char **input, const char *end, apr_hash_t *hash,
> + apr_pool_t *pool)
> +{
> + return parse_top(input, end, hash, pool);
> +}
- application/pgp-signature attachment: stored
Received on Thu May 18 01:09:02 2006