Roberto Reale wrote on Mon, Oct 26, 2015 at 20:56:30 +0100: > I've endeavoured to improve on my previous patch, by adopting a > heuristic approach as follows: > > 1) First and foremost, for each file in the backup copy I try and > assign permissions based on those of the corresponding file in the > source repository, if the latter does indeed exist. > 2) Should this fail, I infer the permissions from the context, i.e. I > choose one "typical file" from the same folder. > 3) Should the folder itself be empty, I assign permissions based on > the current umask. > > Do you deem such an approach to be correct?
A bit late, but: I think (3) should fall back to $REPOS_DIR/db/fs-type (which is guaranteed to exist) rather than to umask. (The umask has the right dimension — permission bits — but using it might surprise users.) Of academic interest [if you accept the change in the above paragraph]: the os.listdir() call had an O(N²) cost where N is the number of files in a directory, aka, was inefficient for unsharded FSFS repositories. I don't have any other comments, but I haven't read the patch closely enough to be comfortable committing it, either. Please feel free to file an issue in our bug tracker about this patch, so we don't forget it: https://issues.apache.org/jira/browse/SVN Cheers, Daniel
