[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: svn commit: r30743 - trunk/subversion/libsvn_client

From: Karl Fogel <kfogel_at_red-bean.com>
Date: Tue, 22 Apr 2008 10:14:33 -0400

"David Glasser" <glasser_at_davidglasser.net> writes:
>> + {
>> + svn_string_t *val = apr_hash_get(prop_hash, propname,
>> + if (val)
>> + val = svn_string_dup(val, perm_pool);
>> +
>> + apr_hash_set(props,
>> + svn_path_join(target_prefix, target_relative, perm_pool),
> Would it make sense to conditionalize the apr_hash_set on val as well,
> so that we don't end up allocating a path in perm_pool for every
> single path (even those without the prop)?

Yes -- indeed, I thought about that, and for reasons that I can no
longer even articulate to myself, decided not to do it.

Oh, I remember what it was: I thought that there might be some reason
why that key would *already* be in the hash, so if val were NULL (which
it could have been before this change too) that would cause that key to
be removed, and there might have been some reason why we were depending
on that.

But, on further consideration, the key can't already be there. So
conditionalizing the hash-set on val would only cause a behavior change
if there were a pre-existing bug in the code.

Thanks for the review, will fix.


To unsubscribe, e-mail: dev-unsubscribe_at_subversion.tigris.org
For additional commands, e-mail: dev-help_at_subversion.tigris.org
Received on 2008-04-22 16:41:23 CEST

This is an archived mail posted to the Subversion Dev mailing list.

This site is subject to the Apache Privacy Policy and the Apache Public Forum Archive Policy.