[PATCH] Follow up to r27817

[Senthil Kumaran S <se...@collab.net>]

Hi,

I am attaching a patch along with this email which is a follow up to r27817, 
based on glasser's review comments.

[[[
Follow up to r27817.

Improve r27817 based on glasser's comments.

* subversion/svndumpfilter/main.c
   (skip_path): New function to skip a path based on the included/excluded
    prefixes
   (renumber_merge_source_rev_range): Give more approriate name to this
    function as skip_missing_merge_source_or_renumber_revs
   (skip_missing_merge_source_or_renumber_revs): Function renamed from
    renumber_merge_source_rev_range. Fix logical bug in include/exclude prefix
    path skipping and avoid possible crashes in dereferencing revmap_start and
    revmap_end
   (set_node_property): Update call to renumber_merge_source_rev_range, to the
    renamed function skip_missing_merge_source_or_renumber_revs

Patch by: Senthil Kumaran <se...@collab.net>
]]]

Thank You.

--
Senthil Kumaran S
http://www.stylesen.org/

Re: [PATCH] Follow up to r27817

[Daniel Rall <dl...@collab.net>]

On Thu, 15 Nov 2007, Senthil Kumaran S wrote:

> Hi,
> 
> I am attaching a patch along with this email which is a follow up to 
> r27817, based on glasser's review comments.

I committed the skip_path()-related portions of this patch in r27832,
using the new helper throughout this source file.  I'm looking at the
rest of the patch now...

Re: [PATCH] Follow up to r27817

[Senthil Kumaran S <se...@collab.net>]

Hi Dan,

Daniel Rall wrote:
> Senthil, I committed the remainder of these changes, heavily modified,
> in r27845.  Please review them to, and make sure I didn't screw
> anything up.  :P

Consider the following commands with the dump file present here 
http://subversion.tigris.org/nonav/issues/showattachment.cgi/798/2982.dump.

<snip>
$SVNDUMPFILTER include trunk branch2 branch10 branch7 
--skip-missing-merge-sources --drop-empty-revs --renumber-revs < /tmp/2982.dump 
 > /tmp/k
$SVNADMIN create $REPOS1 

$SVNADMIN load $REPOS1 < /tmp/k 

$SVN co file://$REPOS1 $WC3 

$SVN pl -vR $WC3
</snip>

The output with the current code (r27845) is:

Properties on '/tmp/wc3/branch2':
   svn:mergeinfo : /trunk:1-2
Properties on '/tmp/wc3/trunk':
   svn:mergeinfo : /branch1:4-62
Properties on '/tmp/wc3/branch7':
   svn:mergeinfo : /trunk:1-3
Properties on '/tmp/wc3/branch10':
   svn:mergeinfo : /trunk:1-4

If we have a closer look at the above output we can see in the svn:mergeinfo of 
/trunk, we have /branch1:4-62 which is wrong, since in the renumbered revisions 
we have 62 => 20. On the other hand we did not include "branch1" in the prefix 
at all, so there should be no svn:mergeinfo having "branch1" in its merge 
source (thats why I reconstructed the mergeinfo in my original patch using 
modified_mergeinfo hash).

I ve created a patch to solve this problem - 
http://svn.haxx.se/dev/archive-2007-11/0720.shtml

With the above patch of mine the output will be the following:

Properties on '/tmp/wc3/branch2':
   svn:mergeinfo : /trunk:1-2
Properties on '/tmp/wc3/trunk':
   svn:mergeinfo :
Properties on '/tmp/wc3/branch7':
   svn:mergeinfo : /trunk:1-3
Properties on '/tmp/wc3/branch10':
   svn:mergeinfo : /trunk:1-4

In the above output since "branch1" is not included we have a empty svn:mergeinfo.

> Also, it would be fantastic if you could introduce a new test suite
> (svndumpfilter_tests.py) for this issue, and for issue #3020.  I
> noticed that you've already got a dump file for the latter; you can
> incorporate it using a svndumpfilter_tests_data/ directory, like you
> see elsewhere in the command-line test suite.

Sure Dan, I will come up with a test suite for svndumpfilter.

Thank You.

--
Senthil Kumaran S
http://www.stylesen.org/

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

Re: [PATCH] Follow up to r27817

[Daniel Rall <dl...@collab.net>]

On Thu, 15 Nov 2007, Senthil Kumaran S wrote:
...
> I am attaching a patch along with this email which is a follow up to 
> r27817, based on glasser's review comments.
> 
> [[[
> Follow up to r27817.
> 
> Improve r27817 based on glasser's comments.
> 
> * subversion/svndumpfilter/main.c
...
>   (skip_missing_merge_source_or_renumber_revs): Function renamed from
>    renumber_merge_source_rev_range. Fix logical bug in include/exclude 
>    prefix
>    path skipping and avoid possible crashes in dereferencing revmap_start 
>    and
>    revmap_end
>   (set_node_property): Update call to renumber_merge_source_rev_range, to 
>   the
>    renamed function skip_missing_merge_source_or_renumber_revs

Senthil, I committed the remainder of these changes, heavily modified,
in r27845.  Please review them to, and make sure I didn't screw
anything up.  :P

I'm also a little leary of this new --skip-missing-merge-sources
option for 'svndumpfilter', because it's somewhat inconsistent with
what we did in libsvn_repos/load.c (for merge source path prefix
adjustment), where we make the assumption that all merge source paths
are present in the dump file that you're loading.  The use case is
slightly different here, since we are explicitly filtering out part of
the dump file, but I did want to point out the inconsistency (which
perhaps we're okay with).

Also, it would be fantastic if you could introduce a new test suite
(svndumpfilter_tests.py) for this issue, and for issue #3020.  I
noticed that you've already got a dump file for the latter; you can
incorporate it using a svndumpfilter_tests_data/ directory, like you
see elsewhere in the command-line test suite.