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.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:31:14 CEST