Return-Path: X-Original-To: apmail-subversion-dev-archive@minotaur.apache.org Delivered-To: apmail-subversion-dev-archive@minotaur.apache.org Received: from mail.apache.org (hermes.apache.org [140.211.11.3]) by minotaur.apache.org (Postfix) with SMTP id C195E9CC7 for ; Mon, 3 Oct 2011 16:03:58 +0000 (UTC) Received: (qmail 54346 invoked by uid 500); 3 Oct 2011 16:03:58 -0000 Delivered-To: apmail-subversion-dev-archive@subversion.apache.org Received: (qmail 54308 invoked by uid 500); 3 Oct 2011 16:03:58 -0000 Mailing-List: contact dev-help@subversion.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Delivered-To: mailing list dev@subversion.apache.org Received: (qmail 54299 invoked by uid 99); 3 Oct 2011 16:03:58 -0000 Received: from nike.apache.org (HELO nike.apache.org) (192.87.106.230) by apache.org (qpsmtpd/0.29) with ESMTP; Mon, 03 Oct 2011 16:03:58 +0000 X-ASF-Spam-Status: No, hits=2.2 required=5.0 tests=HTML_MESSAGE,SPF_PASS X-Spam-Check-By: apache.org Received-SPF: pass (nike.apache.org: domain of kamesh@collab.net designates 204.11.126.183 as permitted sender) Received: from [204.11.126.183] (HELO maa-exchmb.maa.corp.collab.net) (204.11.126.183) by apache.org (qpsmtpd/0.29) with ESMTP; Mon, 03 Oct 2011 16:03:51 +0000 X-MimeOLE: Produced By Microsoft Exchange V6.5 Content-class: urn:content-classes:message MIME-Version: 1.0 Content-Type: multipart/alternative; boundary="----_=_NextPart_001_01CC81E5.FB6BDE5C" Subject: RE: [PATCH] python script for issue #3961, fixing the bogus mergeinfo Date: Mon, 3 Oct 2011 21:32:08 +0530 Message-ID: <535B15FD639A0F4282F12C1B8F3BF6070462D0@maa-exchmb.maa.corp.collab.net> X-MS-Has-Attach: X-MS-TNEF-Correlator: Thread-Topic: [PATCH] python script for issue #3961, fixing the bogus mergeinfo Thread-Index: Acx27XZ9KrEHBARLTcyhaDEPb44FyQK+FdfD References: <4E4D2984.6010500@collab.net> <4E4DFF65.8050905@collab.net> <4E6DC8F3.8090708@collab.net> <535B15FD639A0F4282F12C1B8F3BF607046270@maa-exchmb.maa.corp.collab.net> <4E7773CA.7030006@collab.net> <4E7774B9.30708@collab.net> From: "Kamesh Jayachandran" To: "Prabhu Gnana Sundar" , X-Virus-Checked: Checked by ClamAV on apache.org This is a multi-part message in MIME format. ------_=_NextPart_001_01CC81E5.FB6BDE5C Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Thanks Prabhu. I fixed your bogus mergeinfo summary output which was always giving the = full mergeinfo. I committed this script in r1178435. With regards Kamesh Jayachandran -----Original Message----- From: Prabhu Gnana Sundar [mailto:prabhugs@collab.net] Sent: Mon 9/19/2011 10:28 PM To: dev@subversion.apache.org Subject: Re: [PATCH] python script for issue #3961, fixing the bogus = mergeinfo =20 Attaching a cleaner patch... Removed a couple of commented lines... Regards Prabhu On Monday 19 September 2011 10:24 PM, Prabhu Gnana Sundar wrote: > Thanks Kamesh for your valuable review... > > > On Monday 12 September 2011 06:25 PM, Kamesh Jayachandran wrote: >> >> I like this patch. We need it. >> > Thank you... >> >> I found this bogus mergeinfo's were causing 404 file not found, >> which were sometimes causing the merge to fail, making it run longer=20 >> than necessary. >> >> Few comments. >> >> >> 1. >> > opts, args =3D my_getopt(sys.argv[1:], "h?fRc", ["help", "fix",=20 >> "recursive", "commit"]) >> >> If you do not want to support 'commit' you can remove it as well. >> > Yeah... I removed it now.. >> >> >> 2. I ran it against a working copy of=20 >> https://ctf.open.collab.net/svn/repos/svnedge/trunk/console >> and got the following output >> >> [kamesh@kamesh console]$ python /tmp/mergeinfo-sanitizer.py . >> The mergeinfo has to be sanitized/reverse-merged for the following = as: >> /branches/CTF_MODE/SvnEdge: 515-766, >> >> >> [kamesh@kamesh console]$ python /tmp/mergeinfo-sanitizer.py --fix . >> [kamesh@kamesh console]$ svn di >> Property changes on: . >> ___________________________________________________________________ >> Modified: svn:mergeinfo >> Reverse-merged /branches/CTF_MODE/SvnEdge:r512-515 >> >> Based on my analysis on the above repo, I could see=20 >> /branches/CTF_MODE/SvnEdge:r512-515 is bogus >> and hence need to be removed. Anywway your script does it correctly=20 >> with --fix. >> >> I would suggest changing the output of default invocation(i.e no = --fix) >> > Changed the output as suggested... > >> >> >> Bogus mergeinfo summary: >> Target 1: . >> /branches/CTF_MODE/SvnEdge: 512-515, >> /merge/source2: X-Y, >> .......... >> >> Target 2: subtree_path >> /merge/source3: A-B >> .... >> >> Target 3: sub_sub_tree >> /merge/source4: C-D >> ...... >> >> Run with --fix to fix this in your working copy and commit yourself. >> >> >> 3. Your script seemed to sanitize to the depth of 'immediates' by=20 >> default. What is the rational behind that? >> > No rational behind it, I was using it as default for my own=20 > development purpose. > Now fixed it. > > >> 4. I ran your script against=20 >> https://svn.apache.org/repos/asf/subversion/trunk >> >> And got the following output >> >> The path=20 >> = /subversion/branches/diff-optimizations/subversion/libsvn_subr/adler32.c = >> is not found >> The path=20 >> = /subversion/branches/diff-optimizations/subversion/include/private/svn_ad= ler32.h=20 >> is not found >> >> The mergeinfo has to be sanitized/reverse-merged for the following = as: >> /subversion/trunk/subversion/libsvn_subr/svn_temp_serializer.c: >> /subversion/branches/tree-conflicts/subversion/include/svn_string.h:=20 >> 872524-873154,868290-872329, >> /subversion/trunk/subversion/include/private/svn_temp_serializer.h: >> = /subversion/branches/diff-optimizations/subversion/libsvn_subr/adler32.c:= >> /subversion/branches/tree-conflicts/subversion/libsvn_diff/diff.h:=20 >> 872524-873154,868290-872329, >> /subversion/trunk/subversion/libsvn_fs_fs/temp_serializer.h: >> /subversion/branches/svn-patch-improvements: 918520-934609, >> /subversion/trunk/subversion/libsvn_fs_fs/temp_serializer.c: >> /subversion/branches/tree-conflicts/subversion/libsvn_subr/hash.c:=20 >> 872524-873154,868290-872329, >> = /subversion/branches/diff-optimizations/subversion/include/svn_string.h: = >> 1031270-1037352, >> = /subversion/branches/diff-optimizations-bytes/subversion/include/private/= svn_adler32.h:=20 >> 1054361-1067789, >> = /subversion/branches/svn-patch-improvements/subversion/include/svn_string= .h:=20 >> 918520-934609, >> = /subversion/branches/diff-optimizations/subversion/libsvn_subr/hash.c: = 1031270-1037352, >> /subversion/branches/tree-conflicts: 872524-873154,868290-872329, >> = /subversion/branches/diff-optimizations/subversion/include/private/svn_ad= ler32.h: >> = /subversion/branches/diff-optimizations-bytes/subversion/libsvn_subr/adle= r32.c:=20 >> 1054361-1067789, >> = /subversion/branches/svn-patch-improvements/subversion/libsvn_diff/diff.h= :=20 >> 918520-934609, >> /subversion/branches/diff-optimizations: 1031270-1037352, >> = /subversion/branches/svn-patch-improvements/subversion/libsvn_subr/hash.c= :=20 >> 918520-934609, >> = /subversion/branches/integrate-cache-item-serialization/subversion/libsvn= _fs_fs/temp_serializer.h:=20 >> 1068738-1068739, >> = /subversion/branches/integrate-cache-item-serialization/subversion/libsvn= _subr/svn_temp_serializer.c:=20 >> 1068723-1068739, >> = /subversion/branches/integrate-cache-item-serialization/subversion/libsvn= _fs_fs/temp_serializer.c:=20 >> 1068738-1068739, >> /subversion/trunk/subversion/libsvn_diff/diff.h: 1037352-1054248, >> /subversion/trunk/subversion/libsvn_subr/adler32.c: 1054276-1054360, >> /subversion/trunk/subversion/include/private/svn_adler32.h:=20 >> 1054276-1054360, >> = /subversion/branches/integrate-cache-item-serialization/subversion/includ= e/private/svn_temp_serializer.h:=20 >> 1068723-1068739, >> /subversion/trunk/subversion/libsvn_diff/util.c: 1037352-1054251, >> >> >> path not found error need to be handled. error should be on STDERR. >> > now it is being handled and sent to the stderr... > >> 5. In main() you can call check_local_modifications before=20 >> get_original_mergeinfo(). >> > That sounds really good though I don't see 'propget' as a costly=20 > operation. > >> 6. In get_original_mergeinfo() >> >> > if depth =3D=3D 3: >> Use the named constant than the arbitrary value. >> >> > if depth =3D=3D 3: >> > for entry in mergeinfo_catalog: >> > pathwise_mergeinfo =3D pathwise_mergeinfo +=20 >> mergeinfo_catalog[entry] + "\n" >> >> >> I think you do something wrong with the above concatenation of=20 >> mergeinfos of different targets. >> >> > else: >> > pathwise_mergeinfo =3D mergeinfo_catalog[wcpath] >> >> > return core.svn_mergeinfo_parse(pathwise_mergeinfo, temp_pool) >> > Thanks for pointing this Kamesh... Fixed this in the attached patch... >> >> >> 7. In get_new_location_segments(): >> >> > for revision_range in parsed_original_mergeinfo[path]: >> > try: >> > ra.get_location_segments(ra_session, "", = revision_range.end, >> > revision_range.end,=20 >> revision_range.start + 1, receiver) >> > except svn.core.SubversionException: >> > try: >> > ra.get_location_segments(ra_session, "",=20 >> core.SVN_INVALID_REVNUM, >> > revision_range.end,=20 >> revision_range.start + 1, receiver) >> >> Not sure why you run location segments against HEAD upon exception. >> > I was just thinking that the source path could even be present outside = > the revision-ranges. > But looks like I am doing something weird here... So fixed it by=20 > removing the part that runs > the location segments against the HEAD... >> >> >> >> > except svn.core.SubversionException: >> > print " The path %s is not found " % path >> >> 8. Function parse_args is not used. >> > Removed it. This was mistakenly left out... >> >> >> 9. Function receiver can have more meaningful name. >> > Changed the name ... >> >> >> >> 10. You need to organize your functions in such a way that it appears = >> in some logical order. >> For example I started my review from 'main()' which is in the middle=20 >> of the file and I move above and below that function to review = further. >> > Organized :) > > > > Attached the recent patch with this mail... Please share your = thoughts... > > > Thanks and regards > Prabhu ------_=_NextPart_001_01CC81E5.FB6BDE5C Content-Type: text/html; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable RE: [PATCH] python script for issue #3961, fixing the bogus = mergeinfo

