Re: [PATCH][merge-tracking]sqlite mergeinfo indexing for bdb repositories

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.

...
> 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.
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?

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.)