You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Max Bowsher <ma...@ukf.net> on 2005/04/24 14:44:09 UTC
Keywords-as-hash final (I hope) version
Here is what I hope to be the final version of the actual API change portion
of the keywords-as-hash patch.
I would appreciate any reviews, especially regarding whether there are any
missing "const"s in the new APIs.
I have removed the implemetation of a $UUID$ keyword from this patch,
because it is an unrelated feature, and I don't recall any discussion on
whether $UUID$ was a suitable keyword name. Might other things apart from
repositories have UUIDs one day?
Max.
[[[
Revise keywords API - represent keywords as a hash for better extensibility.
Implement internal printf-like format characters for keyword expansion.
A part of issue #890. Note that this only introduces the new APIs - making
the rest of the code make use of them will be a separate commit.
Based on a patch by John Peacock <jp...@rowman.com>.
* subversion/includes/svn_subst.h:
(svn_subst_keywords_t): Deprecated.
(svn_subst_build_keywords2): Interface change; hash instead of struct.
(svn_subst_build_keywords): Deprecated.
(svn_subst_keywords_differ2): Interface change.
A new argument apr_pool_t *pool and use hash instead of struct.
(svn_subst_keywords_differ): Deprecated.
(svn_subst_translate_stream2): Interface change; hash instead of struct.
(svn_subst_translate_stream): Deprecated.
(svn_subst_copy_and_translate3): Interface change; hash instead of
struct.
(svn_subst_copy_and_translate2): Deprecated.
(svn_subst_translate_cstring2): Interface change; hash instead of struct.
(svn_subst_translate_cstring): Deprecated.
* subversion/includes/svn_types.h: (SVN_KEYWORD_REVISION_FORMAT,
(SVN_KEYWORD_DATE_FORMAT, SVN_KEYWORD_AUTHOR_FORMAT)
(SVN_KEYWORD_URL_FORMAT, SVN_KEYWORD_ID_FORMAT):
New defines - format strings for keyword expansion.
* subversion/libsvn_subr/svn_subst.c:
(keyword_printf): New private function;
printf-style formatting of keywords based on format strings.
(kwstruct_to_kwhash): New private function;
convert keywords struct into keywords hash.
(svn_subst_build_keywords): Convert to API compatibility wrapper.
(svn_subst_build_keywords2): Build keywords using keyword_printf().
(translate_keyword): Interface changes. Also, look up the keyword in the
passed in buffer, instead of trying to translate all possibilities.
(svn_subst_keywords_differ): Retain unchanged for API compatibility.
(svn_subst_keywords_differ2): New function;
compare two hashes instead of comparing individual structure elements.
(svn_subst_translate_stream): Convert to API compatibility wrapper.
(svn_subst_translate_stream2): Change interface only.
(svn_subst_translate_cstring): Convert to API compatibility wrapper.
(svn_subst_translate_cstring2): Update function call to new API version.
(svn_subst_copy_and_translate2): Convert to API combatibility wrapper.
(svn_subst_copy_and_translate3): Update function call to new API version.
(svn_subst_translate_string): Update function call to new API version.
(svn_subst_detranslate_string): Update function call to new API version.
]]]
Re: Keywords-as-hash final (I hope) version
Posted by John Peacock <jp...@rowman.com>.
Max Bowsher wrote:
> Here is what I hope to be the final version of the actual API change
> portion of the keywords-as-hash patch.
Looks good to me; it definitely benefitted from a more thorough rewrite.
> I have removed the implemetation of a $UUID$ keyword from this patch,
> because it is an unrelated feature, and I don't recall any discussion on
> whether $UUID$ was a suitable keyword name.
That's completely fair. The UUID case was requested and I implemented it mostly
to show how easy it was to extend this code to support other keywords.
Personally, I think the UUID is an internal repository concept which doesn't
provide much in the way of useful information to carbon-based readers. The
repos-url, on the other hand, would by much more useful to be exposed to humans
(different problem ;-).
> Based on a patch by John Peacock <jp...@rowman.com>.
Truth in advertising - the patch I submitted was closely based on plasma's
(plasmaball@pchome.com.tw) original patch,
http://www.contactor.se/~dast/svn/archive-2003-05/1975.shtml
brought up to date with the core code changes that occurred over time. The only
code I wrote myself was the compatibility layer and the Python tests (which are
not part of this patch).
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
Re: Keywords-as-hash final (I hope) version
Posted by Max Bowsher <ma...@ukf.net>.
Julian Foad wrote:
> Max Bowsher wrote:
>> Here we go again with the keywords-as-hash thing...
>
> Excellent. Congratulations on making the patch so neatly scoped and
> self-contained: it is nice to see it only touching subst.c and the
> corresponding header svn_subst.h. Thus, I'm able to review it.
>
>> OK, fixed all the issues except one...
>>
>>>> + {
>>>> + const char *key;
>>>> + svn_string_t *a_val, *b_val;
>>>> +
>>>> + apr_hash_this (hi, (void *) &key, NULL, (void *) &a_val);
>>>
>>> This is incorrect. You should cast to (void**), and that's not
>>> conformant. Casting pointer-to-pointer-to-sometype to void** doesn't
>>> work. You need two temporary void* variables as in many other places.
>>
>> (void**) works for me - is it a GCC extension?
>> If we really can't use (void**), then I would like to use (void*) - it
>> is, after all accurate, as a void* is a pointer to anything - even
>> another void*.
>
> Without looking deeper, I don't know what is best. I suggest you copy
> what
> we do everywhere else if possible. If we later find a better way, we can
> change everywhere all together.
What we have at the moment is a patchwork quilt of different styles :-(
But, brane and lundblad helped me to understand on #svn-dev, so I will be
fixing this up per lundblad's recommendation above.
>> [[[
>> Revise keywords API - represent keywords as a hash for better
>> extensibility. Implement internal printf-like format characters for
>> keyword expansion. A part of issue #890. Note that this only introduces
>> the new APIs - making
>> the rest of the code make use of them will be a separate commit.
>> Based on a patch by John Peacock <jp...@rowman.com>, which was in turn
>> based on a patch by "plasma" <pl...@pchome.com.tw>.
>> Significant review from Peter Lundblad.
>>
>> * subversion/includes/svn_subst.h:
>> (svn_subst_keywords_t): Deprecated.
>> (svn_subst_build_keywords2): Interface change; hash instead of struct.
>
> All of the new functions should be described with the word "New".
> Otherwise it sounds like the interface to this function was changed,
> whereas really it is the recommended interface to this functionality that
> has changed.
Noted.
>> (svn_subst_build_keywords): Deprecated.
>> (svn_subst_keywords_differ2): Interface change.
>> A new argument apr_pool_t *pool and use hash instead of struct.
>> (svn_subst_keywords_differ): Deprecated.
>> (svn_subst_translate_stream3): Interface change; hash instead of struct.
>> (svn_subst_translate_stream2): Deprecated.
>> (svn_subst_copy_and_translate3): Interface change; hash instead of
>> struct.
>> (svn_subst_copy_and_translate2): Deprecated.
>> (svn_subst_translate_cstring2): Interface change; hash instead of
>> struct.
>> (svn_subst_translate_cstring): Deprecated.
>>
>> * subversion/libsvn_subr/svn_subst.c:
>
> The file is "subst.c" not "svn_subst.c".
Oops!
>> (keyword_printf): New private function;
>> printf-style formatting of keywords based on format strings.
>> (date_prop_to_human): Swallowed by keyword_printf.
>> (kwstruct_to_kwhash): New private function;
>> convert keywords struct into keywords hash.
>> (svn_subst_build_keywords): Convert to API compatibility wrapper.
>> (svn_subst_build_keywords2): Build keywords using keyword_printf().
>> (translate_keyword): Interface changes. Also, look up the keyword in the
>> passed in buffer, instead of trying to translate all possibilities.
>> (svn_subst_keywords_differ): Retain unchanged for API compatibility.
>
> Don't mention that function, as it is untouched.
Yes, but the fact that a deprecated function is _not_ being turned into a
compat wrapper is noteworthy.
>> (svn_subst_keywords_differ2): New function;
>> compare two hashes instead of comparing individual structure elements.
>> (svn_subst_translate_stream2): Convert to API compatibility wrapper.
>> (svn_subst_translate_stream3): Change interface only.
>> (svn_subst_translate_cstring): Convert to API compatibility wrapper.
>> (svn_subst_translate_cstring2): Update function call to new API version.
>> (svn_subst_copy_and_translate2): Convert to API combatibility wrapper.
>> (svn_subst_copy_and_translate3): Update function call to new API
>> version.
>
> Reminder: say "New".
Noted.
>> - kw->id = svn_string_createf (pool, "%s %s %s %s",
>> - base_name,
>> - rev,
>> - human_date ? human_date : "",
>> - author ? author : "");
>> + id_val = keyword_printf ("%b %d %a %r", rev, url, date,
>> author,
>> + pool);
>
> Oops... Here you seem to have changed the order of the keywords, putting
> the revision last where it was previously second.
Yikes! Thanks for catching that!
>> +svn_boolean_t
>> +svn_subst_keywords_differ2 (apr_hash_t *a,
>> + apr_hash_t *b,
>> + svn_boolean_t compare_values,
>> + apr_pool_t *pool)
>> +{
>> + apr_hash_index_t *hi;
>>
>> + if ((a == NULL) && (b == NULL))
>> + return FALSE;
>> +
>> + if (((a == NULL) && (b != NULL)) ||
>> + ((a != NULL) && (b == NULL)) ||
>
> That simplifies to:
>
> if (((a == NULL) || (b == NULL)) ||
>
> since we know at this point that if one of them is null the other isn't.
Done.
>> + (apr_hash_count (a) != apr_hash_count (b)))
>> + return TRUE;
>> +
>> + /* The hashes are both non-NULL, and have the same number of items.
>> + * We must check that every item of A is present in B. */
>> + for (hi = apr_hash_first(pool, a); hi; hi = apr_hash_next(hi))
>> + {
>> + const char *key;
>> + svn_string_t *a_val, *b_val;
>> +
>> + apr_hash_this (hi, (const void **)&key, NULL, (void **)&a_val);
>> + b_val = apr_hash_get (b, key, APR_HASH_KEY_STRING);
>
> Make that more efficient by getting the key length as well, and using it
> instead of APR_HASH_KEY_STRING.
Done.
>> +
>> + if (!b_val || (compare_values && svn_string_compare (a_val,
>> b_val)))
>
> I think that's wrong because svn_string_compare returns true if the
> strings
> are equal, not if they differ. (Grrr... a Boolean function should have a
> name that indicates its sense, like "string_equal", or "keywords_differ".)
Grr indeed!
I just assumed "compare" meant a negative/zero/positive return, in the style
of "strcmp".
Fixed.
OK, now since some pretty non-trivial errors have been found, I'm going to
give the whole thing a careful read through before I post a new version.
Max.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Keywords-as-hash final (I hope) version
Posted by Julian Foad <ju...@btopenworld.com>.
Max Bowsher wrote:
> Here we go again with the keywords-as-hash thing...
Excellent. Congratulations on making the patch so neatly scoped and
self-contained: it is nice to see it only touching subst.c and the
corresponding header svn_subst.h. Thus, I'm able to review it.
> OK, fixed all the issues except one...
>
>>> + {
>>> + const char *key;
>>> + svn_string_t *a_val, *b_val;
>>> +
>>> + apr_hash_this (hi, (void *) &key, NULL, (void *) &a_val);
>>
>> This is incorrect. You should cast to (void**), and that's not
>> conformant. Casting pointer-to-pointer-to-sometype to void** doesn't
>> work. You need two temporary void* variables as in many other places.
>
> (void**) works for me - is it a GCC extension?
> If we really can't use (void**), then I would like to use (void*) - it
> is, after all accurate, as a void* is a pointer to anything - even
> another void*.
Without looking deeper, I don't know what is best. I suggest you copy what we
do everywhere else if possible. If we later find a better way, we can change
everywhere all together.
> [[[
> Revise keywords API - represent keywords as a hash for better extensibility.
> Implement internal printf-like format characters for keyword expansion.
> A part of issue #890. Note that this only introduces the new APIs - making
> the rest of the code make use of them will be a separate commit.
> Based on a patch by John Peacock <jp...@rowman.com>, which was in turn
> based on a patch by "plasma" <pl...@pchome.com.tw>.
> Significant review from Peter Lundblad.
>
> * subversion/includes/svn_subst.h:
> (svn_subst_keywords_t): Deprecated.
> (svn_subst_build_keywords2): Interface change; hash instead of struct.
All of the new functions should be described with the word "New". Otherwise it
sounds like the interface to this function was changed, whereas really it is
the recommended interface to this functionality that has changed.
> (svn_subst_build_keywords): Deprecated.
> (svn_subst_keywords_differ2): Interface change.
> A new argument apr_pool_t *pool and use hash instead of struct.
> (svn_subst_keywords_differ): Deprecated.
> (svn_subst_translate_stream3): Interface change; hash instead of struct.
> (svn_subst_translate_stream2): Deprecated.
> (svn_subst_copy_and_translate3): Interface change; hash instead of struct.
> (svn_subst_copy_and_translate2): Deprecated.
> (svn_subst_translate_cstring2): Interface change; hash instead of struct.
> (svn_subst_translate_cstring): Deprecated.
>
> * subversion/libsvn_subr/svn_subst.c:
The file is "subst.c" not "svn_subst.c".
> (keyword_printf): New private function;
> printf-style formatting of keywords based on format strings.
> (date_prop_to_human): Swallowed by keyword_printf.
> (kwstruct_to_kwhash): New private function;
> convert keywords struct into keywords hash.
> (svn_subst_build_keywords): Convert to API compatibility wrapper.
> (svn_subst_build_keywords2): Build keywords using keyword_printf().
> (translate_keyword): Interface changes. Also, look up the keyword in the
> passed in buffer, instead of trying to translate all possibilities.
> (svn_subst_keywords_differ): Retain unchanged for API compatibility.
Don't mention that function, as it is untouched.
> (svn_subst_keywords_differ2): New function;
> compare two hashes instead of comparing individual structure elements.
> (svn_subst_translate_stream2): Convert to API compatibility wrapper.
> (svn_subst_translate_stream3): Change interface only.
> (svn_subst_translate_cstring): Convert to API compatibility wrapper.
> (svn_subst_translate_cstring2): Update function call to new API version.
> (svn_subst_copy_and_translate2): Convert to API combatibility wrapper.
> (svn_subst_copy_and_translate3): Update function call to new API version.
Reminder: say "New".
> (svn_subst_translate_string): Update function call to new API version.
> (svn_subst_detranslate_string): Update function call to new API version.
> ]]]
> Index: subversion/libsvn_subr/subst.c
> ===================================================================
> --- subversion/libsvn_subr/subst.c (revision 14802)
> +++ subversion/libsvn_subr/subst.c (working copy)
[...]
> + * %a author of this revision
> + * %b basename of the URL of this file
> + * %d short format of date of this revision
> + * %D long format of date of this revision
> + * %r number of this revision
> + * %u URL of this file
(If this were a public API, I would also expect it to accept "%%" for inserting
a literal "%" character, and document what hapens to invalid codes, but it
isn't so that's OK.)
> + *
> + * All memory is allocated out of @a pool.
> + */
> +static svn_string_t *
> +keyword_printf (const char *fmt,
> + const char *rev,
> + const char *url,
> + apr_time_t date,
> + const char *author,
> + apr_pool_t *pool)
> {
> + svn_stringbuf_t *value = svn_stringbuf_ncreate ("", 0, pool);
> + const char *cur;
> + int n;
A minor style nit: since "cur" and "n" are used locally in the body of the loop
(and not intended to preserve a value from one iteration to the next), it would
be clearer if they were declared locally in the body of the loop. But that
only becomes an actual problem when functions grow much bigger than this, so
it's not really important here.
> +
> + for (;;)
> {
> + cur = fmt;
>
[...]
> - kw->id = svn_string_createf (pool, "%s %s %s %s",
> - base_name,
> - rev,
> - human_date ? human_date : "",
> - author ? author : "");
> + id_val = keyword_printf ("%b %d %a %r", rev, url, date, author,
> + pool);
Oops... Here you seem to have changed the order of the keywords, putting the
revision last where it was previously second.
> +svn_boolean_t
> +svn_subst_keywords_differ2 (apr_hash_t *a,
> + apr_hash_t *b,
> + svn_boolean_t compare_values,
> + apr_pool_t *pool)
> +{
> + apr_hash_index_t *hi;
>
> + if ((a == NULL) && (b == NULL))
> + return FALSE;
> +
> + if (((a == NULL) && (b != NULL)) ||
> + ((a != NULL) && (b == NULL)) ||
That simplifies to:
if (((a == NULL) || (b == NULL)) ||
since we know at this point that if one of them is null the other isn't.
> + (apr_hash_count (a) != apr_hash_count (b)))
> + return TRUE;
> +
> + /* The hashes are both non-NULL, and have the same number of items.
> + * We must check that every item of A is present in B. */
> + for (hi = apr_hash_first(pool, a); hi; hi = apr_hash_next(hi))
> + {
> + const char *key;
> + svn_string_t *a_val, *b_val;
> +
> + apr_hash_this (hi, (const void **)&key, NULL, (void **)&a_val);
> + b_val = apr_hash_get (b, key, APR_HASH_KEY_STRING);
Make that more efficient by getting the key length as well, and using it
instead of APR_HASH_KEY_STRING.
> +
> + if (!b_val || (compare_values && svn_string_compare (a_val, b_val)))
I think that's wrong because svn_string_compare returns true if the strings are
equal, not if they differ. (Grrr... a Boolean function should have a name that
indicates its sense, like "string_equal", or "keywords_differ".)
> + return TRUE;
> + }
> +
> + return FALSE;
> +}
> +
Have fun!
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Keywords-as-hash final (I hope) version
Posted by Max Bowsher <ma...@ukf.net>.
Here we go again with the keywords-as-hash thing...
Peter N. Lundblad wrote:
> On Sun, 24 Apr 2005, Max Bowsher wrote:
>
>> Here is what I hope to be the final version of the actual API change
>> portion of the keywords-as-hash patch.
>>
>> I would appreciate any reviews, especially regarding whether there are
>> any missing "const"s in the new APIs.
>>
> Didn't find anything major, except for const apr_hash_t* in the APIs (see
> below).
OK, fixed all the issues except one...
>> + {
>> + const char *key;
>> + svn_string_t *a_val, *b_val;
>> +
>> + apr_hash_this (hi, (void *) &key, NULL, (void *) &a_val);
>
> This is incorrect. You should cast to (void**), and that's not
> conformant. Casting pointer-to-pointer-to-sometype to void** doesn't
> work. You need two temporary void* variables as in many other places.
(void**) works for me - is it a GCC extension?
If we really can't use (void**), then I would like to use (void*) - it is,
after all accurate, as a void* is a pointer to anything - even another
void*.
I've also made additional changes - eliminated the changes to svn_types.h
entirely, and swallowed the implementation of date_prop_to_human into
keyword_prinf.
Max.
[[[
Revise keywords API - represent keywords as a hash for better extensibility.
Implement internal printf-like format characters for keyword expansion.
A part of issue #890. Note that this only introduces the new APIs - making
the rest of the code make use of them will be a separate commit.
Based on a patch by John Peacock <jp...@rowman.com>, which was in turn
based on a patch by "plasma" <pl...@pchome.com.tw>.
Significant review from Peter Lundblad.
* subversion/includes/svn_subst.h:
(svn_subst_keywords_t): Deprecated.
(svn_subst_build_keywords2): Interface change; hash instead of struct.
(svn_subst_build_keywords): Deprecated.
(svn_subst_keywords_differ2): Interface change.
A new argument apr_pool_t *pool and use hash instead of struct.
(svn_subst_keywords_differ): Deprecated.
(svn_subst_translate_stream3): Interface change; hash instead of struct.
(svn_subst_translate_stream2): Deprecated.
(svn_subst_copy_and_translate3): Interface change; hash instead of struct.
(svn_subst_copy_and_translate2): Deprecated.
(svn_subst_translate_cstring2): Interface change; hash instead of struct.
(svn_subst_translate_cstring): Deprecated.
* subversion/libsvn_subr/svn_subst.c:
(keyword_printf): New private function;
printf-style formatting of keywords based on format strings.
(date_prop_to_human): Swallowed by keyword_printf.
(kwstruct_to_kwhash): New private function;
convert keywords struct into keywords hash.
(svn_subst_build_keywords): Convert to API compatibility wrapper.
(svn_subst_build_keywords2): Build keywords using keyword_printf().
(translate_keyword): Interface changes. Also, look up the keyword in the
passed in buffer, instead of trying to translate all possibilities.
(svn_subst_keywords_differ): Retain unchanged for API compatibility.
(svn_subst_keywords_differ2): New function;
compare two hashes instead of comparing individual structure elements.
(svn_subst_translate_stream2): Convert to API compatibility wrapper.
(svn_subst_translate_stream3): Change interface only.
(svn_subst_translate_cstring): Convert to API compatibility wrapper.
(svn_subst_translate_cstring2): Update function call to new API version.
(svn_subst_copy_and_translate2): Convert to API combatibility wrapper.
(svn_subst_copy_and_translate3): Update function call to new API version.
(svn_subst_translate_string): Update function call to new API version.
(svn_subst_detranslate_string): Update function call to new API version.
]]]
Re: Keywords-as-hash final (I hope) version
Posted by Max Bowsher <ma...@ukf.net>.
Julian Foad wrote:
> Ping! Max, did you have time to address Peter's comments?
I've addressed the majority of them, I need to finish the last few points
I've got saved from his review, and post a new patch.
I have updated the patch w.r.t. the late-breaking revving of
svn_subst_translate_stream, too.
I'll try to get a new patch out soon, but university is keeping me quite
busy at the moment.
Max.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Keywords-as-hash final (I hope) version
Posted by Julian Foad <ju...@btopenworld.com>.
Ping! Max, did you have time to address Peter's comments?
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Keywords-as-hash final (I hope) version
Posted by "Peter N. Lundblad" <pe...@famlundblad.se>.
On Sun, 24 Apr 2005, Max Bowsher wrote:
> Here is what I hope to be the final version of the actual API change portion
> of the keywords-as-hash patch.
>
> I would appreciate any reviews, especially regarding whether there are any
> missing "const"s in the new APIs.
>
Didn't find anything major, except for const apr_hash_t* in the APIs (see
below).
> Index: subversion/include/svn_subst.h
> ===================================================================
> --- subversion/include/svn_subst.h (revision 14425)
> +++ subversion/include/svn_subst.h (working copy)
> @@ -75,7 +75,10 @@
> const char *value);
>
>
> -/** Values used in keyword expansion. */
> +/** Values used in keyword expansion.
> + *
> + * @deprecated Provided for backward compatibility with the 1.1 API.
Shouldn't this be 1.2?
> + */
> typedef struct svn_subst_keywords_t
> {
> /** @{ */
> @@ -90,16 +93,36 @@
> } svn_subst_keywords_t;
>
>
> -/** Fill in an <tt>svn_subst_keywords_t *</tt> @a kw with the appropriate
> - * contents given a @a keywords_string (the contents of the svn:keywords
> +/**
> + * Set @a *kw to a new keywords hash filled with the appropriate contents
> + * given an @a keywords_string (the contents of the svn:keywords
> * property for the file in question), the revision @a rev, the @a url,
> - * the @a date the file was committed on, and the @a author of the last
> - * commit. Any of these can be @c NULL to indicate that the information is
> - * not present, or @c 0 for @a date.
> + * the @a date the file was committed on, the @a author of the last
> + * commit. Any of these can be @c NULL
> + * to indicate that the information is not present, or @c 0 for @a date.
> + *
> + * Hash keys are of type <tt>const char *</tt>.
> + * Hash values are of type <tt>svn_string_t *</tt>.
> *
> * All memory is allocated out of @a pool.
> + *
> + * @since New in 1.3.
This is inconsistent with where we put @since everywhere else, but it seems
you're following the new recommendation to not put it at the start. Fine!
> @@ -111,6 +134,9 @@
>
> /** Return @c TRUE if @a a and @a b do not hold the same keywords.
> *
> + * @a a and @a b are hashes of the form produced by
> + * svn_subst_build_keywords2().
> + *
> * If @a compare_values is @c TRUE, "same" means that the @a a and @a b
> * contain exactly the same set of keywords, and the values of corresponding
> * keywords match as well. Else if @a compare_values is @c FALSE, then
> @@ -121,6 +147,17 @@
> * equivalent to holding no keywords.
> */
Missing @since.
> svn_boolean_t
> +svn_subst_keywords_differ2 (const apr_hash_t *a,
> + const apr_hash_t *b,
> + svn_boolean_t compare_values,
> + apr_pool_t *pool);
Making hashes const is problematic, as we'll see below.
> +
> @@ -129,6 +166,8 @@
> /** Copy and translate the data in stream @a src into stream @a dst. It is
> * assumed that @a src is a readable stream and @a dst is a writable stream.
> *
> + * @since New in 1.3.
> + *
> * If @a eol_str is non-@c NULL, replace whatever bytestring @a src uses to
> * denote line endings with @a eol_str in the output. If @a src has an
> * inconsistent line ending style, then: if @a repair is @c FALSE, return
Where should we put @since in the future? Last in the docstring, after
the first paragraph, or something else? I'm not sure if you applied a
consistent rule or it just slipped in here:-)
> @@ -151,7 +190,8 @@
> * @a keywords must be non-@c NULL.
> *
> * Recommendation: if @a expand is false, then you don't care about the
> - * keyword values, so pass empty strings as non-null signifiers.
> + * keyword values, so use empty strings as non-null signifiers when you
> + * build the keywords hash.
> *
> * Notes:
> *
> @@ -159,6 +199,19 @@
> * convenient way to get @a eol_str and @a keywords if in libsvn_wc.
> */
Missing @since.
> svn_error_t *
> +svn_subst_translate_stream2 (svn_stream_t *src,
> + svn_stream_t *dst,
> + const char *eol_str,
> + svn_boolean_t repair,
> + apr_hash_t *keywords,
Here the hash isn't const, but I think that's correc.t
> + svn_boolean_t expand);
> +
> +/** Similar to @c svn_subst_translate_stream2 except relies upon a
The function name should have () at the end and no @c.
> + * @c svn_subst_keywords_t struct instead of a hash for the keywords.
> + *
> + * @deprecated Provided for backward compatibility with the 1.2 API.
> + */
> +svn_error_t *
> svn_subst_translate_stream (svn_stream_t *src,
> svn_stream_t *dst,
> const char *eol_str,
> @@ -168,7 +221,7 @@
>
>
> /**
> - * @since New in 1.1.
> + * @since New in 1.3.
> *
Where should we place @since? :-)
> * Convenience routine: a variant of svn_subst_translate_stream()
"varinat of svn_subst_translate_stream2()" ?
> * which operates on files. (See previous docstring for details.) In
Phrases like "previous docstring" are no good in doxygen comments.
> @@ -186,6 +239,23 @@
> * copy.
> */
> svn_error_t *
> +svn_subst_copy_and_translate3 (const char *src,
> + const char *dst,
> + const char *eol_str,
> + svn_boolean_t repair,
> + apr_hash_t *keywords,
Same re constness here.
> + svn_boolean_t expand,
> + svn_boolean_t special,
> + apr_pool_t *pool);
> +
> +/**
> + * @deprecated Provided for backward compatibility with the 1.2 API.
> + * @since New in 1.1.
these should be further down.
> + *
> + * Similar to svn_subst_copy_and_translate3() except that @a keywords is a
> + * @c svn_subst_keywords_t struct instead of a keywords hash.
> + */
> +svn_error_t *
> svn_subst_copy_and_translate2 (const char *src,
> const char *dst,
> const char *eol_str,
> @@ -222,6 +294,21 @@
> * copy.
> */
> svn_error_t *
> +svn_subst_translate_cstring2 (const char *src,
> + const char **dst,
> + const char *eol_str,
> + svn_boolean_t repair,
> + apr_hash_t *keywords,
> + svn_boolean_t expand,
> + apr_pool_t *pool);
> +
> +/**
> + * @deprecated Provided for backward compatibility with the 1.2 API.
> + *
Move at the end.
> + * Similar to svn_subst_translate_cstring2() except that @a keywords is a
> + * @c svn_subst_keywords_t struct instead of a keywords hash.
> + */
> +svn_error_t *
> svn_subst_translate_cstring (const char *src,
> const char **dst,
> const char *eol_str,
> Index: subversion/include/svn_types.h
> ===================================================================
> --- subversion/include/svn_types.h (revision 14425)
> +++ subversion/include/svn_types.h (working copy)
> @@ -261,27 +261,42 @@
> /** Medium version of LastChangedRevision, matching the one CVS uses */
> #define SVN_KEYWORD_REVISION_MEDIUM "Revision"
>
> +/** Format string for Revision */
> +#define SVN_KEYWORD_REVISION_FORMAT "%r"
Should have @since clause. Same for the other constants below.
> Index: subversion/libsvn_subr/subst.c
> ===================================================================
> --- subversion/libsvn_subr/subst.c (revision 14425)
> +++ subversion/libsvn_subr/subst.c (working copy)
> @@ -118,6 +118,166 @@
> return SVN_NO_ERROR;
> }
>
> +
> +/* Helper function for svn_subst_build_keywords */
> +
> +/* Given a printf-like format string, return a string with proper
> + * information filled in.
> + *
> + * Important API note: This function is the core of the implementation of
> + * svn_subst_build_keywords (all versions), and as such must implement the
> + * tolerance of NULL and zero inputs that that function's documention
> + * stipulates.
> + *
> + * The codes of format:
> + *
...
Is it necessary to document the format specifiers here? Couldn't you
just point to the #defines in svn_types.h?
> +static svn_string_t *
> +keyword_printf (const char *fmt,
> + const char *rev,
> + const char *url,
> + apr_time_t date,
> + const char *author,
> + apr_pool_t *pool)
> +{
> + svn_stringbuf_t *value = svn_stringbuf_ncreate ("", 0, pool);
> + const char *cur;
> + int n;
> +
> + for (;;)
> + {
> + cur = fmt;
> +
> + while (*cur != '\0' && *cur != '%')
> + cur++;
> +
> + if ((n = cur - fmt) > 0) /* Do we have an as-is string? */
> + svn_stringbuf_appendbytes (value, fmt, n);
> +
> + if (*cur == '\0')
> + break;
> +
> + switch (cur[1])
> + {
> + case 'a': /* author of this revision */
> + if (author)
> + svn_stringbuf_appendcstr (value, author);
> + break;
> + case 'b': /* basename of this file */
> + if (url)
> + {
> + const char *base_name = NULL;
> + base_name = svn_path_basename (url, pool);
Initialization direclty followed by assignment.
> +
> + svn_stringbuf_appendcstr (value, base_name);
> + }
> + break;
> + case 'd': /* short format of date of this revision */
> + if (date)
> + {
> + const char *human_date = NULL;
What the point of the initialization here?
> +
> + if (!date_prop_to_human (&human_date, FALSE, date, pool))
> + svn_stringbuf_appendcstr (value, human_date);
date_prop_to_human return an snv_error_t * (but currently it always
returns SVN_NO_ERROR). Potential future leak.
> + }
> + break;
> + case 'D': /* long format of date of this revision */
> + if (date)
> + {
> + const char *human_date = NULL;
Redundant init.
> +
> + if (!date_prop_to_human (&human_date, TRUE, date, pool))
Leak.
> + svn_stringbuf_appendcstr (value, human_date);
> + }
> + break;
> + case 'r': /* number of this revision */
> + if (rev)
> + svn_stringbuf_appendcstr (value, rev);
> + break;
> + case 'u': /* URL of this file */
> + if (url)
> + svn_stringbuf_appendcstr (value, url);
> + break;
> + case '\0': /* '%' as the last character of the string. */
> + svn_stringbuf_appendbytes (value, cur, 1);
> + /* Now go back one character, since this was just a one character
> + * sequence, whereas all others are two characters, and we do not
> + * want to skip the null terminator entirely and carry on
> + * formatting random memory contents. */
> + cur--;
> + break;
> + default: /* Unrecognized code, just print it literally. */
> + svn_stringbuf_appendbytes (value, cur, 2);
> + break;
> + }
> +
> + /* Format code is processed - skip it, and get ready for next chunk. */
> + fmt = cur + 2;
> + }
> +
> + return svn_string_create_from_buf (value, pool);
> +}
> +
> +/* Convert an old-style svn_subst_keywords_t struct
> + * into a new-style keywords hash. */
Maybe note that the values are not dup'd into POOL, so this is not a
deep copy and that KWSTRUCT may be NULL.
> +static apr_hash_t *
> +kwstruct_to_kwhash (const svn_subst_keywords_t *kwstruct,
> + apr_pool_t *pool)
> +{
> + apr_hash_t *kwhash;
> +
> + if (kwstruct == NULL)
> + return NULL;
> +
> + kwhash = apr_hash_make(pool);
> +
> + if (kwstruct->revision)
> + {
> + apr_hash_set (kwhash, SVN_KEYWORD_REVISION_LONG,
> + APR_HASH_KEY_STRING, kwstruct->revision);
...
> svn_error_t *
> svn_subst_build_keywords (svn_subst_keywords_t *kw,
> const char *keywords_val,
> @@ -127,8 +287,53 @@
> const char *author,
> apr_pool_t *pool)
> {
> + apr_hash_t *kwhash;
> + const svn_string_t *val;
> + svn_error_t * err = svn_subst_build_keywords2(&kwhash, keywords_val, rev,
> + url, date, author,
> + pool);
Missing space before left paren.
> +
> + /* The behaviour of pre-1.2 svn_subst_build_keywords, which we are
> + * replicating here, is to write to a slot in the svn_subst_keywords_t
> + * only if the relevant keyword was present in keywords_val, otherwise
> + * leaving that slot untouched. */
> +
> + val = apr_hash_get(kwhash, SVN_KEYWORD_REVISION_LONG, APR_HASH_KEY_STRING);
> + if (val)
> + kw->revision = val;
> +
Why is this necessary if the above returned an error? Worse, is kwhash
guaranteed to be valid if an error was returned?
...
> + return err;
> +}
> +
> +svn_error_t *
> +svn_subst_build_keywords2 (apr_hash_t **kw,
> + const char *keywords_val,
> + const char *rev,
> + const char *url,
> + apr_time_t date,
> + const char *author,
> + apr_pool_t *pool)
> +{
> apr_array_header_t *keyword_tokens;
> int i;
> + apr_hash_t *kwhash = apr_hash_make (pool);
> + *kw = kwhash;
To answer a question from above, yes it is guaranteed to be valid
*currently*. But that could change and the deprecated code
forgotten...
Why the extra temporary here? What's wrong with using *kw below
instead of kwhash?
>
> keyword_tokens = svn_cstring_split (keywords_val, " \t\v\n\b\r\f",
> TRUE /* chop */, pool);
> @@ -377,8 +603,12 @@
> translate_keyword (char *buf,
> apr_size_t *len,
> svn_boolean_t expand,
> - const svn_subst_keywords_t *keywords)
> + const apr_hash_t *keywords)
> {
> + const svn_string_t *value;
> + char keyword_name[SVN_KEYWORD_MAX_LEN + 1];
> + int i;
> +
> /* Make sure we gotz good stuffs. */
> assert (*len <= SVN_KEYWORD_MAX_LEN);
> assert ((buf[0] == '$') && (buf[*len - 1] == '$'));
> @@ -387,87 +617,21 @@
> if (! keywords)
> return FALSE;
>
> - /* Revision */
> - if (keywords->revision)
> - {
> - if (translate_keyword_subst (buf, len,
> - SVN_KEYWORD_REVISION_LONG,
> - (sizeof (SVN_KEYWORD_REVISION_LONG)) - 1,
> - expand ? keywords->revision : NULL))
> - return TRUE;
> + /* Extract the name of the keyword */
> + for (i = 0; i < *len - 2 && buf[i + 1] != ':'; i++)
> + keyword_name[i] = *(buf + i + 1);
> + keyword_name[i] = 0;
>
> - if (translate_keyword_subst (buf, len,
> - SVN_KEYWORD_REVISION_MEDIUM,
> - (sizeof (SVN_KEYWORD_REVISION_MEDIUM)) - 1,
> - expand ? keywords->revision : NULL))
> - return TRUE;
> + value = apr_hash_get ((apr_hash_t *)keywords, keyword_name,
> + APR_HASH_KEY_STRING);
Casting away const makes me nervous!
>
> - if (translate_keyword_subst (buf, len,
> - SVN_KEYWORD_REVISION_SHORT,
> - (sizeof (SVN_KEYWORD_REVISION_SHORT)) - 1,
> - expand ? keywords->revision : NULL))
> - return TRUE;
> - }
> -
> - /* Date */
> - if (keywords->date)
> + if (value)
> {
> - if (translate_keyword_subst (buf, len,
> - SVN_KEYWORD_DATE_LONG,
> - (sizeof (SVN_KEYWORD_DATE_LONG)) - 1,
> - expand ? keywords->date : NULL))
> - return TRUE;
> -
> - if (translate_keyword_subst (buf, len,
> - SVN_KEYWORD_DATE_SHORT,
> - (sizeof (SVN_KEYWORD_DATE_SHORT)) - 1,
> - expand ? keywords->date : NULL))
> - return TRUE;
> + return translate_keyword_subst (buf, len,
> + keyword_name, strlen(keyword_name),
Missing gspace before left paren.
> + expand ? value : NULL);
> }
>
...
> @@ -584,7 +748,43 @@
> return FALSE;
> }
>
> +svn_boolean_t
> +svn_subst_keywords_differ2 (const apr_hash_t *a,
> + const apr_hash_t *b,
> + svn_boolean_t compare_values,
> + apr_pool_t *pool)
> +{
> + apr_hash_index_t *hi;
>
> + if ((a == NULL) && (b == NULL))
> + return FALSE;
> +
> + if (((a == NULL) && (b != NULL)) ||
> + ((a != NULL) && (b == NULL)) ||
> + /* Unequal number of contents */
> + (apr_hash_count((apr_hash_t *) a) !=
> + apr_hash_count((apr_hash_t *) b)))
Ouch! Cast away constness again!
> + {
> + return TRUE;
> + }
Very minor, but why use three lines where one would suffice?
> +
> + /* The hashes have the same number of items.
> + * We must check that every item of A is present in B. */
> + for (hi = apr_hash_first(pool, (apr_hash_t *) a); hi; hi = apr_hash_next(hi))
Guess what! Don't like these const_casts (as the C++ programmer in may
screams).
I think we need to live with the fact that APR defines its hashes API
without the hashtable being const. I don't like it, but those casts
are error prone and confusing. And if others think this should stay,
please create two temporaries at the top of the function and isolate
the casts there.
> + {
> + const char *key;
> + svn_string_t *a_val, *b_val;
> +
> + apr_hash_this (hi, (void *) &key, NULL, (void *) &a_val);
This is incorrect. You should cast to (void**), and that's not
conformant. Casting pointer-to-pointer-to-sometype to void** doesn't
work. You need two temporary void* variables as in many other places.
> + b_val = apr_hash_get ((apr_hash_t *) b, key, APR_HASH_KEY_STRING);
> +
Get rid of this cast.
> + if (!b_val || (compare_values && svn_string_compare (a_val, b_val)))
> + return TRUE;
> + }
> +
> + return FALSE;
> +}
...
> @@ -711,6 +931,21 @@
> svn_boolean_t expand,
> apr_pool_t *pool)
> {
> + apr_hash_t *kh = kwstruct_to_kwhash (keywords, pool);
> +
> + return svn_subst_translate_cstring2 (src, dst, eol_str, repair,
> + kh, expand, pool);
> +}
Strange indentation.
Best,
//Peter
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org