You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Karl Fogel <kf...@newton.ch.collab.net> on 2002/08/27 18:04:15 UTC
Re: [PATCH] Add Id keyword
Applied your second patch, in revision 3079. The code was excellent,
btw -- my tweaks were: documentation, the elimination of one logical
subchange, a different choice for a new function name, the use of
strcasecmp() instead of strcmp(), and a regression test.
Next time, it would help if you could
* Send one patch per mail message; or at least put some clear
divider between the two alternatives.
* Write a log message for each patch (see HACKING for details).
* Include doc strings for new functions, yes, even file-static
functions :-).
* Try not to mix changes that could be submitted separately. I'm
thinking of the change to the $Date$ keyword in the second patch
below. In my commit, I used your preferred format for $Id$, but
left out your change to $Date$. (I'd like to leave $Date$ the
way it is, anyway. But even if we do decide to shorten it, we
should do that as a separate change, not mixed in with the
addition of the $Id$ keyword.) By the way, I know that using
different date formats for $Id$ vs $Date$ may interfere with the
general customizable keywords proposal; but we can cross that
bridge when we come to it, and anyway we already had a problem
because $Id$ uses the basename instead of the full URL.
Finally, regression tests are optional, of course, but it's worth
having a try... Turned out to be pretty easy to modify existing tests
to test this new keyword.
Thanks,
-Karl
epg@pretzelnet.org writes:
> We've already been through why a customizable, format-string keyword
> is valuable. Unfortunately, this is post-1.0 because we need a mechanism
> for the server to send configuration data to the client. In the meantime,
> this patch will provide the concise Id keyword we're used to from CVS, and
> provide projects a single place to patch to replace Id with $NetBSD$
> or whatever. Not as good as the format-string based keyword, but much
> easier than CVS.
>
> I also provide a second patch. IMNSHO, the date used in the keyword
> should not be client-customizable. Nor does it need to be as verbose.
> The second patch implements a new function to format the date string.
>
> The first patch gives us this:
>
> $Id: bar 146 2002-07-28 16:13:53 -0500 (Sun, 28 Jul 2002) epg $
> $Date: 2002-07-28 16:13:53 -0500 (Sun, 28 Jul 2002) $
>
> The second patch gives us this:
>
> $Id: bar 148 2002-07-28 21:30:43 epg $
> $Date: 2002-07-28 21:30:43 $
>
> Index: ./subversion/include/svn_types.h
> ===================================================================
> --- ./subversion/include/svn_types.h
> +++ ./subversion/include/svn_types.h Sun Jul 28 15:44:00 2002
> @@ -318,6 +318,11 @@
> #define SVN_KEYWORD_URL_LONG "HeadURL"
> #define SVN_KEYWORD_URL_SHORT "URL"
>
> +/* A short combination of the above four keywords. Note that this
> + should go away once the above, more complete solution is
> + implemented. */
> +
> +#define SVN_KEYWORD_ID "Id"
>
>
> /*** Shared function types ***/
> Index: ./subversion/include/svn_wc.h
> ===================================================================
> --- ./subversion/include/svn_wc.h
> +++ ./subversion/include/svn_wc.h Sun Jul 28 15:44:38 2002
> @@ -1381,6 +1381,7 @@
> const svn_string_t *date;
> const svn_string_t *author;
> const svn_string_t *url;
> + const svn_string_t *id;
> } svn_wc_keywords_t;
>
>
> Index: ./subversion/libsvn_wc/translate.c
> ===================================================================
> --- ./subversion/libsvn_wc/translate.c
> +++ ./subversion/libsvn_wc/translate.c Sun Jul 28 16:07:31 2002
> @@ -284,6 +284,15 @@
> return TRUE;
> }
>
> + 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;
> }
> @@ -926,6 +935,25 @@
> keywords->url = svn_string_create (entry->url, pool);
> else
> keywords->url = svn_string_create ("", pool);
> + }
> + else if ((! strcmp (keyword, SVN_KEYWORD_ID)))
> + {
> + if (entry && (entry->cmt_rev && entry->cmt_date
> + && entry->cmt_author && entry->url))
> + {
> + char *basename = svn_path_basename (entry->url, pool);
> + svn_string_t *rev = svn_string_createf (pool, "%" SVN_REVNUM_T_FMT,
> + entry->cmt_rev);
> + const char *date = svn_time_to_human_nts (entry->cmt_date, pool);
> +
> + keywords->id = svn_string_createf (pool, "%s %s %s %s",
> + basename,
> + rev->data,
> + date,
> + entry->cmt_author);
> + }
> + else
> + keywords->id = svn_string_create ("", pool);
> }
> else
> *is_valid_p = FALSE;
> Index: ./subversion/include/svn_types.h
> ===================================================================
> --- ./subversion/include/svn_types.h
> +++ ./subversion/include/svn_types.h Sun Jul 28 15:44:00 2002
> @@ -318,6 +318,11 @@
> #define SVN_KEYWORD_URL_LONG "HeadURL"
> #define SVN_KEYWORD_URL_SHORT "URL"
>
> +/* A short combination of the above four keywords. Note that this
> + should go away once the above, more complete solution is
> + implemented. */
> +
> +#define SVN_KEYWORD_ID "Id"
>
>
> /*** Shared function types ***/
> Index: ./subversion/include/svn_wc.h
> ===================================================================
> --- ./subversion/include/svn_wc.h
> +++ ./subversion/include/svn_wc.h Sun Jul 28 15:44:38 2002
> @@ -1381,6 +1381,7 @@
> const svn_string_t *date;
> const svn_string_t *author;
> const svn_string_t *url;
> + const svn_string_t *id;
> } svn_wc_keywords_t;
>
>
> Index: ./subversion/libsvn_wc/translate.c
> ===================================================================
> --- ./subversion/libsvn_wc/translate.c
> +++ ./subversion/libsvn_wc/translate.c Sun Jul 28 16:25:35 2002
> @@ -284,6 +284,15 @@
> return TRUE;
> }
>
> + 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;
> }
> @@ -861,6 +870,38 @@
> }
>
>
> +static const char *
> +time_to_date_keyword (apr_time_t t, apr_pool_t *pool)
> +{
> + const char *t_cstr;
> + apr_time_exp_t exploded_time;
> +
> + /* We toss apr_status_t return value here -- for one thing, caller
> + should pass in good information. But also, where APR's own code
> + calls these functions it tosses the return values, and
> + furthermore their current implementations can only return success
> + anyway. */
> +
> + /* We get the date in GMT now -- and expect the tm_gmtoff and
> + tm_isdst to be not set. We also ignore the weekday and yearday,
> + since those are not needed. */
> +
> + apr_time_exp_gmt (&exploded_time, t);
> +
> + /* It would be nice to use apr_strftime(), but APR doesn't give a
> + way to convert back, so we wouldn't be able to share the format
> + string between the writer and reader. */
> + t_cstr = apr_psprintf (pool, "%04d-%02d-%02d %02d:%02d:%02d",
> + exploded_time.tm_year + 1900,
> + exploded_time.tm_mon + 1,
> + exploded_time.tm_mday,
> + exploded_time.tm_hour,
> + exploded_time.tm_min,
> + exploded_time.tm_sec);
> +
> + return t_cstr;
> +}
> +
> /* Helper for svn_wc__get_keywords().
>
> If KEYWORD is a valid keyword, look up its value in ENTRY, fill in
> @@ -907,7 +948,7 @@
> {
> if (entry && (entry->cmt_date))
> keywords->date = svn_string_create
> - (svn_time_to_human_nts (entry->cmt_date, pool), pool);
> + (time_to_date_keyword (entry->cmt_date, pool), pool);
> else
> keywords->date = svn_string_create ("", pool);
> }
> @@ -926,6 +967,25 @@
> keywords->url = svn_string_create (entry->url, pool);
> else
> keywords->url = svn_string_create ("", pool);
> + }
> + else if ((! strcmp (keyword, SVN_KEYWORD_ID)))
> + {
> + if (entry && (entry->cmt_rev && entry->cmt_date
> + && entry->cmt_author && entry->url))
> + {
> + char *basename = svn_path_basename (entry->url, pool);
> + svn_string_t *rev = svn_string_createf (pool, "%" SVN_REVNUM_T_FMT,
> + entry->cmt_rev);
> + const char *date = time_to_date_keyword (entry->cmt_date, pool);
> +
> + keywords->id = svn_string_createf (pool, "%s %s %s %s",
> + basename,
> + rev->data,
> + date,
> + entry->cmt_author);
> + }
> + else
> + keywords->id = svn_string_create ("", pool);
> }
> else
> *is_valid_p = FALSE;
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
> For additional commands, e-mail: dev-help@subversion.tigris.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org