You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by John Peacock <jp...@rowman.com> on 2004/04/23 17:07:37 UTC
[PATCH] Phase 2 of Keywords as Hash
And here is the second part of the Keywords as Hash patch, making all other
Subversion libraries use the new libsvn_subst keyhash functions. No public API
interface is touched and all tests pass.
Just to restate the issues:
1) The existing keywords (typedef struct) is resistent to future expansion
2) plasma wrote a new implementation using hashes and a printf-like formatting
3) I brought that up to speed with the current code base, and additionally wrote
API compatibility layers so that this could be considered for 1.1.0
4) These two patches do not include any new behavior, but are merely precursors
to later keyword extensions
See Issue 890 for further details (including links to previous list discussion).
Log entry inline and patch attached.
John
==================================================================
Change all libraries to use new keyword hash functions from libsvn_subst
See issue #890 for details.
* subversion/libsvn_wc/merge.c
(svn_wc_merge): use key hashes, new svn_wc__get_keywords() parameters,
and svn_subst_copy_and_translate2()
* subversion/libsvn_wc/translate.h
(svn_wc__get_keywords): change signature to use keyword hash
* subversion/libsvn_wc/props.c
(validate_eol_prop_against_file):
(svn_wc_prop_set): use svn_subst_translate_stream2(), keyword hashes,
and new svn_wc__get_keywords()
* subversion/libsvn_wc/adm_crawler.c
(restore_file): use keyword hashes, svn_subst_copy_and_translate2(),
and new svn_wc__get_keywords()
* subversion/libsvn_wc/log.c
(file_xfer_under_path):
(install_committed_file): use keyword hashes, new svn_wc__get_keywords(),
and svn_subst_copy_and_translate2()
* subversion/libsvn_wc/adm_ops.c
(revert_admin_things): use keyword hashes, new svn_wc__get_keywords(),
and svn_subst_copy_and_translate2()
* subversion/libsvn_wc/translate.c
(svn_wc_translated_file): use keyword hashes, new svn_wc__get_keywords(),
and svn_subst_copy_and_translate2()
(svn_wc__get_keywords): change signature to use keyword hashes and
svn_subst_build_keywords2()
* subversion/libsvn_client/export.c
(copy_versioned_files): use keyword hashes, new svn_wc__get_keywords(),
and svn_subst_copy_and_translate2()
(struct file_baton): add props hash
(add_file): allocate storage for props hash
(change_file_prop): store properties in file_baton
* subversion/libsvn_client/cat.c
(svn_client_cat): use keyword hashes, svn_subst_build_keywords2(),
and svn_subst_translate_stream2()
* subversion/libsvn_client/commit.c
(send_file_contents): use keyword hashes, svn_subst_build_keywords2(),
and svn_subst_translate_stream2()
* subversion/tests/libsvn_wc/translate-test.c
(substitute_and_verify): use keyword hashes
--
John Peacock
Director of Information Research and Technology
Rowman & Littlefield Publishing Group
4501 Forbes Boulevard
Suite H
Lanham, MD 20706
301-459-3366 x.5010
fax 301-429-5748
Re: [PATCH] Phase 2 of Keywords as Hash
Posted by Garrett Rooney <ro...@electricjellyfish.net>.
John Peacock wrote:
> Garrett Rooney wrote:
>
>> At first glance this stuff seems good, I'm just curious why you're
>> typedefing your own name for apr_hash_t, since I don't recall any of
>> the other places in the codebase doing that. To me, the special
>> typename implies that it does somethign special, except it doesn't,
>> it's just a normal apr_hash_t...
>
>
> I'd be perfectly happy to regenerate sans the special typedef. In the
> earlier phases of this patch, it was actually replacing the
> svn_subst_keywords_t struct, which of course wasn't good for API
> compatibility.
Yeah, I think that would be better. No promise when I'll get a chance
to look at this, as I'm short on time lately, but I am paying attention,
and I do think this stuff is worthwhile, at least in that it gets us
closer to doing some of the more interesting things with properties that
I'm hoping for in the future.
-garrett
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Phase 2 of Keywords as Hash
Posted by John Peacock <jp...@rowman.com>.
Garrett Rooney wrote:
> At first glance this stuff seems good, I'm just curious why you're
> typedefing your own name for apr_hash_t, since I don't recall any of the
> other places in the codebase doing that. To me, the special typename
> implies that it does somethign special, except it doesn't, it's just a
> normal apr_hash_t...
I'd be perfectly happy to regenerate sans the special typedef. In the earlier
phases of this patch, it was actually replacing the svn_subst_keywords_t struct,
which of course wasn't good for API compatibility.
John
--
John Peacock
Director of Information Research and Technology
Rowman & Littlefield Publishing Group
4501 Forbes Boulevard
Suite H
Lanham, MD 20706
301-459-3366 x.5010
fax 301-429-5748
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Phase 2 of Keywords as Hash
Posted by Garrett Rooney <ro...@electricjellyfish.net>.
John Peacock wrote:
> And here is the second part of the Keywords as Hash patch, making all
> other Subversion libraries use the new libsvn_subst keyhash functions.
> No public API interface is touched and all tests pass.
>
> Just to restate the issues:
>
> 1) The existing keywords (typedef struct) is resistent to future expansion
>
> 2) plasma wrote a new implementation using hashes and a printf-like
> formatting
>
> 3) I brought that up to speed with the current code base, and
> additionally wrote API compatibility layers so that this could be
> considered for 1.1.0
>
> 4) These two patches do not include any new behavior, but are merely
> precursors to later keyword extensions
>
> See Issue 890 for further details (including links to previous list
> discussion). Log entry inline and patch attached.
At first glance this stuff seems good, I'm just curious why you're
typedefing your own name for apr_hash_t, since I don't recall any of the
other places in the codebase doing that. To me, the special typename
implies that it does somethign special, except it doesn't, it's just a
normal apr_hash_t...
-garrett
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
[PATCH] Phase 2 of Keywords as Hash - Take 2
Posted by John Peacock <jp...@rowman.com>.
And again, the Phase 2 patch without a custom typedef.
John
==================================================================
Change all libraries to use new keyword hash functions from libsvn_subst
See issue #890 for details.
* subversion/libsvn_wc/merge.c
(svn_wc_merge): use key hashes, new svn_wc__get_keywords() parameters,
and svn_subst_copy_and_translate2()
* subversion/libsvn_wc/translate.h
(svn_wc__get_keywords): change signature to use keyword hash
* subversion/libsvn_wc/props.c
(validate_eol_prop_against_file):
(svn_wc_prop_set): use svn_subst_translate_stream2(), keyword hashes,
and new svn_wc__get_keywords()
* subversion/libsvn_wc/adm_crawler.c
(restore_file): use keyword hashes, svn_subst_copy_and_translate2(),
and new svn_wc__get_keywords()
* subversion/libsvn_wc/log.c
(file_xfer_under_path):
(install_committed_file): use keyword hashes, new svn_wc__get_keywords(),
and svn_subst_copy_and_translate2()
* subversion/libsvn_wc/adm_ops.c
(revert_admin_things): use keyword hashes, new svn_wc__get_keywords(),
and svn_subst_copy_and_translate2()
* subversion/libsvn_wc/translate.c
(svn_wc_translated_file): use keyword hashes, new svn_wc__get_keywords(),
and svn_subst_copy_and_translate2()
(svn_wc__get_keywords): change signature to use keyword hashes and
svn_subst_build_keywords2()
* subversion/libsvn_client/export.c
(copy_versioned_files): use keyword hashes, new svn_wc__get_keywords(),
and svn_subst_copy_and_translate2()
(struct file_baton): add props hash
(add_file): allocate storage for props hash
(change_file_prop): store properties in file_baton
* subversion/libsvn_client/cat.c
(svn_client_cat): use keyword hashes, svn_subst_build_keywords2(),
and svn_subst_translate_stream2()
* subversion/libsvn_client/commit.c
(send_file_contents): use keyword hashes, svn_subst_build_keywords2(),
and svn_subst_translate_stream2()
* subversion/tests/libsvn_wc/translate-test.c
(substitute_and_verify): use keyword hashes
--
John Peacock
Director of Information Research and Technology
Rowman & Littlefield Publishing Group
4720 Boston Way
Lanham, MD 20706
301-459-3366 x.5010
fax 301-429-5747
Re: [PATCH] Phase 3 of Keywords as Hash - Properties as Keywords
Posted by Garrett Rooney <ro...@electricjellyfish.net>.
John Peacock wrote:
> Garrett Rooney wrote:
>
>> I'm not sure I like the idea of any and all properties all of a sudden
>> becoming things that can be expanded as keywords. It seems like it
>> will limit our ability to add new keywords in the future, since it
>> will break things for people who already use whatever name we decided
>> on as a property for some other use.
>
>
> I don't really expect anyone (besides me) to load this patch any time
> soon. Like I said in my intro, I think this is probably a 2.0.0 feature,
> so there is plenty of time to discuss whether/how it gets implemented.
I wouldn't necessarily say 2.0, but I do think it requires more design work.
>>
>> Perhaps the user should have to specify explicitly somewhere (in a
>> local config file now, server side config file eventually) what
>> properties are used as keywords? This would also allow the
>> possibility of specifying alternate names for existing keywords (i.e.
>> as the *BSD's use $FreeBSD$ or $NetBSD$ as alternative spellings for
>> $Id$).
>
>
> That was one of the original reasons why the original discussion came
> up. Unfortunately, it is dependent on server managed configuration
> files, which doesn't exist in Subversion at the present time. I agree
> that it is one of the more useful extensions that could be done.
Well, if you're willing to tackle that, I'm willing to review it ;-)
>>> + /* emit a warning if one has been created */
>>> + if (warn)
>>> + {
>>> + svn_handle_warning (stderr, warn);
>>> + }
>>
>>
>>
>> You can't just spew stuff to stderr inside library code. If we need a
>> way to handle a warning, you should add a callback function/baton
>> pair, so it can be called when warnings occur.
>
>
> Sorry, I thought that was what I _was_ doing by calling
> svn_handle_warning(). Care to point me to some code which does the sort
> of thing you are recommending?
I can't easily check at the moment, since svn.collab.net seems to be
down, but IIRC svn_handle_warning just prints to the filehandle (stderr
in this case) you pass it.
I don't know off the top of my head of any code that uses a
callback/baton pair for warnings (I imagine there is some... Actually,
I think the libsvn_fs stuff has a warning callback that gets called, you
might want to look there), but the idea is just to pass in a function
pointer and a void pointer to some user specified data as arguments to
the function in question, and the application gets to decide what to do
with the info you give it. In the case of the svn application it would
likely just call svn_handle_warning, but in other apps it might not.
>> I'm also not especially thrilled with the 'bail if it has a newline'
>> and 'cut it off at some arbitrary size' stuff... I suppose we can't
>> avoid the newline thing, but the arbitrary size really bothers me...
>
>
> The limits are there in the current codebase, check out this code from
> subversion/libsvn_subr/subst.c:translate_keyword_subst():
>
> ...
> /* "$keyword: value $" */
> if (vallen > (SVN_KEYWORD_MAX_LEN - 5))
> vallen = SVN_KEYWORD_MAX_LEN - 5;
> ...
>
> so the concept of truncating isn't something I came up with on my own. ;)
Interesting, I wasn't aware of that.
> I thought in both the newline and size cases, it was better to warn and
> continue than to throw an error. When I was testing this patch, I
> discovered that if I threw an error for either out-of-bounds conditions,
> it would cascade up 4 or 5 layers. For example, the code to expand
> keywords is called in libsvn_wc /after/ the file has been committed to
> the repository, so throwing an error leaves the WC in an inconsistent
> state.
Yeah, I agree that a warning is a good idea, we just need to find the
right way to do it. Perhaps a function pointer/baton pair for this
purpose could be added to the svn_client_ctx_t structure, so it could be
used in various places it is needed.
-garrett
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Phase 3 of Keywords as Hash - Properties as Keywords
Posted by John Peacock <jp...@rowman.com>.
Garrett Rooney wrote:
> I'm not sure I like the idea of any and all properties all of a sudden
> becoming things that can be expanded as keywords. It seems like it will
> limit our ability to add new keywords in the future, since it will break
> things for people who already use whatever name we decided on as a
> property for some other use.
I don't really expect anyone (besides me) to load this patch any time soon.
Like I said in my intro, I think this is probably a 2.0.0 feature, so there is
plenty of time to discuss whether/how it gets implemented.
I'm going to use it myself for one and only one purpose: embed the $VERSION in
my Perl modules, so for modules where the XS code (C library) changes but the
POD (doc) doesn't change, I can just increment the version in the properties for
the master file. I have a script sketched out which will be called 'release'
which will do all of the mechanical work of cutting a new CPAN release (so I
don't leave anything out again ;).
>
> Perhaps the user should have to specify explicitly somewhere (in a local
> config file now, server side config file eventually) what properties are
> used as keywords? This would also allow the possibility of specifying
> alternate names for existing keywords (i.e. as the *BSD's use $FreeBSD$
> or $NetBSD$ as alternative spellings for $Id$).
That was one of the original reasons why the original discussion came up.
Unfortunately, it is dependent on server managed configuration files, which
doesn't exist in Subversion at the present time. I agree that it is one of the
more useful extensions that could be done.
>> + /* emit a warning if one has been created */
>> + if (warn)
>> + {
>> + svn_handle_warning (stderr, warn);
>> + }
>
>
> You can't just spew stuff to stderr inside library code. If we need a
> way to handle a warning, you should add a callback function/baton pair,
> so it can be called when warnings occur.
Sorry, I thought that was what I _was_ doing by calling svn_handle_warning().
Care to point me to some code which does the sort of thing you are recommending?
> I'm also not especially thrilled with the 'bail if it has a newline' and
> 'cut it off at some arbitrary size' stuff... I suppose we can't avoid
> the newline thing, but the arbitrary size really bothers me...
The limits are there in the current codebase, check out this code from
subversion/libsvn_subr/subst.c:translate_keyword_subst():
...
/* "$keyword: value $" */
if (vallen > (SVN_KEYWORD_MAX_LEN - 5))
vallen = SVN_KEYWORD_MAX_LEN - 5;
...
so the concept of truncating isn't something I came up with on my own. ;)
I thought in both the newline and size cases, it was better to warn and continue
than to throw an error. When I was testing this patch, I discovered that if I
threw an error for either out-of-bounds conditions, it would cascade up 4 or 5
layers. For example, the code to expand keywords is called in libsvn_wc /after/
the file has been committed to the repository, so throwing an error leaves the
WC in an inconsistent state.
John
--
John Peacock
Director of Information Research and Technology
Rowman & Littlefield Publishing Group
4501 Forbes Boulevard
Suite H
Lanham, MD 20706
301-459-3366 x.5010
fax 301-429-5748
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Phase 3 of Keywords as Hash - Properties as Keywords
Posted by Garrett Rooney <ro...@electricjellyfish.net>.
John Peacock wrote:
> Finally, the addition of a new feature: Properties as Keywords. Permits
> user defined keywords, allowing properties to be embedded in source code
> files. While not as useful as it might be with inherited properties, it
> is still of some use, if only as a proof of concept for the keywords as
> hash rewrite. I'm not sure that this is a 1.1.0 candidate.
I'm not sure I like the idea of any and all properties all of a sudden
becoming things that can be expanded as keywords. It seems like it will
limit our ability to add new keywords in the future, since it will break
things for people who already use whatever name we decided on as a
property for some other use.
Perhaps the user should have to specify explicitly somewhere (in a local
config file now, server side config file eventually) what properties are
used as keywords? This would also allow the possibility of specifying
alternate names for existing keywords (i.e. as the *BSD's use $FreeBSD$
or $NetBSD$ as alternative spellings for $Id$).
Also some minor comments:
> Patch attached and log entry inline.
>
> John
>
> ==================================================================
> Custom user-defined keywords using properties
>
> * subversion/libsvn_subr/subst.c
> (svn_subst_build_keywords2): Add stanza to use properties as keywords
>
> * subversion/tests/clients/cmdline/prop_tests.py
> (keyword_props): new tests to exercise properties as keywords feature
>
>
> ------------------------------------------------------------------------
>
> Index: subversion/libsvn_subr/subst.c
> ==================================================================
> --- subversion/libsvn_subr/subst.c (revision 8737)
> +++ subversion/libsvn_subr/subst.c (local)
> @@ -429,6 +429,64 @@
> * placed here to check and see if one of the repository-defined
> * keywords matches
> */
> + else /* a keyword that doesn't match (may be a property) */
> + {
> + if (props)
> + {
> + svn_string_t *prop_val =
> + apr_hash_get(props, keyword, strlen(keyword));
> +
> + /* do no harm if they didn't set that property */
> + if (prop_val)
> + {
> + char *found_nl;
> + svn_error_t *warn = NULL;
> + svn_stringbuf_t *prop_buf =
> + svn_stringbuf_create_from_string (prop_val, pool);
> + apr_size_t last_char = prop_buf->len;
> +
> + /* make sure the property is only a single line */
> + if ((found_nl = strchr (prop_buf->data, '\n')) != NULL)
> + {
> + warn = svn_error_createf (SVN_ERR_BAD_PROP_KIND, NULL,
> + "Property used as keyword "
> + "contains newline: '%s'\n",
> + keyword);
> +
> + /* shorten the string to the newline character */
> + last_char = (apr_size_t)(found_nl - prop_buf->data);
> + }
> +
> + /* cannot have more than SVN_KEYWORD_MAX_LEN characters
> + * in a keyword value
> + */
> + if ( last_char > SVN_KEYWORD_MAX_LEN )
> + {
> + warn = svn_error_createf (SVN_ERR_BAD_PROP_KIND, NULL,
> + "Property used as keyword "
> + "contains too many characters:"
> + "'%s'\n", keyword);
> +
> + last_char = SVN_KEYWORD_MAX_LEN;
> + }
> +
> + /* emit a warning if one has been created */
> + if (warn)
> + {
> + svn_handle_warning (stderr, warn);
> + }
You can't just spew stuff to stderr inside library code. If we need a
way to handle a warning, you should add a callback function/baton pair,
so it can be called when warnings occur.
> + /* chop any extra characters off */
> + if (last_char < prop_buf->len)
> + svn_stringbuf_chop (prop_buf,
> + (prop_buf->len - last_char));
> +
> + apr_hash_set (kw, keyword,
> + APR_HASH_KEY_STRING,
> + svn_string_create_from_buf(prop_buf,pool));
> + }
> + }
> + }
I'm also not especially thrilled with the 'bail if it has a newline' and
'cut it off at some arbitrary size' stuff... I suppose we can't avoid
the newline thing, but the arbitrary size really bothers me...
-garrett
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
[PATCH] Phase 3 of Keywords as Hash - Properties as Keywords
Posted by John Peacock <jp...@rowman.com>.
Finally, the addition of a new feature: Properties as Keywords. Permits user
defined keywords, allowing properties to be embedded in source code files.
While not as useful as it might be with inherited properties, it is still of
some use, if only as a proof of concept for the keywords as hash rewrite. I'm
not sure that this is a 1.1.0 candidate.
Patch attached and log entry inline.
John
==================================================================
Custom user-defined keywords using properties
* subversion/libsvn_subr/subst.c
(svn_subst_build_keywords2): Add stanza to use properties as keywords
* subversion/tests/clients/cmdline/prop_tests.py
(keyword_props): new tests to exercise properties as keywords feature
--
John Peacock
Director of Information Research and Technology
Rowman & Littlefield Publishing Group
4501 Forbes Boulevard
Suite H
Lanham, MD 20706
301-459-3366 x.5010
fax 301-429-5748