I like this patch. We need it.
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.
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)
<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?
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.
5. In main() you can call check_local_modifications before get_original_mergeinfo().
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)
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.
> except svn.core.SubversionException:
> print " The path %s is not found " % path
8. Function parse_args is not used.
9. Function receiver can have more meaningful 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.
With regards
Kamesh Jayachandran
-----Original Message-----
From: Prabhu Gnana Sundar [mailto:prabhugs_at_collab.net]
Sent: Mon 9/12/2011 2:25 PM
To: dev_at_subversion.apache.org
Subject: Re: [PATCH] python script for issue #3961, fixing the bogus mergeinfo
On Friday 19 August 2011 11:45 AM, Prabhu Gnana Sundar wrote:
> Correcting the issue number as #3961...
>
Minor fix in one of the commented descriptions... Attaching the updated
script.
Please share your thoughts.
Thanks and regards
Prabhu
Received on 2011-09-12 14:57:53 CEST