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

[David James]

On 3/2/06, Giovanni Bajo <rasky at develer.com> wrote:
> +1 on the general idea and thanks for the cleanup. Some comments:
Thanks! I committed my patch just before I saw your mail, in r18695.
I've committed a followup in r18696 to address your suggestions.

> >> +def find_changes(url, begin, end, show_merges=False):
>
> find_merges, more than show_merges. It doesn't really show anything.
Yes, this is better.

>
> >> +    revs = []
> >> +    merges = {}
>
> Move these immediatly above the loop that fills them.
Done.

>
> >> +    rev = None
>
> Unneded.
Removed.

>
> >> +    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.
Done.

> >> +            if merge_props != previous_merge_props:
> >> +                merges[rev] = merge_props
>
> Forgot to update previous_merge_props.
Thanks -- fortunately, I caught this before committing so it's fixed in r18695.

> >>+        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
Yes, that is better. I've updated the code to follow your suggestions.

Cheers,

David


--
David James -- http://www.cs.toronto.edu/~james