Re: [rfc/patch] Fix issue 734, import should 'mkdir -p'

David Kimdon <dwhedon@debian.org> writes:

> On Sat, May 03, 2003 at 11:15:27AM -0500, cmpilato@collab.net wrote:
> > Actually, the patch needs to be in the correct style before I exert
> > any energy on it. David, if you can whip this sucker into style-wise,
> > I'll give it a look-see implementation-wise.
>
> oops, sorry about that, here's try #3.
>
> -David
>
> Fix issue #735: Create directories on import.
>
> * subversion/libsvn_client/commit.c (import) : Call mkdir on
> components of 'new_entry' prior to performing the actual import.
>
> * subversion/tests/clients/cmdline/basic_tests.py (basic_import) : Change
> existing test case to use a path to the third argument to import and
> verify that the new directories are created properly.
>
> * doc/book/book/ch03.xml : Change text and import example to show how
> subversion can create subdirectories on import.
>
> * doc/book/book/ch08.xml : Remove warning that the leading elements of
> the third argument to import must exist. Subversion will now create
> these directories.

I've reviewed this patch, and generally it looked pretty good. Only
a couple of obvious problems I saw:

> + for (i = 0; i < dirs->nelts; i++)
> + {
> + if (new_dir_baton)
> + {
> + if (!batons)
> + {
> + batons = apr_array_make (pool, 1, sizeof (void *));
> + }
> +
> + *((void **) apr_array_push (batons)) = new_dir_baton;
> +
> + }
> +
> + new_path = svn_path_join (new_path, ((char **)dirs->elts)[i],
> + pool);
> +
> + SVN_ERR (editor->add_directory (new_path, root_baton,
> + NULL, SVN_INVALID_REVNUM,
> + pool, &new_dir_baton));
> + }

In this loop, you're continually calling add_directory with a parent
dir baton of root_baton, instead of using the dir_baton you got from
the previous iteration.

> /* Close up the show; it's time to go home. */
> + if (new_dir_baton)
> + SVN_ERR (editor->close_directory (new_dir_baton, pool));
> +
> + {
> + void **baton;
> +
> + while ((baton = (void **) apr_array_pop (batons)))
> + {
> + SVN_ERR (editor->close_directory (*baton, pool));
> + }
> + }
> +

Here, you dive into the array without making sure it's non-NULL.

--
Aside from those things, it all looks pretty good.  I've corrected the
above problems on my local machine, and am gonna do a little
hand-testing to verify that *I* didn't break something.  After that,
I'll commit crediting you with the patch.
Of course, there is one other thing bugging me.  It's not about your
patch, it's about the original issue.
Am I alone in thinking that it's ridiculous that import takes 3
arguments at all?  I mean, in my ideal world, import would look like:
   svn import DESTINATION SOURCE [...]
If destination didn't exist, it and all the intermediate directories
needed would be created.  If there were multiple SOURCE(s), then
DESTINATION would be created as a directory, and each of the sources
added to it by their basenames.
I'd like to argue this design before your patch goes in, if that's
alright with you.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org