Karl Fogel wrote:
> "C. Michael Pilato" <cmpilato@collab.net> writes:
>
>>I just noticed (as part of a conversation with Philipp Marek re: Issue 1954)
>>that in r12632, Karl added code to svn_fs_make_file(), _make_dir(), and
>>_copy() to call svn_path_check_valid() before creating new paths in the
>>repository.
>>
>>Um. Why?
>>
>>We have a repository layer for adding such superficial sanity checks.
>>There's nothing that I can think of that would cause filenames with control
>>characters to be a problem in libsvn_fs, unless the FSFS backend can't hack
>>it. Is that the case? If so, should we not have a) put the protection in
>>the repos layer, and b) also put it in the FSFS backend only? Why is the
>>BDB backend unnecessarily crippled like so?
>
>
> Hmm. It's not a superficial sanity check, IMHO.
>
> Subversion has rules about what constitutes a valid path. Whatever
> those rules are, svn_path_check_valid() should be implementing them.
Well, one of the three Subversion RA layers has rules about what constitutes
validity of the transmission protocol. Our entries file format has a
different set of rules about what is valid and what isn't (though I guess is
used to be the same set applied to RA-DAV). Our dumpfile system has a third
set (though it might effectively be the same as the new entries file
scenario, now -- no newlines).
> In short, why not implement path checking at the lowest level? Is
> that not the proper place to enforce a core requirement of Subversion,
> as indicated by svn_path_check_valid()'s doc string?
Because "the lowest level" != "Subversion". Subversion is, IMO, not
libsvn_fs. Not libsvn_wc. Not libsvn_subr. Nor neon. Nor any one of the
many pieces that make up our version control system. It's the integration
of those many pieces in a particular fashion. So it's perfectly okay for
"Subversion" to disallow a certain type of behavior yet that restriction not
be enforced by every individual library on which Subversion version is based.
> I can understand the argument that "libsvn_fs is supposed to provide a
> generic filesystem-style versioned storage layer, and libsvn_repos is
> supposed to provide repository functionality on top of that". But
> until now, libsvn_fs had no non-Subversion uses, and it would have
> made little sense to put functionality into libsvn_fs that Subversion
> itself would never take advantage of, meanwhile increasing the risk of
> buggy data getting into someone's repository.
In this case, the effort wasn't about adding functionality to libsvn_fs that
wasn't previously there. The effort made was in removing functionality it
already had. But sure, there's an argument that says, "What if folks used
libsvn_fs directly in such a way that their later use of that API via
'Subversion' caused them problems?" The answer to that is (or should be)
obvious. If our modules aren't allowed to have all the functionality they
can have even when that functionality extends beyond the capabilities of
other modules, then I honestly question the utility of having that code be
modularized.
> Just don't underestimate the risk/difficulty of moving them to
> libsvn_repos. There are a lot of calls to check, including all the
> ones we add in the future. IMHO it's fairly likely that we'll mess up
> eventually, and someone will end up with bogus paths in a Subversion
> repository.
No problems here; I've no underestimations in progress. :-)
--
C. Michael Pilato <cmpilato@collab.net>
CollabNet <> www.collab.net <> Distributed Development On Demand
Received on Sat Jul 15 04:42:56 2006