You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Daniel Shahaf <da...@elego.de> on 2012/09/07 21:36:32 UTC
Re: svn commit: r1381808 - in
/subversion/trunk/subversion/libsvn_fs_fs: fs_fs.c fs_fs.h
I think this fix is wrong. svn_fs_fs__read_noderev() should return the
parsed noderev as is --- if it's a revision file and the noderev
contains an is-fresh-txn-root field, then it needs to reflect that.
stefan2@apache.org wrote on Thu, Sep 06, 2012 at 23:52:17 -0000:
> Author: stefan2
> Date: Thu Sep 6 23:52:16 2012
> New Revision: 1381808
>
> URL: http://svn.apache.org/viewvc?rev=1381808&view=rev
> Log:
> Address issue #4031 by ignoring the is-fresh-txn-root flag when
> we read from committed revisions.
>
> Note: This does not fix the root cause but fixed svnadmin verify
> for existing repositories that contain empty revisions.
>
> * subversion/libsvn_fs_fs/fs_fs.h
> (svn_fs_fs__read_noderev): add allow_for_txn_roots parameter
> * subversion/libsvn_fs_fs/fs_fs.c
> (svn_fs_fs__read_noderev): adapt implementation
> (get_node_revision_body): adapt caller
>
> Modified:
> subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c
> subversion/trunk/subversion/libsvn_fs_fs/fs_fs.h
>
> Modified: subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c
> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c?rev=1381808&r1=1381807&r2=1381808&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c (original)
> +++ subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c Thu Sep 6 23:52:16 2012
> @@ -2284,6 +2284,7 @@ get_node_revision_body(node_revision_t *
> SVN_ERR(svn_fs_fs__read_noderev(noderev_p,
> svn_stream_from_aprfile2(revision_file, FALSE,
> pool),
> + svn_fs_fs__id_txn_id(id) != NULL,
> pool));
>
> /* The noderev is not in cache, yet. Add it, if caching has been enabled. */
> @@ -2293,6 +2294,7 @@ get_node_revision_body(node_revision_t *
> svn_error_t *
> svn_fs_fs__read_noderev(node_revision_t **noderev_p,
> svn_stream_t *stream,
> + svn_boolean_t allow_for_txn_roots,
> apr_pool_t *pool)
> {
> apr_hash_t *headers;
> @@ -2423,7 +2425,9 @@ svn_fs_fs__read_noderev(node_revision_t
> }
>
> /* Get whether this is a fresh txn root. */
> - value = apr_hash_get(headers, HEADER_FRESHTXNRT, APR_HASH_KEY_STRING);
> + value = allow_for_txn_roots
> + ? apr_hash_get(headers, HEADER_FRESHTXNRT, APR_HASH_KEY_STRING)
> + : NULL;
> noderev->is_fresh_txn_root = (value != NULL);
>
> /* Get the mergeinfo count. */
>
> Modified: subversion/trunk/subversion/libsvn_fs_fs/fs_fs.h
> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/fs_fs.h?rev=1381808&r1=1381807&r2=1381808&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/libsvn_fs_fs/fs_fs.h (original)
> +++ subversion/trunk/subversion/libsvn_fs_fs/fs_fs.h Thu Sep 6 23:52:16 2012
> @@ -94,11 +94,13 @@ svn_fs_fs__write_noderev(svn_stream_t *o
> apr_pool_t *pool);
>
> /* Read a node-revision from STREAM. Set *NODEREV to the new structure,
> - allocated in POOL. */
> + allocated in POOL. If ALLOW_FOR_TXN_ROOTS is FALSE, the is-fresh-txn-root
> + flag will be ignored. */
> /* ### Currently used only by fs_fs.c */
> svn_error_t *
> svn_fs_fs__read_noderev(node_revision_t **noderev,
> svn_stream_t *stream,
> + svn_boolean_t allow_for_txn_roots,
> apr_pool_t *pool);
>
>
>
>
Re: svn commit: r1381808 - in /subversion/trunk/subversion/libsvn_fs_fs:
fs_fs.c fs_fs.h
Posted by Stefan Fuhrmann <st...@wandisco.com>.
On Sat, Sep 8, 2012 at 7:41 PM, Daniel Shahaf <d....@daniel.shahaf.name>wrote:
> Stefan Fuhrmann wrote on Sat, Sep 08, 2012 at 01:32:54 +0200:
> > On Fri, Sep 7, 2012 at 9:36 PM, Daniel Shahaf <da...@elego.de> wrote:
> >
> > > I think this fix is wrong. svn_fs_fs__read_noderev() should return the
> > > parsed noderev as is --- if it's a revision file and the noderev
> > > contains an is-fresh-txn-root field, then it needs to reflect that.
> > >
> >
> > The problem is that the whole situation is very messy:
> >
> > * the flag basically says "this is still under construction,
> > redirect data lookup as required". For committed data,
> > this is clearly an invalid state or at least useless info.
> >
> > * there is some bug that will leave this flag in empty
> > revisions. This bug needs to be found an fixed (not
> > a big problem, just needs to be done).
> > * _existing_ repositories have cases of this otherwise
> > benign corruption.These cases need to be handled
> > gracefully. 1.8 checks for issue #4129 will report all
> > those repositories as corrupted.
> >
> > * the fix for issue #2608 makes svn_fs_fs__dag_get_revision
> > report the "wrong" revision.
> >
> > The only other way to make that work would be an
> > additional flag in noderev structure that allows for the
> > "is-fresh-txn-root" flag to be set but ignoring it in all
> > other code.
> >
> > Would you be more happy with that solution?
> >
>
> Well, I see two things here:
>
> - "It would be cleaner to have svn_fs_fs__read_noderev() a simple
> unserializer" --- done in r1382333.
>
Yup that plus your second commit looks like a better solution.
When I wrote the code, I assumed that the parser function
would be invoked from more places.
- "The users of is_fresh_txn_root should be refactored" --- I wonder if
> we couldn't just remove the need for the field altogether without much
> work; see my last comment on issue #4031 for details.
>
I don't know too much about the commit process. So, I have
to pass on that one for now.
> Sorry for the brevity yesterday, I think that commit review would have
> benefited from being written today rather than yesterday.
>
Well, I've been on working holidays. Your review didn't
trouble me too much ;)
-- Stefan^2.
> > --
> > *
> >
> > Join us this October at Subversion Live
> > 2012<http://www.wandisco.com/svn-live-2012>
> > for two days of best practice SVN training, networking, live demos,
> > committer meet and greet, and more! Space is limited, so get signed up
> > today<http://www.wandisco.com/svn-live-2012>
> > !
> > *
>
--
*
Join us this October at Subversion Live
2012<http://www.wandisco.com/svn-live-2012>
for two days of best practice SVN training, networking, live demos,
committer meet and greet, and more! Space is limited, so get signed up
today<http://www.wandisco.com/svn-live-2012>
!
*
Re: svn commit: r1381808 - in /subversion/trunk/subversion/libsvn_fs_fs:
fs_fs.c fs_fs.h
Posted by Stefan Fuhrmann <st...@wandisco.com>.
On Sat, Sep 8, 2012 at 7:41 PM, Daniel Shahaf <d....@daniel.shahaf.name>wrote:
> Stefan Fuhrmann wrote on Sat, Sep 08, 2012 at 01:32:54 +0200:
> > On Fri, Sep 7, 2012 at 9:36 PM, Daniel Shahaf <da...@elego.de> wrote:
> >
> > > I think this fix is wrong. svn_fs_fs__read_noderev() should return the
> > > parsed noderev as is --- if it's a revision file and the noderev
> > > contains an is-fresh-txn-root field, then it needs to reflect that.
> > >
> >
> > The problem is that the whole situation is very messy:
> >
> > * the flag basically says "this is still under construction,
> > redirect data lookup as required". For committed data,
> > this is clearly an invalid state or at least useless info.
> >
> > * there is some bug that will leave this flag in empty
> > revisions. This bug needs to be found an fixed (not
> > a big problem, just needs to be done).
> > * _existing_ repositories have cases of this otherwise
> > benign corruption.These cases need to be handled
> > gracefully. 1.8 checks for issue #4129 will report all
> > those repositories as corrupted.
> >
> > * the fix for issue #2608 makes svn_fs_fs__dag_get_revision
> > report the "wrong" revision.
> >
> > The only other way to make that work would be an
> > additional flag in noderev structure that allows for the
> > "is-fresh-txn-root" flag to be set but ignoring it in all
> > other code.
> >
> > Would you be more happy with that solution?
> >
>
> Well, I see two things here:
>
> - "It would be cleaner to have svn_fs_fs__read_noderev() a simple
> unserializer" --- done in r1382333.
>
Yup that plus your second commit looks like a better solution.
When I wrote the code, I assumed that the parser function
would be invoked from more places.
- "The users of is_fresh_txn_root should be refactored" --- I wonder if
> we couldn't just remove the need for the field altogether without much
> work; see my last comment on issue #4031 for details.
>
I don't know too much about the commit process. So, I have
to pass on that one for now.
> Sorry for the brevity yesterday, I think that commit review would have
> benefited from being written today rather than yesterday.
>
Well, I've been on working holidays. Your review didn't
trouble me too much ;)
-- Stefan^2.
> > --
> > *
> >
> > Join us this October at Subversion Live
> > 2012<http://www.wandisco.com/svn-live-2012>
> > for two days of best practice SVN training, networking, live demos,
> > committer meet and greet, and more! Space is limited, so get signed up
> > today<http://www.wandisco.com/svn-live-2012>
> > !
> > *
>
--
*
Join us this October at Subversion Live
2012<http://www.wandisco.com/svn-live-2012>
for two days of best practice SVN training, networking, live demos,
committer meet and greet, and more! Space is limited, so get signed up
today<http://www.wandisco.com/svn-live-2012>
!
*
Re: svn commit: r1381808 - in
/subversion/trunk/subversion/libsvn_fs_fs: fs_fs.c fs_fs.h
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Stefan Fuhrmann wrote on Sat, Sep 08, 2012 at 01:32:54 +0200:
> On Fri, Sep 7, 2012 at 9:36 PM, Daniel Shahaf <da...@elego.de> wrote:
>
> > I think this fix is wrong. svn_fs_fs__read_noderev() should return the
> > parsed noderev as is --- if it's a revision file and the noderev
> > contains an is-fresh-txn-root field, then it needs to reflect that.
> >
>
> The problem is that the whole situation is very messy:
>
> * the flag basically says "this is still under construction,
> redirect data lookup as required". For committed data,
> this is clearly an invalid state or at least useless info.
>
> * there is some bug that will leave this flag in empty
> revisions. This bug needs to be found an fixed (not
> a big problem, just needs to be done).
> * _existing_ repositories have cases of this otherwise
> benign corruption.These cases need to be handled
> gracefully. 1.8 checks for issue #4129 will report all
> those repositories as corrupted.
>
> * the fix for issue #2608 makes svn_fs_fs__dag_get_revision
> report the "wrong" revision.
>
> The only other way to make that work would be an
> additional flag in noderev structure that allows for the
> "is-fresh-txn-root" flag to be set but ignoring it in all
> other code.
>
> Would you be more happy with that solution?
>
Well, I see two things here:
- "It would be cleaner to have svn_fs_fs__read_noderev() a simple
unserializer" --- done in r1382333.
- "The users of is_fresh_txn_root should be refactored" --- I wonder if
we couldn't just remove the need for the field altogether without much
work; see my last comment on issue #4031 for details.
Sorry for the brevity yesterday, I think that commit review would have
benefited from being written today rather than yesterday.
> -- Stefan^2.
>
> --
> *
>
> Join us this October at Subversion Live
> 2012<http://www.wandisco.com/svn-live-2012>
> for two days of best practice SVN training, networking, live demos,
> committer meet and greet, and more! Space is limited, so get signed up
> today<http://www.wandisco.com/svn-live-2012>
> !
> *
Re: svn commit: r1381808 - in
/subversion/trunk/subversion/libsvn_fs_fs: fs_fs.c fs_fs.h
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Stefan Fuhrmann wrote on Sat, Sep 08, 2012 at 01:32:54 +0200:
> On Fri, Sep 7, 2012 at 9:36 PM, Daniel Shahaf <da...@elego.de> wrote:
>
> > I think this fix is wrong. svn_fs_fs__read_noderev() should return the
> > parsed noderev as is --- if it's a revision file and the noderev
> > contains an is-fresh-txn-root field, then it needs to reflect that.
> >
>
> The problem is that the whole situation is very messy:
>
> * the flag basically says "this is still under construction,
> redirect data lookup as required". For committed data,
> this is clearly an invalid state or at least useless info.
>
> * there is some bug that will leave this flag in empty
> revisions. This bug needs to be found an fixed (not
> a big problem, just needs to be done).
> * _existing_ repositories have cases of this otherwise
> benign corruption.These cases need to be handled
> gracefully. 1.8 checks for issue #4129 will report all
> those repositories as corrupted.
>
> * the fix for issue #2608 makes svn_fs_fs__dag_get_revision
> report the "wrong" revision.
>
> The only other way to make that work would be an
> additional flag in noderev structure that allows for the
> "is-fresh-txn-root" flag to be set but ignoring it in all
> other code.
>
> Would you be more happy with that solution?
>
Well, I see two things here:
- "It would be cleaner to have svn_fs_fs__read_noderev() a simple
unserializer" --- done in r1382333.
- "The users of is_fresh_txn_root should be refactored" --- I wonder if
we couldn't just remove the need for the field altogether without much
work; see my last comment on issue #4031 for details.
Sorry for the brevity yesterday, I think that commit review would have
benefited from being written today rather than yesterday.
> -- Stefan^2.
>
> --
> *
>
> Join us this October at Subversion Live
> 2012<http://www.wandisco.com/svn-live-2012>
> for two days of best practice SVN training, networking, live demos,
> committer meet and greet, and more! Space is limited, so get signed up
> today<http://www.wandisco.com/svn-live-2012>
> !
> *
Re: svn commit: r1381808 - in /subversion/trunk/subversion/libsvn_fs_fs:
fs_fs.c fs_fs.h
Posted by Stefan Fuhrmann <st...@wandisco.com>.
On Fri, Sep 7, 2012 at 9:36 PM, Daniel Shahaf <da...@elego.de> wrote:
> I think this fix is wrong. svn_fs_fs__read_noderev() should return the
> parsed noderev as is --- if it's a revision file and the noderev
> contains an is-fresh-txn-root field, then it needs to reflect that.
>
The problem is that the whole situation is very messy:
* the flag basically says "this is still under construction,
redirect data lookup as required". For committed data,
this is clearly an invalid state or at least useless info.
* there is some bug that will leave this flag in empty
revisions. This bug needs to be found an fixed (not
a big problem, just needs to be done).
* _existing_ repositories have cases of this otherwise
benign corruption.These cases need to be handled
gracefully. 1.8 checks for issue #4129 will report all
those repositories as corrupted.
* the fix for issue #2608 makes svn_fs_fs__dag_get_revision
report the "wrong" revision.
The only other way to make that work would be an
additional flag in noderev structure that allows for the
"is-fresh-txn-root" flag to be set but ignoring it in all
other code.
Would you be more happy with that solution?
-- Stefan^2.
--
*
Join us this October at Subversion Live
2012<http://www.wandisco.com/svn-live-2012>
for two days of best practice SVN training, networking, live demos,
committer meet and greet, and more! Space is limited, so get signed up
today<http://www.wandisco.com/svn-live-2012>
!
*