Re: Patch: Fixed length keyword string expansion

On 2004-10-10 20:24+0100, Tom Molesworth wrote:
>
> Just posted an issue containing a patch for fixed-size keyword expansion.

I have few comments and a suggestion about it:

First of, you missed the log entry for you patch. =)

Log:

This patch implements fixed length keywords for SVN. See issue #2095
for furthers details. Patch provided by
Tom Molesworth <tom at molesworth name>.

* subversion/libsvn_subr/subst.c
   (translate_keyword_subst): Check first for fixed length keywords.

Index: subversion/libsvn_subr/subst.c
===================================================================
--- subversion/libsvn_subr/subst.c (revision 11396)
+++ subversion/libsvn_subr/subst.c (working copy)
@@ -239,8 +239,36 @@
 
   buf_ptr = buf + 1 + keyword_len;
 
+ /* Check for fixed-length expansion. */
+
+ if ((buf_ptr[0] == ':')
+ && (buf_ptr[1] == ':')
+ && (buf_ptr[2] == ' ')) /* "$keyword:: " */

Issue #2095 says:
    "Thus, spaces or other characters after the "$Keyword::" tag
    indicate..."

But this check will allow only spaces. I don't know which character is
good for filling, spaces are hard to count. Should we allow ' ' and
'.' as fillers? What do you think?

Moreover, we are using stricter checks for expanded keywords latter
in the code, and the fixed length keywords are already expanded by
definition (More of that at the end of my email).

  /* Check for expanded keyword. */
  else if ((*len >= 4 + keyword_len ) /* holds at least "$keyword: $" */
           && (buf_ptr[0] == ':') /* first char after keyword is ':' */
           && (buf_ptr[1] == ' ') /* second char after keyword is ' ' */
           && (buf[*len - 2] == ' ')) /* has ' ' for next to last character */
    {

+ {
+ /* $keyword::...$, *len remains unchanged */
+
+ apr_size_t vallen = *len - (5 + keyword_len);

$Author:: jaa $
^ ^^^ ^^
1 234 56

This '5' is suspicious, because there are 6 chars which are not
included to the value in fixed length keywords. I think that better
way to do that would be:

        apr_size_t vallen = *len - (6 + keyword_len);
...
            if (value->len =< vallen) { /* replacement not as long as template,

(If value has exactly same length as free space, we are still looping
at the end, that's fine, see my suggestion at the end)

+ if (value && (vallen > 0)) {
+ if (value->len < vallen) { /* replacement not as long as template,
+ pad with spaces */
+ strncpy (buf_ptr + 3, value->data, value->len);
+ buf_ptr += 4 + value->len;
+ while (*buf_ptr != '$')
+ *(buf_ptr++) = ' ';
+ }
+ else
+ {
+ /* replacement needs truncating */
+ strncpy (buf_ptr + 3, value->data, vallen - 1);

Should be
        strncpy (buf_ptr + 3, value->data, vallen);
if we are using 6 instead of 5.

+ buf[*len - 2] = ' ';
+ buf[*len - 1] = '$';
+ }
+ }
+ return TRUE;
+ }

The rest of the issues are:

1) A pointer to the book's todo about this new feature
2) Test cases for fixed length keywords
   http://svn.collab.net/repos/svn/trunk/subversion/tests/clients/cmdline/trans_tests.py

And now the suggestion:

I would like to see that we somehow mark truncated keywords, by
putting some special character at the end of the string or by putting '$'
immediately after value, when it is truncated.

For example:
$Author: lognam#$
or
$Author: reallylongnam$

The second case will loose a little bit our semantics for keywords (no
space before last '$'). I think that's fine with fixed length keywords,
but I like heard your and others Dev's opinion about that. If we
implement that, then we like to loop to the end in every case, so that
we will clean up the truncate marker.

Otherwise your patch looks good, and I like the general idea of fixed
length keywords.

BR, Jani

-- 
Jani Averbach
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org