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 Näslund <da...@longitudo.com> on 2010/03/12 22:52:55 UTC
[PATCH] Follow-up to r922176 was:Re: svn commit: r922176 - in
/subversion/trunk/subversion: include/svn_wc.h
libsvn_wc/revision_status.c svnversion/main.c
On Fri, Mar 12, 2010 at 05:43:18AM -0500, Greg Stein wrote:
> On Fri, Mar 12, 2010 at 03:21, <da...@apache.org> wrote:
> >...
> > +++ subversion/trunk/subversion/libsvn_wc/revision_status.c Fri Mar 12 08:21:45 2010
> >...
> > �{
> > - �struct status_baton *sb = baton;
> > + �struct walk_baton *wb = baton;
> > + �svn_revnum_t changed_rev;
> > + �svn_revnum_t revision;
> > + �svn_depth_t depth;
> > + �svn_wc__db_status_t status;
> > + �svn_boolean_t wc_root;
> > + �svn_boolean_t switched;
>
> wc_root and switched can be moved into a tighter scope.
Fixed!
> > - �if (status->entry->depth != svn_depth_exclude)
> > + �/* Added files have a revision of no interest */
> > + �if (revision != SVN_INVALID_REVNUM)
> > � � {
> > - � � �sb->result->switched |= status->switched;
> > - � � �sb->result->modified |= (status->text_status != svn_wc_status_normal);
> > - � � �sb->result->modified |= (status->prop_status != svn_wc_status_normal
> > - � � � � � � � � � � � � � � � && status->prop_status != svn_wc_status_none);
> > + � � �svn_revnum_t item_rev = (wb->committed
> > + � � � � � � � � � � � � � � � ? changed_rev
> > + � � � � � � � � � � � � � � � : revision);
>
> I think this may introduce a bug. Depending on wb->committed, we look
> at different revision values. And it may be that REVISION is valid,
> but CHANGED_REV is not. I would suggest moving the assignment of
> ITEM_REV one block out, and using that in the primary if() test.
Fixed, although I must admit that I don't truly understand how
changed_rev and revision differs!
My change of the caller svnversion/main.c in r922176 caused a problem
when svnversion was invoked on a newly added path. It is under version
control but has no revision number. At the moment '-1' is returned for
such a path. I intend to fix that in a separate patch.
[[[
Follow-up to r922176. Fix that tree changes were not considered when
determining if the wc has modifications.
* subversion/libsvn_wc/revision_status.c
(analyze_status): Determine from status if a path has been added or
deleted. Do some optimizations to avoid having to do a text
comparison for determining if a wc has modifications.
Suggested by: gstein
rhuijben
Approved by: ?
]]]
Re: [PATCH] v2. Follow-up to r922176
Posted by Daniel Näslund <da...@longitudo.com>.
On Fri, Apr 02, 2010 at 10:22:56AM +0200, Daniel Näslund wrote:
> Hi Julian!
>
> On Wed, Mar 31, 2010 at 10:19:10AM +0100, Julian Foad wrote:
> > On Tue, 2010-03-30, Daniel Näslund wrote:
> > > Ping! This patch has not been revieved
> >
> > [...]
> > > > [[[
> > > > Follow-up to r922176. Fix that tree changes were not considered when
> > > > determining if the wc has modifications.
> > > >
> > > > * subversion/libsvn_wc/revision_status.c
> > > > (analyze_status): Determine from status if a path has been added or
> > > > deleted. Do some optimizations to avoid having to do a text
> > > > comparison for determining if a wc has modifications.
> > > >
> > > > Suggested by: gstein
> > > > rhuijben
> > > > Approved by: ?
> > > > ]]]
Committed in r930184 with a +1 from gstein on #svn-dev.
Re: [PATCH] v2. Follow-up to r922176
Posted by Daniel Näslund <da...@longitudo.com>.
On Fri, Apr 02, 2010 at 10:22:56AM +0200, Daniel Näslund wrote:
> Hi Julian!
>
> On Wed, Mar 31, 2010 at 10:19:10AM +0100, Julian Foad wrote:
> > On Tue, 2010-03-30, Daniel Näslund wrote:
> > > Ping! This patch has not been revieved
> >
> > [...]
> > > > [[[
> > > > Follow-up to r922176. Fix that tree changes were not considered when
> > > > determining if the wc has modifications.
> > > >
> > > > * subversion/libsvn_wc/revision_status.c
> > > > (analyze_status): Determine from status if a path has been added or
> > > > deleted. Do some optimizations to avoid having to do a text
> > > > comparison for determining if a wc has modifications.
> > > >
> > > > Suggested by: gstein
> > > > rhuijben
> > > > Approved by: ?
> > > > ]]]
Committed in r930184 with a +1 from gstein on #svn-dev.
Re: [PATCH] v2. Follow-up to r922176
Posted by Julian Foad <ju...@wandisco.com>.
On Fri, 2010-04-02, Daniel Näslund wrote:
> On Wed, Mar 31, 2010 at 10:19:10AM +0100, Julian Foad wrote:
> > On Tue, 2010-03-30, Daniel Näslund wrote:
> > > > /* An svn_wc__node_found_func_t callback function for analyzing the status
> > > > - * of nodes */
> > > > + * of nodes. Optimized to avoid text compares and unneccessary checks of
> > > > + * already set values. */
> >
> > Be more specific: Which nodes does it analyze? How does it return the
> > result? What kinds of status can it report? (A reference to somewhere
> > else is fine, if the details are already explained somewhere else.)
> > Then the "Optimised ..." sentence should make more sense.
>
> I've clarified exactly how the function is optimized and what parameters
> it takes. A doc comment should say what a func does, rather than how it
> does it. In that sence, my comment is a bit off. Still, someone reading
> a static func is surely going to look at the implementation as well.
When I asked for a more specific doc string, I was under the impression
that you had written this function originally (in an earlier patch). I
have just looked again and seen that you didn't, you just converted it
from a different older function. If I had noticed that originally, I
would not have asked you for so much.
The new version is better, thank you.
> > > > @@ -71,24 +71,36 @@
> > > > wb->result->sparse_checkout = TRUE;
> > > > return SVN_NO_ERROR;
> > > > }
> > > > + else if (status == svn_wc__db_status_not_present)
> > > > + {
> > > > + return SVN_NO_ERROR;
> > > > + }
> > > > + else if (status == svn_wc__db_status_added
> > > > + || status == svn_wc__db_status_obstructed_add
> > > > + || status == svn_wc__db_status_deleted
> > > > + || status == svn_wc__db_status_obstructed_delete)
> > > > + {
> > > > + wb->result->modified = TRUE;
> > > > + }
> >
> > (Minor nit: "else" is redundant after a "return". I don't particularly
> > mind, but somebody objected to them the other day.)
>
> I'm insisting on my if-else construction. Just a matter of personal
> preferences. I think the if-else construction ties the function that
> part together and makes it more readable.
Ah, I didn't remember that it was *you* having that discussion a week or
two ago, I just remembered that somebody said it to somebody. Yes,
that's fine.
> Can I get your +1 for this?
I see it's already committed, and yes, +1 from me too. Thanks.
- Julian
[PATCH] v2. Follow-up to r922176
Posted by Daniel Näslund <da...@longitudo.com>.
Hi Julian!
On Wed, Mar 31, 2010 at 10:19:10AM +0100, Julian Foad wrote:
> On Tue, 2010-03-30, Daniel N�slund wrote:
> > Ping! This patch has not been revieved
>
> [...]
> > > [[[
> > > Follow-up to r922176. Fix that tree changes were not considered when
> > > determining if the wc has modifications.
> > >
> > > * subversion/libsvn_wc/revision_status.c
> > > (analyze_status): Determine from status if a path has been added or
> > > deleted. Do some optimizations to avoid having to do a text
> > > comparison for determining if a wc has modifications.
> > >
> > > Suggested by: gstein
> > > rhuijben
> > > Approved by: ?
> > > ]]]
> >
> > > Index: subversion/libsvn_wc/revision_status.c
> > > ===================================================================
> > > --- subversion/libsvn_wc/revision_status.c (revision 922398)
> > > +++ subversion/libsvn_wc/revision_status.c (arbetskopia)
> > > @@ -41,7 +41,8 @@
> > > };
> > >
> > > /* An svn_wc__node_found_func_t callback function for analyzing the status
> > > - * of nodes */
> > > + * of nodes. Optimized to avoid text compares and unneccessary checks of
> > > + * already set values. */
>
> Be more specific: Which nodes does it analyze? How does it return the
> result? What kinds of status can it report? (A reference to somewhere
> else is fine, if the details are already explained somewhere else.)
> Then the "Optimised ..." sentence should make more sense.
I've clarified exactly how the function is optimized and what parameters
it takes. A doc comment should say what a func does, rather than how it
does it. In that sence, my comment is a bit off. Still, someone reading
a static func is surely going to look at the implementation as well.
> > > static svn_error_t *
> > > analyze_status(const char *local_abspath,
> > > void *baton,
> > > @@ -50,10 +51,9 @@
> > > struct walk_baton *wb = baton;
> > > svn_revnum_t changed_rev;
> > > svn_revnum_t revision;
> > > + svn_revnum_t item_rev;
> > > svn_depth_t depth;
> > > svn_wc__db_status_t status;
> > > - svn_boolean_t wc_root;
> > > - svn_boolean_t switched;
> > >
> > > SVN_ERR(svn_wc__db_read_info(&status, NULL, &revision, NULL,
> > > NULL, NULL, &changed_rev,
> > > @@ -71,24 +71,36 @@
> > > wb->result->sparse_checkout = TRUE;
> > > return SVN_NO_ERROR;
> > > }
> > > + else if (status == svn_wc__db_status_not_present)
> > > + {
> > > + return SVN_NO_ERROR;
> > > + }
> > > + else if (status == svn_wc__db_status_added
> > > + || status == svn_wc__db_status_obstructed_add
> > > + || status == svn_wc__db_status_deleted
> > > + || status == svn_wc__db_status_obstructed_delete)
> > > + {
> > > + wb->result->modified = TRUE;
> > > + }
>
> (Minor nit: "else" is redundant after a "return". I don't particularly
> mind, but somebody objected to them the other day.)
I'm insisting on my if-else construction. Just a matter of personal
preferences. I think the if-else construction ties the function that
part together and makes it more readable.
Can I get your +1 for this?
Daniel
[PATCH] v2. Follow-up to r922176
Posted by Daniel Näslund <da...@longitudo.com>.
Hi Julian!
On Wed, Mar 31, 2010 at 10:19:10AM +0100, Julian Foad wrote:
> On Tue, 2010-03-30, Daniel Näslund wrote:
> > Ping! This patch has not been revieved
>
> [...]
> > > [[[
> > > Follow-up to r922176. Fix that tree changes were not considered when
> > > determining if the wc has modifications.
> > >
> > > * subversion/libsvn_wc/revision_status.c
> > > (analyze_status): Determine from status if a path has been added or
> > > deleted. Do some optimizations to avoid having to do a text
> > > comparison for determining if a wc has modifications.
> > >
> > > Suggested by: gstein
> > > rhuijben
> > > Approved by: ?
> > > ]]]
> >
> > > Index: subversion/libsvn_wc/revision_status.c
> > > ===================================================================
> > > --- subversion/libsvn_wc/revision_status.c (revision 922398)
> > > +++ subversion/libsvn_wc/revision_status.c (arbetskopia)
> > > @@ -41,7 +41,8 @@
> > > };
> > >
> > > /* An svn_wc__node_found_func_t callback function for analyzing the status
> > > - * of nodes */
> > > + * of nodes. Optimized to avoid text compares and unneccessary checks of
> > > + * already set values. */
>
> Be more specific: Which nodes does it analyze? How does it return the
> result? What kinds of status can it report? (A reference to somewhere
> else is fine, if the details are already explained somewhere else.)
> Then the "Optimised ..." sentence should make more sense.
I've clarified exactly how the function is optimized and what parameters
it takes. A doc comment should say what a func does, rather than how it
does it. In that sence, my comment is a bit off. Still, someone reading
a static func is surely going to look at the implementation as well.
> > > static svn_error_t *
> > > analyze_status(const char *local_abspath,
> > > void *baton,
> > > @@ -50,10 +51,9 @@
> > > struct walk_baton *wb = baton;
> > > svn_revnum_t changed_rev;
> > > svn_revnum_t revision;
> > > + svn_revnum_t item_rev;
> > > svn_depth_t depth;
> > > svn_wc__db_status_t status;
> > > - svn_boolean_t wc_root;
> > > - svn_boolean_t switched;
> > >
> > > SVN_ERR(svn_wc__db_read_info(&status, NULL, &revision, NULL,
> > > NULL, NULL, &changed_rev,
> > > @@ -71,24 +71,36 @@
> > > wb->result->sparse_checkout = TRUE;
> > > return SVN_NO_ERROR;
> > > }
> > > + else if (status == svn_wc__db_status_not_present)
> > > + {
> > > + return SVN_NO_ERROR;
> > > + }
> > > + else if (status == svn_wc__db_status_added
> > > + || status == svn_wc__db_status_obstructed_add
> > > + || status == svn_wc__db_status_deleted
> > > + || status == svn_wc__db_status_obstructed_delete)
> > > + {
> > > + wb->result->modified = TRUE;
> > > + }
>
> (Minor nit: "else" is redundant after a "return". I don't particularly
> mind, but somebody objected to them the other day.)
I'm insisting on my if-else construction. Just a matter of personal
preferences. I think the if-else construction ties the function that
part together and makes it more readable.
Can I get your +1 for this?
Daniel
Re: [PATCH] Follow-up to r922176 was:Re: svn commit: r922176 - in
/subversion/trunk/subversion: include/svn_wc.h libsvn_wc/revision_status.c
svnversion/main.c
Posted by Julian Foad <ju...@wandisco.com>.
On Tue, 2010-03-30, Daniel Näslund wrote:
> Ping! This patch has not been revieved
[...]
> > [[[
> > Follow-up to r922176. Fix that tree changes were not considered when
> > determining if the wc has modifications.
> >
> > * subversion/libsvn_wc/revision_status.c
> > (analyze_status): Determine from status if a path has been added or
> > deleted. Do some optimizations to avoid having to do a text
> > comparison for determining if a wc has modifications.
> >
> > Suggested by: gstein
> > rhuijben
> > Approved by: ?
> > ]]]
>
> > Index: subversion/libsvn_wc/revision_status.c
> > ===================================================================
> > --- subversion/libsvn_wc/revision_status.c (revision 922398)
> > +++ subversion/libsvn_wc/revision_status.c (arbetskopia)
> > @@ -41,7 +41,8 @@
> > };
> >
> > /* An svn_wc__node_found_func_t callback function for analyzing the status
> > - * of nodes */
> > + * of nodes. Optimized to avoid text compares and unneccessary checks of
> > + * already set values. */
Be more specific: Which nodes does it analyze? How does it return the
result? What kinds of status can it report? (A reference to somewhere
else is fine, if the details are already explained somewhere else.)
Then the "Optimised ..." sentence should make more sense.
> > static svn_error_t *
> > analyze_status(const char *local_abspath,
> > void *baton,
> > @@ -50,10 +51,9 @@
> > struct walk_baton *wb = baton;
> > svn_revnum_t changed_rev;
> > svn_revnum_t revision;
> > + svn_revnum_t item_rev;
> > svn_depth_t depth;
> > svn_wc__db_status_t status;
> > - svn_boolean_t wc_root;
> > - svn_boolean_t switched;
> >
> > SVN_ERR(svn_wc__db_read_info(&status, NULL, &revision, NULL,
> > NULL, NULL, &changed_rev,
> > @@ -71,24 +71,36 @@
> > wb->result->sparse_checkout = TRUE;
> > return SVN_NO_ERROR;
> > }
> > + else if (status == svn_wc__db_status_not_present)
> > + {
> > + return SVN_NO_ERROR;
> > + }
> > + else if (status == svn_wc__db_status_added
> > + || status == svn_wc__db_status_obstructed_add
> > + || status == svn_wc__db_status_deleted
> > + || status == svn_wc__db_status_obstructed_delete)
> > + {
> > + wb->result->modified = TRUE;
> > + }
(Minor nit: "else" is redundant after a "return". I don't particularly
mind, but somebody objected to them the other day.)
The rest looks fine.
- Julian
Re: [PATCH] Follow-up to r922176 was:Re: svn commit: r922176 - in
/subversion/trunk/subversion: include/svn_wc.h libsvn_wc/revision_status.c
svnversion/main.c
Posted by Daniel Näslund <da...@longitudo.com>.
Ping! This patch has not been revieved
On Fri, Mar 12, 2010 at 10:52:55PM +0100, Daniel Näslund wrote:
> On Fri, Mar 12, 2010 at 05:43:18AM -0500, Greg Stein wrote:
> > On Fri, Mar 12, 2010 at 03:21, <da...@apache.org> wrote:
> > >...
> > > +++ subversion/trunk/subversion/libsvn_wc/revision_status.c Fri Mar 12 08:21:45 2010
> > >...
> > > {
> > > - struct status_baton *sb = baton;
> > > + struct walk_baton *wb = baton;
> > > + svn_revnum_t changed_rev;
> > > + svn_revnum_t revision;
> > > + svn_depth_t depth;
> > > + svn_wc__db_status_t status;
> > > + svn_boolean_t wc_root;
> > > + svn_boolean_t switched;
> >
> > wc_root and switched can be moved into a tighter scope.
>
> Fixed!
>
> > > - if (status->entry->depth != svn_depth_exclude)
> > > + /* Added files have a revision of no interest */
> > > + if (revision != SVN_INVALID_REVNUM)
> > > {
> > > - sb->result->switched |= status->switched;
> > > - sb->result->modified |= (status->text_status != svn_wc_status_normal);
> > > - sb->result->modified |= (status->prop_status != svn_wc_status_normal
> > > - && status->prop_status != svn_wc_status_none);
> > > + svn_revnum_t item_rev = (wb->committed
> > > + ? changed_rev
> > > + : revision);
> >
> > I think this may introduce a bug. Depending on wb->committed, we look
> > at different revision values. And it may be that REVISION is valid,
> > but CHANGED_REV is not. I would suggest moving the assignment of
> > ITEM_REV one block out, and using that in the primary if() test.
>
> Fixed, although I must admit that I don't truly understand how
> changed_rev and revision differs!
>
> My change of the caller svnversion/main.c in r922176 caused a problem
> when svnversion was invoked on a newly added path. It is under version
> control but has no revision number. At the moment '-1' is returned for
> such a path. I intend to fix that in a separate patch.
>
> [[[
> Follow-up to r922176. Fix that tree changes were not considered when
> determining if the wc has modifications.
>
> * subversion/libsvn_wc/revision_status.c
> (analyze_status): Determine from status if a path has been added or
> deleted. Do some optimizations to avoid having to do a text
> comparison for determining if a wc has modifications.
>
> Suggested by: gstein
> rhuijben
> Approved by: ?
> ]]]
> Index: subversion/libsvn_wc/revision_status.c
> ===================================================================
> --- subversion/libsvn_wc/revision_status.c (revision 922398)
> +++ subversion/libsvn_wc/revision_status.c (arbetskopia)
> @@ -41,7 +41,8 @@
> };
>
> /* An svn_wc__node_found_func_t callback function for analyzing the status
> - * of nodes */
> + * of nodes. Optimized to avoid text compares and unneccessary checks of
> + * already set values. */
> static svn_error_t *
> analyze_status(const char *local_abspath,
> void *baton,
> @@ -50,10 +51,9 @@
> struct walk_baton *wb = baton;
> svn_revnum_t changed_rev;
> svn_revnum_t revision;
> + svn_revnum_t item_rev;
> svn_depth_t depth;
> svn_wc__db_status_t status;
> - svn_boolean_t wc_root;
> - svn_boolean_t switched;
>
> SVN_ERR(svn_wc__db_read_info(&status, NULL, &revision, NULL,
> NULL, NULL, &changed_rev,
> @@ -71,24 +71,36 @@
> wb->result->sparse_checkout = TRUE;
> return SVN_NO_ERROR;
> }
> + else if (status == svn_wc__db_status_not_present)
> + {
> + return SVN_NO_ERROR;
> + }
> + else if (status == svn_wc__db_status_added
> + || status == svn_wc__db_status_obstructed_add
> + || status == svn_wc__db_status_deleted
> + || status == svn_wc__db_status_obstructed_delete)
> + {
> + wb->result->modified = TRUE;
> + }
>
> - if (status == svn_wc__db_status_not_present)
> - return SVN_NO_ERROR;
> -
> if (! wb->result->switched)
> {
> + svn_boolean_t wc_root;
> + svn_boolean_t switched;
> +
> SVN_ERR(svn_wc__check_wc_root(&wc_root, NULL, &switched, wb->db,
> local_abspath, scratch_pool));
>
> wb->result->switched |= switched;
> }
>
> + item_rev = (wb->committed
> + ? changed_rev
> + : revision);
> +
> /* Added files have a revision of no interest */
> - if (revision != SVN_INVALID_REVNUM)
> + if (item_rev != SVN_INVALID_REVNUM)
> {
> - svn_revnum_t item_rev = (wb->committed
> - ? changed_rev
> - : revision);
>
> if (wb->result->min_rev == SVN_INVALID_REVNUM
> || item_rev < wb->result->min_rev)
> @@ -101,22 +113,27 @@
>
> if (! wb->result->modified)
> {
> - svn_boolean_t text_mod;
> svn_boolean_t props_mod;
>
> SVN_ERR(svn_wc__props_modified(&props_mod, wb->db, local_abspath,
> scratch_pool));
> + wb->result->modified |= props_mod;
> + }
>
> + if (! wb->result->modified)
> + {
> + svn_boolean_t text_mod;
> +
> SVN_ERR(svn_wc__internal_text_modified_p(&text_mod, wb->db,
> local_abspath,
> FALSE,
> TRUE,
> scratch_pool));
> - wb->result->modified |= (text_mod || props_mod);
> + wb->result->modified |= text_mod;
> }
>
> - wb->result->sparse_checkout |= ((depth != svn_depth_infinity
> - && depth != svn_depth_unknown));
> + wb->result->sparse_checkout |= (depth != svn_depth_infinity
> + && depth != svn_depth_unknown);
> return SVN_NO_ERROR;
> }
>
Re: [PATCH] Follow-up to r922176 was:Re: svn commit: r922176 - in
/subversion/trunk/subversion: include/svn_wc.h
libsvn_wc/revision_status.c svnversion/main.c
Posted by Daniel Näslund <da...@longitudo.com>.
On Fri, Mar 12, 2010 at 07:42:53PM -0600, Hyrum K. Wright wrote:
> On Mar 12, 2010, at 3:52 PM, Daniel Näslund wrote:
> >>> - if (status->entry->depth != svn_depth_exclude)
> >>> + /* Added files have a revision of no interest */
> >>> + if (revision != SVN_INVALID_REVNUM)
> >>> {
> >>> - sb->result->switched |= status->switched;
> >>> - sb->result->modified |= (status->text_status != svn_wc_status_normal);
> >>> - sb->result->modified |= (status->prop_status != svn_wc_status_normal
> >>> - && status->prop_status != svn_wc_status_none);
> >>> + svn_revnum_t item_rev = (wb->committed
> >>> + ? changed_rev
> >>> + : revision);
> >>
> >> I think this may introduce a bug. Depending on wb->committed, we look
> >> at different revision values. And it may be that REVISION is valid,
> >> but CHANGED_REV is not. I would suggest moving the assignment of
> >> ITEM_REV one block out, and using that in the primary if() test.
> >
> > Fixed, although I must admit that I don't truly understand how
> > changed_rev and revision differs!
>
> 'Changed rev' is the last time the object was modified in the repo.
> 'Revision' refers to which revision the working copy thinks this
> object is up-to-date to.
>
> For instance, if I change foo.c in r10, and then later update to r12,
> if foo.c wasn't modified by the update, the last changed revision
> would still be r10, but revision would be r12. You've every right to
> be confused, btw, since in wc-1 entry->revision is overloaded about 4
> different ways.
Ah, it was that simple. I got a bit twisted up when reading
wc-metadata.sql and forgot the big picture. I was sure there was
something not-obvious to it. Glad it wasn't.
Thanks,
Daniel
Re: [PATCH] Follow-up to r922176 was:Re: svn commit: r922176 - in
/subversion/trunk/subversion: include/svn_wc.h
libsvn_wc/revision_status.c svnversion/main.c
Posted by Daniel Näslund <da...@longitudo.com>.
On Fri, Mar 12, 2010 at 07:42:53PM -0600, Hyrum K. Wright wrote:
> On Mar 12, 2010, at 3:52 PM, Daniel Näslund wrote:
> >>> - if (status->entry->depth != svn_depth_exclude)
> >>> + /* Added files have a revision of no interest */
> >>> + if (revision != SVN_INVALID_REVNUM)
> >>> {
> >>> - sb->result->switched |= status->switched;
> >>> - sb->result->modified |= (status->text_status != svn_wc_status_normal);
> >>> - sb->result->modified |= (status->prop_status != svn_wc_status_normal
> >>> - && status->prop_status != svn_wc_status_none);
> >>> + svn_revnum_t item_rev = (wb->committed
> >>> + ? changed_rev
> >>> + : revision);
> >>
> >> I think this may introduce a bug. Depending on wb->committed, we look
> >> at different revision values. And it may be that REVISION is valid,
> >> but CHANGED_REV is not. I would suggest moving the assignment of
> >> ITEM_REV one block out, and using that in the primary if() test.
> >
> > Fixed, although I must admit that I don't truly understand how
> > changed_rev and revision differs!
>
> 'Changed rev' is the last time the object was modified in the repo.
> 'Revision' refers to which revision the working copy thinks this
> object is up-to-date to.
>
> For instance, if I change foo.c in r10, and then later update to r12,
> if foo.c wasn't modified by the update, the last changed revision
> would still be r10, but revision would be r12. You've every right to
> be confused, btw, since in wc-1 entry->revision is overloaded about 4
> different ways.
Ah, it was that simple. I got a bit twisted up when reading
wc-metadata.sql and forgot the big picture. I was sure there was
something not-obvious to it. Glad it wasn't.
Thanks,
Daniel
Re: [PATCH] Follow-up to r922176 was:Re: svn commit: r922176 - in /subversion/trunk/subversion: include/svn_wc.h libsvn_wc/revision_status.c svnversion/main.c
Posted by "Hyrum K. Wright" <hy...@mail.utexas.edu>.
On Mar 12, 2010, at 3:52 PM, Daniel Näslund wrote:
> On Fri, Mar 12, 2010 at 05:43:18AM -0500, Greg Stein wrote:
>> On Fri, Mar 12, 2010 at 03:21, <da...@apache.org> wrote:
>>> ...
>>> +++ subversion/trunk/subversion/libsvn_wc/revision_status.c Fri Mar 12 08:21:45 2010
>>> ...
>>> {
>>> - struct status_baton *sb = baton;
>>> + struct walk_baton *wb = baton;
>>> + svn_revnum_t changed_rev;
>>> + svn_revnum_t revision;
>>> + svn_depth_t depth;
>>> + svn_wc__db_status_t status;
>>> + svn_boolean_t wc_root;
>>> + svn_boolean_t switched;
>>
>> wc_root and switched can be moved into a tighter scope.
>
> Fixed!
>
>>> - if (status->entry->depth != svn_depth_exclude)
>>> + /* Added files have a revision of no interest */
>>> + if (revision != SVN_INVALID_REVNUM)
>>> {
>>> - sb->result->switched |= status->switched;
>>> - sb->result->modified |= (status->text_status != svn_wc_status_normal);
>>> - sb->result->modified |= (status->prop_status != svn_wc_status_normal
>>> - && status->prop_status != svn_wc_status_none);
>>> + svn_revnum_t item_rev = (wb->committed
>>> + ? changed_rev
>>> + : revision);
>>
>> I think this may introduce a bug. Depending on wb->committed, we look
>> at different revision values. And it may be that REVISION is valid,
>> but CHANGED_REV is not. I would suggest moving the assignment of
>> ITEM_REV one block out, and using that in the primary if() test.
>
> Fixed, although I must admit that I don't truly understand how
> changed_rev and revision differs!
'Changed rev' is the last time the object was modified in the repo. 'Revision' refers to which revision the working copy thinks this object is up-to-date to.
For instance, if I change foo.c in r10, and then later update to r12, if foo.c wasn't modified by the update, the last changed revision would still be r10, but revision would be r12. You've every right to be confused, btw, since in wc-1 entry->revision is overloaded about 4 different ways.
> My change of the caller svnversion/main.c in r922176 caused a problem
> when svnversion was invoked on a newly added path. It is under version
> control but has no revision number. At the moment '-1' is returned for
> such a path. I intend to fix that in a separate patch.
>
> [[[
> Follow-up to r922176. Fix that tree changes were not considered when
> determining if the wc has modifications.
>
> * subversion/libsvn_wc/revision_status.c
> (analyze_status): Determine from status if a path has been added or
> deleted. Do some optimizations to avoid having to do a text
> comparison for determining if a wc has modifications.
>
> Suggested by: gstein
> rhuijben
> Approved by: ?
> ]]]
> <revision_status_follow_up.diff.txt>
Re: [PATCH] Follow-up to r922176 was:Re: svn commit: r922176 - in
/subversion/trunk/subversion: include/svn_wc.h libsvn_wc/revision_status.c
svnversion/main.c
Posted by Daniel Näslund <da...@longitudo.com>.
Ping! This patch has not been revieved
On Fri, Mar 12, 2010 at 10:52:55PM +0100, Daniel Näslund wrote:
> On Fri, Mar 12, 2010 at 05:43:18AM -0500, Greg Stein wrote:
> > On Fri, Mar 12, 2010 at 03:21, <da...@apache.org> wrote:
> > >...
> > > +++ subversion/trunk/subversion/libsvn_wc/revision_status.c Fri Mar 12 08:21:45 2010
> > >...
> > > {
> > > - struct status_baton *sb = baton;
> > > + struct walk_baton *wb = baton;
> > > + svn_revnum_t changed_rev;
> > > + svn_revnum_t revision;
> > > + svn_depth_t depth;
> > > + svn_wc__db_status_t status;
> > > + svn_boolean_t wc_root;
> > > + svn_boolean_t switched;
> >
> > wc_root and switched can be moved into a tighter scope.
>
> Fixed!
>
> > > - if (status->entry->depth != svn_depth_exclude)
> > > + /* Added files have a revision of no interest */
> > > + if (revision != SVN_INVALID_REVNUM)
> > > {
> > > - sb->result->switched |= status->switched;
> > > - sb->result->modified |= (status->text_status != svn_wc_status_normal);
> > > - sb->result->modified |= (status->prop_status != svn_wc_status_normal
> > > - && status->prop_status != svn_wc_status_none);
> > > + svn_revnum_t item_rev = (wb->committed
> > > + ? changed_rev
> > > + : revision);
> >
> > I think this may introduce a bug. Depending on wb->committed, we look
> > at different revision values. And it may be that REVISION is valid,
> > but CHANGED_REV is not. I would suggest moving the assignment of
> > ITEM_REV one block out, and using that in the primary if() test.
>
> Fixed, although I must admit that I don't truly understand how
> changed_rev and revision differs!
>
> My change of the caller svnversion/main.c in r922176 caused a problem
> when svnversion was invoked on a newly added path. It is under version
> control but has no revision number. At the moment '-1' is returned for
> such a path. I intend to fix that in a separate patch.
>
> [[[
> Follow-up to r922176. Fix that tree changes were not considered when
> determining if the wc has modifications.
>
> * subversion/libsvn_wc/revision_status.c
> (analyze_status): Determine from status if a path has been added or
> deleted. Do some optimizations to avoid having to do a text
> comparison for determining if a wc has modifications.
>
> Suggested by: gstein
> rhuijben
> Approved by: ?
> ]]]
> Index: subversion/libsvn_wc/revision_status.c
> ===================================================================
> --- subversion/libsvn_wc/revision_status.c (revision 922398)
> +++ subversion/libsvn_wc/revision_status.c (arbetskopia)
> @@ -41,7 +41,8 @@
> };
>
> /* An svn_wc__node_found_func_t callback function for analyzing the status
> - * of nodes */
> + * of nodes. Optimized to avoid text compares and unneccessary checks of
> + * already set values. */
> static svn_error_t *
> analyze_status(const char *local_abspath,
> void *baton,
> @@ -50,10 +51,9 @@
> struct walk_baton *wb = baton;
> svn_revnum_t changed_rev;
> svn_revnum_t revision;
> + svn_revnum_t item_rev;
> svn_depth_t depth;
> svn_wc__db_status_t status;
> - svn_boolean_t wc_root;
> - svn_boolean_t switched;
>
> SVN_ERR(svn_wc__db_read_info(&status, NULL, &revision, NULL,
> NULL, NULL, &changed_rev,
> @@ -71,24 +71,36 @@
> wb->result->sparse_checkout = TRUE;
> return SVN_NO_ERROR;
> }
> + else if (status == svn_wc__db_status_not_present)
> + {
> + return SVN_NO_ERROR;
> + }
> + else if (status == svn_wc__db_status_added
> + || status == svn_wc__db_status_obstructed_add
> + || status == svn_wc__db_status_deleted
> + || status == svn_wc__db_status_obstructed_delete)
> + {
> + wb->result->modified = TRUE;
> + }
>
> - if (status == svn_wc__db_status_not_present)
> - return SVN_NO_ERROR;
> -
> if (! wb->result->switched)
> {
> + svn_boolean_t wc_root;
> + svn_boolean_t switched;
> +
> SVN_ERR(svn_wc__check_wc_root(&wc_root, NULL, &switched, wb->db,
> local_abspath, scratch_pool));
>
> wb->result->switched |= switched;
> }
>
> + item_rev = (wb->committed
> + ? changed_rev
> + : revision);
> +
> /* Added files have a revision of no interest */
> - if (revision != SVN_INVALID_REVNUM)
> + if (item_rev != SVN_INVALID_REVNUM)
> {
> - svn_revnum_t item_rev = (wb->committed
> - ? changed_rev
> - : revision);
>
> if (wb->result->min_rev == SVN_INVALID_REVNUM
> || item_rev < wb->result->min_rev)
> @@ -101,22 +113,27 @@
>
> if (! wb->result->modified)
> {
> - svn_boolean_t text_mod;
> svn_boolean_t props_mod;
>
> SVN_ERR(svn_wc__props_modified(&props_mod, wb->db, local_abspath,
> scratch_pool));
> + wb->result->modified |= props_mod;
> + }
>
> + if (! wb->result->modified)
> + {
> + svn_boolean_t text_mod;
> +
> SVN_ERR(svn_wc__internal_text_modified_p(&text_mod, wb->db,
> local_abspath,
> FALSE,
> TRUE,
> scratch_pool));
> - wb->result->modified |= (text_mod || props_mod);
> + wb->result->modified |= text_mod;
> }
>
> - wb->result->sparse_checkout |= ((depth != svn_depth_infinity
> - && depth != svn_depth_unknown));
> + wb->result->sparse_checkout |= (depth != svn_depth_infinity
> + && depth != svn_depth_unknown);
> return SVN_NO_ERROR;
> }
>