You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Daniel Shahaf <d....@daniel.shahaf.name> on 2015/12/21 15:57:46 UTC
Re: [PATCH] improved behaviour of tools/backup/hot-backup.py
Roberto Reale wrote on Mon, Oct 26, 2015 at 20:56:30 +0100:
> I've endeavoured to improve on my previous patch, by adopting a
> heuristic approach as follows:
>
> 1) First and foremost, for each file in the backup copy I try and
> assign permissions based on those of the corresponding file in the
> source repository, if the latter does indeed exist.
> 2) Should this fail, I infer the permissions from the context, i.e. I
> choose one "typical file" from the same folder.
> 3) Should the folder itself be empty, I assign permissions based on
> the current umask.
>
> Do you deem such an approach to be correct?
A bit late, but:
I think (3) should fall back to $REPOS_DIR/db/fs-type (which is
guaranteed to exist) rather than to umask. (The umask has the right
dimension — permission bits — but using it might surprise users.)
Of academic interest [if you accept the change in the above paragraph]:
the os.listdir() call had an O(N²) cost where N is the number of files
in a directory, aka, was inefficient for unsharded FSFS repositories.
I don't have any other comments, but I haven't read the patch closely
enough to be comfortable committing it, either. Please feel free to
file an issue in our bug tracker about this patch, so we don't forget
it: https://issues.apache.org/jira/browse/SVN
Cheers,
Daniel
Re: [PATCH] improved behaviour of tools/backup/hot-backup.py
Posted by Roberto Reale <ro...@gmail.com>.
Hi folks,
I am going to submit the patch to the issue tracker. Am I right?
Roberto
On Mon, Dec 28, 2015 at 4:01 PM, Daniel Shahaf <d....@daniel.shahaf.name> wrote:
> Roberto Reale wrote on Wed, Dec 23, 2015 at 11:22:14 +0100:
>> To begin with, I edited the patch to implement Daniel's (a)-proposal.
>
>> + # "typical file" from the same folder (case C). Should the folder itself
>> + # be empty, we assign permissions based on those of a file which is
>> + # guaranteed to exist (case F).
> ...
>> + # (T) Assign permissions based on those of the file db/fs-type in
>> + # the source repository.
>
> "(case F)" and "(T)" refer to the same thing, one of them is a typo.
>
> Could you please submit the patch to the issue tracker?
>
Re: [PATCH] improved behaviour of tools/backup/hot-backup.py
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Roberto Reale wrote on Wed, Dec 23, 2015 at 11:22:14 +0100:
> To begin with, I edited the patch to implement Daniel's (a)-proposal.
> + # "typical file" from the same folder (case C). Should the folder itself
> + # be empty, we assign permissions based on those of a file which is
> + # guaranteed to exist (case F).
...
> + # (T) Assign permissions based on those of the file db/fs-type in
> + # the source repository.
"(case F)" and "(T)" refer to the same thing, one of them is a typo.
Could you please submit the patch to the issue tracker?
Re: [PATCH] improved behaviour of tools/backup/hot-backup.py
Posted by Roberto Reale <ro...@gmail.com>.
To begin with, I edited the patch to implement Daniel's (a)-proposal.
Cheers,
Roberto
On Wed, Dec 23, 2015 at 10:11 AM, Daniel Shahaf <d....@daniel.shahaf.name> wrote:
> Roberto Reale wrote on Mon, Dec 21, 2015 at 16:25:10 +0100:
>> Of course I would be glad to modify the patch, but... shall we content
>> ourselves with the somewhat elusive fs-type? :)
>
> Using fs-type would break backwards compatibility with BDB repositories
> created by svnadmin 1.0.x (or earlier) that have never been dump/load-ed.
>
> Now, since hot-backup.py was in tools/ in 1.0, it's formally covered by
> the same backwards compatibility promise as everything else.
>
> However, I don't think there are many people who use a repository
> created by 1.0 that has never been dump/load-ed by any later version of
> svnadmin; that is, I think few if any people would be affected by
> breaking compatibility in this specific case. (And those who *would* be
> affected would receive an error message, as opposed to silent misbehaviour.)
>
> So, how about we (a) make the patch use fs-type, (b) issue an erratumน
> stating the breakage and providing the workaround?
>
> Alternatively, we could just find some file that's guaranteed to exist
> in all 1.0 BDB repositories and use that file if fs-type is missing.
>
> Cheers,
>
> Daniel
>
> น See notes/api-errata/.
Re: [PATCH] improved behaviour of tools/backup/hot-backup.py
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Roberto Reale wrote on Mon, Dec 21, 2015 at 16:25:10 +0100:
> Of course I would be glad to modify the patch, but... shall we content
> ourselves with the somewhat elusive fs-type? :)
Using fs-type would break backwards compatibility with BDB repositories
created by svnadmin 1.0.x (or earlier) that have never been dump/load-ed.
Now, since hot-backup.py was in tools/ in 1.0, it's formally covered by
the same backwards compatibility promise as everything else.
However, I don't think there are many people who use a repository
created by 1.0 that has never been dump/load-ed by any later version of
svnadmin; that is, I think few if any people would be affected by
breaking compatibility in this specific case. (And those who *would* be
affected would receive an error message, as opposed to silent misbehaviour.)
So, how about we (a) make the patch use fs-type, (b) issue an erratum¹
stating the breakage and providing the workaround?
Alternatively, we could just find some file that's guaranteed to exist
in all 1.0 BDB repositories and use that file if fs-type is missing.
Cheers,
Daniel
¹ See notes/api-errata/.
Re: [PATCH] improved behaviour of tools/backup/hot-backup.py
Posted by Roberto Reale <ro...@gmail.com>.
Of course I would be glad to modify the patch, but... shall we content
ourselves with the somewhat elusive fs-type? :)
Roberto
On Mon, Dec 21, 2015 at 4:23 PM, Daniel Shahaf <d....@daniel.shahaf.name>
wrote:
> On Mon, Dec 21, 2015, at 15:16, Bert Huijben wrote:
> >
> >
> > > -----Original Message-----
> > > From: Daniel Shahaf [mailto:d.s@daniel.shahaf.name]
> > > Sent: maandag 21 december 2015 15:58
> > > To: Roberto Reale <ro...@gmail.com>
> > > Cc: Philip Martin <ph...@wandisco.com>;
> > > dev@subversion.apache.org
> > > Subject: Re: [PATCH] improved behaviour of tools/backup/hot-backup.py
> >
> > >
> > > I think (3) should fall back to $REPOS_DIR/db/fs-type (which is
> > > guaranteed to exist) rather than to umask. (The umask has the right
> > > dimension — permission bits — but using it might surprise users.)
> >
> > Are you sure this 'must exist'?
> >
> > When I accidentally tried to use a wrong directory some weeks ago (while
> > working on libsvn_fs_git) I found that when that file is missing we
> assume
> > that the filesystem is BDB.
>
> Without checking the code, I suspect you're right, and that file only
> exists
> for repositories created by svn 1.1 or later...
>
> Good catch :-)
>
> Daniel
>
Re: [PATCH] improved behaviour of tools/backup/hot-backup.py
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
On Mon, Dec 21, 2015, at 15:16, Bert Huijben wrote:
>
>
> > -----Original Message-----
> > From: Daniel Shahaf [mailto:d.s@daniel.shahaf.name]
> > Sent: maandag 21 december 2015 15:58
> > To: Roberto Reale <ro...@gmail.com>
> > Cc: Philip Martin <ph...@wandisco.com>;
> > dev@subversion.apache.org
> > Subject: Re: [PATCH] improved behaviour of tools/backup/hot-backup.py
>
> >
> > I think (3) should fall back to $REPOS_DIR/db/fs-type (which is
> > guaranteed to exist) rather than to umask. (The umask has the right
> > dimension — permission bits — but using it might surprise users.)
>
> Are you sure this 'must exist'?
>
> When I accidentally tried to use a wrong directory some weeks ago (while
> working on libsvn_fs_git) I found that when that file is missing we assume
> that the filesystem is BDB.
Without checking the code, I suspect you're right, and that file only exists
for repositories created by svn 1.1 or later...
Good catch :-)
Daniel
RE: [PATCH] improved behaviour of tools/backup/hot-backup.py
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: Daniel Shahaf [mailto:d.s@daniel.shahaf.name]
> Sent: maandag 21 december 2015 15:58
> To: Roberto Reale <ro...@gmail.com>
> Cc: Philip Martin <ph...@wandisco.com>;
> dev@subversion.apache.org
> Subject: Re: [PATCH] improved behaviour of tools/backup/hot-backup.py
>
> I think (3) should fall back to $REPOS_DIR/db/fs-type (which is
> guaranteed to exist) rather than to umask. (The umask has the right
> dimension — permission bits — but using it might surprise users.)
Are you sure this 'must exist'?
When I accidentally tried to use a wrong directory some weeks ago (while working on libsvn_fs_git) I found that when that file is missing we assume that the filesystem is BDB.
Bert