This is OK except that for no keywords, svn_wc__get_keywords() sometimes
returns NULL (if there is no list) and sometimes an empty hash (if there is an
empty list, because svn_subst_build_keywords2() does so).
The problem is that the keywords-as-hash API is inconsistent in how it
represents "no keywords": svn_subst_build_keywords2() generates an empty hash,
but all of the other functions expect NULL (but don't require it).
I agonised over this all afternoon, wondering if the keywords-as-hash API is
acceptable, and researched the existing use of apr_hash_t in our APIs, finding
that we hardly ever provide or accept NULL. However, the existing use of
keywords did use NULL (for svn_subst_keywords_t *), so I decided the API is OK
as it is.
So, I fixed the implementation of svn_wc__get_keywords() to do what it says.
It doesn't matter much what it does, since it's private, as long as it is
useful and functionally correct.
I'll commit this as soon as the regression tests finish running, along with
parts 3 and 4 which are fine.
Thanks for reviving this.
My tweaks are noted below.
- Julian
John Peacock wrote:
> === subversion/libsvn_wc/translate.h
> ==================================================================
> --- subversion/libsvn_wc/translate.h (/mirror/trunk) (revision 15746)
> +++ subversion/libsvn_wc/translate.h (/local/kwhash2) (revision 15746)
> @@ -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. The KEYWORDS hash will be
> + allocated 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, set KEYWORDS to NULL. ADM_ACCESS must be an
> + access baton for PATH.
"set *KEYWORDS to NULL".
> 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
> - the empty string ("").
> + corresponding value is available, set that hash element of KEYWORDS
> + to NULL.
I re-worded this, since a hash element can't have a null value. I've also
rearranged this doc string a bit to keep related pieces of information together.
> === subversion/libsvn_wc/translate.c
> ==================================================================
[...]
> +svn_wc__get_keywords (apr_hash_t **keywords,
> const char *path,
> svn_wc_adm_access_t *adm_access,
> const char *force_list,
> apr_pool_t *pool)
> {
> const char *list;
> - svn_subst_keywords_t tmp_keywords = { 0 };
> const svn_wc_entry_t *entry = NULL;
>
> - /* Start by assuming no keywords. */
> - *keywords = NULL;
> -
> /* Choose a property list to parse: either the one that came into
> this function, or the one attached to PATH. */
> if (force_list == NULL)
> @@ -198,21 +194,22 @@
>
> /* The easy answer. */
> if (list == NULL)
> - return SVN_NO_ERROR;
> + {
> + *keywords = NULL; /* caller didn't initialize so we will */
> + return SVN_NO_ERROR;
> + }
>
> SVN_ERR (svn_wc_entry (&entry, path, adm_access, FALSE, pool));
>
> - SVN_ERR (svn_subst_build_keywords (&tmp_keywords,
> - list,
> - apr_psprintf (pool, "%ld",
> - entry->cmt_rev),
> - entry->url,
> - entry->cmt_date,
> - entry->cmt_author,
> - pool));
> + SVN_ERR (svn_subst_build_keywords2 (keywords,
> + list,
> + apr_psprintf (pool, "%ld",
> + entry->cmt_rev),
> + entry->url,
> + entry->cmt_date,
> + entry->cmt_author,
> + pool));
Here I've added a statement: if it is an empty hash then set the pointer to
NULL, to comply with the doc string.
[...]
> return SVN_NO_ERROR;
> }
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Wed Sep 21 20:27:47 2005