Karl Fogel <kfogel@google.com> wrote on 08/24/2006 07:09:09 PM:
> Paul Burba <paulb@softlanding.com> writes:
> > Index: subversion/libsvn_client/add.c
> > ===================================================================
> > --- subversion/libsvn_client/add.c (revision 21223)
> > +++ subversion/libsvn_client/add.c (working copy)
> > @@ -322,12 +322,40 @@
> > /* 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);
> > +
> > + err = svn_io_dir_read(&this_entry, flags, dir, subpool);
> > +
> > + if (err)
> > + {
> > + /* 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));
> > + }
> > + break;
> > + }
>
> Yup. Might reverse the test order so that the first test is for the
> positive case (i.e., remove the "!", reverse the bodies, and nix the
> else's comment). The first time I read it, I got thrown by the
> failure case being first, I think because the success case has a
> longer body, which isn't typical.
The order of the if...else in my patch to copy.c follows that in add.c*,
which likely follows that in
http://svnbook.red-bean.com/en/1.1/ch08s05.html. FWIW I would write an
if...else block to put the *substantially* shorter block first, for
readability. But as both blocks are of similar in size in this case,
swapping the order is a bit clearer, and since I'm here I did that in both
add.c and copy.c. Also moved the break into the success case and tweaked
the comments a bit to better represent the fact we are *in* the loop now,
not outside it.
*Interestingly, this code dates way back to r1...when Dinosaurs ruled the
earth ;-)
> But it might just be my overeager
> eye looking too hard for clues.
<snip>
> I think it would be fine to commit them together, since they're
> conceptually doing the same thing. Only one is a followup to r21152,
> but you can just place that note accordingly in the log message.
Committed together r21259.
Thanks 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 Fri Aug 25 16:41:26 2006