Re: [PATCH] Add dirs to subject of mailer.py

On Sun, Jan 12, 2003 at 09:43:36PM -0000, Chris Foote wrote:
> This patch adds the directories to the subject line of the mail.

Grr... I didn't see this until after I wrote the same thing :-(

For future reference, though, I can provide some feedback on this patch. In
fact, there is something that applies to svnlook, too:

>...
> + dirschanged = []
> + for fname, change in changes.items():
> + if change.item_type == ChangeCollector.FILE:
> + fname = _svn_dirname(fname)
> + if fname != '/':
> + fname = re.sub(r'^(.+)[/\\]$', r'\1', fname)

This re.sub() is way more expensive than simply testing the last character
for a slash and slicing it off. For example:

  if fname != '/' and fname[-1] == '/':
    fname = fname[:-1]

(prolly still gotta watch out for the empty string; that is actually the
 path for root. so maybe insert an "and fname" in that if stmt).

> + if fname not in dirschanged:
> + dirschanged.append(fname)

oof. To generate a set of unique items in Python, it will be much faster to
use a dictionary. The "not in dirschanged" is a linear scan over the list.
Thus, you can do something like:

    dirschanged = { }
    ...
      dirchanges[fname] = None

Then, to get the list:

    dirschange = dirschanged.keys()

> + dirschanged.sort()
> +
> + commondir = ''
> + if '/' not in dirschanged and len(dirschanged) > 1:
> + firstline = dirschanged.pop(0)

I did the same pop(), but it doesn't matter which end the thing comes from,
so I just dropped the 0 out.

> + commonpieces = firstline.split('/')
> + for line in dirschanged:
> + pieces = line.split('/')
> + i = 0
> + while i < len(pieces) and i < len(commonpieces):
> + if pieces[i] != commonpieces[i]:
> + commonpieces = commonpieces[:i]
> + break
> + i+=1

This algorithm, which svnlook uses, too, doesn't quite work right. If
"pieces" is shorter than "commonpieces", then commonpieces will never get
trimmed down.

Note that I did:

    del commonpieces[i:]

That modifies the list in place rather than copying.

> +
> + dirschanged = [firstline] + dirschanged

I didn't bother to save firstline, and just rebuilt the dirschanged from the
keys of the dictionary that I used. (or, if stripping common bits, then just
building a list from the stripped paths)

> + if len(commonpieces):
> + commondir = string.join(commonpieces, '/')
> + new_dirschanged = []
> + for dir in dirschanged:
> + if dir == commondir:
> + dir = '.'
> + else:
> + dir = string.replace(dir, commondir + '/', "")

Faster/easier to use string slices here.

I definitely appreciate you taking the time to develop the patch! Stupid
race condition, though :-( Go ahead and check out my update and take a look
and provide any feedback/testing.

Thanks!

Cheers,
-g

-- 
Greg Stein, http://www.lyra.org/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org