On 01.04.2015 12:37, Daniel Shahaf wrote: > 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.
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. Sergey's patch removes the /contents/ of existing directories, leaving the parent, if it existed, untouched. -- Brane