Thanks Prabhu.

I fixed your bogus mergeinfo summary output which was always giving the = full mergeinfo.

I committed this script in r1178435.

With regards
Kamesh Jayachandran




-----Original Message-----
From: Prabhu Gnana Sundar [mailto:prabhugs@collab.net]
Sent: Mon 9/19/2011 10:28 PM
To: dev@subversion.apache.org
Subject: Re: [PATCH] python script for issue #3961, fixing the bogus = mergeinfo


Attaching a cleaner patch... Removed a couple of commented lines...


Regards
Prabhu



On Monday 19 September 2011 10:24 PM, Prabhu Gnana Sundar wrote:
> Thanks Kamesh for your valuable review...
>
>
> On Monday 12 September 2011 06:25 PM, Kamesh Jayachandran = wrote:
>>
>> I like this patch. We need it.
>>
> Thank you...
>>
>> I found this bogus mergeinfo's were causing 404 file not = found,
>> which were sometimes causing the merge to fail, making it run = longer
>> than necessary.
>>
>> Few comments.
>>
>>
>> 1.
>> >    opts, args =3D my_getopt(sys.argv[1:], = "h?fRc", ["help", "fix",
>> "recursive", "commit"])
>>
>> If you do not want to support 'commit' you can remove it as = well.
>>
> Yeah... I removed it now..
>>
>>
>> 2. I ran it against a working copy of
>> http= s://ctf.open.collab.net/svn/repos/svnedge/trunk/console
>> and got the following output
>>
>> [kamesh@kamesh console]$ python  = /tmp/mergeinfo-sanitizer.py .
>> The mergeinfo has to be sanitized/reverse-merged for the = following as:
>> /branches/CTF_MODE/SvnEdge: 515-766,
>>
>>
>> [kamesh@kamesh console]$ python  = /tmp/mergeinfo-sanitizer.py --fix .
>> [kamesh@kamesh console]$ svn di
>> Property changes on: .
>> = ___________________________________________________________________
>> Modified: svn:mergeinfo
>>    Reverse-merged = /branches/CTF_MODE/SvnEdge:r512-515
>>
>> Based on my analysis on the above repo, I could see
>> /branches/CTF_MODE/SvnEdge:r512-515 is bogus
>> and hence need to be removed. Anywway your script does it = correctly
>> with --fix.
>>
>> I would suggest changing the output of default invocation(i.e = no --fix)
>>
> Changed the output as suggested...
>
>>
>> <snip>
>> Bogus mergeinfo summary:
>> Target 1: .
>> /branches/CTF_MODE/SvnEdge: 512-515,
>> /merge/source2: X-Y,
>> ..........
>>
>> Target 2: subtree_path
>> /merge/source3: A-B
>> ....
>>
>> Target 3: sub_sub_tree
>> /merge/source4: C-D
>> ......
>>
>> Run with --fix to fix this in your working copy and commit = yourself.
>>
>>
>> 3. Your script seemed to sanitize to the depth of 'immediates' = by
>> default. What is the rational behind that?
>>
> No rational behind it, I was using it as default for my own
> development purpose.
> Now fixed it.
>
>
>> 4. I ran your script against
>> https://svn.ap= ache.org/repos/asf/subversion/trunk
>>
>> And got the following output
>> <snip>
>>  The path
>> = /subversion/branches/diff-optimizations/subversion/libsvn_subr/adler32.c<= BR> >> is not found
>>  The path
>> = /subversion/branches/diff-optimizations/subversion/include/private/svn_ad= ler32.h
>> is not found
>>
>> The mergeinfo has to be sanitized/reverse-merged for the = following as:
>> = /subversion/trunk/subversion/libsvn_subr/svn_temp_serializer.c:
>> = /subversion/branches/tree-conflicts/subversion/include/svn_string.h:
>> 872524-873154,868290-872329,
>> = /subversion/trunk/subversion/include/private/svn_temp_serializer.h:
>> = /subversion/branches/diff-optimizations/subversion/libsvn_subr/adler32.c:=
>> = /subversion/branches/tree-conflicts/subversion/libsvn_diff/diff.h:
>> 872524-873154,868290-872329,
>> = /subversion/trunk/subversion/libsvn_fs_fs/temp_serializer.h:
>> /subversion/branches/svn-patch-improvements: 918520-934609,
>> = /subversion/trunk/subversion/libsvn_fs_fs/temp_serializer.c:
>> = /subversion/branches/tree-conflicts/subversion/libsvn_subr/hash.c:
>> 872524-873154,868290-872329,
>> = /subversion/branches/diff-optimizations/subversion/include/svn_string.h:<= BR> >> 1031270-1037352,
>> = /subversion/branches/diff-optimizations-bytes/subversion/include/private/= svn_adler32.h:
>> 1054361-1067789,
>> = /subversion/branches/svn-patch-improvements/subversion/include/svn_string= .h:
>> 918520-934609,
>> = /subversion/branches/diff-optimizations/subversion/libsvn_subr/hash.c: = 1031270-1037352,
>> /subversion/branches/tree-conflicts: = 872524-873154,868290-872329,
>> = /subversion/branches/diff-optimizations/subversion/include/private/svn_ad= ler32.h:
>> = /subversion/branches/diff-optimizations-bytes/subversion/libsvn_subr/adle= r32.c:
>> 1054361-1067789,
>> = /subversion/branches/svn-patch-improvements/subversion/libsvn_diff/diff.h= :
>> 918520-934609,
>> /subversion/branches/diff-optimizations: 1031270-1037352,
>> = /subversion/branches/svn-patch-improvements/subversion/libsvn_subr/hash.c= :
>> 918520-934609,
>> = /subversion/branches/integrate-cache-item-serialization/subversion/libsvn= _fs_fs/temp_serializer.h:
>> 1068738-1068739,
>> = /subversion/branches/integrate-cache-item-serialization/subversion/libsvn= _subr/svn_temp_serializer.c:
>> 1068723-1068739,
>> = /subversion/branches/integrate-cache-item-serialization/subversion/libsvn= _fs_fs/temp_serializer.c:
>> 1068738-1068739,
>> /subversion/trunk/subversion/libsvn_diff/diff.h: = 1037352-1054248,
>> /subversion/trunk/subversion/libsvn_subr/adler32.c: = 1054276-1054360,
>> /subversion/trunk/subversion/include/private/svn_adler32.h:
>> 1054276-1054360,
>> = /subversion/branches/integrate-cache-item-serialization/subversion/includ= e/private/svn_temp_serializer.h:
>> 1068723-1068739,
>> /subversion/trunk/subversion/libsvn_diff/util.c: = 1037352-1054251,
>> </snip>
>>
>> path not found error need to be handled. error should be on = STDERR.
>>
> now it is being handled and sent to the stderr...
>
>> 5. In main() you can call check_local_modifications before
>> get_original_mergeinfo().
>>
> That sounds really good though I don't see 'propget' as a = costly
> operation.
>
>> 6. In get_original_mergeinfo()
>>
>> > if depth =3D=3D 3:
>> Use the named constant than the arbitrary value.
>>
>> >  if depth =3D=3D 3:
>> >    for entry in mergeinfo_catalog:
>> >      pathwise_mergeinfo =3D = pathwise_mergeinfo +
>> mergeinfo_catalog[entry] + "\n"
>>
>>
>> I think you do something wrong with the above concatenation = of
>> mergeinfos of different targets.
>>
>> >  else:
>> >    pathwise_mergeinfo =3D = mergeinfo_catalog[wcpath]
>>
>> >  return core.svn_mergeinfo_parse(pathwise_mergeinfo, = temp_pool)
>>
> Thanks for pointing this Kamesh... Fixed this in the attached = patch...
>>
>>
>> 7. In get_new_location_segments():
>>
>> >      for revision_range in = parsed_original_mergeinfo[path]:
>> >        try:
>> >          = ra.get_location_segments(ra_session, "", = revision_range.end,
>> = >           &nb= sp;           &nbs= p;           = revision_range.end,
>> revision_range.start + 1, receiver)
>> >        except = svn.core.SubversionException:
>> >          = try:
>> = >            = ra.get_location_segments(ra_session, "",
>> core.SVN_INVALID_REVNUM,
>> = >           &nb= sp;           &nbs= p;            = ; revision_range.end,
>> revision_range.start + 1, receiver)
>>
>> Not sure why you run location segments against HEAD upon = exception.
>>
> I was just thinking that the source path could even be present = outside
> the revision-ranges.
> But looks like I am doing something weird here... So fixed it = by
> removing the part that runs
> the location segments against the HEAD...
>>
>>
>>
>> >          = except svn.core.SubversionException:
>> = >            = print " The path %s is not found " % path
>>
>> 8. Function parse_args is not used.
>>
> Removed it. This was mistakenly left out...
>>
>>
>> 9. Function receiver can have more meaningful name.
>>
> Changed the name ...
>>
>>
>>
>> 10. You need to organize your functions in such a way that it = appears
>> in some logical order.
>> For example I started my review from 'main()' which is in the = middle
>> of the file and I move above and below that function to review = further.
>>
> Organized :)
>
>
>
> Attached the recent patch with this mail... Please share your = thoughts...
>
>
> Thanks and regards
> Prabhu

------_=_NextPart_001_01CC81E5.FB6BDE5C--