You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Masaru Tsuchiyama <m....@gmail.com> on 2013/08/04 02:36:24 UTC
[PATCH] add support for svnrdump to svn-backup-dumps.py
Hello
I attach a patch to add support for svnrdump to svn-backup-dumps.py.
[[[
add support for svnrdump to svn-backup-dumps.py
* tools/server-side/svn-backup-dumps.py
() : fix comment
() : update to ver 0.7
() : import urlparse
(SvnBackup.__init__) : fix message in SvnBackupException constructor.
(SvnBackup.__init__) : check whether the second parameter is a local
path or an URL.
(SvnBackup.__init__) : check whether --deltas option is specified when
the second parameter is an URL.
(SvnBackup.__init__) : set svn and svnrdump paths.
(SvnBackup.exec_cmd_nt) : open null device and pass it to Popen
when printerr is False.
(SvnBackup.get_head_rev) : rename this to get_head_rev_for_local.
(SvnBackup.get_head_rev_for_url) : get HEAD revision for an URL.
(SvnBackup.get_head_rev) : call get_head_rev_for_local for a local
path, get_head_rev_for_url for an URL
(SvnBackup.create_dump) : invoke svnadmin for a local path,
svnrdump for an URL.
() : fix help message.
() : add --svnrdump-path and --svn-path.
]]]
Regards.
--
Masaru Tsuchiyama <m....@gmail.com>
Re: [PATCH] add support for svnrdump to svn-backup-dumps.py
Posted by Masaru Tsuchiyama <m....@gmail.com>.
Hello
(2013/08/08 22:47), C. Michael Pilato wrote:
> On 08/08/2013 09:40 AM, Daniel Shahaf wrote:
>> On Thu, Aug 08, 2013 at 09:26:09AM -0400, C. Michael Pilato wrote:
>>> This tells the options parser that there are no command-line options
>>> which follow, which would keep self.__repospath from being treated as an
>>> option in the unlikely scenario that it begins with a hyphen.
I added "--" to the command line options.
[[[
add support for svnrdump to svn-backup-dumps.py
* tools/server-side/svn-backup-dumps.py
() : fix comment
() : update to ver 0.7
() : import urlparse
(SvnBackup.__init__) : fix message in SvnBackupException constructor.
(SvnBackup.__init__) : check whether the second parameter is a local
path or an URL.
(SvnBackup.__init__) : check whether --deltas option is specified when
the second parameter is an URL.
(SvnBackup.__init__) : set svn and svnrdump paths.
(SvnBackup.__init__) : set flags for --trust-server-cert and
--no-auth-cache.
(SvnBackup.exec_cmd_nt) : open null device and pass it to Popen
when printerr is False.
(SvnBackup.get_extra_param) : get extra option for svn and svnrdump.
always pass --non-interactive to
svn and svnrdump to make sure.
(SvnBackup.get_head_rev) : rename this to get_head_rev_for_local.
(SvnBackup.get_head_rev_for_url) : get HEAD revision for an URL.
(SvnBackup.get_head_rev) : call get_head_rev_for_local for a local
path, get_head_rev_for_url for an URL
(SvnBackup.transfer_smb): pass self.__print_stderr to self.exec_cmd().
(SvnBackup.create_dump) : invoke svnadmin for a local path,
svnrdump for an URL.
(SvnBackup.create_dump) : pass "--" before self.__repospath.
() : fix help message.
() : add --svnrdump-path and --svn-path.
() : add --trust-server-cert.
() : add --no-auth-cache.
() : add --non-interactive.
() : add --print-stderr.
]]]
--
Masaru Tsuchiyama <m....@gmail.com>
Re: [PATCH] add support for svnrdump to svn-backup-dumps.py
Posted by Masaru Tsuchiyama <m....@gmail.com>.
I updated for head revision.
[[[
add support for svnrdump to svn-backup-dumps.py
* tools/server-side/svn-backup-dumps.py
() : fix comment
() : update to ver 0.7
() : import urlparse
(SvnBackup.__init__) : fix message in SvnBackupException constructor.
(SvnBackup.__init__) : check whether the second parameter is a local
path or an URL.
(SvnBackup.__init__) : check whether --deltas option is specified when
the second parameter is an URL.
(SvnBackup.__init__) : set svn and svnrdump paths.
(SvnBackup.__init__) : set flags for --trust-server-cert and
--no-auth-cache.
(SvnBackup.exec_cmd_nt) : open null device and pass it to Popen
when printerr is False.
(SvnBackup.get_extra_param) : get extra option for svn and svnrdump.
always pass --non-interactive to
svn and svnrdump to make sure.
(SvnBackup.get_head_rev) : rename this to get_head_rev_for_local.
(SvnBackup.get_head_rev_for_url) : get HEAD revision for an URL.
(SvnBackup.get_head_rev) : call get_head_rev_for_local for a local
path, get_head_rev_for_url for an URL
(SvnBackup.transfer_smb): pass self.__print_stderr to self.exec_cmd().
(SvnBackup.create_dump) : invoke svnadmin for a local path,
svnrdump for an URL.
(SvnBackup.create_dump) : pass "--" before self.__repospath.
() : fix help message.
() : add --svnrdump-path and --svn-path.
() : add --trust-server-cert.
() : add --no-auth-cache.
() : add --non-interactive.
() : add --print-stderr.
]]]
--
Masaru Tsuchiyama <m....@gmail.com>
Re: [PATCH] add support for svnrdump to svn-backup-dumps.py
Posted by masaru tsuchiyama <m....@gmail.com>.
Hello
Could you review my patch?
2013-09-08 23:25 GMT+09:00 Masaru Tsuchiyama <m....@gmail.com>:
> Ben Reser wrote:
> > On 8/18/13 5:34 AM, Daniel Shahaf wrote:
> >> Haven't looked at the rest of the patch, perhaps Ben will as he reviewed
> >> an earlier iteration. (but I know he is a bit busy this week, including
> >> a release tomorrow and several other things..)
> >
> > It's starred in my mailbox.
> >
>
> Hello Ben
>
> Are you still busy?
>
> --
> Masaru Tsuchiyama <m....@gmail.com>
>
Re: [PATCH] add support for svnrdump to svn-backup-dumps.py
Posted by Masaru Tsuchiyama <m....@gmail.com>.
Ben Reser wrote:
> On 8/18/13 5:34 AM, Daniel Shahaf wrote:
>> Haven't looked at the rest of the patch, perhaps Ben will as he reviewed
>> an earlier iteration. (but I know he is a bit busy this week, including
>> a release tomorrow and several other things..)
>
> It's starred in my mailbox.
>
Hello Ben
Are you still busy?
--
Masaru Tsuchiyama <m....@gmail.com>
Re: [PATCH] add support for svnrdump to svn-backup-dumps.py
Posted by Ben Reser <be...@reser.org>.
On 8/18/13 5:34 AM, Daniel Shahaf wrote:
> Haven't looked at the rest of the patch, perhaps Ben will as he reviewed
> an earlier iteration. (but I know he is a bit busy this week, including
> a release tomorrow and several other things..)
It's starred in my mailbox.
Re: [PATCH] add support for svnrdump to svn-backup-dumps.py
Posted by Masaru Tsuchiyama <m....@gmail.com>.
Daniel Shahaf wrote:
>> + # pass "--" to tell commands that 'self.__repospath' is not a command-line option.
>> + cmd = [ self.__svn_path, "log", "-l 1", "-q", "--", self.__repospath ]
>
> ["-l 1"] should be either ["-l1"] or ["-l", "1"].
Fixed in the attached patch.
--
Masaru Tsuchiyama <m....@gmail.com>
Re: [PATCH] add support for svnrdump to svn-backup-dumps.py
Posted by Daniel Shahaf <da...@apache.org>.
Masaru Tsuchiyama wrote on Thu, Aug 15, 2013 at 15:06:12 +0900:
> I added "--" to the command line options.
Thanks.
> + # pass "--" to tell commands that 'self.__repospath' is not a command-line option.
> + cmd = [ self.__svn_path, "log", "-l 1", "-q", "--", self.__repospath ]
["-l 1"] should be either ["-l1"] or ["-l", "1"].
Haven't looked at the rest of the patch, perhaps Ben will as he reviewed
an earlier iteration. (but I know he is a bit busy this week, including
a release tomorrow and several other things..)
Cheers,
Daniel
Re: [PATCH] add support for svnrdump to svn-backup-dumps.py
Posted by Masaru Tsuchiyama <m....@gmail.com>.
Hello
(2013/08/08 22:47), C. Michael Pilato wrote:
> On 08/08/2013 09:40 AM, Daniel Shahaf wrote:
>> On Thu, Aug 08, 2013 at 09:26:09AM -0400, C. Michael Pilato wrote:
>>> This tells the options parser that there are no command-line options
>>> which follow, which would keep self.__repospath from being treated as an
>>> option in the unlikely scenario that it begins with a hyphen.
I added "--" to the command line options.
[[[
add support for svnrdump to svn-backup-dumps.py
* tools/server-side/svn-backup-dumps.py
() : fix comment
() : update to ver 0.7
() : import urlparse
(SvnBackup.__init__) : fix message in SvnBackupException constructor.
(SvnBackup.__init__) : check whether the second parameter is a local
path or an URL.
(SvnBackup.__init__) : check whether --deltas option is specified when
the second parameter is an URL.
(SvnBackup.__init__) : set svn and svnrdump paths.
(SvnBackup.__init__) : set flags for --trust-server-cert and
--no-auth-cache.
(SvnBackup.exec_cmd_nt) : open null device and pass it to Popen
when printerr is False.
(SvnBackup.get_extra_param) : get extra option for svn and svnrdump.
always pass --non-interactive to
svn and svnrdump to make sure.
(SvnBackup.get_head_rev) : rename this to get_head_rev_for_local.
(SvnBackup.get_head_rev_for_url) : get HEAD revision for an URL.
(SvnBackup.get_head_rev) : call get_head_rev_for_local for a local
path, get_head_rev_for_url for an URL
(SvnBackup.transfer_smb): pass self.__print_stderr to self.exec_cmd().
(SvnBackup.create_dump) : invoke svnadmin for a local path,
svnrdump for an URL.
(SvnBackup.create_dump) : pass "--" before self.__repospath.
() : fix help message.
() : add --svnrdump-path and --svn-path.
() : add --trust-server-cert.
() : add --no-auth-cache.
() : add --non-interactive.
() : add --print-stderr.
]]]
--
Masaru Tsuchiyama <m....@gmail.com>
Re: [PATCH] add support for svnrdump to svn-backup-dumps.py
Posted by "C. Michael Pilato" <cm...@collab.net>.
On 08/08/2013 09:40 AM, Daniel Shahaf wrote:
> On Thu, Aug 08, 2013 at 09:26:09AM -0400, C. Michael Pilato wrote:
>> This tells the options parser that there are no command-line options
>> which follow, which would keep self.__repospath from being treated as an
>> option in the unlikely scenario that it begins with a hyphen.
>
> Right. 99% of the time whether the '--' is there doesn't matter, but I like
> putting it there always. You never know if a line of code won't get copied
> to another script that passes user input as the repos_path argument...
>
> I'm not suggesting we fix that throughout the entire script or the entire
> tools/ dir, but I am saying it would be nice for new code (i.e., code added
> in the patch or modified by it) to use '--' properly.
FWIW, I completely agree.
Re: [PATCH] add support for svnrdump to svn-backup-dumps.py
Posted by Daniel Shahaf <da...@apache.org>.
On Thu, Aug 08, 2013 at 09:26:09AM -0400, C. Michael Pilato wrote:
> This tells the options parser that there are no command-line options
> which follow, which would keep self.__repospath from being treated as an
> option in the unlikely scenario that it begins with a hyphen.
Right. 99% of the time whether the '--' is there doesn't matter, but I like
putting it there always. You never know if a line of code won't get copied
to another script that passes user input as the repos_path argument...
I'm not suggesting we fix that throughout the entire script or the entire
tools/ dir, but I am saying it would be nice for new code (i.e., code added
in the patch or modified by it) to use '--' properly.
Re: [PATCH] add support for svnrdump to svn-backup-dumps.py
Posted by "C. Michael Pilato" <cm...@collab.net>.
On 08/08/2013 09:17 AM, Masaru Tsuchiyama wrote:
>> Also, it'd be good practice to pass "--" in front of self.__repospath,
>> but that
>> appears to be a preexisting problem in the code (i.e., not introduced
>> by your
>> patch).
>
> What is the purpose in passing "--"?
This tells the options parser that there are no command-line options
which follow, which would keep self.__repospath from being treated as an
option in the unlikely scenario that it begins with a hyphen.
Re: [PATCH] add support for svnrdump to svn-backup-dumps.py
Posted by Daniel Shahaf <da...@elego.de>.
Masaru Tsuchiyama wrote on Thu, Aug 08, 2013 at 22:17:05 +0900:
> > > + cmd = [ self.__svnadmin_path, "dump",
> > > + "--incremental", "-r", revparam,
> self.__repospath ]
> >
> > That doesn't look right. If self.__repospath can be a local path to a
> > repository root, you shouldn't pass it as an argument to 'svn' a few
> lines
> > above.
>
> If self.__repospath is a local path to a repository,
> I don't pass it to svn. See get_head_rev().
>
In the patch I reviewed, there was a
+ cmd = [ self.__svn_path, "youngest", self.__repospath ]
call a few lines above. I left it in the quoted context of my review.
I guess you mean you fixed that issue in the new iteration of the patch.
>> Also, it'd be good practice to pass "--" in front of self.__repospath, but that
>> appears to be a preexisting problem in the code (i.e., not introduced by your
>> patch).
>
> What is the purpose in passing "--"?
>
Indicating that further argv arguments are paths or URLs and not
options, even if they begin with '-'. (Note that we set
os->interleave=1 in sub_main().)
> > ... and while at it, use r"" literals to avoid clashes with a
> potential future
> > "r\d" backslash escape sequence.
>
> Do you worry about changing format change of 'svn log'
> in later versions of svn?
>
No, I worry about the Python language defining a meaning to the escape
sequence
\d
inside a double-quoted string, just like it already defines
\n
to be a newline character (and not literal backslash followed by literal
small letter N).
>
Thanks for the fixes.
Daniel
Re: [PATCH] add support for svnrdump to svn-backup-dumps.py
Posted by Masaru Tsuchiyama <m....@gmail.com>.
> Typo, "revision_regex"
fixed at the attached patch.
> > + cmd = [ self.__svnadmin_path, "dump",
> > + "--incremental", "-r", revparam,
self.__repospath ]
>
> That doesn't look right. If self.__repospath can be a local path to a
> repository root, you shouldn't pass it as an argument to 'svn' a few
lines
> above.
If self.__repospath is a local path to a repository,
I don't pass it to svn. See get_head_rev().
> Also, it'd be good practice to pass "--" in front of self.__repospath, but that
> appears to be a preexisting problem in the code (i.e., not introduced by your
> patch).
What is the purpose in passing "--"?
> ... and while at it, use r"" literals to avoid clashes with a
potential future
> "r\d" backslash escape sequence.
Do you worry about changing format change of 'svn log'
in later versions of svn?
If so, 'svn log' is not called because future versions of svn
supports 'svn youngest'.
I add --non-interactive option to the script.
[[[
add support for svnrdump to svn-backup-dumps.py
* tools/server-side/svn-backup-dumps.py
() : fix comment
() : update to ver 0.7
() : import urlparse
(SvnBackup.__init__) : fix message in SvnBackupException constructor.
(SvnBackup.__init__) : check whether the second parameter is a local
path or an URL.
(SvnBackup.__init__) : check whether --deltas option is specified when
the second parameter is an URL.
(SvnBackup.__init__) : set svn and svnrdump paths.
(SvnBackup.__init__) : set flags for --trust-server-cert and
--no-auth-cache.
(SvnBackup.exec_cmd_nt) : open null device and pass it to Popen
when printerr is False.
(SvnBackup.get_extra_param) : get extra option for svn and svnrdump.
always pass --non-interactive to
svn and svnrdump to make sure.
(SvnBackup.get_head_rev) : rename this to get_head_rev_for_local.
(SvnBackup.get_head_rev_for_url) : get HEAD revision for an URL.
(SvnBackup.get_head_rev) : call get_head_rev_for_local for a local
path, get_head_rev_for_url for an URL
(SvnBackup.create_dump) : invoke svnadmin for a local path,
svnrdump for an URL.
() : fix help message.
() : add --svnrdump-path and --svn-path.
() : add --trust-server-cert.
() : add --no-auth-cache.
() : add --non-interactive.
]]]
Re: [PATCH] add support for svnrdump to svn-backup-dumps.py
Posted by Daniel Shahaf <da...@apache.org>.
> > + revison_regex = re.compile("^r(\d+)")
>
> Typo, "revision_regex"
>
... and while at it, use r"" literals to avoid clashes with a potential future
"r\d" backslash escape sequence.
Re: [PATCH] add support for svnrdump to svn-backup-dumps.py
Posted by Daniel Shahaf <da...@apache.org>.
Thanks for sending text/plain patches!
> +++ tools/server-side/svn-backup-dumps.py (working copy)
> + def get_head_rev_for_url(self):
> + extra_param = self.get_extra_param()
> +
> + # use 'svn yougest' to get the HEAD revision of URL
> + # 'svn yougest' is supported on subversion 1.9 or laster.
> + cmd = [ self.__svn_path, "youngest", self.__repospath ]
> + cmd[2:2] = extra_param
> + r = self.exec_cmd(cmd)
> + if r[0] == 0 and len(r[2]) == 0:
> + return int(r[1].strip())
> +
> + # use 'svn log' to get the latest revision of URL
> + # it may be different from the HEAD revision.
> + cmd = [ self.__svn_path, "log", "-l 1", "-q", self.__repospath ]
> + cmd[2:2] = extra_param
> + r = self.exec_cmd(cmd)
> + if r[0] == 0 and len(r[2]) == 0:
> + revison_regex = re.compile("^r(\d+)")
Typo, "revision_regex"
> + cmd = [ self.__svnadmin_path, "dump",
> + "--incremental", "-r", revparam, self.__repospath ]
That doesn't look right. If self.__repospath can be a local path to a
repository root, you shouldn't pass it as an argument to 'svn' a few lines
above.
Also, it'd be good practice to pass "--" in front of self.__repospath, but that
appears to be a preexisting problem in the code (i.e., not introduced by your
patch).
> + else:
> + cmd = [ self.__svnrdump_path, "dump",
> + "--incremental", "-r", revparam, self.__repospath ]
Cheers,
Daniel
(haven't read the whole patch; these two issues just jumped out on a quick scan)
Re: [PATCH] add support for svnrdump to svn-backup-dumps.py
Posted by Masaru Tsuchiyama <m....@gmail.com>.
Hello
> I guess you don't need --non-interactive because standard input won't
> be a terminal. But you need a way to set the --username/--password
> args and maybe --trust-server-cert options to svnrdump.
>
> Otherwise looks ok to me.
>
Thank you for the comment.
I attach a fixed patch.
[[[
add support for svnrdump to svn-backup-dumps.py
* tools/server-side/svn-backup-dumps.py
() : fix comment
() : update to ver 0.7
() : import urlparse
(SvnBackup.__init__) : fix message in SvnBackupException constructor.
(SvnBackup.__init__) : check whether the second parameter is a local
path or an URL.
(SvnBackup.__init__) : check whether --deltas option is specified when
the second parameter is an URL.
(SvnBackup.__init__) : set svn and svnrdump paths.
(SvnBackup.__init__) : set flags for --trust-server-cert and
--no-auth-cache.
(SvnBackup.exec_cmd_nt) : open null device and pass it to Popen
when printerr is False.
(SvnBackup.get_extra_param) : get extra option for svn and svnrdump.
always pass --non-interactive to
svn and svnrdump to make sure.
(SvnBackup.get_head_rev) : rename this to get_head_rev_for_local.
(SvnBackup.get_head_rev_for_url) : get HEAD revision for an URL.
(SvnBackup.get_head_rev) : call get_head_rev_for_local for a local
path, get_head_rev_for_url for an URL
(SvnBackup.create_dump) : invoke svnadmin for a local path,
svnrdump for an URL.
() : fix help message.
() : add --svnrdump-path and --svn-path.
() : add --trust-server-cert.
() : add --no-auth-cache.
]]]
Regards.
--
Masaru Tsuchiyama <m....@gmail.com>
Re: [PATCH] add support for svnrdump to svn-backup-dumps.py
Posted by Ben Reser <be...@reser.org>.
On Sat, Aug 3, 2013 at 5:36 PM, Masaru Tsuchiyama <m....@gmail.com> wrote:
> Hello
>
> I attach a patch to add support for svnrdump to svn-backup-dumps.py.
>
> [[[
> add support for svnrdump to svn-backup-dumps.py
>
> * tools/server-side/svn-backup-dumps.py
> () : fix comment
> () : update to ver 0.7
> () : import urlparse
> (SvnBackup.__init__) : fix message in SvnBackupException constructor.
> (SvnBackup.__init__) : check whether the second parameter is a local
> path or an URL.
> (SvnBackup.__init__) : check whether --deltas option is specified when
> the second parameter is an URL.
> (SvnBackup.__init__) : set svn and svnrdump paths.
> (SvnBackup.exec_cmd_nt) : open null device and pass it to Popen
> when printerr is False.
> (SvnBackup.get_head_rev) : rename this to get_head_rev_for_local.
> (SvnBackup.get_head_rev_for_url) : get HEAD revision for an URL.
> (SvnBackup.get_head_rev) : call get_head_rev_for_local for a local
> path, get_head_rev_for_url for an URL
> (SvnBackup.create_dump) : invoke svnadmin for a local path,
> svnrdump for an URL.
> () : fix help message.
> () : add --svnrdump-path and --svn-path.
> ]]]
I guess you don't need --non-interactive because standard input won't
be a terminal. But you need a way to set the --username/--password
args and maybe --trust-server-cert options to svnrdump.
Otherwise looks ok to me.
Re: [PATCH] add support for svnrdump to svn-backup-dumps.py
Posted by Masaru Tsuchiyama <m....@gmail.com>.
Hi.
Does anybody comment my patch?
(2013/08/04 9:36), Masaru Tsuchiyama wrote:
> Hello
>
> I attach a patch to add support for svnrdump to svn-backup-dumps.py.
>
> [[[
> add support for svnrdump to svn-backup-dumps.py
>
> * tools/server-side/svn-backup-dumps.py
> () : fix comment
> () : update to ver 0.7
> () : import urlparse
> (SvnBackup.__init__) : fix message in SvnBackupException constructor.
> (SvnBackup.__init__) : check whether the second parameter is a local
> path or an URL.
> (SvnBackup.__init__) : check whether --deltas option is specified when
> the second parameter is an URL.
> (SvnBackup.__init__) : set svn and svnrdump paths.
> (SvnBackup.exec_cmd_nt) : open null device and pass it to Popen
> when printerr is False.
> (SvnBackup.get_head_rev) : rename this to get_head_rev_for_local.
> (SvnBackup.get_head_rev_for_url) : get HEAD revision for an URL.
> (SvnBackup.get_head_rev) : call get_head_rev_for_local for a local
> path, get_head_rev_for_url for an URL
> (SvnBackup.create_dump) : invoke svnadmin for a local path,
> svnrdump for an URL.
> () : fix help message.
> () : add --svnrdump-path and --svn-path.
> ]]]
>
> Regards.
>
--
Masaru Tsuchiyama <m....@gmail.com>