John Peacock <jpeacock@rowman.com> writes:
> --- 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.
> +*/
> 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
Document the C types of the hash keys and values.
> @@ -96,6 +101,21 @@
> * All memory is allocated out of @a pool.
> */
> svn_error_t *
> +svn_subst_build_keywords2 (apr_hash_t *kw,
Any reason why you chose apr_hash_t* rather than apr_hash_t**?
> + 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()
> + *
> ==================================================================
> --- subversion/libsvn_wc/merge.c (/mirror/trunk) (revision 12276)
> +++ subversion/libsvn_wc/merge.c (/local/keywords) (revision 12276)
> @@ -48,7 +48,7 @@
> const char *mt_pt, *mt_bn;
> apr_file_t *tmp_f, *result_f;
> svn_boolean_t is_binary;
> - svn_subst_keywords_t *keywords;
> + apr_hash_t *keywords = apr_hash_make(pool);
From the context lines it appears that this file uses "fn (" rather
than "fn(".
> const char *eol;
> const svn_wc_entry_t *entry;
> svn_boolean_t contains_conflicts, special;
> @@ -229,18 +229,18 @@
>
> /* Create LEFT and RIGHT backup files, in expanded form.
> We use merge_target's current properties to do the translation. */
> - SVN_ERR (svn_wc__get_keywords (&keywords, merge_target, adm_access,
> + SVN_ERR (svn_wc__get_keywords (keywords, merge_target, adm_access,
> NULL, pool));
> SVN_ERR (svn_wc__get_eol_style (NULL, &eol, merge_target, adm_access,
> pool));
> SVN_ERR (svn_wc__get_special (&special, merge_target, adm_access,
> pool));
> - SVN_ERR (svn_subst_copy_and_translate2 (left,
> + SVN_ERR (svn_subst_copy_and_translate3 (left,
> left_copy,
> eol, eol ? TRUE : FALSE,
> keywords, TRUE, special,
> pool));
> - SVN_ERR (svn_subst_copy_and_translate2 (right,
> + SVN_ERR (svn_subst_copy_and_translate3 (right,
> right_copy,
> eol, eol ? TRUE : FALSE,
> keywords, TRUE, special,
> @@ -288,13 +288,13 @@
> if (*merge_outcome != svn_wc_merge_unchanged && ! dry_run)
> {
> /* replace MERGE_TARGET with the new merged file, expanding. */
> - SVN_ERR (svn_wc__get_keywords (&keywords, merge_target, adm_access,
> + SVN_ERR (svn_wc__get_keywords (keywords, merge_target, adm_access,
> NULL, pool));
> SVN_ERR (svn_wc__get_eol_style (NULL, &eol, merge_target, adm_access,
> pool));
> SVN_ERR (svn_wc__get_special (&special, merge_target, adm_access,
> pool));
> - SVN_ERR (svn_subst_copy_and_translate2 (result_target, merge_target,
> + SVN_ERR (svn_subst_copy_and_translate3 (result_target, merge_target,
> eol, eol ? TRUE : FALSE,
> keywords, TRUE, special,
> pool));
> === subversion/libsvn_wc/translate.h
> ==================================================================
> --- subversion/libsvn_wc/translate.h (/mirror/trunk) (revision 12276)
> +++ subversion/libsvn_wc/translate.h (/local/keywords) (revision 12276)
> @@ -67,19 +67,20 @@
> const char *eol);
>
> /* Expand keywords for the file at PATH, by parsing a
> - whitespace-delimited list of keywords. If any keywords are found
> - in the list, allocate *KEYWORDS from POOL, and then populate its
> - entries with the related keyword values (also allocated in POOL).
> - If no keywords are found in the list, or if there is no list, set
> - *KEYWORDS to NULL. ADM_ACCESS must be an access baton for PATH.
> + whitespace-delimited list of keywords. Calling function must have
> + already allocated KEYWORDS from POOL. If any keywords are found
> + in the list, then populate the KEYWORDS hash entries with the related
> + keyword values (also allocated in POOL). If no keywords are found in
> + the list, or if there is no list, leave KEYWORDS hash empty. ADM_ACCESS
> + must be an access baton for PATH.
Document the C types of the hash keys and values (or perhaps refer to
svn_subst_build_keywords2).
>
> If FORCE_LIST is non-null, use it as the list; else use the
> SVN_PROP_KEYWORDS property for PATH. In either case, use PATH to
> expand keyword values. If a keyword is in the list, but no
> - corresponding value is available, set that element of *KEYWORDS to
> + corresponding value is available, set that hash element of KEYWORDS to
> the empty string ("").
> */
> -svn_error_t *svn_wc__get_keywords (svn_subst_keywords_t **keywords,
> +svn_error_t *svn_wc__get_keywords (apr_hash_t *keywords,
Any reason why you chose apr_hash_t* rather than apr_hash_t**? You
have forced every caller to allocate the hash.
> const char *path,
> svn_wc_adm_access_t *adm_access,
> const char *force_list,
> === subversion/libsvn_wc/props.c
> ==================================================================
> --- subversion/libsvn_wc/props.c (/mirror/trunk) (revision 12276)
> +++ subversion/libsvn_wc/props.c (/local/keywords) (revision 12276)
> @@ -973,8 +973,8 @@
> here is whether or not the function fails on inconsistent line
> endings. The function is "translating" to an empty stream. This
> is sneeeeeeeeeeeaky. */
> - err = svn_subst_translate_stream (read_stream, write_stream,
> - "", FALSE, NULL, FALSE);
> + err = svn_subst_translate_stream2 (read_stream, write_stream,
> + "", FALSE, NULL, FALSE);
> if (err && err->apr_err == SVN_ERR_IO_INCONSISTENT_EOL)
> return svn_error_createf (SVN_ERR_ILLEGAL_TARGET, err,
> _("File '%s' has inconsistent newlines"),
> @@ -998,7 +998,7 @@
> svn_error_t *err;
> apr_hash_t *prophash;
> apr_file_t *fp = NULL;
> - svn_subst_keywords_t *old_keywords;
> + apr_hash_t *old_keywords = apr_hash_make(pool);
"fn (" rather than "fn(" (although if you make svn_wc__get_keywords
allocate the hash this bit goes away :)
> svn_stringbuf_t *new_value = NULL;
> svn_node_kind_t kind;
> enum svn_prop_kind prop_kind = svn_property_kind (NULL, name);
> @@ -1105,7 +1105,7 @@
> * from the old one.
> */
> if (kind == svn_node_file && strcmp (name, SVN_PROP_KEYWORDS) == 0)
> - SVN_ERR (svn_wc__get_keywords (&old_keywords, path, adm_access, NULL,
> + SVN_ERR (svn_wc__get_keywords (old_keywords, path, adm_access, NULL,
> pool));
>
> /* Now we have all the properties in our hash. Simply merge the new
> @@ -1134,11 +1134,11 @@
>
> if (kind == svn_node_file && strcmp (name, SVN_PROP_KEYWORDS) == 0)
> {
> - svn_subst_keywords_t *new_keywords;
> - SVN_ERR (svn_wc__get_keywords (&new_keywords, path, adm_access, NULL,
> + apr_hash_t *new_keywords = apr_hash_make(pool);
> + SVN_ERR (svn_wc__get_keywords (new_keywords, path, adm_access, NULL,
> pool));
>
> - if (svn_subst_keywords_differ (old_keywords, new_keywords, FALSE))
> + if (svn_subst_keywords_differ2 (old_keywords, new_keywords, FALSE, pool))
> {
> const char *base_name;
> svn_wc_entry_t tmp_entry;
> === subversion/libsvn_wc/adm_crawler.c
> ==================================================================
> --- subversion/libsvn_wc/adm_crawler.c (/mirror/trunk) (revision 12276)
> +++ subversion/libsvn_wc/adm_crawler.c (/local/keywords) (revision 12276)
> @@ -61,7 +61,7 @@
> apr_pool_t *pool)
> {
> const char *text_base_path, *tmp_text_base_path;
> - svn_subst_keywords_t *keywords;
> + apr_hash_t *keywords = apr_hash_make(pool);
"fn (" rather than "fn(", I think I'll stop pointing this out!
> const char *eol;
> const svn_wc_entry_t *entry;
> svn_wc_entry_t newentry;
> @@ -78,7 +78,7 @@
> FALSE, pool));
>
> SVN_ERR (svn_wc__get_eol_style (NULL, &eol, file_path, adm_access, pool));
> - SVN_ERR (svn_wc__get_keywords (&keywords,
> + SVN_ERR (svn_wc__get_keywords (keywords,
> file_path, adm_access, NULL, pool));
> SVN_ERR (svn_wc__get_special (&special, file_path, adm_access, pool));
>
> @@ -87,7 +87,7 @@
> sure to do any eol translations or keyword substitutions,
> as dictated by the property values. If these properties
> are turned off, then this is just a normal copy. */
> - SVN_ERR (svn_subst_copy_and_translate2 (tmp_text_base_path,
> + SVN_ERR (svn_subst_copy_and_translate3 (tmp_text_base_path,
> file_path,
> eol, FALSE, /* don't repair */
> keywords,
> ==================================================================
> --- subversion/libsvn_subr/subst.c (/mirror/trunk) (revision 12276)
> +++ subversion/libsvn_subr/subst.c (/local/keywords) (revision 12276)
> @@ -118,6 +118,184 @@
> 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.
> + *
> + * The codes of format:
> + *
> + * %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
> + * %U UUID of the repository
> + *
> + * 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,
> + const char *uuid,
> + apr_pool_t *pool)
> +{
> + svn_stringbuf_t *value = svn_stringbuf_ncreate ("", 0, pool);
> + const char *cur;
> + char ch;
> + int n;
> +
> + for (;;)
> + {
> + for (cur = fmt; (ch = *cur) != '\0' && ch != '%'; cur++)
> + /* void */;
> + if ( (n = cur - fmt) > 0) /* Do we have a as-is string? */
> + svn_stringbuf_appendbytes (value, fmt, n);
A personal preference: don't use assignments in for/if statements.
Normally I would not mention it but you have two in succession and
they don't aid readability.
> +
> + if (ch == '\0')
> + break;
> +
> + cur++; /* skip '%' */
> + ch = *cur++;
> + switch (ch)
> + {
> + 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);
> +
> + svn_stringbuf_appendcstr (value, base_name);
> + }
> + break;
> + case 'd': /* short format of date of this revision */
> + if (date)
> + {
> + const char *human_date = NULL;
> +
> + if (!date_prop_to_human (&human_date, FALSE, date, pool))
> + svn_stringbuf_appendcstr (value, human_date);
> + }
> + break;
> + case 'D': /* long format of date of this revision */
> + if (date)
> + {
> + const char *human_date = NULL;
> +
> + if (!date_prop_to_human (&human_date, TRUE, date, pool))
> + 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 'U': /* UUID of the repository */
> + if (uuid)
> + svn_stringbuf_appendcstr (value, uuid);
> + break;
> + default: /* %?, print ? as is. */
> + svn_stringbuf_appendbytes (value, &ch, 1);
> + break;
> + }
> +
> + /* Format code is processed. Get ready for next chunk. */
> + fmt = cur;
> + }
> +
> + return svn_string_create_from_buf (value, pool);
> +}
> +
> +/* Convert the old-style keywords struct into the new keywords hash */
> +static apr_hash_t *
> +keywords_to_keyhash (svn_subst_keywords_t *kw,
> + apr_pool_t *pool)
> +{
> + apr_hash_t *khash = apr_hash_make(pool);
> +
> + /* if there are no keywords set, just return immediately */
> + if (kw == NULL)
> + return khash;
Odd identation.
> +
> + if (kw->revision)
> + {
> + apr_hash_set (khash, SVN_KEYWORD_REVISION_LONG,
> + APR_HASH_KEY_STRING, kw->revision);
> + apr_hash_set (khash, SVN_KEYWORD_REVISION_MEDIUM,
> + APR_HASH_KEY_STRING, kw->revision);
> + apr_hash_set (khash, SVN_KEYWORD_REVISION_SHORT,
> + APR_HASH_KEY_STRING, kw->revision);
> + }
> + if (kw->date)
> + {
> + apr_hash_set (khash, SVN_KEYWORD_DATE_LONG,
> + APR_HASH_KEY_STRING, kw->date);
> + apr_hash_set (khash, SVN_KEYWORD_DATE_SHORT,
> + APR_HASH_KEY_STRING, kw->date);
> + }
> + if (kw->author)
> + {
> + apr_hash_set (khash, SVN_KEYWORD_AUTHOR_LONG,
> + APR_HASH_KEY_STRING, kw->author);
> + apr_hash_set (khash, SVN_KEYWORD_AUTHOR_SHORT,
> + APR_HASH_KEY_STRING, kw->author);
> + }
> + if (kw->url)
> + {
> + apr_hash_set (khash, SVN_KEYWORD_URL_LONG,
> + APR_HASH_KEY_STRING, kw->url);
> + apr_hash_set (khash, SVN_KEYWORD_URL_SHORT,
> + APR_HASH_KEY_STRING, kw->url);
> + }
> + if (kw->id)
> + {
> + apr_hash_set (khash, SVN_KEYWORD_ID,
> + APR_HASH_KEY_STRING, kw->id);
> + }
> +
> + return khash;
> +}
> +
> +/* Convert new style keyhash into keyword struct */
> +static void
> +keyhash_to_keywords (svn_subst_keywords_t *kw,
> + apr_hash_t *khash,
> + apr_pool_t *pool)
> +{
> + /* Attempt to fill all of the default keyword slots
> + * In each case, if the hash doesn't contain the corresponding
> + * keyword value, the struct element will be null
> + */
> +
> + kw->revision = apr_hash_get(khash, SVN_KEYWORD_REVISION_LONG,
> + APR_HASH_KEY_STRING);
> +
> + kw->date = apr_hash_get(khash, SVN_KEYWORD_DATE_LONG,
> + APR_HASH_KEY_STRING);
> +
> + kw->author = apr_hash_get(khash, SVN_KEYWORD_AUTHOR_LONG,
> + APR_HASH_KEY_STRING);
> +
> + kw->url = apr_hash_get(khash, SVN_KEYWORD_URL_LONG,
> + APR_HASH_KEY_STRING);
> +
> + kw->id = apr_hash_get(khash, SVN_KEYWORD_ID,
> + APR_HASH_KEY_STRING);
The old code would use "" for missing attributes, this uses NULL if
the keyword is not found in the hash.
> +}
> +
> svn_error_t *
> svn_subst_build_keywords (svn_subst_keywords_t *kw,
> const char *keywords_val,
> @@ -127,6 +305,26 @@
> const char *author,
> apr_pool_t *pool)
> {
> + apr_hash_t *khash = apr_hash_make(pool);
> + svn_error_t * kerror = svn_subst_build_keywords2(khash, keywords_val, rev,
> + url, date, author, NULL,
> + pool);
> +
> + keyhash_to_keywords(kw, khash, pool);
You have switched to "fn(" for this function.
> +
> + return kerror;
> +}
> +
> +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,
> + const char *uuid,
> + apr_pool_t *pool)
> +{
> apr_array_header_t *keyword_tokens;
> int i;
>
> @@ -141,45 +339,92 @@
> || (! strcmp (keyword, SVN_KEYWORD_REVISION_MEDIUM))
> || (! strcasecmp (keyword, SVN_KEYWORD_REVISION_SHORT)))
> {
> - kw->revision = svn_string_create (rev, pool);
> - }
> + svn_string_t *revision_val;
> +
> + revision_val = keyword_printf (SVN_KEYWORD_REVISION_FORMAT,
> + rev, url, date, author,
> + uuid, pool);
> + apr_hash_set (kw, SVN_KEYWORD_REVISION_LONG,
> + APR_HASH_KEY_STRING, revision_val);
> + apr_hash_set (kw, SVN_KEYWORD_REVISION_MEDIUM,
> + APR_HASH_KEY_STRING, revision_val);
> + apr_hash_set (kw, SVN_KEYWORD_REVISION_SHORT,
> + APR_HASH_KEY_STRING, revision_val);
> + }
> else if ((! strcmp (keyword, SVN_KEYWORD_DATE_LONG))
> || (! strcasecmp (keyword, SVN_KEYWORD_DATE_SHORT)))
> {
> if (date)
> {
> - const char *human_date;
> + svn_string_t *date_val;
>
> - SVN_ERR (date_prop_to_human (&human_date, TRUE, date, pool));
> + date_val = keyword_printf (SVN_KEYWORD_DATE_FORMAT,
> + rev, url, date, author,
> + uuid, pool);
> + apr_hash_set (kw, SVN_KEYWORD_DATE_LONG,
> + APR_HASH_KEY_STRING, date_val);
> + apr_hash_set (kw, SVN_KEYWORD_DATE_SHORT,
> + APR_HASH_KEY_STRING, date_val);
> + }
> + else
> + {
> + svn_string_t *date_val;
>
> - kw->date = svn_string_create (human_date, pool);
> + date_val = svn_string_create ("", pool);
> + apr_hash_set (kw, SVN_KEYWORD_DATE_LONG,
> + APR_HASH_KEY_STRING, date_val);
> + apr_hash_set (kw, SVN_KEYWORD_DATE_SHORT,
> + APR_HASH_KEY_STRING, date_val);
> }
> - else
> - kw->date = svn_string_create ("", pool);
> }
> else if ((! strcmp (keyword, SVN_KEYWORD_AUTHOR_LONG))
> || (! strcasecmp (keyword, SVN_KEYWORD_AUTHOR_SHORT)))
> {
> - kw->author = svn_string_create (author ? author : "", pool);
> + svn_string_t *author_val;
> +
> + author_val = keyword_printf (SVN_KEYWORD_AUTHOR_FORMAT,
> + rev, url, date, author,
> + uuid, pool);
> + apr_hash_set (kw, SVN_KEYWORD_AUTHOR_LONG,
> + APR_HASH_KEY_STRING, author_val);
> + apr_hash_set (kw, SVN_KEYWORD_AUTHOR_SHORT,
> + APR_HASH_KEY_STRING, author_val);
> }
> else if ((! strcmp (keyword, SVN_KEYWORD_URL_LONG))
> || (! strcasecmp (keyword, SVN_KEYWORD_URL_SHORT)))
> {
> - kw->url = svn_string_create (url ? url : "", pool);
> + svn_string_t *url_val;
> +
> + url_val = keyword_printf (SVN_KEYWORD_URL_FORMAT,
> + rev, url, date, author,
> + uuid, pool);
> + apr_hash_set (kw, SVN_KEYWORD_URL_LONG,
> + APR_HASH_KEY_STRING, url_val);
> + apr_hash_set (kw, SVN_KEYWORD_URL_SHORT,
> + APR_HASH_KEY_STRING, url_val);
> }
> + else if ((! strcmp (keyword, SVN_KEYWORD_UUID_LONG))
> + || (! strcasecmp (keyword, SVN_KEYWORD_UUID_SHORT)))
> + {
> + svn_string_t *uuid_val;
> +
> + uuid_val = keyword_printf (SVN_KEYWORD_UUID_FORMAT,
> + rev, url, date, author,
> + uuid, pool);
> + apr_hash_set (kw, SVN_KEYWORD_UUID_LONG,
> + APR_HASH_KEY_STRING, uuid_val);
> + apr_hash_set (kw, SVN_KEYWORD_UUID_SHORT,
> + APR_HASH_KEY_STRING, uuid_val);
> + }
> else if ((! strcasecmp (keyword, SVN_KEYWORD_ID)))
> {
> - const char *base_name = url ? svn_path_basename (url, pool) : "";
> - const char *human_date = NULL;
> + svn_string_t *id_val;
>
> - if (date)
> - SVN_ERR (date_prop_to_human (&human_date, FALSE, date, pool));
> -
> - kw->id = svn_string_createf (pool, "%s %s %s %s",
> - base_name,
> - rev,
> - human_date ? human_date : "",
> - author ? author : "");
> + id_val = keyword_printf (SVN_KEYWORD_ID_FORMAT,
> + rev, url, date, author,
> + uuid, pool);
> + apr_hash_set (kw, SVN_KEYWORD_ID,
> + APR_HASH_KEY_STRING, id_val);
> }
> }
>
> @@ -378,8 +623,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 key[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] == '$'));
> @@ -388,86 +637,20 @@
> 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;
> + for (i = 0; i < *len - 2 && buf[i + 1] != ':'; i++)
> + key[i] = *(buf + i + 1);
> + key[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, key, APR_HASH_KEY_STRING);
>
> - 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))
> + key, strlen(key),
> + expand ? value : 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;
> }
>
> - /* Author */
> - if (keywords->author)
> - {
> - if (translate_keyword_subst (buf, len,
> - SVN_KEYWORD_AUTHOR_LONG,
> - (sizeof (SVN_KEYWORD_AUTHOR_LONG)) - 1,
> - expand ? keywords->author : NULL))
> - return TRUE;
> -
> - if (translate_keyword_subst (buf, len,
> - SVN_KEYWORD_AUTHOR_SHORT,
> - (sizeof (SVN_KEYWORD_AUTHOR_SHORT)) - 1,
> - expand ? keywords->author : NULL))
> - return TRUE;
> - }
> -
> - /* URL */
> - if (keywords->url)
> - {
> - if (translate_keyword_subst (buf, len,
> - SVN_KEYWORD_URL_LONG,
> - (sizeof (SVN_KEYWORD_URL_LONG)) - 1,
> - expand ? keywords->url : NULL))
> - return TRUE;
> -
> - if (translate_keyword_subst (buf, len,
> - SVN_KEYWORD_URL_SHORT,
> - (sizeof (SVN_KEYWORD_URL_SHORT)) - 1,
> - expand ? keywords->url : NULL))
> - return TRUE;
> - }
> -
> - /* Id */
> - if (keywords->id)
> - {
> - if (translate_keyword_subst (buf, len,
> - SVN_KEYWORD_ID,
> - (sizeof (SVN_KEYWORD_ID)) - 1,
> - expand ? keywords->id : NULL))
> - return TRUE;
> - }
> -
> /* No translations were successful. Return FALSE. */
> return FALSE;
> }
> @@ -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);
A new top level pool :( I think that means it bypasses the
applications allocator, but I don't see what else you can do. I think
we should document it at least.
> +
> + /* 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);
Why the casts? This is a brand new interface, what's gone wrong?
> +
> + /* and then call the real function */
> + result = svn_subst_keywords_differ2 (ahash,bhash,compare_values,pool);
spaces after commas please.
> +
> + /* 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)))
> + /* Unequal number of contents */
> + || (apr_hash_count(lame_a) != apr_hash_count(lame_b)))
> {
> - return FALSE;
> + return TRUE;
> }
> - 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);
Please look at other apr_hash_this uses and avoid the cast.
> + 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 */
> @@ -596,6 +777,28 @@
> const svn_subst_keywords_t *keywords,
> svn_boolean_t expand)
> {
> + apr_pool_t *pool = svn_pool_create (NULL);
Another top level pool.
> + apr_hash_t *kh =
> + keywords_to_keyhash((svn_subst_keywords_t *)keywords,
> + pool);
> +
> + /* call the real function */
> + svn_error_t *err = svn_subst_translate_stream2 (s, d, eol_str,
> + repair, kh, expand);
> +
> + /* Always clean up after ourselves */
> + svn_pool_destroy (pool);
> + return err;
> +}
> +
> +svn_error_t *
> +svn_subst_translate_stream2 (svn_stream_t *s, /* src stream */
> + svn_stream_t *d, /* dst stream */
> + const char *eol_str,
> + svn_boolean_t repair,
> + apr_hash_t *keywords,
> + svn_boolean_t expand)
> +{
> char buf[SVN_STREAM_CHUNK_SIZE + 1];
> const char *p, *interesting;
> apr_size_t len, readlen;
> @@ -714,6 +917,22 @@
> svn_boolean_t expand,
> apr_pool_t *pool)
> {
> + apr_hash_t *kh =
> + keywords_to_keyhash((svn_subst_keywords_t *)keywords, pool);
> +
> + return svn_subst_translate_cstring2 (src, dst, eol_str, repair,
> + kh, expand, pool);
> +}
> +
> +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)
> +{
> svn_stringbuf_t *src_stringbuf, *dst_stringbuf;
> svn_stream_t *src_stream, *dst_stream;
> svn_error_t *err;
> @@ -733,8 +952,8 @@
> dst_stream = svn_stream_from_stringbuf (dst_stringbuf, pool);
>
> /* Translate src stream into dst stream. */
> - err = svn_subst_translate_stream (src_stream, dst_stream,
> - eol_str, repair, keywords, expand);
> + err = svn_subst_translate_stream2 (src_stream, dst_stream,
> + eol_str, repair, keywords, expand);
> if (err)
> {
> svn_error_clear (svn_stream_close (src_stream));
> @@ -923,6 +1142,24 @@
> svn_boolean_t special,
> apr_pool_t *pool)
> {
> + apr_hash_t *kh =
> + keywords_to_keyhash((svn_subst_keywords_t *)keywords, pool);
> +
> + return svn_subst_copy_and_translate3 (src, dst, eol_str,
> + repair, kh, expand, special,
> + pool);
> +}
> +
> +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)
> +{
> const char *dst_tmp = NULL;
> svn_stream_t *src_stream, *dst_stream;
> apr_file_t *s = NULL, *d = NULL; /* init to null important for APR */
> @@ -975,8 +1212,8 @@
> dst_stream = svn_stream_from_aprfile (d, subpool);
>
> /* Translate src stream into dst stream. */
> - err = svn_subst_translate_stream (src_stream, dst_stream,
> - eol_str, repair, keywords, expand);
> + err = svn_subst_translate_stream2 (src_stream, dst_stream,
> + eol_str, repair, keywords, expand);
> if (err)
> {
> if (err->apr_err == SVN_ERR_IO_INCONSISTENT_EOL)
> @@ -1046,13 +1283,13 @@
> SVN_ERR (svn_utf_cstring_to_utf8 (&val_utf8, value->data, pool));
> }
>
> - SVN_ERR (svn_subst_translate_cstring (val_utf8,
> - &val_utf8_lf,
> - "\n", /* translate to LF */
> - FALSE, /* no repair */
> - NULL, /* no keywords */
> - FALSE, /* no expansion */
> - pool));
> + SVN_ERR (svn_subst_translate_cstring2 (val_utf8,
> + &val_utf8_lf,
> + "\n", /* translate to LF */
> + FALSE, /* no repair */
> + NULL, /* no keywords */
> + FALSE, /* no expansion */
> + pool));
>
> *new_value = svn_string_create (val_utf8_lf, pool);
>
> @@ -1076,13 +1313,13 @@
> return SVN_NO_ERROR;
> }
>
> - SVN_ERR (svn_subst_translate_cstring (value->data,
> - &val_neol,
> - APR_EOL_STR, /* 'native' eol */
> - FALSE, /* no repair */
> - NULL, /* no keywords */
> - FALSE, /* no expansion */
> - pool));
> + SVN_ERR (svn_subst_translate_cstring2 (value->data,
> + &val_neol,
> + APR_EOL_STR, /* 'native' eol */
> + FALSE, /* no repair */
> + NULL, /* no keywords */
> + FALSE, /* no expansion */
> + pool));
>
> if (for_output)
> {
Apart from some interface queries my comments are mainly whitespace
issues. I don't have any strong feelings about issue 890 (I don't
use keywords) but the code looks reasonable.
--
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Mon Jan 17 00:05:27 2005