Re: svn commit: r27817 - trunk/subversion/svndumpfilter

Hi David,

David Glasser wrote:
>> /*** Code. ***/
>> @@ -151,6 +152,7 @@
>> svn_boolean_t drop_empty_revs;
>> svn_boolean_t do_renumber_revs;
>> svn_boolean_t preserve_revprops;
>> + svn_boolean_t skip_missing_merge_sources;
>> apr_array_header_t *prefixes;
>>
>> /* Input and output streams. */
>> @@ -627,6 +629,64 @@
>> }
>>
>>
>> +/* Renumber revisions for valid merge sources in mergeinfo. */
>> +static svn_error_t *
>> +renumber_merge_source_rev_range(const char **mergeinfo_val,
>
> Why return const char * if the only caller actually wants
> svn_string_t?

Since I am passing a "subpool" from the caller whose scope is short lived
within this "if (strcmp(name, SVN_PROP_MERGE_INFO) == 0)" condition, and
"value" is accessed outside of this if condition, which will require a string
duplication done explicitly, when I return a svn_string_t, I think it will be
better to have const char *.

Also in the function I do the following:

   svn_mergeinfo_to_stringbuf(&merge_val, modified_mergeinfo, pool);

   *mergeinfo_val = merge_val->data;

If I need to return a svn_string_t, I must use "svn_mergeinfo_to_string" which
is a function present in the private header "svn_mergeinfo_private.h".

If the above is not a concern, then I can resubmit a patch with the function
signature changed to svn_string_t.

> Given that this patch had a couple of serious flaws, I'm curious: what
> was your testing strategy for this patch? (It does look like we are
> lacking in automated tests for svndumpfilter, which is a shame.)

I used this dump file
http://subversion.tigris.org/nonav/issues/showattachment.cgi/798/2982.dump
along with the following script (with various combinations of svndumpfilter
subcommands such as --drop-empty-revs, --renumber-revs) to test this patch. I
apologize I missed to check exclude closely :(

<snip>

#!/bin/bash

SVN=svn
SVNADMIN=svnadmin
SVNDUMPFILTER=svndumpfilter

REPOS=/tmp/repos
REPOS1=/tmp/repos1
WC1=/tmp/wc1

rm -rf $REPOS1 $WC1

$SVNADMIN dump $REPOS > /tmp/l
$SVNDUMPFILTER include trunk branch1 branch2 branch10 branch7
--skip-missing-merge-sources --drop-empty-revs --renumber-revs < 2982.dump > /tmp/k
$SVNADMIN create $REPOS1
$SVNADMIN load $REPOS1 < /tmp/k
$SVN co file://$REPOS1 $WC1
$SVN pl -vR $WC1

</snip>

I ve come up with a new patch based on your review comments.

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