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

[Giovanni Bajo <ra...@develer.com>]

>David James <dj...@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


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

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

[Giovanni Bajo <ra...@develer.com>]

David James <dj...@collab.net> 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.


Thanks!
To me, it's fine to address comments either in followups or before the actual
commit, as long as we still go through the code review process.

Giovanni Bajo


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

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

[David James <dj...@collab.net>]

On 3/2/06, Giovanni Bajo <ra...@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