On Sun, Aug 4, 2013 at 7:51 AM, Daniel Shahaf <[email protected]> wrote:
> [email protected] 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.
