[PATCH + RFC]: Fixed length keywords

[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 + RFC]: Fixed length keywords

[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

[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

[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