1.6.x backport of "svnadmin hotcopy" for symlinks - API change?

[Julian Foad <ju...@wandisco.com>]

> * r950445, 950468
>    Fix for issue #2591 "'svnadmin hotcopy' does not replicate symlinks".
>    Justification:
>      This fix helps server administrators who has common files symlinked
>      across their repositories and take frequent backups of their repos
>      using svnadmin hotcopy.
>    Notes:
>      r950445 fixes the issue and r950468 is a test for this issue.
>    Votes:
>      +1: stylesen, cmpilato

I tested this change and it works.

However, it changes the semantics of the API svn_io_dir_walk().
Strictly speaking, we should not do that in a back-port (nor even in
trunk except by revving the API).

Previously, svn_io_dir_walk() reported only files and dirs.  With this
change it reports symlinks in the same way that it reports files (even
if they point to a directory).

The question is: do we regard that change as an acceptable bug fix and
allow it, or do we require the behaviour of the existing API to be left
unchanged and get the "hotcopy" behaviour change some other way?

I voted "-0" in r955255.

- Julian

Re: 1.6.x backport of "svnadmin hotcopy" for symlinks - API change?

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

On 06/17/2010 09:47 AM, Daniel Shahaf wrote:
> Julian Foad wrote on Thu, 17 Jun 2010 at 14:53 +0100:
>> On Wed, 2010-06-16, C. Michael Pilato wrote:
>>> If we decide that we need to rev this API, though, then let's do the sane
>>
>> (Did you mean "rev this API" or merely "change it in any way"?)
>>
>>> thing and teach the function to return *all* the known APR file types so we
>>> don't have to revisit this API again later.  We can backport a private
>>> implementation of the function for the purposes of fixing the bug in 1.6.x.
>>
>> Yes, we might as well make svn_io_dir_walk() return all possible kinds
>> of node, now that we're changing it at all.
>>
> 
> Or we might say: "This returns APR_REG, APR_DIR, and may return more types 
> in the future; don't assume this is an exhaustive list".
> 
>> Oh, and the doc string needs updating: it explicitly says "files and
>> directories".
>>
> 
> Symlinks are files?  In a way?  :-)
> 
> (Yes, couldn't hurt to be explicit.)

I'm revving svn_io_dir_walk() now, and building into the new docstring the
possibility that we'll deliver additional filetypes through the interface in
the future.

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

Re: 1.6.x backport of "svnadmin hotcopy" for symlinks - API change?

[Daniel Shahaf <d....@daniel.shahaf.name>]

Julian Foad wrote on Thu, 17 Jun 2010 at 14:53 +0100:
> On Wed, 2010-06-16, C. Michael Pilato wrote:
> > If we decide that we need to rev this API, though, then let's do the sane
> 
> (Did you mean "rev this API" or merely "change it in any way"?)
> 
> > thing and teach the function to return *all* the known APR file types so we
> > don't have to revisit this API again later.  We can backport a private
> > implementation of the function for the purposes of fixing the bug in 1.6.x.
> 
> Yes, we might as well make svn_io_dir_walk() return all possible kinds
> of node, now that we're changing it at all.
> 

Or we might say: "This returns APR_REG, APR_DIR, and may return more types 
in the future; don't assume this is an exhaustive list".

> Oh, and the doc string needs updating: it explicitly says "files and
> directories".
> 

Symlinks are files?  In a way?  :-)

