[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: [PATCH] Resend: A step toward custom keyword

From: Branko Čibej <brane_at_xbc.nu>
Date: 2003-07-04 02:07:06 CEST

Right! I'm finally taking the time to review this.

First, a general question: Would people like to see this feature in 1.0?
Let's see a show of hands...

I think the general approach is sane, although there are things I don't
like about the implementation itself. Comments below.

>+/** Given an 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
>+ * %p property value (not implemented)
>+ * %r number of this revision
>+ * %u URL of this file
>
Hm, it might be better to use %U instead of %b -- I think it's a better
mnemonic, similar to the %d,%D pair.

>+ *
>+ * All memory is allocated out of @a pool.
>+ */
>+svn_string_t *
>+svn_subst_keyword_printf (const char *fmt,
>+ const char *rev,
>+ const char *url,
>+ apr_time_t date,
>+ const char *author,
>+ apr_hash_t *props,
>+ apr_pool_t *pool);
>
Ouch. If you do this, the interface of this function changes every time
you add a non-prop value. You gen rev, url, date and author in an
svn_entries_t anyway. Perhaps passing a hash of the revprops wouldn't
hurt, either, then you could have %p for node props and %P for
committed-rev props.

[snip]

>- SVN_ERR (svn_subst_build_keywords (&tmp_keywords,
>+ SVN_ERR (svn_wc_prop_list (&props, path, adm_access, pool));
>+ SVN_ERR (svn_subst_build_keywords (tmp_keywords,
> list,
> apr_psprintf (pool, "%" SVN_REVNUM_T_FMT,
> entry->revision),
> entry->url,
> entry->cmt_date,
> entry->cmt_author,
>+ props,
> pool));
>
See? We're converting stuff from the entry struct into parameters, when
we could just pass in the entry pointer itself.

[snip]

>+svn_error_t *
> svn_subst_build_keywords (svn_subst_keywords_t *kw,
> const char *keywords_val,
> const char *rev,
> const char *url,
> apr_time_t date,
> const char *author,
>+ apr_hash_t *props,
> apr_pool_t *pool)
>
This, and
[snip]

>+svn_string_t *
>+svn_subst_keyword_printf (const char *fmt,
>+ const char *rev,
>+ const char *url,
>+ apr_time_t date,
>+ const char *author,
>+ apr_hash_t *props,
>+ apr_pool_t *pool)
>
this would be much more elegant if the format parsing, keyword value
generation, etc. was table-based. The huge, unreadable if-elseif horror
would just go away.

-- 
Brane Čibej   <brane_at_xbc.nu>   http://www.xbc.nu/brane/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Fri Jul 4 02:07:53 2003

This is an archived mail posted to the Subversion Dev mailing list.

This site is subject to the Apache Privacy Policy and the Apache Public Forum Archive Policy.