subversion-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Branko ─îibej <>
Subject Re: [PATCH] Don't remove empty directories when creating repositories (in some cases)
Date Wed, 01 Apr 2015 11:58:39 GMT
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

View raw message