On 05.09.2012 15:24, Stefan Küng wrote: > On Wed, Sep 5, 2012 at 1:14 AM, Branko Čibej <br...@wandisco.com> wrote: >> On 04.09.2012 22:24, Stefan Küng wrote: >>> Attached is my first draft of the new svn_config_dup() API. >>> >>> * it does not return an svn_error_t but the duplicate hash directly. >>> The only API call that could return an error is svn_config_create() >>> but that API also returns SVN_NO_ERROR unconditionally (for now at >>> least). >>> * no error checking is done for now, will add some checks for failed >>> memory allocations later. >>> * hash keys are duplicated as well (allocated in the passed pool) >>> >>> Please have a look at this. I think it's correct, at least my first >>> tests show that it works and solves my problem in TSVN with multiple >>> threads as well. >>> >>> be back tomorrow... >> Index: subversion/include/svn_config.h >> =================================================================== >> --- subversion/include/svn_config.h (revision 1380737) >> +++ subversion/include/svn_config.h (working copy) >> @@ -626,6 +626,16 @@ >> const char *fname, >> apr_pool_t *pool); >> >> +/** Return a deep copy of the config hash. >> + * @note use this function instead of repeated calls >> + * to svn_config_get_config to avoid reading the config file >> + * over and over. >> + * >> + * @since New in 1.8. >> + */ >> +apr_hash_t * >> +svn_config_dup(apr_hash_t * config, apr_pool_t * pool); >> >> >> As I said, the return value must be a svn_config_t*. >> More comments below (I'll ignore coding style). > Why must it be an svn_config_t* ? > That makes no sense: we want to duplicate the config hash.
Ah, I begin to understand; you want to duplicate the result of svn_config_get_config, instead of the result of svn_config_create/svn_config_read. I tend to think these are two separate functions: svn_error_t *svn_config_dup(svn_config_t **cfgp, svn_config_t *src, apr_pool_t *pool); svn_error_t *svn_config_copy_config(apr_hash_t **cfg_hash, apr_hash_t *src_hash, apr_pool_t *pool); Sorry about the misunderstanding. > > And here; svn_config_set already has code to create option > > and section records, it only needs to be factored out. > Yes, but I don't like the idea of using svn_config_set to do this > because it does more than is necessary here. > We don't have to check whether to replace or add an entry because we > always add (we're copying). > > And the question is: do we want an exact copy or a new object, > initialized with the same data? I'm not suggesting you use svn_config_set, that would be wrong, because it fiddles with value expansions. I'm suggesting that you factor out the parts of svn_config_set that create section and options into separate static helper functions, so that the new code and svn_config_set can both use those. That's better than duplicating the code. -- Brane -- Certified & Supported Apache Subversion Downloads: http://www.wandisco.com/subversion/download
