Hi, solo turn. Here are some more comments on this patch, in addition
to Justin's and Philip's comments which you've already seen. (You
said you were working on a new version of the patch; I hope my
feedback is timely.)
> currently svn add -R stops on the first directory which exists, and
> you have to fiddle out all the files you added by your own. this
> patch allows to run svn -R in the wc-root, and add all new files.
>
> Index: ./subversion/libsvn_client/add.c
> ===================================================================
> --- ./subversion/libsvn_client/add.c
> +++ ./subversion/libsvn_client/add.c 2002-08-18 20:40:44.593890000
> +0200
> @@ -51,10 +51,19 @@
> svn_wc_adm_access_t *dir_access;
>
> /* Add this directory to revision control. */
> - SVN_ERR (svn_wc_add (dirname, adm_access,
> + err = svn_wc_add (dirname, adm_access,
> NULL, SVN_INVALID_REVNUM,
> - notify_added, notify_baton, pool));
> + notify_added, notify_baton, pool);
>
> + if (err) {
> + if (!(err->apr_err == SVN_ERR_ENTRY_EXISTS)) {
> + /* ignore "exists" errors, but nothing else */
> + return err;
> + }
> + /* reset err */
> + err = SVN_NO_ERROR;
> + }
> +
As others pointed out: just use `!=' for the conditional, and don't
forget to call svn_error_clear_all(err) in the reset case.
Also, since you're testing for SVN_ERR_ENTRY_EXISTS explicitly, you
should make sure the documentation for svn_wc_add() in svn_wc.h
promises that particular error under this circumstance. You may
"know" that it returns that error, but if a caller is going to depend
on this behavior, then the svn_wc_add() doc string should promise it.
And, farther down, I think you also need to change this code in the
loop over the directory entries (entries from disk, not from the
entries file, that is):
if (this_entry.filetype == APR_DIR)
/* Recurse. */
SVN_ERR (add_dir_recursive (fullpath, dir_access,
notify_added, notify_baton,
subpool));
else if (this_entry.filetype == APR_REG)
SVN_ERR (svn_wc_add (fullpath, dir_access, NULL, SVN_INVALID_REVNUM,
notify_added, notify_baton, subpool));
In the file case here, the call to svn_wc_add() is again wrapped in
SVN_ERR. Yet isn't this just the same situation as the directory case
you covered in your initial patch, and therefore needs the same
explicit error test and possible error reset? Specifically, I'm
thinking of when the directory is already under revision control,
*and* has some (but not all) files inside it under revision control.
The non-recursive case above currently will error on those files, when
it should just ignore them. (?)
Finally, a patch like this really needs a regression test, probably it
would go in subversion/tests/clients/cmdline/schedule_tests.py.
If you think you won't have time to do all this in a new patch, that's
fine, just please let us know so someone else can take care of it.
But I'm hoping you will... :-)
Thanks,
-K
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Thu Aug 29 21:23:49 2002