Daniel Rall wrote:
> On Wed, 11 Oct 2006, Kamesh Jayachandran wrote:
>
>
>> Daniel Rall wrote:
>>
>>> Thanks for the patch, Kamesh. I've made some changes to it and the
>>> change log message (attached), but I have some additional questions:
>>>
>>> Is it 1) appropriate and 2) necessary to push knowledge of skels
>>> corresponding to specific properties (e.g. SVN_FS_PROP_TXN_MERGEINFO)
>>> down into fs_skels.c (rather than knowledge which ends at the concept
>>> of "proplists", as on trunk)? To me, that file seems like a more
>>> generic utility which shouldn't contain such specific knowledge.
>>>
>> I think somehow we can't avoid that. Someway or the other parsing
>> code(which should sit inside the util/fs_skel.c) should know about
>> 'SVN_FS_PROP_TXN_MERGEINFO'. Or we should treat the value of
>> 'svn:txn-mergeinfo' as 'atom' and parse it outside?
>>
>
> I chatted with Mike Pilato about this a bit. If we're initially using
> sqlite for the merge info index, we should not be attempting to
> store/retrieve merge info properties as structured data! They should
> be stored simply as atoms.
>
>
> If we wanted to jump directly to storing merge info in BerkeleyDB,
> we'd need to replace all usage of the sqlite-based svn_fs_merge_info.h
> APIs in the BDB backend. While we continue prototyping an end-to-end
> Merge Tracking solution, I would prefer to avoid this redundancy for
> the sake of simplicity.
>
Fine, I can implement 'svn:txn-mergeinfo' as like every other property
and put a temporary parsing function to understand this 'atom' and give
it as 'apr_hash_t'.
>
> ...
>
>> I will go through you patch and get back.
>>
>
> The remaining tweaks were mostly stylistic.
>
> Along those lines, I question the safety of changing the signature of
> svn_fs_base__dag_commit_txn() to remove its pool argument. This
> change makes the assumption that anything allocated from TRAIL->pool
> has the same lifetime as TRAIL, which may not always be correct.
>
Just out of curiosity would like to know when trail->pool would have a
short life time than trail itself so that come call back routines can
cause memory corruption? Is ok till we have BDB backed mergeinfo?
> While it is clearly okay for the current caller, txn_body_commit()
> (which already passes in trail->pool), I hesitate to make this type of
> interface change for a "protected" API (it could later cause bugs).
> If the function was file-static (completely private), I would be in
> favor of this.
>
> Also, the interface into svn_fs_base__dag_*() seems to consistently
> use txn_id, rather than txn. You seem to have made a change to txn to
> accomodate svn_fs_merge_info__update_index(). Is this interface
> discrepancy really worth it?
>
Can I get svn_fs_txn_t* from txn_id?. I tried but got SIGABORT as we had
already opened a trail.
> I've tweaked the patch accordingly, and updated the doc string for
> svn_fs_base__dag_commit_txn() (which was not updated in the previous
> patches).
>
>
> I'm also seeing 'stat_tests.py 20' fail over ra_local under BDB on the
> merge-tracking branch. I have not confirmed whether that failure is
> related to these changes. (I seem to recall you mentioning this in
> one of our previous conversations, but don't recall exactly what'cha
> said.)
>
It fails for BDB alone in trunk too. (Cause seems to be 'svn status -u'
testcase expects update list in certain order which bdb does not.)
With regards
Kamesh Jayachandran
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Thu Oct 12 15:17:19 2006