I have to confess it was only seeing Philip's review that motivated me to have
a go.
John Peacock wrote:
> Implement printf-like format characters for keyword expansion. Change all
> other libraries to use new keyword hash functions from libsvn_subst
End sentences with a period? (So I'm being nit-picky :-)
> See issue #890 for details.
>
> * includes/svn_subst.h:
> deprecate svn_subst_keywords_t
"(svn_subst_keywords_t): Deprecated."
I think the idea is to parenthesise the name (symbol) of the top-level item
that was changed, whether that item was a function, a variable, a data type, or
anything else.
> (svn_subst_build_keywords2): Interface change. A new argument
> const char *uuid.
> (svn_subst_build_keywords): API compatibility wrapper for previous
> (svn_subst_keywords_differ2): Interface change. A new argument
> apr_pool_t *pool and use hash instead of struct.
> (svn_subst_keywords_differ): API compatibility wrapper for previous
> (svn_subst_copy_and_translate3): Interface change; hash instead of struct
> (svn_subst_copy_and_translate2): compatibility wrapper for translate3
> (svn_subst_copy_and_translate): compatibility wrapper for translate3
I don't think you changed this prototype (svn_subst_copy_and_translate). (It's
still documented as a wrapper for ...2, which is fine.)
> (svn_subst_translate_cstring2): Interface change; hash instead of struct
> (svn_subst_translate_cstring): API compatibility wrapper for previous
> (svn_subst_translate_stream2): Interface change; hash instead of struct
> (svn_subst_translate_stream): API compatibility wrapper for previous
>
> * includes/svn_types.h:
> Define keyword format string for Revision, Date, Author, URL, UUID, ID
"(SVN_KEYWORD_REVISION_FORMAT, SVN_KEYWORD_...): New format strings."
>
> * libsvn_subr/svn_subst.c:
> (keyword_printf): New private function; printf-style formatting of
> keywords based on format strings
> (keywords_to_keyhash): New private function; convert keywords struct into
> keywords hash
> (keyhash_to_keywords): New private function; convert keywords hash into
> keywords struct
> (svn_subst_build_keywords): API combatibility wrapper
> (svn_subst_build_keywords2): Build keywords using keyword_printf().
> (translate_keyword): Interface changes. It now looks up keyword
> passed in buffer, instead of predefined constant string.
> (svn_subst_translate_stream): API combatibility wrapper
> (svn_subst_translate_stream2): Loop over all elements of keyword hash
> instead of structure elements.
> (svn_subst_keywords_differ): API combatibility wrapper
> (svn_subst_keywords_differ2): Compare two hashes instead of comparing
> individual structure elements.
> (svn_subst_copy_and_translate): API combatibility wrapper
> (svn_subst_copy_and_translate2): API combatibility wrapper
> (svn_subst_copy_and_translate3): uses keywords hash instead of struct
>
> * subversion/libsvn_wc/merge.c
> (svn_wc_merge): use key hashes, new svn_wc__get_keywords() parameters,
> and svn_subst_copy_and_translate2()
Uses svn_subst_copy_and_translate3, not ...2?
>
> * subversion/libsvn_wc/translate.h
> (svn_wc__get_keywords): change signature to use keyword hash
>
> * subversion/libsvn_wc/props.c
> (validate_eol_prop_against_file):
> (svn_wc_prop_set): use svn_subst_translate_stream2(), keyword hashes,
> and new svn_wc__get_keywords()
>
> * subversion/libsvn_wc/adm_crawler.c
> (restore_file): use keyword hashes, svn_subst_copy_and_translate3(),
> and new svn_wc__get_keywords()
>
> * subversion/libsvn_wc/log.c
> (file_xfer_under_path):
This looks like a bit like the description is missing. You could write
"(file_xfer_under_path, install_committed_file): ..." to share a description.
> (install_committed_file): use keyword hashes, new svn_wc__get_keywords(),
> and svn_subst_copy_and_translate3()
>
> * subversion/libsvn_wc/adm_ops.c
> (revert_admin_things): use keyword hashes, new svn_wc__get_keywords(),
> and svn_subst_copy_and_translate3()
>
> * subversion/libsvn_wc/translate.c
> (svn_wc_translated_file): use keyword hashes, new svn_wc__get_keywords(),
> and svn_subst_copy_and_translate3()
> (svn_wc__get_keywords): change signature to use keyword hashes and
> svn_subst_build_keywords2()
>
> * subversion/libsvn_client/export.c
> (copy_one_versioned_file): use keyword hashes, new svn_wc__get_keywords(),
> and svn_subst_copy_and_translate3()
> (copy_versioned_files): retrieve uuid for call to copy_one_versioned_file
> (struct edit_baton): add uuid
> (struct file_baton): add uuid
> (add_file): copy uuid from edit baton to file baton during initialization
> (close_file): initialize keyword hash, populate it with
> svn_subst_build_keywords2(), and call svn_subst_copy_and_translate3()
> (svn_client_export3): retrieve uuid and store in edit baton
>
> * subversion/libsvn_client/cat.c
> (svn_client_cat2): use keyword hashes, svn_subst_build_keywords2(),
> and svn_subst_translate_stream2()
>
> * subversion/libsvn_client/commit.c
> (send_file_contents): use keyword hashes, svn_subst_build_keywords2(),
> and svn_subst_translate_stream2()
>
> * subversion/clients/cmdline/util.c
> (svn_cl__edit_externally): use new svn_subst_translate_cstring2()
>
> * subversion/tests/libsvn_wc/translate-test.c
> (substitute_and_verify): use keyword hashes and add uuid parameter
> (expand_uuid and unexpand_uuid): new tests for UUID keyword
> (multiple test cases): reformat parameters and add uuid
Inconsistent indentation in the log message.
I haven't checked the log message throughly - just noticed a few things and
skimmed through the rest.
> ------------------------------------------------------------------------
> === subversion/include/svn_subst.h
> ==================================================================
> --- subversion/include/svn_subst.h (/mirror/trunk) (revision 12276)
> +++ subversion/include/svn_subst.h (/local/keywords) (revision 12276)
> @@ -75,7 +75,9 @@
> const char *value);
>
>
> -/** Values used in keyword expansion. */
> +/* struct svn_subst_keywords_t is @deprecated
> +* Provided for backward compatibility with the 1.1 API.
> +*/
Doxygen comments need to start with "/**".
It doesn't seem like a good idea to delete the description of the item. Even
though it is deprecated, people might have valid reasons for needing to refer
to it for a while.
No need to repeat the name of the item in its comment. I think we would
usually write it like this:
/** Values used in keyword expansion.
*
* @deprecated Provided for backward compatibility with the 1.1 API.
*/
> typedef struct svn_subst_keywords_t
> {
> const svn_string_t *revision;
> @@ -86,8 +88,11 @@
> } svn_subst_keywords_t;
>
>
> -/** Fill in an <tt>svn_subst_keywords_t *</tt> @a kw with the appropriate
> - * contents given an @a keywords_string (the contents of the svn:keywords
> +/**
> + * @since New in 1.2.
> + *
> + * Fill in an <tt>apr_hash_t*</tt> @a kw 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
> @@ -96,6 +101,21 @@
> * All memory is allocated out of @a pool.
> */
> svn_error_t *
> +svn_subst_build_keywords2 (apr_hash_t *kw,
> + const char *keywords_string,
> + const char *rev,
> + const char *url,
> + apr_time_t date,
> + const char *author,
> + const char *uuid,
> + apr_pool_t *pool);
> +
> +/** Similar to svn_subst_build_keywords2()
But you ought to state briefly what's different about it.
> + *
> + * @deprecated Provided for backward compatibility with the 1.1 API.
> + */
> +
> +svn_error_t *
> svn_subst_build_keywords (svn_subst_keywords_t *kw,
> const char *keywords_string,
> const char *rev,
> @@ -104,7 +124,6 @@
> const char *author,
> apr_pool_t *pool);
>
> -
> /** Return @c TRUE if @a a and @a b do not hold the same keywords.
> *
> * If @a compare_values is @c TRUE, "same" means that the @a a and @a b
> @@ -117,6 +136,18 @@
> * equivalent to holding no keywords.
As Philip mentioned for other functions, say something like "@a a and @a b
(hashes mapping (const char *) keyword names to (const char *) keyword
values)", or whatever the correct types and descriptions are.
> */
> 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);
> +
> +/** Similar to svn_subst_keywords_differ2(); provides a pool for performing
> + * the hash comparisons.
In terms of the interface and the users of this function
(svn_subst_keywords_differ), the difference regarding pools is that the user
need not provide a pool. The fact that the function creates a pool internally
is an implementation detail, and such details are not normally relevant to the
interface description. However, as Philip pointed out, the creation of a new
top-level pool is a significant thing which might be worth mentioning here.
How about:
/** Similar to svn_subst_keywords_differ2(), but keywords are passed as @c
* svn_subst_keywords_t structures, and a pool is not passed.
*
* Note: this function created and uses a temporary top-level pool.
Actually, it's not very similar to svn_subst_keywords_differ2() at all in terms
of its interface. It could just keep its old comment.
> + *
> + * @deprecated Provided for backward compatibility with the 1.1 API.
> + */
> +
> +svn_boolean_t
> svn_subst_keywords_differ (const svn_subst_keywords_t *a,
> const svn_subst_keywords_t *b,
> svn_boolean_t compare_values);
> @@ -155,6 +186,18 @@
> * convenient way to get @a eol_str and @a keywords if in libsvn_wc.
> */
> 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,
> + svn_boolean_t expand);
> +
> +/** Similar to svn_subst_translate_stream2()
... but?
> + *
> + * @deprecated Provided for backward compatibility with the 1.1 API.
> + */
> +svn_error_t *
> svn_subst_translate_stream (svn_stream_t *src,
> svn_stream_t *dst,
> const char *eol_str,
> @@ -162,25 +205,9 @@
> const svn_subst_keywords_t *keywords,
> svn_boolean_t expand);
>
> -
> /**
> - * @deprecated Provided for backward compatibility with the 1.0 API.
> + * @since New in 1.2.
> *
> - * Similar to svn_subst_copy_and_translate2 except that @a special is
> - * always set to @c FALSE.
> - */
> -svn_error_t *
> -svn_subst_copy_and_translate (const char *src,
> - const char *dst,
> - const char *eol_str,
> - svn_boolean_t repair,
> - const svn_subst_keywords_t *keywords,
> - svn_boolean_t expand,
> - apr_pool_t *pool);
> -
> -/**
> - * @since New in 1.1.
> - *
> * Convenience routine: a variant of @c svn_subst_translate_stream
> * which operates on files. (See previous docstring for details.) In
Does that "(See previous docstring for details.)" make sense any more?
> * addition, it will create/detranslate a special file if @a special
> @@ -197,6 +224,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,
> + svn_boolean_t expand,
> + svn_boolean_t special,
> + apr_pool_t *pool);
> +
> +/**
> + * @since New in 1.1.
> + * @deprecated Provided for backward compatibility with the 1.1 API.
> + *
> + * Similar to svn_subst_copy_and_translate3 except that struct-style
> + * keywords are converted to hash-style
> + */
> +svn_error_t *
> svn_subst_copy_and_translate2 (const char *src,
> const char *dst,
> const char *eol_str,
> @@ -206,7 +250,24 @@
> svn_boolean_t special,
> apr_pool_t *pool);
>
> -/** Convenience routine: a variant of @c svn_subst_translate_stream which
> +/**
> + * @deprecated Provided for backward compatibility with the 1.0 API.
> + *
> + * Similar to svn_subst_copy_and_translate2 except that @a special is
> + * always set to @c FALSE.
> + */
> +svn_error_t *
> +svn_subst_copy_and_translate (const char *src,
> + const char *dst,
> + const char *eol_str,
> + svn_boolean_t repair,
> + const svn_subst_keywords_t *keywords,
> + svn_boolean_t expand,
> + apr_pool_t *pool);
> +
> +/** @since New in 1.2.
> + *
> + * Convenience routine: a variant of @c svn_subst_translate_stream which
> * operates on cstrings. (See previous docstring for details.)
> *
> * Return a new string in @a *dst, allocated in @a pool, by copying the
> @@ -217,6 +278,20 @@
> * 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.1 API.
> + *
> + * Similar to svn_subst_translate_cstring2() except that keywords struct
> + * is converted to keywords hash
"it takes a keywords struct instead of a keywords hash." (The fact that it
converts it to a hash is an implementation detail; it could work equally well
without doing so.)
> + */
> +svn_error_t *
> svn_subst_translate_cstring (const char *src,
> const char **dst,
> const char *eol_str,
> === subversion/libsvn_subr/subst.c
> ==================================================================
> --- subversion/libsvn_subr/subst.c (/mirror/trunk) (revision 12276)
> +++ subversion/libsvn_subr/subst.c (/local/keywords) (revision 12276)
[...]
> @@ -528,66 +711,64 @@
> const svn_subst_keywords_t *b,
> svn_boolean_t compare_values)
> {
> + svn_boolean_t result = FALSE;
> + apr_pool_t *pool = svn_pool_create (NULL);
> +
> + /* first we have to create a hash of each of the keyword struct's */
> + apr_hash_t *ahash = keywords_to_keyhash((svn_subst_keywords_t *)a,
> + pool);
> + apr_hash_t *bhash = keywords_to_keyhash((svn_subst_keywords_t *)b,
> + pool);
> +
> + /* and then call the real function */
> + result = svn_subst_keywords_differ2 (ahash,bhash,compare_values,pool);
> +
> + /* Always clean up after ourselves */
> + svn_pool_destroy (pool);
> + return result;
> +}
> +
> +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)
> +{
> + svn_boolean_t result = FALSE;
> + apr_hash_index_t *hi;
> + apr_hash_t *lame_a, *lame_b;
> +
> + lame_a = (apr_hash_t *) a;
> + lame_b = (apr_hash_t *) b;
> +
> if (((a == NULL) && (b == NULL)) /* no A or B */
> - /* no A, and B has no contents */
> - || ((a == NULL)
> - && (b->revision == NULL)
> - && (b->date == NULL)
> - && (b->author == NULL)
> - && (b->url == NULL))
> - /* no B, and A has no contents */
> - || ((b == NULL) && (a->revision == NULL)
> - && (a->date == NULL)
> - && (a->author == NULL)
> - && (a->url == NULL))
> - /* neither A nor B has any contents */
> - || ((a != NULL) && (b != NULL)
> - && (b->revision == NULL)
> - && (b->date == NULL)
> - && (b->author == NULL)
> - && (b->url == NULL)
> - && (a->revision == NULL)
> - && (a->date == NULL)
> - && (a->author == NULL)
> - && (a->url == NULL)))
Hooray for rewriting this long-winded function...
> + /* Unequal number of contents */
> + || (apr_hash_count(lame_a) != apr_hash_count(lame_b)))
> {
> - return FALSE;
> + return TRUE;
> }
... but you've changed the result of ((a == NULL) && (b == NULL)) here, now
saying that they differ.
> - else if ((a == NULL) || (b == NULL))
> - return TRUE;
> -
> - /* Else both A and B have some keywords. */
> -
> - if ((! a->revision) != (! b->revision))
> - return TRUE;
> - else if ((compare_values && (a->revision != NULL))
> - && (strcmp (a->revision->data, b->revision->data) != 0))
> - return TRUE;
> -
> - if ((! a->date) != (! b->date))
> - return TRUE;
> - else if ((compare_values && (a->date != NULL))
> - && (strcmp (a->date->data, b->date->data) != 0))
> - return TRUE;
> -
> - if ((! a->author) != (! b->author))
> - return TRUE;
> - else if ((compare_values && (a->author != NULL))
> - && (strcmp (a->author->data, b->author->data) != 0))
> - return TRUE;
> -
> - if ((! a->url) != (! b->url))
> - return TRUE;
> - else if ((compare_values && (a->url != NULL))
> - && (strcmp (a->url->data, b->url->data) != 0))
> - return TRUE;
> -
> - /* Else we never found a difference, so they must be the same. */
> -
> - return FALSE;
> +
> + /* If compare_values is FALSE, we can say A and B are the same now. */
> + if (!compare_values)
> + return FALSE;
> +
> + /* compare_values is TRUE. Compare value by value */
> + for (hi = apr_hash_first(pool, lame_a);
> + hi && !result;
> + hi = apr_hash_next(hi))
> + {
> + const char *key;
> + apr_hash_this (hi, (const void**) &key, NULL, NULL);
> + if (!svn_string_compare (apr_hash_get (lame_a, key,
> + APR_HASH_KEY_STRING),
> + apr_hash_get (lame_b, key,
> + APR_HASH_KEY_STRING)))
> + result = TRUE;
> + }
> +
> + return result;
> }
>
> -
> svn_error_t *
> svn_subst_translate_stream (svn_stream_t *s, /* src stream */
> svn_stream_t *d, /* dst stream */
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Mon Jan 17 14:41:54 2005