You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Tom Molesworth <to...@molesworth.name> on 2004/10/10 19:24:07 UTC
Patch: Fixed length keyword string expansion
Just posted an issue containing a patch for fixed-size keyword expansion.
http://subversion.tigris.org/issues/show_bug.cgi?id=2095
In short: it allows the double-colon tag feature found in other version
control systems, providing the option of enforcing a fixed size on
keyword expansion. This is purely an enhancement; no originality
functionality should be affected as far as I'm aware.
Useful for LabView, as seen here:
http://zone.ni.com/devzone/conceptd.nsf/webmain/2DD6142187653F9A86256A960068BE61
Let me know if I'm doing this the wrong way or if it breaks anything.
cheers,
Tom.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Patch: Fixed length keyword string expansion
Posted by Ben Reser <be...@reser.org>.
On Sun, Oct 10, 2004 at 08:24:07PM +0100, Tom Molesworth wrote:
> Just posted an issue containing a patch for fixed-size keyword expansion.
>
> http://subversion.tigris.org/issues/show_bug.cgi?id=2095
>
> In short: it allows the double-colon tag feature found in other version
> control systems, providing the option of enforcing a fixed size on
> keyword expansion. This is purely an enhancement; no originality
> functionality should be affected as far as I'm aware.
>
> Useful for LabView, as seen here:
>
> http://zone.ni.com/devzone/conceptd.nsf/webmain/2DD6142187653F9A86256A960068BE61
>
> Let me know if I'm doing this the wrong way or if it breaks anything.
+0 from me (I haven't reviewed the patch) but this answers the question
of "How can we put keywords in binary files".
--
Ben Reser <be...@reser.org>
http://ben.reser.org
"Conscience is the inner voice which warns us somebody may be looking."
- H.L. Mencken
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Patch: Fixed length keyword string expansion
Posted by Jani Averbach <ja...@jaa.iki.fi>.
On 2004-10-14 10:14-0600, Jani Averbach wrote:
>
> 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.
And of course putting '$' immediately after value won't fly: if we
have a space in the value, and truncation happens just after space,
you can't tell if keyword is truncated or not.
So we should use some special character as marker:
1) if a keyword is untruncated, it will end ' $'
2) if the keyword is truncated, it will end '#$'
?
BR, Jani
--
Jani Averbach
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH + RFC]: Fixed length keywords
Posted by Tom Molesworth <to...@molesworth.name>.
hi there,
Jani Averbach wrote:
> On 2004-10-14 18:44+0100, Tom Molesworth wrote:
>>I'd recommend "$Keyword:: " as a minimum, and leave all characters after
>>the initial space for filler.
>
>
> I think we have a consensus, the syntax is following at the moment:
>
> "$keyword:: " <value> (" $" | "#$")
>
> where <value> is at least one character, value could contain anything
> except '$' chars, and "#$" indicates that the value is truncated.
Looks good!
>>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.
>
> Well, in fact it must be different, think about following recipe:
...
> RFC:
>
> If someone handles these keywords with old client, it is possible to
> inject to the server "dirty" keywords (keyword strings with value data).
> This causes that these fixed length keywords show up on diffs when
> they shouldn't. How bad is that? I think we can live with it, but
> could we?
Is anyone likely to be using this for text files? The primary reason for
implementing this feature (from my point of view, at least) is to allow
binary files to have version information - therefore "svn diff" is
pretty much a meaningless operation.
I do see your point, and perhaps it would make sense to add code to
handle this... but it would seem that, if the client does not support
the fixed-size keyword format, it will simply return whatever's been
written to the database. If we put in a patch to fill with spaces on
commit, this shouldn't be a problem... even without the patch, older
clients will blindly write this as a standard string without any
conversions, so after the first commit it wouldn't change anyway, thus
keeping the diffs clean. I must confess: I don't know where this code is
linked in (server or client, I'm assuming both?) so there's a good
chance I'm missing something here.
Either way, I don't think the old-client compatibility should be a
problem at all, and "svn diff" is an unlikely operation to be needed
with this format.
> I have planned to commit this sometime next week, if nobody objects.
All looks good, I'll apply your latest versions and run some tests on
the binary files I've been trying it with so far.
cheers,
Tom
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH + RFC]: Fixed length keywords
Posted by Greg Hudson <gh...@MIT.EDU>.
I have a nitpick:
+ if (! value)
[...]
+ else if (value && (max_value_len > 0))
[...]
+ else
+ {
+ /* if we are here, we have a logic error in the code */
+ assert(1 == 0);
+ }
This is silly for a variety of reasons:
* No space before the open paren.
* assert (0) would be more concise.
* Replacing the "else if (...) { ... }" with "else { assert
(max_value_len > 0); ... } would be more straightforward.
* Replacing the "else if (...) { ... }" with just "else { ... }" and
ditching the assertion altogether would be fine, since your effective
assertion is just mirroring the "(6 + keyword_len < *len)" clause in the
outer if block, since max_value_len is *len - (6 + keyword_len)".
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH + RFC]: Fixed length keywords
Posted by Greg Hudson <gh...@MIT.EDU>.
I have a nitpick:
+ if (! value)
[...]
+ else if (value && (max_value_len > 0))
[...]
+ else
+ {
+ /* if we are here, we have a logic error in the code */
+ assert(1 == 0);
+ }
This is silly for a variety of reasons:
* No space before the open paren.
* assert (0) would be more concise.
* Replacing the "else if (...) { ... }" with "else { assert
(max_value_len > 0); ... } would be more straightforward.
* Replacing the "else if (...) { ... }" with just "else { ... }" and
ditching the assertion altogether would be fine, since your effective
assertion is just mirroring the "(6 + keyword_len < *len)" clause in the
outer if block, since max_value_len is *len - (6 + keyword_len)".
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
[PATCH + RFC]: Fixed length keywords
Posted by Jani Averbach <ja...@jaa.iki.fi>.
On 2004-10-14 18:44+0100, Tom Molesworth wrote:
>
> I'd recommend "$Keyword:: " as a minimum, and leave all characters after
> the initial space for filler.
I think we have a consensus, the syntax is following at the moment:
"$keyword:: " <value> (" $" | "#$")
where <value> is at least one character, value could contain anything
except '$' chars, and "#$" indicates that the value is truncated.
> 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.
Well, in fact it must be different, think about following recipe:
echo '$Rev:: $' >> file.txt
svn ci -m 'rev 1' file.txt
echo 'a line' >> file.txt
svn ci -m 'rev 2' file.txt
svn diff -r 1:2 file.txt
You don't like to see keywords line in the diff, so they have to be
unexpanded. At the moment this means filling with spaces.
RFC:
If someone handles these keywords with old client, it is possible to
inject to the server "dirty" keywords (keyword strings with value data).
This causes that these fixed length keywords show up on diffs when
they shouldn't. How bad is that? I think we can live with it, but
could we?
I have planned to commit this sometime next week, if nobody objects.
BR, Jani
A Log entry for patch, patch is an attachment:
This patch implements fixed size keywords for Subversion, see issue
#2095 for more details.
Patch provided by Tom Molesworth <tom at molesworth name>, regression
tests by Jani Averbach <jaa at jaa iki fi>.
* subversion/libsvn_subr/subst.c
(translate_keyword_subst): Added fixed length keyword handling
* subversion/tests/clients/cmdline/diff_tests.py
(verify_exluded_output): New helper function
(diff_keywords): New test, check that keywords won't show up on
diff, and that they show when they should.
* subversion/tests/clients/cmdline/trans_tests.py
(keywords_from_birth): Added testing of fixed length keywords.
(setup_working_copy): Signature changed, it now takes second
argument: length of value field (value_len) for fixed keywords.
(check_keywords): New helper function
(fixed_length_keywords_path): New test path
* subversion/tests/clients/cmdline/svntest/main.py
(canonize_url): New helper function
* doc/book/TODO
Added entry for fixed length keywords.
--
Jani Averbach
Re: Patch: Fixed length keyword string expansion
Posted by Tom Molesworth <to...@molesworth.name>.
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
Re: Patch: Fixed length keyword string expansion
Posted by Jani Averbach <ja...@jaa.iki.fi>.
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