Philip Martin <philip@codematters.co.uk> wrote on 08/26/2006 09:48:16 AM:
> pburba@tigris.org writes:
>
> > Author: pburba
> > Date: Fri Aug 25 07:39:09 2006
> > New Revision: 21259
>
> > @@ -322,12 +322,42 @@
> > /* Read the directory entries one by one and add those things to
> > revision control. */
> > SVN_ERR(svn_io_dir_open(&dir, dirname, pool));
> > - for (err = svn_io_dir_read(&this_entry, flags, dir, subpool);
> > - err == SVN_NO_ERROR;
> > - err = svn_io_dir_read(&this_entry, flags, dir, subpool))
> > +
> > + while (1)
> > {
> > const char *fullpath;
> >
> > + /* Clean out the per-iteration pool. */
> > + svn_pool_clear(subpool);
>
> That's a standard Subversion idiom so adding an "obvious" comment like
> that doesn't really help, if anything it obscures the code.
Agreed, it's quite clear, removed the comment.
> > +
> > + err = svn_io_dir_read(&this_entry, flags, dir, subpool);
> > +
> > + if (err)
> > + {
> > + /* Check if we're done reading the dir's entries. */
> > + if (APR_STATUS_IS_ENOENT(err->apr_err))
> > + {
> > + /* No more entries, close the dir and exit the loop. */
>
> Stating the obvious: "close the dir and exit the loop" is pointless
> and could well become outdated if the code is changed in the future.
>
> There are two comments "Check if ..." and "No more ..." that basically
> cover the same thing, the fact that ENOENT is the end of the loop.
Removed most of the commentary here, leaving only one line at the start of
the if (err) block.
> > + apr_status_t apr_err;
> > +
> > + svn_error_clear(err);
> > + apr_err = apr_dir_close(dir);
> > + if (apr_err)
> > + return svn_error_wrap_apr
> > + (apr_err, _("Can't close directory '%s'"),
> > + svn_path_local_style(dirname, subpool));
> > + break;
> > + }
> > + else
> > + {
> > + /* Some unexpected error reading the dir's entries. */
>
> That comment doesn't add anything.
Removed that.
> > + return svn_error_createf
> > + (err->apr_err, err,
> > + _("Error during recursive add of '%s'"),
> > + svn_path_local_style(dirname, subpool));
> > + }
> > + }
> > +
> > /* Skip entries for this dir and its parent. */
> > if (this_entry.name[0] == '.'
> > && (this_entry.name[1] == '\0'
> > @@ -364,29 +394,6 @@
> > else if (err)
> > return err;
> > }
> > -
> > - /* Clean out the per-iteration pool. */
> > - svn_pool_clear(subpool);
> > - }
> > -
> > - /* Check that the loop exited cleanly. */
> > - if (! (APR_STATUS_IS_ENOENT(err->apr_err)))
> > - {
> > - return svn_error_createf
> > - (err->apr_err, err,
> > - _("Error during recursive add of '%s'"),
> > - svn_path_local_style(dirname, subpool));
> > - }
> > - else /* Yes, it exited cleanly, so close the dir. */
> > - {
> > - apr_status_t apr_err;
> > -
> > - svn_error_clear(err);
> > - apr_err = apr_dir_close(dir);
> > - if (apr_err)
> > - return svn_error_wrap_apr
> > - (apr_err, _("Can't close directory '%s'"),
> > - svn_path_local_style(dirname, subpool));
> > }
> >
> > /* Opened by svn_wc_add */
> >
> > Modified: trunk/subversion/libsvn_wc/copy.c
> > URL: http://svn.collab.
> net/viewvc/svn/trunk/subversion/libsvn_wc/copy.c?
> pathrev=21259&r1=21258&r2=21259
> >
>
==============================================================================
> > --- trunk/subversion/libsvn_wc/copy.c (original)
> > +++ trunk/subversion/libsvn_wc/copy.c Fri Aug 25 07:39:09 2006
> > @@ -161,17 +161,50 @@
> > SVN_ERR(svn_wc_adm_retrieve(&src_child_dir_access, src_access,
> > src_path, pool));
> >
> > + /* Read src_path's entries one by one. */
> > + SVN_ERR(svn_io_dir_open(&dir, src_path, pool));
>
> That comment appears to apply to the svn_io_dir_open line, but
> dir_open doesn't read any entries.
Moved that to proper place before while loop.
> > +
> > /* Create a subpool for iterative memory control. */
> > subpool = svn_pool_create(pool);
>
> Not part of your patch, but another pointless comment (I suppose that
> might explain why you added comments in the same style).
That is correct. You may have misssed it earlier in the thread, but the
loop in add.c looks inspired by an example in the Subversion book:
http://svnbook.red-bean.com/nightly/en/svn-book.html#svn.developer.pools.
Or possibly the book is inspired by the code, anyway I was commenting in a
similar manner. Still being relatively new to C, Subversion, and coding
in general, what is "obvious" to longtime developers is often cryptic to
me (APR_STATUS_IS_ENOENT required a peek at the apr_errno.h for example).
The end result is that I'm sometimes too comment happy and if I find an
overly-commented example in the Subversion code for inspiration, well look
out :-)
> >
> > - /* Read src_path's entries one by one. */
> > - SVN_ERR(svn_io_dir_open(&dir, src_path, pool));
> > - for (err = svn_io_dir_read(&this_entry, flags, dir, subpool);
> > - err == SVN_NO_ERROR;
> > - err = svn_io_dir_read(&this_entry, flags, dir, subpool))
> > + while (1)
> > {
> > const char *src_fullpath;
> >
> > + /* Clean out the per-iteration pool. */
> > + svn_pool_clear(subpool);
>
> Again.
Gone.
> > +
> > + err = svn_io_dir_read(&this_entry, flags, dir, subpool);
> > +
> > + if (err)
> > + {
> > + /* Check if we're done reading the dir's entries. */
> > + if (APR_STATUS_IS_ENOENT(err->apr_err))
> > + {
> > + /* No more entries, close the dir and exit the
loop. */
> > + apr_status_t apr_err;
> > +
> > + svn_error_clear(err);
> > + apr_err = apr_dir_close(dir);
> > + if (apr_err)
> > + return svn_error_wrap_apr(apr_err,
> > + _("Can't close "
> > + "directory '%s'"),
> > +
> svn_path_local_style(src_path,
> > + subpool));
> > + break;
> > + }
> > + else
> > + {
> > + /* Some unexpected error reading the dir's entries.
*/
> > + return svn_error_createf(err->apr_err, err,
> > + _("Error during recursive
copy "
> > + "of '%s'"),
> > + svn_path_local_style(src_path,
> > + subpool));
> > + }
> > + }
> > +
> > /* Skip entries for this dir and its parent. */
> > if (this_entry.name[0] == '.'
> > && (this_entry.name[1] == '\0'
> > @@ -221,31 +254,9 @@
> > subpool));
> > }
> >
> > - /* Clean out the per-iteration pool. */
> > - svn_pool_clear(subpool);
> > -
> > - } /* End for loop */
> > + } /* End while(1) loop */
> >
> > - /* Check that the loop exited cleanly. */
> > - if (! (APR_STATUS_IS_ENOENT(err->apr_err)))
> > - {
> > - return svn_error_createf(err->apr_err, err,
> > - _("Error during recursive copyof
'%s'"),
> > - svn_path_local_style(src_path,
> > - subpool));
> > - }
> > - else /* Yes, it exited cleanly, so close the dir. */
> > - {
> > - apr_status_t apr_err;
> > -
> > - svn_error_clear(err);
> > - apr_err = apr_dir_close(dir);
> > - if (apr_err)
> > - return svn_error_wrap_apr(apr_err,
> > - _("Can't close directory
'%s'"),
> > - svn_path_local_style(src_path,
> > - subpool));
> > - }
> > + svn_pool_destroy(subpool);
> >
> > } /* End else src_is_added. */
> >
Committed r21302.
Thanks as always for the review,
Paul B.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Mon Aug 28 18:01:52 2006