hi there,
Jani Averbach wrote:
> First of, you missed the log entry for you patch. =)
ah. Just did "svn patch" and left it at that, sorry :)
> 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?
I'd recommend "$Keyword:: " as a minimum, and leave all characters after
the initial space for filler. Any character after that initial space
should be accepted as filler material, because this means someone can
use semantics such as "$Date: NNNN-NN-NN NN:NN:NN.NNN $" (or whatever
the real date format is). Since we will always replace the filler with
the subversion information and pad out the rest with spaces (at least,
that's what the patch currently does), the exact contents shouldn't
matter too much.
My preference would be to amend #2095 to read 'spaces or other
characters following a single space after the "$Keyword::" tag', or
similar. The patch currently allows the behaviour as I've just described.
> 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 */
> {
But this expanded keyword check is in an "else" clause on the original
expanded-fixed-length-keyword test (if fixed {...} else if expanded
{...}), and also it is explicitly testing for "$Keyword: " -
single-colon, single-space sequence.
The fixed-size keyword code does not differentiate between original text
and "expanded" (substituted) text - the text will be replaced every time
no matter what. It seems a cleaner solution this way, rather than
handling fixed-length-keywords in the original "expanded" keyword
testing code. Granted, my comments in the code did not make this
particularly clear.
Let me know if I'm missing your point here.
> + /* $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:
Effectively: strlen("$:: $") = 5, where the first space is included, but
the second is not. This ensures that, no matter what goes between ":: "
and "$", it will end up replaced with the text, and leave us with " $"
at the end. I used the "it compiles therefore it works" programming
methodology, however.
> 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)
I'll double-check the way it handles strings shorter, equal and greater
in length with some test cases, but I might as well run through the
python tests for this (see below).
"It seemed to be doing the right thing", but hasn't been comprehensively
tested. I think this was my original test-case generation script:
C=0; while [ $C -lt 400 ]; do printf "$Id::%-${C}.-${C}s$\n";
C=`expr $C + 1`; done
> The rest of the issues are:
>
> 1) A pointer to the book's todo about this new feature
Didn't realise there was one - only prior reference I saw was an earlier
mailing list posting (someone asked for it, someone else expressed lack
of interest, that seemed to be about the extent of it though). Will dig
that one out and attach it to the issue.
> 2) Test cases for fixed length keywords
> http://svn.collab.net/repos/svn/trunk/subversion/tests/clients/cmdline/trans_tests.py
Interesting. This is classed as a "commandline client" test? Or does the
path refer to what *runs* the tests?
I have to admit that I'd only looked at the subversion code for the
first time ever about two hours before submitting the patch, and I
wasn't very careful about reading the HACKING document. My excuse was
that the feature was the decision point over whether our company used
subversion or something else (read: proprietary, expensive and IMHO not
as good)... so I wanted a quick fix :)
Anyhow, I'll provide a patch for the tests as well - need to learn
Python first, so it will take an hour or two.
> 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.
The second approach looks good. Not too bothered about truncation -
you've gotta expect that with a fixed size, and for my company's
purposes, it's not an issue, but if you'd like I could post a new
version of the patch with this implemented?
> Otherwise your patch looks good, and I like the general idea of fixed
> length keywords.
Thanks for taking the time to look into it.
TM
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Fri Oct 15 20:12:49 2004