Julian Foad wrote on Wed, Apr 01, 2015 at 11:30:22 +0100: > Daniel, Brane: > > 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.
I understand this. > So it's fine to test it in this way. > 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. > 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. I haven't independently verified > this but it sounds completely logical and likely to be the case and > the proposed fix is good in my opinion. > Yes, "Don't rmdir it if we didn't mkdir it" sounds like the Right Thing to do to me, too. Cheers, Daniel > - Julian > > > On 1 April 2015 at 11:11, Daniel Shahaf <d...@daniel.shahaf.name> wrote: > > Sergey Raevskiy wrote on Tue, Mar 31, 2015 at 16:31:55 +0300: > >> > Wouldn't it be easier, and a smaller patch, to add a check in > >> > svn_repos_create that the FS type is valid /before/ creating the > >> > repository structure? This would also avoid adding yet another public > >> > API. > >> > >> As I understand there is no public API for checking if FS-type is valid and > >> corresponding FS module is available (for DSO-enabled builds). > >> > >> Even if we do a preliminary check like that, there are a number of other > >> errors that could occur during module loading and filesystem creation. > >> I used 'invalid-fs-type' in test just as simple and predictable way to fail > >> the filesystem creation. > >> > > > > I don't think it's a very robust way to cause svn_repos_create() to > > fail, precisely for the reason brane mention: the implementation might > > trip on the invalid FS value (DSO support notwithstanding) regardless of > > the disk status. > > > > 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. > > > > Cheers, > > > > Daniel > > > >> I agree that it would be better to avoid adding yet another public API, but > >> svn_io_remove_dir_contents() function might be useful for both Subversion > >> and third-party public API clients.
