You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Karl Fogel <kf...@newton.ch.collab.net> on 2001/11/06 16:01:28 UTC
property code cleanup
While reviewing Philip Martin's recent patch to svn_client_propget(),
I got a closer look at some of our recent property changes. The doc
string for svn_client_propget() left me a little lost as to the
precise form of the return-by-reference.
Eventually I was able to determine that it's using
`apr_table_entry_t'. But that's bad: it forces the property value to
be a `char *', meaning no prop values with nulls in them. We decided
long ago that property *names* didn't have to support binary strings,
but property values are Anything Goes.
Kevin Pilch-Bisson, I think this was your commit 290? Can you fix it
please? Suggest returning them in a hash, where the keys are `char *'
propnames and the values are `svn_string_t *'.
Thanks,
-K
/* Returns an apr_table_t of filenames and property values, in *PROPS,
allocated in POOL. If TARGET is a file or RECURSE is false, there will
be only a single element, with key TARGET, and value the value of PROPNAME
in TARGET. If recurse is true and TARGET is a directory, the *PROPS will
contain a list of node names and property values for TARGET and all of
its children. The nodenames will be of rooted from the same place as
TARGET. */
svn_error_t *
svn_client_propget (apr_table_t **props,
svn_stringbuf_t *propname,
svn_stringbuf_t *target,
svn_boolean_t recurse,
apr_pool_t *pool);
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: property code cleanup
Posted by Karl Fogel <kf...@newton.ch.collab.net>.
Kevin Pilch-Bisson <ke...@pilch-bisson.net> writes:
> On Tue, Nov 06, 2001 at 12:02:38PM -0600, Karl Fogel wrote:
> > Having returned values be dependent on cwd is very rare in Subversion.
> > The more usual behavior is like I described, or even better: the paths
> > should just include TARGET as prefix. That way, everything behaves
> > quite intuitively: for example, you ask for prop X on file F, and you
> > get back a hash with key F and some value.
>
> That is indeed what happens, just (obviously) can't think of an appropriate
> way to say that in a docstring.
May I pontificate some more? A general principle:
It doesn't matter how awkward the doc string gets, it *must* be
precise. Readable but ambiguous prose is okay for humans, but not
for programmers. :-)
That's not to say that style is unimportant -- if you can say it well,
by all means do so. But if not, you _still_ have to say it, even if
awkwardly. The most important thing is not to leave people guessing.
-K
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: property code cleanup
Posted by Kevin Pilch-Bisson <ke...@pilch-bisson.net>.
On Tue, Nov 06, 2001 at 12:02:38PM -0600, Karl Fogel wrote:
> Having returned values be dependent on cwd is very rare in Subversion.
> The more usual behavior is like I described, or even better: the paths
> should just include TARGET as prefix. That way, everything behaves
> quite intuitively: for example, you ask for prop X on file F, and you
> get back a hash with key F and some value.
That is indeed what happens, just (obviously) can't think of an appropriate
way to say that in a docstring.
>
> Or relative to TARGET, or whatever. The important thing is that
> TARGET determines what those returned paths look like, not some
> external factor. The external cwd factor may still determine how
> TARGET gets *interpreted*, but that's fine & standard practice. Think
> of `ls', for example:
>
> newton$ ls -l README
> -rw-r--r-- 1 kfogel users 2175 Oct 25 11:13 README
> newton$ ls -l ../subversion/README
> -rw-r--r-- 1 kfogel users 2175 Oct 25 11:13 ../subversion/README
> newton$ ls -l /home/kfogel/src/subversion/README
> -rw-r--r-- 1 kfogel users 2175 Oct 25 11:13 /home/kfogel/src/\
> subversion/README
>
> It's the same file every time, but `ls' returns its name according to
> how I asked for it. Subversion's libraries all work the same way.
>
Right, that's what I want too.
--
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Kevin Pilch-Bisson http://www.pilch-bisson.net
"Historically speaking, the presences of wheels in Unix
has never precluded their reinvention." - Larry Wall
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Re: property code cleanup
Posted by Karl Fogel <kf...@newton.ch.collab.net>.
Kevin Pilch-Bisson <ke...@pilch-bisson.net> writes:
> I can't think of anything. However, I do have a question. How are we going
> to deal with printing binary props? Currently we just printf the property
> value, which won't work for binary properties.
I don't know, but we don't need to decide now (there are lots of
things that need to happen for, say, i18n). The important thing is
that that problem remain restricted to client implementations --
internally, the libraries must support binary props.
> > /* Set *PROPS to a hash table whose keys are `char *' paths
> > * indicating the relative path from TARGET to a node on which
>
> Actually, the keys represent the relative path from where the command is
> called. The path starts as TARGET, and gets new filename or dirnames added
> to it as it recurses. Should I state:
>
> "whose keys are `char *' paths indicating the relative path, including TARGET,
> to a node on which property PROPNAME is set" ?
>
> Alternatively, the function could be modified to perform as your docstring
> suggests.
Having returned values be dependent on cwd is very rare in Subversion.
The more usual behavior is like I described, or even better: the paths
should just include TARGET as prefix. That way, everything behaves
quite intuitively: for example, you ask for prop X on file F, and you
get back a hash with key F and some value.
Or relative to TARGET, or whatever. The important thing is that
TARGET determines what those returned paths look like, not some
external factor. The external cwd factor may still determine how
TARGET gets *interpreted*, but that's fine & standard practice. Think
of `ls', for example:
newton$ ls -l README
-rw-r--r-- 1 kfogel users 2175 Oct 25 11:13 README
newton$ ls -l ../subversion/README
-rw-r--r-- 1 kfogel users 2175 Oct 25 11:13 ../subversion/README
newton$ ls -l /home/kfogel/src/subversion/README
-rw-r--r-- 1 kfogel users 2175 Oct 25 11:13 /home/kfogel/src/\
subversion/README
It's the same file every time, but `ls' returns its name according to
how I asked for it. Subversion's libraries all work the same way.
-K
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: property code cleanup
Posted by Kevin Pilch-Bisson <ke...@pilch-bisson.net>.
On Tue, Nov 06, 2001 at 11:27:43AM -0600, Karl Fogel wrote:
> Kevin Pilch-Bisson <ke...@pilch-bisson.net> writes:
> > I wasn't aware of that, and indeed was comtemplating using either a table
> > or a hash.
>
> Gotcha. Hmmm, if you can think of any appropriate place(s) where this
> important fact -- about prop values permitting binary -- should be
> stated, go ahead and add it there.
>
I can't think of anything. However, I do have a question. How are we going
to deal with printing binary props? Currently we just printf the property
value, which won't work for binary properties.
>
> > Thinking of a doc-string along the following lines:
> >
> > /* Returns an apr_hash_t where the key is a char * representing the path
> > from the pwd to the node on which property PROPNAME is set, and the key
> > is the value of PROPNAME on that node. If TARGET is a file or RECURSE is
> > false, there will be only a single element in *PROPS, with key TARGET->data.
> > */
> >
> > Comments?
>
> Sure. This is going to be fairly picky, so please keep in mind the
> wise words Jim Blandy said to me many years ago (before totally taking
> apart some Elisp code I'd written): "A sincere and thorough critique
> is the highest form of flattery."
Don't worry about my feelings. If I hadn't wanted criticism for the doc string,
I wouldn't have asked for comments. :) Thanks for being honest about it.
>
> First, put the doc string in the direct, active voice (see example
> below) :-). But more importantly, it needs to be much, much more
> precise. For example, `pwd' is a meaningless concept in this
> function, as in most of Subversion. What you really mean is that the
> keys are paths relative to TARGET (I think). And what types are the
> property values? It doesn't say. What does it do when the property
> is absent on a given entity? It doesn't say.
Good points, all.
>
> Here's a possible doc string; I think it's factually correct, but it
> may still have errors. Feel free to use it or just treat it as a
> model, as you find appropriate:
Excellent, and almost correct.
>
> /* Set *PROPS to a hash table whose keys are `char *' paths
> * indicating the relative path from TARGET to a node on which
Actually, the keys represent the relative path from where the command is
called. The path starts as TARGET, and gets new filename or dirnames added
to it as it recurses. Should I state:
"whose keys are `char *' paths indicating the relative path, including TARGET,
to a node on which property PROPNAME is set" ?
Alternatively, the function could be modified to perform as your docstring
suggests.
> * property PROPNAME is set, and whose values are `svn_string_t *'
> * representing the property value for PROPNAME at that path.
> * Allocate *PROPS, its keys, and its values in POOL.
> *
> * Don't store any path, not even TARGET, if it does not have a
> * property named PROPNAME.
> *
> * If TARGET is a file or RECURSE is false, *PROPS will have at most
> * one element.
> *
> * If error, don't touch *PROPS, otherwise *PROPS is a hash table
> * even if empty.
> */
>
> Note how certain specifities are deliberately *not* included:
>
> It doesn't say "Set *PROPS to an `apr_hash_t *'", because the
> prototype of the function determines that itself. A doc string should
> write out a C type only when the function prototype won't determine
> that type. When the prototype does determine, it would be redundant
> (and distracting) to use anything other than regular prose at that
> point in the documentation.
>
> It also avoids referring unnecessarily to inner members of arguments
> or return values (e.g., "TARGET->data"). Again, such precision is
> attention-getting, and therefore should only be used for things that
> actually deserve the extra attention, or which cannot be deduced any
> other way.
>
> Hope this helps.
Definitely.
>
> > I thought of having a hash of hashes (or table of hashes), similar
> > to proplist, but thought this would be redundant, since we are only
> > getting a single property.
>
> Agree, yeah. Go with the shallowest data type that does the job.
>
My only reason for thinking of the other approach is that given the amount
of code we have which creates a hash from propname->propval, I thought it might
be counter intuitive to have one function which returns a hash mapping
nodename->propval. All in all, though, it is the simplest approach, and with
a good docstring it shouldn't be confusing :)
--
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Kevin Pilch-Bisson http://www.pilch-bisson.net
"Historically speaking, the presences of wheels in Unix
has never precluded their reinvention." - Larry Wall
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Re: property code cleanup
Posted by Karl Fogel <kf...@newton.ch.collab.net>.
Kevin Pilch-Bisson <ke...@pilch-bisson.net> writes:
> I wasn't aware of that, and indeed was comtemplating using either a table
> or a hash.
Gotcha. Hmmm, if you can think of any appropriate place(s) where this
important fact -- about prop values permitting binary -- should be
stated, go ahead and add it there.
> Thinking of a doc-string along the following lines:
>
> /* Returns an apr_hash_t where the key is a char * representing the path
> from the pwd to the node on which property PROPNAME is set, and the key
> is the value of PROPNAME on that node. If TARGET is a file or RECURSE is
> false, there will be only a single element in *PROPS, with key TARGET->data.
> */
>
> Comments?
Sure. This is going to be fairly picky, so please keep in mind the
wise words Jim Blandy said to me many years ago (before totally taking
apart some Elisp code I'd written): "A sincere and thorough critique
is the highest form of flattery."
First, put the doc string in the direct, active voice (see example
below) :-). But more importantly, it needs to be much, much more
precise. For example, `pwd' is a meaningless concept in this
function, as in most of Subversion. What you really mean is that the
keys are paths relative to TARGET (I think). And what types are the
property values? It doesn't say. What does it do when the property
is absent on a given entity? It doesn't say.
Here's a possible doc string; I think it's factually correct, but it
may still have errors. Feel free to use it or just treat it as a
model, as you find appropriate:
/* Set *PROPS to a hash table whose keys are `char *' paths
* indicating the relative path from TARGET to a node on which
* property PROPNAME is set, and whose values are `svn_string_t *'
* representing the property value for PROPNAME at that path.
* Allocate *PROPS, its keys, and its values in POOL.
*
* Don't store any path, not even TARGET, if it does not have a
* property named PROPNAME.
*
* If TARGET is a file or RECURSE is false, *PROPS will have at most
* one element.
*
* If error, don't touch *PROPS, otherwise *PROPS is a hash table
* even if empty.
*/
Note how certain specifities are deliberately *not* included:
It doesn't say "Set *PROPS to an `apr_hash_t *'", because the
prototype of the function determines that itself. A doc string should
write out a C type only when the function prototype won't determine
that type. When the prototype does determine, it would be redundant
(and distracting) to use anything other than regular prose at that
point in the documentation.
It also avoids referring unnecessarily to inner members of arguments
or return values (e.g., "TARGET->data"). Again, such precision is
attention-getting, and therefore should only be used for things that
actually deserve the extra attention, or which cannot be deduced any
other way.
Hope this helps.
> I thought of having a hash of hashes (or table of hashes), similar
> to proplist, but thought this would be redundant, since we are only
> getting a single property.
Agree, yeah. Go with the shallowest data type that does the job.
-K
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: property code cleanup
Posted by Ben Collins-Sussman <su...@collab.net>.
Kevin Pilch-Bisson <ke...@pilch-bisson.net> writes:
> Comments?
>
> I thought of having a hash of hashes (or table of hashes), similar
> to proplist, but thought this would be redundant, since we are only
> getting a single property.
Or you could define a "property" struct:
{
const char *
const svn_string_t *
}
... and return an array_header_t of those. (I think that struct
already exists somewhere in libsvn_wc, not sure...)
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: property code cleanup
Posted by Kevin Pilch-Bisson <ke...@pilch-bisson.net>.
On Tue, Nov 06, 2001 at 10:01:28AM -0600, Karl Fogel wrote:
> While reviewing Philip Martin's recent patch to svn_client_propget(),
> I got a closer look at some of our recent property changes. The doc
> string for svn_client_propget() left me a little lost as to the
> precise form of the return-by-reference.
>
> Eventually I was able to determine that it's using
> `apr_table_entry_t'. But that's bad: it forces the property value to
> be a `char *', meaning no prop values with nulls in them. We decided
> long ago that property *names* didn't have to support binary strings,
> but property values are Anything Goes.
>
I wasn't aware of that, and indeed was comtemplating using either a table
or a hash.
>
> Kevin Pilch-Bisson, I think this was your commit 290? Can you fix it
> please? Suggest returning them in a hash, where the keys are `char *'
> propnames and the values are `svn_string_t *'.
>
No problem, especially since it seems my network is working again.
> Thanks,
> -K
>
> /* Returns an apr_table_t of filenames and property values, in *PROPS,
> allocated in POOL. If TARGET is a file or RECURSE is false, there will
> be only a single element, with key TARGET, and value the value of PROPNAME
> in TARGET. If recurse is true and TARGET is a directory, the *PROPS will
> contain a list of node names and property values for TARGET and all of
> its children. The nodenames will be of rooted from the same place as
> TARGET. */
Thinking of a doc-string along the following lines:
/* Returns an apr_hash_t where the key is a char * representing the path
from the pwd to the node on which property PROPNAME is set, and the key
is the value of PROPNAME on that node. If TARGET is a file or RECURSE is
false, there will be only a single element in *PROPS, with key TARGET->data.
*/
Comments?
I thought of having a hash of hashes (or table of hashes), similar to proplist,
but thought this would be redundant, since we are only getting a single
property.
> svn_error_t *
> svn_client_propget (apr_table_t **props,
> svn_stringbuf_t *propname,
> svn_stringbuf_t *target,
> svn_boolean_t recurse,
> apr_pool_t *pool);
>
--
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Kevin Pilch-Bisson http://www.pilch-bisson.net
"Historically speaking, the presences of wheels in Unix
has never precluded their reinvention." - Larry Wall
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Re: property code cleanup
Posted by Greg Stein <gs...@lyra.org>.
On Tue, Nov 06, 2001 at 02:06:35PM -0600, Karl Fogel wrote:
> Greg Stein <gs...@lyra.org> writes:
> > Quick comment: there is *no* reason to ever use apr_table_t in our code.
> >
> > We should always be using an apr_hash_t.
>
> Personally, I'm not going to remember this by rote, but I will
> remember it if you explain *why*. :-)
apr_table_t is a legacy type. Its basic characteristics are like a hash
table (mapping keys to values). However, the keys/values can *only* be
null-term strings. Multiple values are allowed, so it isn't *quite* a hash
table. The keys are case insensitive. And lastly, they are O(#keys) on most
operations.
In almost every case, when somebody uses an apr_table_t, they really meant
to use an apr_hash_t.
Cheers,
-g
--
Greg Stein, http://www.lyra.org/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: property code cleanup
Posted by Karl Fogel <kf...@newton.ch.collab.net>.
Greg Stein <gs...@lyra.org> writes:
> Quick comment: there is *no* reason to ever use apr_table_t in our code.
>
> We should always be using an apr_hash_t.
Personally, I'm not going to remember this by rote, but I will
remember it if you explain *why*. :-)
?,
-K
> On Tue, Nov 06, 2001 at 10:01:28AM -0600, Karl Fogel wrote:
> >...
> > /* Returns an apr_table_t of filenames and property values, in *PROPS,
> > allocated in POOL. If TARGET is a file or RECURSE is false, there will
> > be only a single element, with key TARGET, and value the value of PROPNAME
> > in TARGET. If recurse is true and TARGET is a directory, the *PROPS will
> > contain a list of node names and property values for TARGET and all of
> > its children. The nodenames will be of rooted from the same place as
> > TARGET. */
> > svn_error_t *
> > svn_client_propget (apr_table_t **props,
> > svn_stringbuf_t *propname,
> > svn_stringbuf_t *target,
> > svn_boolean_t recurse,
> > apr_pool_t *pool);
>
> --
> Greg Stein, http://www.lyra.org/
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
> For additional commands, e-mail: dev-help@subversion.tigris.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: property code cleanup
Posted by Greg Stein <gs...@lyra.org>.
Quick comment: there is *no* reason to ever use apr_table_t in our code.
We should always be using an apr_hash_t.
Cheers,
-g
On Tue, Nov 06, 2001 at 10:01:28AM -0600, Karl Fogel wrote:
>...
> /* Returns an apr_table_t of filenames and property values, in *PROPS,
> allocated in POOL. If TARGET is a file or RECURSE is false, there will
> be only a single element, with key TARGET, and value the value of PROPNAME
> in TARGET. If recurse is true and TARGET is a directory, the *PROPS will
> contain a list of node names and property values for TARGET and all of
> its children. The nodenames will be of rooted from the same place as
> TARGET. */
> svn_error_t *
> svn_client_propget (apr_table_t **props,
> svn_stringbuf_t *propname,
> svn_stringbuf_t *target,
> svn_boolean_t recurse,
> apr_pool_t *pool);
--
Greg Stein, http://www.lyra.org/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org