Re: Outdated check in import?

On Thu, Apr 19, 2012 at 10:29 AM, C. Michael Pilato <cmpilato_at_collab.net> wrote:
> On 04/19/2012 09:26 AM, Hyrum K Wright wrote:
>> I've been poking around import, and I discovered this snippet of
>> decade-old code:
>>
>>   /* The repository doesn't know about the reserved administrative
>>      directory. */
>>   if (new_entries->nelts
>>       /* What's this, what's this?  This assignment is here because we
>>          use the value to construct the error message just below.  It
>>          may not be aesthetically pleasing, but it's less ugly than
>>          calling APR_ARRAY_IDX twice. */
>>       && svn_wc_is_adm_dir(temp = APR_ARRAY_IDX(new_entries,
>>                                                 new_entries->nelts - 1,
>>                                                 const char *),
>>                            scratch_pool))
>>     return svn_error_createf
>>       (SVN_ERR_CL_ADM_DIR_RESERVED, NULL,
>>        _("'%s' is a reserved name and cannot be imported"),
>>        svn_dirent_local_style(temp, scratch_pool));
>>
>> new_entries is the list of basenames of to-be-created parents (similar
>> to what happens with 'mkdir --parents').  It appears this is a very
>> specific check that if the second-to-last of those basenames is the wc
>> admin directory, don't do the import.
>>
>> I'm trying to figure out if this is still a valid check.  It feels
>> like it's looking at a very specific case, and there are any number of
>> ways a determined user to circumvent this check.  And even if they
>> did, it's not illegal, folks would just end up with '.svn' in the
>> repository.
>>
>> It feels like this is just a relic of a bygone era when users were
>> still experimenting with Subversion, and I just have a hard time
>> seeing it as needed these days.
>>
>> Thoughts?
>
> As you know, code doesn't become useless just because it's a decade old.  :-)
>
> We decided as a project to disallow imports of the .svn/ directory, not
> because the repository couldn't handle it, or because the project was young
> and naive (as you imply), but because a subsequent checkout of the
> repository at the parent at the .svn/ directory would be impossible.  (The
> checkout would fail to create the versioned .svn/ child because .svn/ would
> already be present on disk, etc.)
>
> Certainly, with Subversion 1.7 that's a bit less of an issue because a .svn/
> subdirectory in the checkout dataset is, in theory, problematic only when it
> would appear at the top-level of the working copy.  But admins can't really
> control which version of Subversion is used to access their repositories,
> and this just seems like one of those areas where it's more likely that
> folks will accidentally try something that's stupid than that they are
> intentionally trying something clever.  Besides, as you say, if they
> *really* want to do this, there are workarounds.

This is a good description of the rationale, and I agree that
committing .svn directories is probably a bad idea. It's just that
the check quoted above feels woefully single-minded: "Check the
immediate precursor to the import target, and reject if it is an
administrative directory. Don't worry about other parents, and don't
bother with the target itself. Just put the blinders on and focus on
a single path component." If the objective is to disallow .svn
directories in the repository, then lets solve the problem generally,
rather than playing whack-a-mole with 'svn import'.

Now admittedly, in Ye Olde Early Days, this check might have *been*
the general solution. But is it now superfluous with other checks
happening elsewhere in Subversion?

> Summary:  I see no reason to relax the check.  As I've said before:  "Guns
> don't kill people; people kill people -- but that doesn't mean we hand out
> loaded guns as party favors."

I see you haven't visited Texas in a while....

-Hyrum

-- 
uberSVN: Apache Subversion Made Easy
http://www.uberSVN.com/