I just have some comments on formatting, for the C code, and a couple of
comments about the Python tests.
Madan U Sreenivasan wrote:
> [[[
> Fixed Issue #2219: Canonicalize values in svn:keywords
Add a full stop and a blank line, please.
> * subversion/libsvn_wc/subst.h
> Added comment to refer to svn_canonicalize_keywords()
> in props.c
You should name the item that is affected:
(svn_subst_keywords_t): Added comment to refer to
svn_canonicalize_keywords() in props.c
[...]
> Index: subversion/libsvn_wc/props.c
> ===================================================================
> --- subversion/libsvn_wc/props.c (revision 13317)
> +++ subversion/libsvn_wc/props.c (working copy)
> @@ -987,6 +987,82 @@
> }
>
>
> +/** Return in an <tt>svn_stringbuf_t *</tt> with the
Delete "with".
> + * canonicalized equivalent of @a value
Delete the extra space; add a full stop.
> + * Repeated instances of the same keyword are ignored.
Delete the extra space.
> + *
> + * All memory is allocated out of @a pool.
> + */
> +static svn_stringbuf_t *
> +svn_canonicalize_keywords (const svn_string_t *value,
> + apr_pool_t *pool)
Delete the extra whitespace.
> +{
> + /* Structure to be used for canonicalization of svn:keywords */
> + typedef struct canon_table_s
> + {
> + const char *prop_value; /* the property value */
> + const int index; /* the index to the corresponding canonical form */
> + const int flag; /* bit pattern to flag that this canonical form
> + ** is already set */
> + } canon_table_t;
> + static canon_table_t canon_kw_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 }
> + };
> + const int sizeof_canon_kw_table =
> + sizeof(canon_kw_table)/sizeof(canon_table_t) ;
Odd spacing. We usually indent by 2 spaces, and put a space before opening
parentheses, around operators like "/", and not before semicolons. Also, it
seems that we prefer to put an operator at the beginning of the continuation
line rather than at the end of the first line. So:
const int sizeof_canon_kw_table
= sizeof (canon_kw_table) / sizeof (canon_table_t);
> +
> + apr_array_header_t *keyword_tokens;
> + svn_stringbuf_t *canonicalized_value = NULL ;
Delete the extra space.
> + 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_kw_table; j++)
> + {
> + /* see if a equivalent standard form exists */
"an equivalent"
> + if ((! strcasecmp (canon_kw_table[j].prop_value, keyword))
> + && (! (flags & canon_kw_table[j].flag )))
> + {
> + /* If so, canonicalize and prepare output */
> + if (!canonicalized_value)
> + canonicalized_value =
> + svn_stringbuf_create (
> + canon_kw_table[canon_kw_table[j].index].prop_value,
> + pool);
Again, in the rest of the Subversion code I don't often see a line split after
"=" or "(", so:
if (!canonicalized_value)
canonicalized_value = svn_stringbuf_create
(canon_kw_table[canon_kw_table[j].index].prop_value,
pool);
> + else
> + {
> + svn_stringbuf_appendcstr (canonicalized_value, " ");
Too much indentation.
> + svn_stringbuf_appendcstr (
> + canonicalized_value,
> + canon_kw_table[canon_kw_table[j].index].prop_value);
svn_stringbuf_appendcstr
(canonicalized_value,
canon_kw_table[canon_kw_table[j].index].prop_value);
> + }
> + flags |= canon_kw_table[j].flag ;
Delete the extra spaces (two).
> + break; /* goto the next token from the input */
> + }
> + }
> + }
> +
> + return canonicalized_value ;
Delete the extra space.
> +}
> +
> Index: subversion/tests/clients/cmdline/trans_tests.py
> ===================================================================
> --- subversion/tests/clients/cmdline/trans_tests.py (revision 13318)
> +++ subversion/tests/clients/cmdline/trans_tests.py (working copy)
[...]
> + if (len(lines) == 23):
> + # LastChangedRevision
> + if(re.match("LastChangedRevision:\$LastChangedRevision\$", lines[2])):
> + print "LastChangedRevision expansion failed for", url_path
> + raise svntest.Failure
> + if not (re.match("lastchangedrevision:\$lastchangedrevision\$", lines[3])):
> + print "lastchangedrevision expansion failed for", url_path
> + raise svntest.Failure
> + #Revision
> + if(re.match("Revision:\$Revision\$", lines[4])):
> + print "Revision expansion failed for", url_path
> + raise svntest.Failure
> + if not (re.match("revision:\$revision\$", lines[5])):
> + print "revision expansion failed for", url_path
> + raise svntest.Failure
> + #Rev
> + if(re.match("Rev:\$Rev\$", lines[6])):
> + print "Rev expansion failed for", url_path
> + raise svntest.Failure
> + if not (re.match("rev:\$rev\$", lines[7])):
> + print "rev expansion failed for", url_path
> + raise svntest.Failure
> + #LastChangedDate
> + if(re.match("LastChangedDate:\$LastChangedDate\$", lines[9])):
> + print "LastChangedDate expansion failed for", url_path
> + raise svntest.Failure
> + if not (re.match("lastchangeddate:\$lastchangeddate\$", lines[10])):
> + print "lastchangeddate expansion failed for", url_path
> + raise svntest.Failure
> + #Date
> + if(re.match("Date:\$Date\$", lines[11])):
> + print "Date expansion failed for", url_path
> + raise svntest.Failure
> + if not (re.match("date:\$date\$", lines[12])):
> + print "date expansion failed for", url_path
> + raise svntest.Failure
> + #LastChangedBy
> + if(re.match("LastChangedBy:\$LastChangedBy\$", lines[14])):
> + print "LastChangedBy expansion failed for", url_path
> + raise svntest.Failure
> + if not (re.match("lastchangedby:\$lastchangedby\$", lines[15])):
> + print "lastchangedby expansion failed for", url_path
> + raise svntest.Failure
> + #Author
> + if(re.match("Author:\$Author\$", lines[16])):
> + print "Author expansion failed for", url_path
> + raise svntest.Failure
> + if not (re.match("author:\$author\$", lines[17])):
> + print "author expansion failed for", url_path
> + raise svntest.Failure
> + #HeadURL
> + if not (re.match("HeadURL:\$HeadURL: (http|file|svn|svn\\+ssh)://", lines[19])):
> + print "HeadURL expansion failed for", url_path
> + raise svntest.Failure
> + if not (re.match("headurl:\$headurl\$", lines[20])):
> + print "headurl expansion failed for", url_path
> + raise svntest.Failure
> + if not (re.match("URL:\$URL: (http|file|svn|svn\\+ssh)://", lines[21])):
> + print "URL expansion failed for", url_path
> + raise svntest.Failure
> + if not (re.match("url:\$url\$", lines[22])):
> + print "url expansion failed for", url_path
> + raise svntest.Failure
Could you please factor out some of this repetition? It's hard to review as it is.
In the "HeadURL" block, every "if" says "if not", whereas in the other blocks
there is a pair of "if <capitalised>" and "if not <lower-case>". That looks
suspicious.
> + else:
> + print "File is in an inconsistent state"
> + raise svntest.Failure
> + fp.close()
> +
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Wed Mar 9 18:26:13 2005