On Sun, Mar 29, 2009 at 10:46:40PM +0800, Edmund Wong wrote:
> Here's a cleaner patch with help from Arfrever, Stefan, and Daniel.
>
> Notes on changes:
> - Did away the whole format_nbr check. Since the format
> file was already being overwritten/created,
> there's little point in duplicating so much
> code in both cases (format file exists or not).
>
> - Added entries_parsed variable to help determine
> if the entries file was parsed correctly. If not,
> the format file is also skipped.
>
> Thanks for your patience, guys.
>
> Index: change-svn-wc-format.py
> ===================================================================
> --- change-svn-wc-format.py (revision 36838)
> +++ change-svn-wc-format.py (working copy)
> @@ -84,7 +84,8 @@
> if self.verbosity:
> print("Processing directory '%s'" % dirname)
> entries = Entries(os.path.join(dirname, get_adm_dir(), "entries"))
> -
> +
You're adding whitespace above, not necessary.
> + entries_parsed = True
> if self.verbosity:
> print("Parsing file '%s'" % entries.path)
> try:
> @@ -94,7 +95,17 @@
> raise
> sys.stderr.write("%s, skipping\n" % e)
> sys.stderr.flush()
> + entries_parsed = False
>
> + if not entries_parsed:
I think you want
if entries_parsed:
> + format = Format(os.path.join(dirname, get_adm_dir(), "format"))
> + if self.verbosity:
> + print("Updating file '%s'" % format.path)
> + format.write_format(format_nbr, self.verbosity)
> + else:
> + if self.verbosity:
> + print("Skipping file '%s'" % format.path)
> +
> if self.verbosity:
> print("Checking whether WC format can be converted")
> try:
> @@ -298,7 +309,26 @@
> 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):
> + format_string = '%d\n'
> + if os.path.exists(self.path):
> + if verbosity >= 1:
> + print("%s file will be updated." % self.path)
Saying 'file' is redundant. We can just say:
print("%s will be updated." % self.path)
> + else:
> + if verbosity >= 1:
> + print("%s does not exist, creating it." % self.path)
> + format = open(self.path, "w")
Does this open() call work if the file has perms 0400 before we
try to open it? Can you test what happens if the user has no
write permission to the file (e.g. the file is owned by a
different user)?
> + os.chmod(self.path,0600)
> + format.write(format_string % format_nbr)
> + format.close()
> + os.chmod(self.path, 0400)
> +
> class LocalException(Exception):
> """Root of local exception class hierarchy."""
> pass
Otherwise it's great.
Can you also provide a log message as described in
http://subversion.tigris.org/hacking.html#patches ?
Thanks,
Stefan
Received on 2009-03-29 23:10:21 CEST