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 = 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
> https://ctf.open.collab.net/svn/repos/svnedge/trunk/console
> and got the following output
>
> [kamesh_at_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_at_kamesh console]$ python /tmp/mergeinfo-sanitizer.py --fix .
> [kamesh_at_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.apache.org/repos/asf/subversion/trunk
>
> And got the following output
> <snip>
> The path
> /subversion/branches/diff-optimizations/subversion/libsvn_subr/adler32.c
> is not found
> The path
> /subversion/branches/diff-optimizations/subversion/include/private/svn_adler32.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:
> 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_adler32.h:
> /subversion/branches/diff-optimizations-bytes/subversion/libsvn_subr/adler32.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/include/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 == 3:
> Use the named constant than the arbitrary value.
>
> > if depth == 3:
> > for entry in mergeinfo_catalog:
> > pathwise_mergeinfo = pathwise_mergeinfo +
> mergeinfo_catalog[entry] + "\n"
>
>
> I think you do something wrong with the above concatenation of
> mergeinfos of different targets.
>
> > else:
> > pathwise_mergeinfo = 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,
> revision_range.start + 1, receiver)
> > except svn.core.SubversionException:
> > try:
> > ra.get_location_segments(ra_session, "",
> core.SVN_INVALID_REVNUM,
> > 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
Received on 2011-09-19 18:55:11 CEST