"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.
If those rules are that control characters are not valid to store in a
Subversion repository, then that's what this change enforces. (I
don't remember the exact rules, nor is it relevant to this discussion.
The important thing is that svn_path_check_valid() knows them.)
If we enforce those rules at the libsvn_repos layer, then every place
where libsvn_repos creates a path would need to have the check. No
one will ever remember to do that every time (would we have thought of
svn_repos_load_fs2()'s dumpstream interpretation code, for example?).
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?
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.
Now with fsvs, I guess that's changing -- libsvn_fs has a life of its
own. That means we need to address the question of whether libsvn_fs
is supposed to provide generic versioned-tree functionality,
independently of Subversion, or whether it should keep Subversion's
restrictions on what paths can be stored. When we answer that
question, we'll know where to put the path checks. Until recently,
this choice didn't exist, and in that universe, I think their current
location makes sense.
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.
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Sat Jul 15 00:03:09 2006