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
Received on 2010-06-17 15:54:23 CEST