[Svnmerge] Re: [PATCH] svnmerge.py: Refactor analyze_revs

[Giovanni Bajo]

>David James <djames at collab.net> wrote:

>> [[[
>> Refactor analyze_revs, creating a separate function called
>> "find_changes" for finding which revisions and merges affect a given
>> URL.
>>
>> * contrib/client-side/svnmerge.py
>>   (find_changes): New function.
>>   (analyze_revs): Use find_changes function.
>> ]]]


+1 on the general idea and thanks for the cleanup. Some comments:

>> +def find_changes(url, begin, end, show_merges=False):

find_merges, more than show_merges. It doesn't really show anything.

>> +    revs = []
>> +    merges = {}

Move these immediatly above the loop that fills them.

>> +    rev = None

Unneded.

>> +    previous_merge_props = {}

The start value should probably be None, as you don't know the state of the
property before the first change that modifies it in the [begin, end] range.

>> +    for line in launchsvn("log %s" % log_opts):

Many thanks for using this form, as launchsvn() will change to return a pipe
ASAP.


>> +            if merge_props != previous_merge_props:
>> +                merges[rev] = merge_props

Forgot to update previous_merge_props.


>>+        old_props = {}
>>+        for rev in merge_revs:
>>+            new_props = merges[rev]
>>             old_revisions = old_props.get(target_dir)
>>             new_revisions = new_props.get(target_dir)
>>
>>             if new_revisions != old_revisions:
>>                 reflected_revs.append("%s" % rev)
>>
>>+            old_props = new_props

I might be wrong, but isns't old_revisions enough?

old_revs = None
for r in merge_revs:
    new_revs = merges[r].get(target_dir)
    if new_revs != old_revs:
       reflected_revs.append(str(r))
   old_revs = new_revs

Giovanni Bajo