Re: [PATCH] svnlook.py: Make it usable as a library

anatoly techtonik wrote:
>>  -__version__ = '2012.05.24'
>
> Because svnlook.py is used in hook scripts, it is usually copied
> out of revision control. Version is useful to track new changes.

That makes sense. I'm not sure what to put in the __version__ string, but the most common way to identify the version of other scripts in the 'tools' directory in our repository seems to be by adding these three lines:

# $HeadURL$
# $LastChangedDate$
# $LastChangedRevision$

Would you be happy with this? I've committed that in r1541878, but am happy to change it.

>>  @@ -76,7 +74,7 @@ class SVNLook(object):
>>         self.txn_ptr = None
>>       if rev is None:
>>         rev = fs.youngest_rev(self.fs_ptr)
>>  -    self.rev = int(rev)
>>  +    self.rev = rev
>
> I believe that this is critical for some API calls and arithmetic that
> self.rev is not a string.

OK, so this is basically for the convenience of callers that might want to pass a string. I'll do it like this:

     # if set, txn takes precendence
     if txn:
       self.txn_ptr = fs.open_txn(self.fs_ptr, txn)
     else:
       self.txn_ptr = None
       if rev is None:
         rev = fs.youngest_rev(self.fs_ptr)
+      else:
+        rev = int(rev)
     self.rev = rev

Also related to revision numbers, I noticed that the 'changed' and 'dirs-changed' and 'diff' commands would fail with rev=0, because they tried to set base_rev=-1. To fix those, I'll add:

   def _walk_tree(self, e_factory, base_rev=None, pass_root=0, callback=None):
     if base_rev is None:
       # a specific base rev was not provided. use the transaction base,
       # or the previous revision
       if self.txn_ptr:
         base_rev = fs.txn_base_revision(self.txn_ptr)
+      elif self.rev == 0:
+        base_rev = 0
       else:
         base_rev = self.rev - 1

(With the current code, a 'return' statement would be equivalent, but this way avoids making assumptions.)

I committed these two changes (and a tweak to the doc string) in revision 1541873.

>>     def cmd_date(self):
>>  -    # duplicate original svnlook format, which is
>>  -    # 2010-02-08 21:53:15 +0200 (Mon, 08 Feb 2010)
>>      secs = self.get_date(unixtime=True)
>>  -    # convert to tuple, detect time zone and format
>>  -    stamp = time.localtime(secs)
>>  -    isdst = stamp.tm_isdst
>>  -    utcoffset = -(time.altzone if (time.daylight and isdst) else
> time.timezone) // 60
>>  -
>>  -    suffix = "%+03d%02d" % (utcoffset // 60, abs(utcoffset) % 60)
>>  -    outstr = time.strftime('%Y-%m-%d %H:%M:%S ', stamp) + suffix
>>  -    outstr += time.strftime(' (%a, %d %b %Y)', stamp)
>>  -    print(outstr)
>>  +    if secs is None:
>>  +      print("")
>>  +    else:
>>  +      # assume secs in local TZ, convert to tuple, and format
>>  +      ### we don't really know the TZ, do we?
>>  +      print(time.strftime('%Y-%m-%d %H:%M', time.localtime(secs)))
>
> I believe that svnlook.py output was different than of svnlook, but I may
> mistake. Anyway, this change seems ok.

You are right that the output was (and is) different. I am happy to change the format to be the same as 'svnlook'); the only problems were (1) it was an unrelated change and (2) it broke if a revision had no date property.

I have added "if secs is None: print('')" to fix the latter, and committed it as revision 1541877.

>>    def get_date(self, unixtime=False):
>>      """return commit timestamp in RFC 3339 format
> (2010-02-08T20:37:25.195000Z)
>>         if unixtime is True, return unix timestamp
>>  -       return None if date is not set (txn properties)
>>  +       return None for a txn, or if date property is not set
>
> What are other cases when date may be empty?

It's possible for a committed revision to not have a svn:date property. Although the property is always created on commit, it can be removed by "svn propdel --revprop -r X svn:date".

> Anyway, LGTM without any more fixes.

Thanks. Let me know if you would like any more changes.

- Julian
Received on 2013-11-14 12:13:49 CET