Re: svn commit: r1097257 - in /subversion/trunk/subversion: include/private/svn_wc_private.h libsvn_client/info.c libsvn_wc/info.c

[Greg Stein <gs...@gmail.com>]

On Wed, Apr 27, 2011 at 17:49,  <hw...@apache.org> wrote:
> Author: hwright
> Date: Wed Apr 27 21:49:00 2011
> New Revision: 1097257
>
> URL: http://svn.apache.org/viewvc?rev=1097257&view=rev
> Log:
> Move the working-copy scraping part of svn_client_info3() to libsvn_wc.
>
> This is pretty much just a straight code copy.  We can adjust a number of
> things after this move (using libsvn_wc-internal APIs and such), those will
> eventually follow.
>
> * subversion/include/private/svn_wc_private.h
>  (svn_wc__get_info):  New.

I know the contents of this function were simply copied from existing
code, but there is a potential error leak in there. The line:

SVN_ERR(svn_wc__get_tree_conflict(&tree_conflict, ctx->wc_ctx,

If that returns with an error, then 'err' will leak. The above line
should use an err2 variable, with appropriate management of the two
errors...

(separately, there is a lot of simplification of the various 'return'
exits at the bottom of the function)

>...

Cheers,
-g

Re: svn commit: r1097257 - in /subversion/trunk/subversion: include/private/svn_wc_private.h libsvn_client/info.c libsvn_wc/info.c

[Julian Foad <ju...@wandisco.com>]

On Wed, 2011-04-27 at 19:13 -0400, Greg Stein wrote:
> On Wed, Apr 27, 2011 at 17:49,  <hw...@apache.org> wrote:
> > Author: hwright
> > Date: Wed Apr 27 21:49:00 2011
> > New Revision: 1097257
> >
> > URL: http://svn.apache.org/viewvc?rev=1097257&view=rev
> > Log:
> > Move the working-copy scraping part of svn_client_info3() to libsvn_wc.
> >
> > This is pretty much just a straight code copy.  We can adjust a number of
> > things after this move (using libsvn_wc-internal APIs and such), those will
> > eventually follow.
> >
> > * subversion/include/private/svn_wc_private.h
> >  (svn_wc__get_info):  New.
> 
> I know the contents of this function were simply copied from existing
> code, but there is a potential error leak in there. The line:
> 
> SVN_ERR(svn_wc__get_tree_conflict(&tree_conflict, ctx->wc_ctx,
> 
> If that returns with an error, then 'err' will leak. The above line
> should use an err2 variable, with appropriate management of the two
> errors...
> 
> (separately, there is a lot of simplification of the various 'return'
> exits at the bottom of the function)

I observe that almost the whole body of svn_wc__get_info() is a
duplicate of the callback function above it, info_found_node_callback().

For a start, we can replace the body by a call to that.  (I'm testing
that now.)

As a follow-up, it looks like the code here is a work-around for
svn_wc__node_walk_children() not providing the behaviour that we want
from it.  This appears to indicate that svn_wc__node_walk_children()
doesn't call the callback function if the target node is non-present and
has a tree conflict, whereas it does call the callback function for any
child node it encounters that is non-present and has a tree conflict.
We should at least document what it does exactly, and at best improve it
and test it.

- Julian