On Sun, Aug 4, 2013 at 7:51 AM, Daniel Shahaf <danielsh_at_elego.de> wrote:
> stefan2_at_apache.org wrote on Sun, Jul 14, 2013 at 12:34:50 -0000:
> > Author: stefan2
> > Date: Sun Jul 14 12:34:49 2013
> > New Revision: 1502964
> >
> > URL: http://svn.apache.org/r1502964
> > Log:
> > On the fsfs-improvements branch: Move more of the commonly used file and
> > path utilities from fs_fs.* to utils.*. Again, we add the svn_fs_fs__
> > prefix to everything in utils.h.
> >
> > Also make all path getters that didn't do so before return a const char*
> > instead of an svn_error_t *.
>
> Can you please refrain from moving a function around (intra-file or
> inter-file) while changing its signature or implementation. That makes
> review harder than it should be.
>
> Case in point: svn_fs_fs__path_rev_absolute() in this commit, and
> svn_fs_fs__write_revnum_file() in another commit.
>
When refactoring code, one also makes it conform to the
naming and parameter conventions that apply to the target
location. In the past, I had been reminded of that by
various people on multiple occasions.
I very much appreciate your review and you already
identified various issues / weaknesses. Hopefully,
you will find the time and energy to complete the review.
But keep in mind that it took me 2 full weeks with a diff
viewer copying difference after difference and grouping
them into somewhat manageable chunks. I would expect
that the review (incl. documenting the findings) will certainly
take a similar amount of time - give or take fancy tool
support.
My goal was directly apply the final code whenever
feasible, i.e. don't require code to be reviewed twice.
Otherwise, we might have just gone with the 400+
commits on the original branch.
>
> Thanks.
>
> Daniel
>
> P.S. the docstring of svn_fs_fs__write_revnum_file() references
> a nonexistent formal parameter.
>
That function is completely misnamed by now.
r1510344 fixed that.
Thanks again!
-- Stefan^2.
Received on 2013-08-04 23:18:28 CEST