Madan U Sreenivasan wrote:
> Hey Guys,
> Find attached the code fix, test case and a modified existing test
> case ( and of course the log )
Overall, very nice. I like how you made sure that you cannot set the same
keyword twice (but you should really add a test for that just to show that it
works). It's also nicer if you include the commit message in your e-mail (i.e.
before any attachments), just so people can see up front what they are going to
review. My personal preference is for the order of files in the commit message
to match up with the order of the files in the diff file (but that's just me).
A couple of formatting things inline (and one philosophical argument):
> Yet to make the doc changes....
>
> Regards,
> Madan.
>
>
> ------------------------------------------------------------------------
>
> Index: subversion/libsvn_wc/props.c
> ===================================================================
> --- subversion/libsvn_wc/props.c (revision 13300)
> +++ subversion/libsvn_wc/props.c (working copy)
> @@ -1061,7 +1061,7 @@
> }
> else if (strcmp (name, SVN_PROP_KEYWORDS) == 0)
> {
> - new_value = svn_stringbuf_create_from_string (value, pool);
> + new_value = svn_subst_canonicalize_keywords (value, pool);
> svn_stringbuf_strip_whitespace (new_value);
> }
> }
> Index: subversion/libsvn_subr/subst.c
> ===================================================================
> --- subversion/libsvn_subr/subst.c (revision 13300)
> +++ subversion/libsvn_subr/subst.c (working copy)
> @@ -47,6 +47,28 @@
> */
> #define SVN_SUBST__SPECIAL_LINK_STR "link"
>
> +/* Structure to be used for canonicalization of svn:keywords */
> +typedef struct canon_table_s
> +{
> + char prop_value[SVN_KEYWORD_MAX_LEN]; /* the keyword */
> + int canon_index; /* the corresponding canonical form */
> + int canon_flag;
> +} canon_table_t;
> +canon_table_t canon_table[] =
> + {
> + { SVN_KEYWORD_REVISION_LONG, 2, 0x0001 },
> + { SVN_KEYWORD_REVISION_SHORT, 2, 0x0001 },
> + { SVN_KEYWORD_REVISION_MEDIUM, 2, 0x0001 },
> + { SVN_KEYWORD_DATE_LONG, 4, 0x0002 },
> + { SVN_KEYWORD_DATE_SHORT, 4, 0x0002 },
> + { SVN_KEYWORD_AUTHOR_LONG, 6, 0x0004 },
> + { SVN_KEYWORD_AUTHOR_SHORT, 6, 0x0004 },
> + { SVN_KEYWORD_URL_SHORT, 7, 0x0008 },
> + { SVN_KEYWORD_URL_LONG, 7, 0x0008 },
> + { SVN_KEYWORD_ID, 9, 0x0010 }
> + };
> +int sizeof_canon_table = sizeof(canon_table)/sizeof(canon_table_t) ;
> +
> void
> svn_subst_eol_style_from_value (svn_subst_eol_style_t *style,
> const char **eol,
> @@ -118,6 +140,55 @@
> return SVN_NO_ERROR;
> }
>
> +/* Canonicalize the @a value var, assuming that it contains
> + a sequence of svn:keywords. */
Since you are creating this from scratch, I would strongly urge you to dump all
of this into props.c as a fully private function and support structures. There
is no real reason IMNSHO why all of the "keywords-aware" code has to live
exclusively in subst.c. This is strictly data validation for some properties
and as such, probably belongs in props.c. Don't take this as gospel, however,
but rather as my position (which I am prepared to argue). ;-)
If you are going to leave this in subst.c (and make it a public function), you
should really move the structure definition and add a function prototype to
subversion/include/svn_subst.h.
> +svn_stringbuf_t *
> +svn_subst_canonicalize_keywords (const svn_string_t *value,
> + apr_pool_t *pool)
> +{
> + apr_array_header_t *keyword_tokens;
> + svn_stringbuf_t *canonicalized_value = NULL ;
> + int flags = 0 ;
> + int i, j;
> +
> + /* tokenize the input */
> + keyword_tokens = svn_cstring_split (value->data, " \t\v\n\b\r\f",
> + TRUE /* chop */, pool);
> +
> + /* for all the tokens */
> + for (i = 0; i < keyword_tokens->nelts; ++i)
> + {
> + const char *keyword = APR_ARRAY_IDX (keyword_tokens, i, const char *);
> +
> + for (j = 0; j < sizeof_canon_table; j++)
> + {
> + /* see if a equivalent standard form exists */
> + if ((! strcasecmp (canon_table[j].prop_value, keyword))
> + && (! (flags & canon_table[j].canon_flag )))
> + {
> + /* If so, canonicalize and prepare output */
> + if (!canonicalized_value)
> + canonicalized_value = svn_stringbuf_create (
> + canon_table[canon_table[j].canon_index].prop_value,
> + pool);
That line is too long (by half) and the indents are wrong. Load
tools/dev/svn-dev.vim or svn-dev.el if you are running vim or emacs respectively
and reformat.
> + else
> + {
> + svn_stringbuf_appendcstr (
> + canonicalized_value,
> + " ");
wrong indent
> + svn_stringbuf_appendcstr (
> + canonicalized_value,
> + canon_table[canon_table[j].canon_index].prop_value);
wrong indent
> + }
> + flags |= canon_table[j].canon_flag ;
> + break; /* goto the next token from the input */
> + }
> + }
> + }
> +
> + return canonicalized_value ;
> +}
> +
> svn_error_t *
> svn_subst_build_keywords (svn_subst_keywords_t *kw,
> const char *keywords_val,
HTH
John
--
John Peacock
Director of Information Research and Technology
Rowman & Littlefield Publishing Group
4720 Boston Way
Lanham, MD 20706
301-459-3366 x.5010
fax 301-429-5747
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Tue Mar 8 13:17:21 2005