On 3/2/06, Giovanni Bajo <rasky@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
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Fri Mar 3 03:18:52 2006