subversion-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Sergey Raevskiy <sergey.raevs...@visualsvn.com>
Subject Re: [PATCH] Don't remove empty directories when creating repositories (in some cases)
Date Wed, 01 Apr 2015 13:20:41 GMT
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).

Mime
View raw message