Thanks for the reviews! > If you want to force fail because of the existing directory, I suggest > you create a directory and then do the equivalent of 'chmod -w' on it.
Unfortunately, making the directory unwritable will cause svn_repos_create() to fail *before* attempting the FS creation and the rollback code will not be invoked. > As I understand it, Sergey isn't trying to fix an issue that's > specifically related to a wrong FS type being given. He's fixing the > "roll-back" code that kicks in if repo creation fails for *any* > reason, and wrong-fs-type is just an example of one particular reason > that can trigger this roll-back. > In case it wasn't clear, he's saying the roll-back code tries to > "undo" creation of the repo directory structure on disk, but that it > undoes too much in some cases -- it deletes the root directory even if > it hadn't created that directory. Yes, this is exactly what I wanted to say. > If svn_repos_create() is changed to validate the FS_TYPE argument > before the mkdir(), then calling svn_repos_create(fs_type="invalid") > will no longer be testing for the problem Sergey is trying to fix. > > That's why I suggested the alternate testing method: because calling > svn_repos_create(fs_type="invalid") is not a *future-proof* way for > reproducing the issue Sergey is attempting to fix. It reproduces the > issue today but it might stop reproducing the issue in the future. I'm agree, the test is not future-proof. Unfortunately I don't see good way to write a future-proof test. As I can understand, passing an invalid 'compatible-version' value (which is proposed by Daniel on IRC) is not future-proof too, since it's also relies on implementation details. Should the test be removed at all? > The intent of the patch is quite different. Currently, almost the first > thing that svn_repos_create does is delete any existing repo directory, > so that if repos creation fails at some later time (e.g., due to a wrong > FS type), a formerly existing (empty) directory will have been deleted. > There's no cleanup code trying to undo what a partially successful > create did. Please correct me if I wrong, but as I can see svn_repos_create() is NOT deleting any existing repo directory before repository creation (as for trunk@HEAD).
