Re: [PATCH] add support for svnrdump to svn-backup-dumps.py

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
Received on 2013-08-08 15:26:07 CEST