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
Received on 2015-04-01 13:59:11 CEST