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_at_HEAD).
Received on 2015-04-01 15:22:45 CEST