You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by David James <dj...@collab.net> on 2006/06/12 21:29:02 UTC
Re: [Svnmerge] [PATCH] Fix for bidirectional merging corner cases
On 5/13/06, Raman Gupta <ro...@fastmail.fm> wrote:
> * contrib/client-side/svnmerge.py
> (analyze_revs): If old_revs is not initialized in the loop that checks revisions
> in which the merge props changed, initialize it. This prevents the first revision
> from being considered reflected even if it is not.
Nice catch, Raman! Could you convert your 'template.sh' test into a
regular test that will run in svnmerge_test.py? If not, don't worry --
I'll get to it.
> Index: svnmerge.py
> [...]
> old_revs = None
> for rev in merge_metadata.changed_revs():
> new_revs = merge_metadata.get(rev).get(target_dir)
> + if old_revs is None:
> + old_revs = get_revlist_prop(url, opts["prop"], rev-1).get(target_dir)
> if new_revs != old_revs:
> reflected_revs.append("%s" % rev)
> old_revs = new_revs
Nice fix! I am sorry that it took me so long to respond to your patch.
There's a small problem, though: What if 'url' was just created in
rev, and therefore does not exist in rev-1? In this case,
get_revlist_prop will throw a LaunchException, and we will need to
deal with this.
I fixed your patch to deal with this problem. I also fixed a similar
bug in the VersionedProperty class and committed your first patch in
r20064.
> Here is a log message/patch for the base + 1 problem (yes, all the tests pass):
>
> Small optimization in obtaining the revisions to be checked for merging. The last
> merged revision is included in analyze_head_revs, but later excluded by the
> algorithm anyway, resulting in some unnecessary work.
>
> * contrib/client-side/svnmerge.py
> (analyze_head_revs): Don't include the last merged revision in the list of
> revisions to be checked for merging.
>
> Patch by: Raman Gupta <ro...@fastmail.fm>
> Review by: ?
>
> Index: svnmerge.py
> [...]
> base = 1
> r = opts["merged-revs"].normalized()
> if r and r[0][0] == 1:
> - base = r[0][1]
> + base = r[0][1] + 1
Looks good. I committed your optimization in r20065.
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
Re: [Svnmerge] [PATCH] Fix for bidirectional merging corner cases
Posted by David James <dj...@collab.net>.
> David James wrote:
> > Nice catch, Raman! Could you convert your 'template.sh' test into a
> > regular test that will run in svnmerge_test.py? If not, don't worry --
> > I'll get to it.
> Sure, but it might take me a few days...
Thanks! CC me when you submit your patch.
> In this new section of code:
>
> try:
> old_value = self.raw_get(log.begin-1)
> except LaunchError:
> # The specified URL might not exist before the
> # range of the log. If so, we can safely assume
> # that the property was empty at that time.
> old_value = { }
>
> Wouldn't it be better to check the situation of the non-existent URL
> explicitly rather than relying on LaunchError? LaunchError might occur
> for other reasons.
Yes, definitely! Can you think of a clean way to implement that?
> In this section of code (the one which my original patch addressed),
> my patch was setting old_revs only for the first loop -- it looks like
> here some extra work is being done because old_revs is initialized at
> the beginning of the loop every time, which shouldn't be required
> because of the last line in the loop:
>
> for rev in merge_metadata.changed_revs():
> old_revs = merge_metadata.get(rev-1).get(target_dir)
> new_revs = merge_metadata.get(rev).get(target_dir)
> if new_revs != old_revs:
> reflected_revs.append("%s" % rev)
> old_revs = new_revs
merge_metadata maintains its own cache of property values, so there's
no need to maintain a separate cache here (and add complexity). Still,
I intended to remove the 'old_revs = new_revs' line because it is not
needed.
I committed a change which simplified and optimized this bit of code
to avoid the cache lookups in r20074, by simply iterating through the
changed revs and changed values.
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
Re: [Svnmerge] [PATCH] Fix for bidirectional merging corner cases
Posted by Raman Gupta <ro...@fastmail.fm>.
David James wrote:
> On 5/13/06, Raman Gupta <ro...@fastmail.fm> wrote:
>> * contrib/client-side/svnmerge.py (analyze_revs): If old_revs is
>> not initialized in the loop that checks revisions in which the
>> merge props changed, initialize it. This prevents the first
>> revision from being considered reflected even if it is not.
>
> Nice catch, Raman! Could you convert your 'template.sh' test into a
> regular test that will run in svnmerge_test.py? If not, don't worry --
> I'll get to it.
Sure, but it might take me a few days...
>> Index: svnmerge.py
>> [...]
>> old_revs = None
>> for rev in merge_metadata.changed_revs():
>> new_revs = merge_metadata.get(rev).get(target_dir)
>> + if old_revs is None:
>> + old_revs = get_revlist_prop(url, opts["prop"],
>> rev-1).get(target_dir)
>> if new_revs != old_revs:
>> reflected_revs.append("%s" % rev)
>> old_revs = new_revs
>
> Nice fix! I am sorry that it took me so long to respond to your patch.
>
> There's a small problem, though: What if 'url' was just created in
> rev, and therefore does not exist in rev-1? In this case,
> get_revlist_prop will throw a LaunchException, and we will need to
> deal with this.
>
> I fixed your patch to deal with this problem. I also fixed a similar
> bug in the VersionedProperty class and committed your first patch in
> r20064.
I'm not too familiar with the relatively new RevisionLog and
VersionedProperty classes, so I can't say I completely follow what
you've done. However, I have a couple of comments anyway.
In this new section of code:
try:
old_value = self.raw_get(log.begin-1)
except LaunchError:
# The specified URL might not exist before the
# range of the log. If so, we can safely assume
# that the property was empty at that time.
old_value = { }
Wouldn't it be better to check the situation of the non-existent URL
explicitly rather than relying on LaunchError? LaunchError might occur
for other reasons.
In this section of code (the one which my original patch addressed),
my patch was setting old_revs only for the first loop -- it looks like
here some extra work is being done because old_revs is initialized at
the beginning of the loop every time, which shouldn't be required
because of the last line in the loop:
for rev in merge_metadata.changed_revs():
old_revs = merge_metadata.get(rev-1).get(target_dir)
new_revs = merge_metadata.get(rev).get(target_dir)
if new_revs != old_revs:
reflected_revs.append("%s" % rev)
old_revs = new_revs
Cheers,
Raman
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org