[PATCH] v2. Follow-up to r922176

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