On Sun, Mar 29, 2009 at 01:30:32AM +0800, Edmund Wong wrote:
> My second attempt at the patch. It's now a proper svn diff
> file.
Thanks!
> Notes: Writes the format_nbr, regardless of what the
> previous value was (and if the format file doesn't
> exist, it creates it with the right value, which
> was the initial reason for this patch -- to create
> the format file)
>
> Since format_nbr is fed into this format:write_format(),
> it's a 'valid' value otherwise it would've choked prior to
> the execution of the format part, no?
>
> I'm still going over it, but I'd like some comments. Should
> I add more code to check the format_nbr, or is it redundant?
The number is already verified in main(), so there's no need
to verify it again.
> Index: change-svn-wc-format.py
> ===================================================================
> --- change-svn-wc-format.py (revision 36814)
> +++ change-svn-wc-format.py (working copy)
> @@ -95,7 +95,18 @@
> sys.stderr.write("%s, skipping\n" % e)
> sys.stderr.flush()
At this point, we might want to skip the creation of the format
file if parsing the entries file failed.
>
> + format = Format(os.path.join(dirname, get_adm_dir(), "format"))
> if self.verbosity:
> + print("Updating file '%s'" % format.path)
> + try:
> + format.write_format(format_nbr, self.verbosity)
> + except UnrecognizedWCFormatException, e:
Maybe I'm missing something, but under what conditions will
format.write_format() raise an UnrecognizedWCFormatException?
It looks like this exception will only be raised by the
write_format() method in the Entries class.
So I think we can get rid of the try-except clause around
format.write_format(format_nbr, self.verbosity)
and the following four lines:
> + if self.error_on_unrecognized:
> + raise
> + sys.stderr.write("%s, skipping\n" % e)
> + sys.stderr.flush()
> +
> + if self.verbosity:
> print("Checking whether WC format can be converted")
> try:
> entries.assert_valid_format(format_nbr, self.verbosity)
> @@ -298,7 +309,41 @@
> rep += "[%s] %s\n" % (Entries.entry_fields[i], self.fields[i])
> return rep
>
> +class Format:
> + """Represents a .svn/format file."""
>
> + def __init__(self, path):
> + self.path = path
> +
> + def write_format(self, format_nbr, verbosity=0):
> + if (len(str(format_nbr))) < 2:
> + frmt_val = '%02'
> + else:
> + frmt_val = '%01'
We can save a line and a variable here by doing:
if (len(str(format_nbr))) < 2:
format_string = '%02d'
else:
format_string = '%01d'
and getting rid of this line:
> + format_string = frmt_val + 'd'
> + # Overwrite all bytes of the format number.
> + if os.path.exists(self.path):
> + format = open(self.path,"r")
> + os.chmod(self.path, 0600)
> + format_line = format.readline()
> + format_line.rstrip()
> + format_nbr_old = int(format_line)
> + format.close()
> + format = open(self.path, "w")
> + format.write(format_string % format_nbr)
> + if verbosity >= 2:
> + print("format file has been updated.")
> + else:
> + if verbosity >= 1:
> + print("Format file does not exist. Creating it now.")
Here we could use either lower case ("format file ..."),
or simply print the filename:
print("%s does not exist, creating it." % self.path)
> + format = open(self.path, "w")
> + os.chmod(self.path,0600)
> + format.write(format_string % format_nbr)
> + if verbosity >= 1:
> + print("Format file has been created.")
The above print statement is somewhat redundant.
We've already told the user we were going to create the file,
and there's nothing much that can go wrong between then and now.
Except e.g. write permission problems, but then we'll probably
get an exception we don't catch anyway, which the user will end
up seeing, right?
Stefan
> + format.close()
> + os.chmod(self.path, 0400)
> +
> class LocalException(Exception):
> """Root of local exception class hierarchy."""
> pass
Received on 2009-03-28 22:40:00 CET