Re: [PATCH] Don't remove empty directories when creating repositories (in some cases)

[Daniel Shahaf <d....@daniel.shahaf.name>]

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.

Re: [PATCH] Don't remove empty directories when creating repositories (in some cases)

[Daniel Shahaf <d....@daniel.shahaf.name>]

Sergey Raevskiy wrote on Wed, Apr 01, 2015 at 18:51:07 +0300:
> After a little discussion with Daniel on IRC I've decided to use
> 'compatible-version' approach.  Setting 'compatible-version' to 1.0.0 should
> force to fail creation of new FSFS filesystem, since it first appeared in
> Subversion 1.1.  This should also be true for FSX (or any other hypothetical
> backend of the future).  If this is not true for some FS backend, the test
> would fail, which makes it future-proof.
> 
> The test is skipped for BDB (which is deprecated).

+1 on the test part.  I don't have time to review the fix itself, sorry.

Thanks for revising the patch.

Daniel
(Sorry for late reply, I was offline this week.)

Re: [PATCH] Don't remove empty directories when creating repositories (in some cases)

[Sergey Raevskiy <se...@visualsvn.com>]

After a little discussion with Daniel on IRC I've decided to use
'compatible-version' approach.  Setting 'compatible-version' to 1.0.0 should
force to fail creation of new FSFS filesystem, since it first appeared in
Subversion 1.1.  This should also be true for FSX (or any other hypothetical
backend of the future).  If this is not true for some FS backend, the test
would fail, which makes it future-proof.

The test is skipped for BDB (which is deprecated).

I've attached a new version of my patch (log message remains the same).

Re: [PATCH] Don't remove empty directories when creating repositories (in some cases)

[Sergey Raevskiy <se...@visualsvn.com>]

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).

Re: [PATCH] Don't remove empty directories when creating repositories (in some cases)

[Branko Čibej <br...@wandisco.com>]

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

Re: [PATCH] Don't remove empty directories when creating repositories (in some cases)

[Daniel Shahaf <d....@daniel.shahaf.name>]

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.

Re: [PATCH] Don't remove empty directories when creating repositories (in some cases)

[Julian Foad <ju...@btopenworld.com>]

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. So it's fine to test it in this way.

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.

- 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.