You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Malcolm Rowe <ma...@farside.org.uk> on 2006/09/19 10:09:50 UTC
Re: svn commit: r21540 - in trunk/subversion: libsvn_fs_fs tests/cmdline tests/libsvn_fs
Hi David,
Thanks for fixing this! Just two questions:
On Mon, Sep 18, 2006 at 02:31:25PM -0700, glasser@tigris.org wrote:
> svn_fs_fs__put_node_revision(svn_fs_t *fs,
> const svn_fs_id_t *id,
> node_revision_t *noderev,
> + svn_boolean_t fresh_txn_root,
> apr_pool_t *pool)
> {
> apr_file_t *noderev_file;
> const char *txn_id = svn_fs_fs__id_txn_id(id);
>
> + noderev->is_fresh_txn_root = fresh_txn_root;
> +
> if (! txn_id)
> return svn_error_create(SVN_ERR_FS_CORRUPT, NULL,
> _("Attempted to write to non-transaction"));
Just wondering, but is there any reason that you made this an argument
instead of setting the is_fresh_txn_root value at the callsites? It's a
little... odd to see us changing the noderev as part of writing it out.
Secondly, could you improve the documentation for
svn_fs_node_created_rev() so that we know what invariant we're depending
on?
Thanks,
Malcolm
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r21540 - in trunk/subversion: libsvn_fs_fs tests/cmdline tests/libsvn_fs
Posted by Malcolm Rowe <ma...@farside.org.uk>.
On Tue, Sep 19, 2006 at 11:04:45AM -0400, David Glasser wrote:
> On 9/19/06, Malcolm Rowe <ma...@farside.org.uk> wrote:
> >Oh, okay. But the same applies to all the other contents of the noderev
> >structure :-), doesn't it? I know I'm probably just being picky -
> >I'll make the change myself if you've no objections.
>
> Well, arguably no -- the other parts of the noderev structure need to
> be changed when their actual value changes, whereas this flag needs to
> be cleared whenever you make *any* change to the structure, which
> could happen in many places.
>
> I guess my point is, I'm not sure where I'd put documentation of the
> fact that the flag needs to be cleared on *any* modification, whereas
> this doesn't leave a chance to forget.
>
I've just deleted several paragraphs of analysis disagreeing with you.
You're right, this is the best way to deal with this.
The thing I was getting hung up on is that the noderevs aren't opaque
structures, so this action-at-a-distance is mildly disturbing. But on
the other hand, with one exception, we want to reset that value whenever
we write out the noderev, so I guess it does make sense, even though
we're effectively setting it in places that aren't really applicable,
like rep_write_contents_close(), which deals only with file nodes.
> >Sorry I wasn't clear. We're relying on a certain behaviour from
> >svn_fs_node_created_rev() that's not well documented, specifically that
> >it returns a real revision when called on the root of an unmodified
> >transaction. I was thinking that we should document that in the
> >doc-comments for that function.
>
> Yeah, sounds reasonable.
>
I added a comment in r21555.
Regards,
Malcolm
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r21540 - in trunk/subversion: libsvn_fs_fs tests/cmdline tests/libsvn_fs
Posted by David Glasser <gl...@mit.edu>.
On 9/19/06, Malcolm Rowe <ma...@farside.org.uk> wrote:
> Oh, okay. But the same applies to all the other contents of the noderev
> structure :-), doesn't it? I know I'm probably just being picky -
> I'll make the change myself if you've no objections.
Well, arguably no -- the other parts of the noderev structure need to
be changed when their actual value changes, whereas this flag needs to
be cleared whenever you make *any* change to the structure, which
could happen in many places.
I guess my point is, I'm not sure where I'd put documentation of the
fact that the flag needs to be cleared on *any* modification, whereas
this doesn't leave a chance to forget.
> Sorry I wasn't clear. We're relying on a certain behaviour from
> svn_fs_node_created_rev() that's not well documented, specifically that
> it returns a real revision when called on the root of an unmodified
> transaction. I was thinking that we should document that in the
> doc-comments for that function.
Yeah, sounds reasonable.
--dave
--
David Glasser | glasser@mit.edu | http://www.davidglasser.net/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r21540 - in trunk/subversion: libsvn_fs_fs tests/cmdline tests/libsvn_fs
Posted by Malcolm Rowe <ma...@farside.org.uk>.
On Tue, Sep 19, 2006 at 10:34:59AM -0400, David Glasser wrote:
> On 9/19/06, Malcolm Rowe <ma...@farside.org.uk> wrote:
> >Just wondering, but is there any reason that you made this an argument
> >instead of setting the is_fresh_txn_root value at the callsites? It's a
> >little... odd to see us changing the noderev as part of writing it out.
>
> Heh, I probably should have justified that.
>
> My feeling was, it would be a problem to forget to clear the flag at
> one of the svn_fs_fs__put_node_revision callsites. Forcing it to be an
> argument to the function meant there was no way somebody (myself
> during this patch or somebody else later) could forget to do so.
>
Oh, okay. But the same applies to all the other contents of the noderev
structure :-), doesn't it? I know I'm probably just being picky -
I'll make the change myself if you've no objections.
> >Secondly, could you improve the documentation for
> >svn_fs_node_created_rev() so that we know what invariant we're depending
> >on?
>
> I'm not quite sure what you're referring to here.
>
Sorry I wasn't clear. We're relying on a certain behaviour from
svn_fs_node_created_rev() that's not well documented, specifically that
it returns a real revision when called on the root of an unmodified
transaction. I was thinking that we should document that in the
doc-comments for that function.
Regards,
Malcolm
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r21540 - in trunk/subversion: libsvn_fs_fs tests/cmdline tests/libsvn_fs
Posted by David Glasser <gl...@mit.edu>.
On 9/19/06, Malcolm Rowe <ma...@farside.org.uk> wrote:
> Just wondering, but is there any reason that you made this an argument
> instead of setting the is_fresh_txn_root value at the callsites? It's a
> little... odd to see us changing the noderev as part of writing it out.
Heh, I probably should have justified that.
My feeling was, it would be a problem to forget to clear the flag at
one of the svn_fs_fs__put_node_revision callsites. Forcing it to be an
argument to the function meant there was no way somebody (myself
during this patch or somebody else later) could forget to do so.
> Secondly, could you improve the documentation for
> svn_fs_node_created_rev() so that we know what invariant we're depending
> on?
I'm not quite sure what you're referring to here.
--dave
--
David Glasser | glasser@mit.edu | http://www.davidglasser.net/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org