Unnecessary crippling of libsvn_fs_base. (Please stop it.)

["C. Michael Pilato" <cm...@collab.net>]

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?

-- 
C. Michael Pilato <cm...@collab.net>
CollabNet   <>   www.collab.net   <>   Distributed Development On Demand

Re: Unnecessary crippling of libsvn_fs_base. (Please stop it.)

[Karl Fogel <kf...@google.com>]

"C. Michael Pilato" <cm...@collab.net> writes:
>> 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.

The "functionality" I was referring to is the ability to store
arbitrary path names.  So "put functionality into libsvn_fs that
Subversion itself would never take advantage of" would mean keeping
the ability to have arbitrary paths -- which we didn't do, hence we
"didn't put" or "didn't keep" that functionality.

Sorry if that wasn't clear.

There are purely architectural reasons to modularize, but yeah, it's
true that the boundary between libsvn_fs and libsvn_repos has always
been one of the blurriest in Subversion.

>> 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.  :-)

I'm not vetoing moving the path-validation checks to somewhere else
(e.g., libsvn_repos), but I'm not volunteering, and Cassandra-like I
call out to warn anyone who tries it that it's Harder Than It Looks:

"It's Harder Than It Looks"  :-)

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: Unnecessary crippling of libsvn_fs_base. (Please stop it.)

["C. Michael Pilato" <cm...@collab.net>]

Karl Fogel wrote:
> "C. Michael Pilato" <cm...@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 <cm...@collab.net>
CollabNet   <>   www.collab.net   <>   Distributed Development On Demand

Re: Unnecessary crippling of libsvn_fs_base. (Please stop it.)

[Karl Fogel <kf...@google.com>]

"C. Michael Pilato" <cm...@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

Re: Unnecessary crippling of libsvn_fs_base. (Please stop it.)

[Daniel Rall <dl...@collab.net>]

On Fri, 14 Jul 2006, Garrett Rooney wrote:

> On 7/14/06, Daniel Rall <dl...@collab.net> wrote:
> >On Fri, 14 Jul 2006, C. Michael Pilato wrote:
> >
> >> Greg Hudson wrote:
> >> > On Fri, 2006-07-14 at 12:28 -0400, Garrett Rooney wrote:
> >> >
> >> >>FSFS shouldn't have any problem with such paths AFAIK.
> >> >
> >> >
> >> > FSFS will have trouble with newlines in pathnames... but then again, so
> >> > will our dumpfile format.
> >>
> >> Ah... the dumpfile format.  That's an interesting one...
> >
> >Shouldn't problem characters be escaped when dumping loading, rather
> >than when saving to the FS?
> 
> Back in the day when this discussion occurred it was just decided that
> nobody should have newlines in filenames, since it's evil, and it
> simplifies the dumpfile format (which was a design goal for it, IIRC).

Sure.  But these are users that we're talking about...

Re: Unnecessary crippling of libsvn_fs_base. (Please stop it.)

["C. Michael Pilato" <cm...@collab.net>]

Garrett Rooney wrote:
> On 7/14/06, Daniel Rall <dl...@collab.net> wrote:
> 
>> On Fri, 14 Jul 2006, C. Michael Pilato wrote:
>>
>> > Greg Hudson wrote:
>> > > On Fri, 2006-07-14 at 12:28 -0400, Garrett Rooney wrote:
>> > >
>> > >>FSFS shouldn't have any problem with such paths AFAIK.
>> > >
>> > >
>> > > FSFS will have trouble with newlines in pathnames... but then
>> again, so
>> > > will our dumpfile format.
>> >
>> > Ah... the dumpfile format.  That's an interesting one...
>>
>> Shouldn't problem characters be escaped when dumping loading, rather
>> than when saving to the FS?
> 
> 
> Back in the day when this discussion occurred it was just decided that
> nobody should have newlines in filenames, since it's evil, and it
> simplifies the dumpfile format (which was a design goal for it, IIRC).

Makes sense to me.  But that still leaves allllllll the other unnecessary
control character blockage down at the filesystem level.

-- 
C. Michael Pilato <cm...@collab.net>
CollabNet   <>   www.collab.net   <>   Distributed Development On Demand

Re: Unnecessary crippling of libsvn_fs_base. (Please stop it.)

[Garrett Rooney <ro...@electricjellyfish.net>]

On 7/14/06, Daniel Rall <dl...@collab.net> wrote:
> On Fri, 14 Jul 2006, C. Michael Pilato wrote:
>
> > Greg Hudson wrote:
> > > On Fri, 2006-07-14 at 12:28 -0400, Garrett Rooney wrote:
> > >
> > >>FSFS shouldn't have any problem with such paths AFAIK.
> > >
> > >
> > > FSFS will have trouble with newlines in pathnames... but then again, so
> > > will our dumpfile format.
> >
> > Ah... the dumpfile format.  That's an interesting one...
>
> Shouldn't problem characters be escaped when dumping loading, rather
> than when saving to the FS?

Back in the day when this discussion occurred it was just decided that
nobody should have newlines in filenames, since it's evil, and it
simplifies the dumpfile format (which was a design goal for it, IIRC).

-garrett

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: Unnecessary crippling of libsvn_fs_base. (Please stop it.)

[Daniel Rall <dl...@collab.net>]

On Fri, 14 Jul 2006, C. Michael Pilato wrote:

> Greg Hudson wrote:
> > On Fri, 2006-07-14 at 12:28 -0400, Garrett Rooney wrote:
> > 
> >>FSFS shouldn't have any problem with such paths AFAIK.
> > 
> > 
> > FSFS will have trouble with newlines in pathnames... but then again, so
> > will our dumpfile format.
> 
> Ah... the dumpfile format.  That's an interesting one...

Shouldn't problem characters be escaped when dumping loading, rather
than when saving to the FS?

Re: Unnecessary crippling of libsvn_fs_base. (Please stop it.)

["C. Michael Pilato" <cm...@collab.net>]

Greg Hudson wrote:
> On Fri, 2006-07-14 at 12:28 -0400, Garrett Rooney wrote:
> 
>>FSFS shouldn't have any problem with such paths AFAIK.
> 
> 
> FSFS will have trouble with newlines in pathnames... but then again, so
> will our dumpfile format.

Ah... the dumpfile format.  That's an interesting one...

-- 
C. Michael Pilato <cm...@collab.net>
CollabNet   <>   www.collab.net   <>   Distributed Development On Demand

Re: Unnecessary crippling of libsvn_fs_base. (Please stop it.)

[Greg Hudson <gh...@MIT.EDU>]

On Fri, 2006-07-14 at 12:28 -0400, Garrett Rooney wrote:
> FSFS shouldn't have any problem with such paths AFAIK.

FSFS will have trouble with newlines in pathnames... but then again, so
will our dumpfile format.

Neither back-end should have trouble with other control characters as
far as I know.

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: Unnecessary crippling of libsvn_fs_base. (Please stop it.)

[Garrett Rooney <ro...@electricjellyfish.net>]

On 7/14/06, C. Michael Pilato <cm...@collab.net> wrote:
> 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?

FSFS shouldn't have any problem with such paths AFAIK.

-garrett

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org