(Yes, couldn't hurt to be explicit.)

Re: 1.6.x backport of "svnadmin hotcopy" for symlinks - API change?

[Julian Foad <ju...@wandisco.com>]

On Wed, 2010-06-16, C. Michael Pilato wrote:
> Julian Foad wrote:
> > Previously, svn_io_dir_walk() reported only files and dirs.  With this
> > change it reports symlinks in the same way that it reports files (even
> > if they point to a directory).
> 
> The function has always reported files and directories using exactly the
> same construct:  a call to the callback function with the apr_finto_t for
> that item.  The implementer of the callback had to examine the apr_finfo_t
> to see whether the item in question was a file or a directory.

Ah, OK.  I didn't look in enough detail, and just saw that it used the
same code path for reporting a file or a symlink and a different code
path for reporting a dir.

> The change is simply to return additional types of information.  That it
> does so in the same way it reports files is consistent with the prior
> behavior (that is, not meaningful to note).  Whether or not a symlink points
> to a directory (or to nothing at all, for that matter) is further
> irrelevant.  So, let's rephrase the change to only include the relevant
> information:
> 
>    Previously, svn_io_dir_walk() reported only files and dirs.  With this
>    change it reports symlinks, too.

Yes, much better.  Sorry for skewed phrasing, making it sound worse than
it is.

> > The question is: do we regard that change as an acceptable bug fix and
> > allow it, or do we require the behaviour of the existing API to be left
> > unchanged and get the "hotcopy" behaviour change some other way?
> 
> *shrug*.  I highly doubt that anyone except Subversion (and then only the
> hotcopy code) is using this extremely low-level API.  We don't expose the
> function through our swig bindings, even.  That's not an argument for or
> against compatibility breaking -- just a bit of a reality check in terms of
> damage control.  Any implementer of an svn_io_dir_walk() callback would be
> looking at the apr_finfo_t.type value anyway, though perhaps assuming that
> !REG == DIR (or vice-versa).

Right.  That's the (only) practical issue to consider, I guess.

I'm leaning to trying to be conservative in the changes we make, where
this doesn't constrain development much.  In this case, I'd prefer a
backward-compatible solution for 1.6.x.

Anyone else got a strong opinion?  "Does it matter?" versus "How hard is
it to DTRT in 1.6.12?"


> If we decide that we need to rev this API, though, then let's do the sane

(Did you mean "rev this API" or merely "change it in any way"?)

> thing and teach the function to return *all* the known APR file types so we
> don't have to revisit this API again later.  We can backport a private
> implementation of the function for the purposes of fixing the bug in 1.6.x.

Yes, we might as well make svn_io_dir_walk() return all possible kinds
of node, now that we're changing it at all.

Oh, and the doc string needs updating: it explicitly says "files and
directories".


> (While we're at it, I wonder if we shouldn't reimplement as a high-level
> wrapper around a static recursive function that operates using APR-paths
> instead of doing UTF8-conversion back and forth all over the place.)

*shrug*

- Julian

Re: 1.6.x backport of "svnadmin hotcopy" for symlinks - API change?

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

Julian Foad wrote:
> Previously, svn_io_dir_walk() reported only files and dirs.  With this
> change it reports symlinks in the same way that it reports files (even
> if they point to a directory).

The function has always reported files and directories using exactly the
same construct:  a call to the callback function with the apr_finto_t for
that item.  The implementer of the callback had to examine the apr_finfo_t
to see whether the item in question was a file or a directory.

The change is simply to return additional types of information.  That it
does so in the same way it reports files is consistent with the prior
behavior (that is, not meaningful to note).  Whether or not a symlink points
to a directory (or to nothing at all, for that matter) is further
irrelevant.  So, let's rephrase the change to only include the relevant
information:

   Previously, svn_io_dir_walk() reported only files and dirs.  With this
   change it reports symlinks, too.

> The question is: do we regard that change as an acceptable bug fix and
> allow it, or do we require the behaviour of the existing API to be left
> unchanged and get the "hotcopy" behaviour change some other way?

*shrug*.  I highly doubt that anyone except Subversion (and then only the
hotcopy code) is using this extremely low-level API.  We don't expose the
function through our swig bindings, even.  That's not an argument for or
against compatibility breaking -- just a bit of a reality check in terms of
damage control.  Any implementer of an svn_io_dir_walk() callback would be
looking at the apr_finfo_t.type value anyway, though perhaps assuming that
!REG == DIR (or vice-versa).

If we decide that we need to rev this API, though, then let's do the sane
thing and teach the function to return *all* the known APR file types so we
don't have to revisit this API again later.  We can backport a private
implementation of the function for the purposes of fixing the bug in 1.6.x.

(While we're at it, I wonder if we shouldn't reimplement as a high-level
wrapper around a static recursive function that operates using APR-paths
instead of doing UTF8-conversion back and forth all over the place.)

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