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
