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.s_at_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.
Received on 2015-04-01 12:38:32 CEST