On Tue, 2010-12-07 at 21:57 +0530, Noorul Islam K M wrote:
> Julian Foad <julian.foad_at_wandisco.com> writes:
>
> > On Tue, 2010-12-07 at 16:06 +0530, Noorul Islam K M wrote:
> >
> >> Julian Foad <julian.foad_at_wandisco.com> writes:
> >>
> >> > Noorul Islam K M wrote:
> >> >
> >> >> Log
> >> >> [[[
> >> >> Follow-up to r879452.
> >> >>
> >> >> * subversion/libsvn_ra_local/ra_plugin.c
> >> >> (svn_ra_local__obliterate_path_rev): Replace call to svn_path_join()
> >> >> with svn_dirent_join() function.
> >> >
> >> > Hi Noorul.
> >> >
> >> > Why? Please explain.
> >> >
> >>
> >> Because svn_path_join() is deprecated. I could see similar change done in
> >> r879452. I thought it will be obvious from the log message because I
> >> mentioned the revision.
> >
> > The problem is that svn_dirent_join() is not a simple drop-in
> > replacement for svn_path_join(). The doc string of svn_path_join()
> > says:
> >
> > * New code should use either svn_dirent_join() (for local paths) or
> > * svn_uri_join() (for urls) or svn_relpath_join() (for relative paths).
> > *
> > * @deprecated Provided for backward compatibility with the 1.6 API.
> >
> > So you have to work out which kind of path is being joined. Have a look
> > at where the arguments come from and how the result is used, and read up
> > about the three kinds of path mentioned in the doc string, and work out
> > which one.
> >
> > (There is also a fourth kind of path, "fspath" which means a relative
> > path starting with "/", and the function svn_fspath__join(), which
> > should also be mentioned. I'll update the doc string in a moment, to
> > mention that option.)
> >
> > Also please say how you tested the change: did you run "make
> > check" (which combination?)? Did you step through the code in a
> > debugger and observe the values? Did you test it in another way?
> >
>
> I did not test it because I thought it is an obvious change. I was
> confident that the change was correct. I was sure that it should be
> svn_dirent_join() since I was making the change to function
> svn_ra_local__obliterate_path_rev under subversion/libsvn_ra_local.
This code is indeed in RA-local, but that doesn't mean the paths that it
is processing are local disk paths - they are not.
> But after reading your mail, I felt my assumptions might have been
> wrong. So I went and checked further. It looks like this function is not
> implemented for ra_neon, ra_serf and ra_svn. There are two test cases
> for obliterate command in test/cmdline and they are all marked as
> SKIP. Later I realised that obliterate command is not yet part of svn
> command line or am I missing something?
You are correct. The obliterate stuff is not implemented for the other
RA layers. In addition, I might remove it all from the code base before
we branch for 1.7, because it is not developed far enough to be useful.
If you want to stop working on this patch, that's fine with me, or if
you want to continue, that's fine too.
- Julian
Received on 2010-12-08 16:43:51 CET